architectury / architectury-api

An intermediary api aimed at easing development of multiplatform mods.
https://discord.architectury.dev/
GNU Lesser General Public License v3.0
325 stars 56 forks source link

RegistrySupplier crashing on save with Status Effects #579

Open fzzyhmstrs opened 5 hours ago

fzzyhmstrs commented 5 hours ago

When exiting world or closing game while a player has an active modded status effect registered via Arch API, the game crashes with the following error. MC: 1.21.1 Arch: 13.0.8 Env: Intellij Fabric runClient

java.lang.IllegalStateException: Unregistered holder in ResourceKey[minecraft:root / minecraft:mob_effect]: minecraft:mob_effect@simplyswords:ribbonwrath
    at com.mojang.serialization.DataResult$Error.getOrThrow(DataResult.java:287)
    at com.mojang.serialization.DataResult.getOrThrow(DataResult.java:81)
    at net.minecraft.entity.effect.StatusEffectInstance.writeNbt(StatusEffectInstance.java:332)
    at net.minecraft.entity.LivingEntity.writeCustomDataToNbt(LivingEntity.java:728)
    at net.minecraft.entity.player.PlayerEntity.writeCustomDataToNbt(PlayerEntity.java:838)
    at net.minecraft.server.network.ServerPlayerEntity.writeCustomDataToNbt(ServerPlayerEntity.java:392)
    at net.minecraft.entity.Entity.writeNbt(Entity.java:2281)
    at net.minecraft.server.integrated.IntegratedPlayerManager.savePlayerData(IntegratedPlayerManager.java:29)
    at net.minecraft.server.PlayerManager.saveAllPlayerData(PlayerManager.java:643)
    at net.minecraft.server.MinecraftServer.saveAll(MinecraftServer.java:610)
    at net.minecraft.server.integrated.IntegratedServer.saveAll(IntegratedServer.java:302)
    at net.minecraft.server.integrated.IntegratedServer.tick(IntegratedServer.java:98)
    at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:747)
    at net.minecraft.server.MinecraftServer.method_29739(MinecraftServer.java:288)
    at java.base/java.lang.Thread.run(Thread.java:1583)

looking at the writeNbt bit of the stack, it becomes apparent that the MC registry entry codec "validates" by doing an instanceof check. writeNbt -> CODEC -> StatusEffect.ENTRY_CODEC -> getEntryCodec() -> validateReference

private DataResult<RegistryEntry.Reference<T>> validateReference(RegistryEntry<T> entry) {
    return entry instanceof RegistryEntry.Reference<T> reference
    ? DataResult.success(reference)
    : DataResult.error(() -> "Unregistered holder in " + this.getKey() + ": " + entry);
}

RegistrySupplier is not an instance of Reference, so this fails and the game crashes from getOrThrow(). Neoforged seems to get around this issue by holding the underlying Reference inside the DeferredHolder, and then passing that delegate into the validation instead of itself. Arch doesn't do this (or anything to avoid this hard instanceof check). I've only tested in fabric, but I would guess this problem would also be in Arch neoforge.

fzzyhmstrs commented 4 hours ago

Confirmed that the RegistrySupplier is causing the issues; when I introduce a workaround that provides the Reference instead of the Supplier, all works as intended.

public static RegistryEntry<StatusEffect> getReference(RegistrySupplier<StatusEffect> input) {
    return EFFECT.getRegistrar().getHolder(input.getId());
}