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

Neoforge Biome/Structure Modifiers crashes on recycled Frozen Registries #431

Open
Speiger opened this issue Dec 27, 2023 · 10 comments · May be fixed by #1545
Open

Neoforge Biome/Structure Modifiers crashes on recycled Frozen Registries #431

Speiger opened this issue Dec 27, 2023 · 10 comments · May be fixed by #1545
Labels
1.20 Targeted at Minecraft 1.20 1.21 Targeted at Minecraft 1.21 bug A bug or error triage Needs triaging and confirmation

Comments

@Speiger
Copy link
Contributor

Speiger commented Dec 27, 2023

o/ A Interesting crash with server registry recycling happens. (I technically have it fixed with mixins but i still should report it)

When you create a custom server using the world creation screens information, that works just fine.
But the moment you try to do that a second time the server will simply crash, because the registries haven't been reset 100%

This leads to the following crash:

Crashlog

java.lang.IllegalStateException: Biome Reference{ResourceKey[minecraft:worldgen/biome / minecraft:badlands]=net.minecraft.world.level.biome.Biome@504521ae} already modified
	at net.neoforged.neoforge.common.world.ModifiableBiomeInfo.applyBiomeModifiers(ModifiableBiomeInfo.java:76) ~[neoforge-20.4.49-beta.jar%23196%23203!/:?] {re:mixin,re:classloading,pl:mixin:APP:chunkpregen.mixins.json:common.forge.BiomeModifierRemoverMixin,pl:mixin:A}
	at net.neoforged.neoforge.server.ServerLifecycleHooks.lambda$runModifiers$1(ServerLifecycleHooks.java:200) ~[neoforge-20.4.49-beta.jar%23196%23203!/:?] {re:classloading}
	at java.util.AbstractList$RandomAccessSpliterator.forEachRemaining(AbstractList.java:720) ~[?:?] {}
	at java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762) ~[?:?] {}
	at net.neoforged.neoforge.server.ServerLifecycleHooks.runModifiers(ServerLifecycleHooks.java:199) ~[neoforge-20.4.49-beta.jar%23196%23203!/:?] {re:classloading}
	at net.neoforged.neoforge.server.ServerLifecycleHooks.handleServerAboutToStart(ServerLifecycleHooks.java:90) ~[neoforge-20.4.49-beta.jar%23196%23203!/:?] {re:classloading}
	at net.minecraft.client.server.IntegratedServer.initServer(IntegratedServer.java:72) ~[neoforge-20.4.49-beta.jar%23197!/:?] {re:classloading,pl:runtimedistcleaner:A}
	at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:663) ~[neoforge-20.4.49-beta.jar%23197!/:?] {re:mixin,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B,pl:mixin:APP:chunkpregen.mixins.json:common.server.MinecraftServerMixin,pl:mixin:APP:chunkpregen.mixins.json:common.server.ServerSeedMixin,pl:mixin:A}
	at net.minecraft.server.MinecraftServer.lambda$spin$2(MinecraftServer.java:255) ~[neoforge-20.4.49-beta.jar%23197!/:?] {re:mixin,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B,pl:mixin:APP:chunkpregen.mixins.json:common.server.MinecraftServerMixin,pl:mixin:APP:chunkpregen.mixins.json:common.server.ServerSeedMixin,pl:mixin:A}
	at java.lang.Thread.run(Thread.java:833) ~[?:?] {}

And sadly there is no API to actually reset the biome/structure modifiers.
So a mixin is required to fix this.

To explain why I ran into this problem.
I simply implemented a quick seed switch option where you can cycle through seeds a lot more quickly then normally.
And when you recycle the registries that have already been picked (you can't change them after the fact) changing the seed has barely any effects on registries themselves.
The issue is always appearing, unless you do a 100% registries recreation, which is kinda overkill.
That issue exists since 1.19.0

So yeah.

Some infos about neo version:
I was using: Neoforge 20.4.49-beta

Edit:
This is the code to fix this crash.

Code

private void resetRegistries(MinecraftServer server) {
	final RegistryAccess registries = server.registryAccess();

	registries.registryOrThrow(Registries.BIOME).holders().forEach(biomeHolder -> {
		((BiomeModifierRemoverMixin)biomeHolder.value().modifiableBiomeInfo()).clearModifiers(null);
	});
	registries.registryOrThrow(Registries.STRUCTURE).holders().forEach(structureHolder -> {
		((StructureModifierRemoverMixin)structureHolder.value().modifiableStructureInfo()).clearModifiers(null);
	});
}

It just resets the modifier cache so the modifiers can be reapplied on the next attempt.

@Speiger Speiger added the triage Needs triaging and confirmation label Dec 27, 2023
@sciwhiz12 sciwhiz12 added bug A bug or error 1.20 Targeted at Minecraft 1.20 labels Jan 1, 2024
@TelepathicGrunt
Copy link
Contributor

Is this reproducible with only one mod that has 1 biome modifier (outside your seed cycling stuff)? Is the steps just to make a world, exit to menu, and then remake the world again?

@Speiger
Copy link
Contributor Author

Speiger commented May 5, 2024

@TelepathicGrunt that is reproducable with just forge and 0 biome modifiers.
On startup the biome modifiers are compiled and when a existing cache is present it will straight up throw a crash.
Because it expects no cache to be present.

You simply can't recycle a server instance with different seeds.
You literally have to rebuild the entire registry from scratch everytime you want to change the seed.
Even if nothing but the seed changes.

@TelepathicGrunt
Copy link
Contributor

Hello, is this still an issue in 1.21?

@Speiger
Copy link
Contributor Author

Speiger commented Jul 25, 2024

@TelepathicGrunt does ModifiableBiomeInfo#applyBiomeModifiers still throw an exception if it gets called twice instead of invalidating the cache and regenerating it?

Spoilers: Yes

Edit: Sorry I didn't mean it to be that sassy ^^"

@TelepathicGrunt TelepathicGrunt added the 1.21 Targeted at Minecraft 1.21 label Jul 27, 2024
@TelepathicGrunt
Copy link
Contributor

TelepathicGrunt commented Jul 27, 2024

This seems like it is intended. As far as I understand it, you change the seed for the game, run the IntegratedServer#initServer again with the registry access of already modified biomes, which triggers NeoForge's handleServerAboutToStart that will now run our modifiers and ServerAboutToStartEvent event as well where other mods could also modify the registry access.

So what this seem like is happening, if a biome modifier ran the first time and added Blaze to the Plains biome, running your current code will take the existing registry access and feed it through the pipelines again and cause Blaze to be added again to the already modified Plains biome. So now Blaze is added at twice the intended rate.

And this is going to also impact other mods too that modify the registryAccess in ServerAboutToStartEvent which is, actually a surprisingly lot of mods! For example, many mods have been injecting their pieces into villages by using ServerAboutToStartEvent for when to modify the vanilla Template Pool
https://gist.github.com/TelepathicGrunt/4fdbc445ebcbcbeb43ac748f4b18f342

This means, even if NeoForge removes this crash, your code is still going to duplicate the modifications other mod has done within ServerAboutToStartEvent and that's more of a bug on your side to be honest as those modders have no way to shield themselves from recycling the registry access.

As far as I can tell, it seems Mojang intended for every fresh IntegratedServer to have their own re-created registryAccess data. Even the re-create button in Minecraft does a fresh registryAccess as well.

My question to you is, are you sure you cannot keep the existing IntegratedServer and modify its data in place without needing to spin up a new IntegratedServer? It would work better with other mods beside NeoForge. Like change the seed for IntegratedServer and make sure that new seed is set everywhere it needs to be, then yeet the chunk data and respawn players for a fresh world without needing to tear down and remake RegistryAccess

@Speiger
Copy link
Contributor Author

Speiger commented Jul 28, 2024

Wait so you are saying that BiomeModifier elements that are stored Immutably inside of the Vanilla Registry are being modified as the server loads the world?
Let me rephrase this a bit, isn't the Function https://github.com/neoforged/NeoForge/blob/1.21.x/src/main/java/net/neoforged/neoforge/common/world/ModifiableStructureInfo.java#L67 here ment to create a copy of the Structure information that is modified to prevent the original from being modified in the first place is actually useless?

Like why does this code even exist if the function that is here being effectively meaningless.
The whole point of my issue is that this functionality seems either:

  • 1: Useless and therefor should not exist in the first place (if the modifiers break the original implementation anyways)
  • Or 2: Not cause a crash because it just deleting the original modification and just recreate it, since all information is simply immutable anyways. Both the original information and the modifiers are practically create a safe/secure copy that support mods while protecting the original element. (If this is not a given then go back to the 1)

And why I am always spinning up a new server instead of changing the seed?
Because once the server is started certain files are simply locked into place, and these files are usually world defining elements that you need to recreate from scratch but can't because they are locked as long as the server runs.
The registry itself is never changing and simply a read only map for getting things done.
But anyways even if there are self mutating elements inside the registry these elements should always assume: "If there is already a mutation result, discard it and recreate it" (Which the implementation supports too and we have years of proof that it works stable, with all kinds of combinations of mods)

Anyways, if you try to change a seed without actually deleting all files that represent the previous seed then you basically get into real issues, you need to get as close as possible to a delete/recreate as possible. And the one thing i realized is that the registry itself doesn't require a reset to work just fine. Well outside of neo/forges MutableInformation self destructing for no found reason? And if the registry recreation wouldn't take like 5 times longer (Extraration) then creating a fresh world then I would keep it that way.

Anyways, I hope this doesn't seem like a rant, I am just trying to explain my view point ^^"
It isn't required that this change happens but I also should make you aware that this has been like in place for a long time just fine, its just neo/forge that is a cause for trouble here and pushing them out of the way basically makes things work without downsides.

@Speiger Speiger changed the title Neoforge Biome/Structure Modifiers crashes on recycled datapacks Neoforge Biome/Structure Modifiers crashes on recycled Frozen Registries Jul 28, 2024
@Speiger
Copy link
Contributor Author

Speiger commented Jul 28, 2024

Changed the name since recycled datapacks sounds missleading since the idea being that the registries were already frozen by the time the first server starts up...

@TelepathicGrunt
Copy link
Contributor

I can't speak for the reasons of why the code for the modifiable info classes was setup the way it was. Probably that when it was added, it was also deemed unnecessary to support registry reuse but wanted to provide ability for people to see the unmodified form of the biome/structure if needed.

Though my example with structures was nothing to do with the NeoForge structure modifiers. It was mods, many mods, modifying the TemplatePools directly within ServerAboutToStartEvent. I am fairly certain if you put on a mod that does the village house injection, cycle the seed a ton of times with your mod, and then visit a village. It will be flooded with the modded house because their injection ran multiple times for the already modified TemplatePool. Filling up the list of pieces with the modded piece multiple times. So even if NeoForge changes how it does its modifiers, you still have this hidden incompatibility with other mods and very difficult for players to figure out what is going wrong.

Honestly, I think you are breaking my own mods in some subtle way too by reusing the registry multiple times and feeding it back through the server creation pathway.

@Speiger
Copy link
Contributor Author

Speiger commented Jul 29, 2024

The first aspect is something that should be revisited IMO. And that is what i am asking this entire time.

The second aspect. I will investigate that as soon as i am ready to mod again.
And if there is an issue I will try to find a solution to that. I can get creative easily.
(Especially since i bug support still all the way back to 1.7.10)

Farmers delight it actually a good mod to test with.
Thank you for that potential issue.

@Speiger
Copy link
Contributor Author

Speiger commented Jul 30, 2024

@TelepathicGrunt from looking into the issue you described on my side.
Yes that is actually a bug.
That should be addressed in the next patch (on all mc versions)

Thank you for pointing that out, because i never noticed that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Targeted at Minecraft 1.20 1.21 Targeted at Minecraft 1.21 bug A bug or error triage Needs triaging and confirmation
Projects
None yet
3 participants