Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RegistrySupplier crashing on save with Status Effects #579

Open
fzzyhmstrs opened this issue Nov 16, 2024 · 2 comments
Open

RegistrySupplier crashing on save with Status Effects #579

fzzyhmstrs opened this issue Nov 16, 2024 · 2 comments

Comments

@fzzyhmstrs
Copy link

fzzyhmstrs commented Nov 16, 2024

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
Copy link
Author

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());
}

@qyl27
Copy link

qyl27 commented Dec 19, 2024

Same issue, RegistrySupplier is another implementation of Holder<T>, but in Minecraft, Holder.Reference is excepted.

qyl27 added a commit to CuteNekoOwO/CatsPlus that referenced this issue Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants