level.dat Biome Rules Recurse Infinitely [Problem Area Identified, just needs fix]
ByThePowerOfScience opened this issue · 14 comments
Seems to be the exact same issue as #101, but on the latest version of Citadel. Happening consistently with this pack on all platforms, and both client and server-side.
I spent like twelve hours straight going through it with a debugger trying to find why it was doing that, so if you need any more information, please let me know.
Citadel: citadel-2.6.0-1.20.1
TerraBlender: Disabled
It looks like it might be an incompatibility with Luminous: Nether, though how an MCreator mod could do that is beyond me.
Here's a stringified version of the level.dat I managed to get the server jar to generate: level.txt.gz
(That was so nostalgic btw, having to modify the server jar to bypass the limit. Amazing to see how old-fashioned Minecraft mods were made, down to deleting META-INF to get rid of the checksums and signing.)
Removing Luminous: Nether did not, in fact, fix it. However, I think I found the cause.
In mixin.NoiseGeneratorSettingsMixin, the biome rules are appended to the existing ones unless mergedSurfaceRules is set to true. However, this does not take into account the fact that the previously injected biome rules were actually already deserialized and exist there already. So it just keeps adding them onto the end of the list, forever.
No mod incompatibility, just a Citadel bug because the mergedSurfaceRules flag doesn't persist between restarts.
what exactly would the fix be here? storing mergedSurfaceRules somewhere that gets saved across restarts?
Yeah, pretty much.
Alternately, use MixinExtras' @ModifyReturnValue to allow compat with Terrablender without saving state, instead of modifying this.surfaceRule.
So is this what's causing the worldgen to be so incredibly slow after playing on the server awhile? Is there a fix for this aside from removing Citadel and all mods that require it?
Id like to know as well. Ive been searching for a solution for the past 2 months. Frustrating
Id like to know as well. Ive been searching for a solution for the past 2 months. Frustrating
So is this what's causing the worldgen to be so incredibly slow after playing on the server awhile? Is there a fix for this aside from removing Citadel and all mods that require it?
Until the issue is resolved in the mod, the only way to fix it temporarily is to open up level.dat in NBTExplorer and remove the infinitely recursing rule.
what exactly would the fix be here? storing mergedSurfaceRules somewhere that gets saved across restarts?
@AlexModGuy I figured it out!!! I've been having this issue for a solid year now and I keep trying to fix it with Mixins, but I finally figured it out!!!!!
Problem
The problem is this: every time the level.dat is saved, the new biome rules rewrap the old ones that already include the biome rules wrapping the old ones, causing infinite nesting resulting in an eventual crash.
The reason this happens is that we're modifying the getter, which is the same thing used by the codec when serializing it! Even my change that uses @ModifyReturnValue doesn't fix anything, because we're still re-adding the biome rules each and every time the game is run!
Solution
There are two ways to fix this:
Possibility 1
Add persistent data to the Level (aka just use WorldData) to tell us that we've already added our biome rules.
The pro of this is that it's much faster, as it's all cached. We can still freely modify the surfaceRule field to cache the calculation when we add them, but we have to do it once and only once ever.
However, the issue now becomes "What if we need to add new biome rules later? How do we tell what we've added so we only change that instead of wrapping the whole thing again?" This will require a datafixer to visit and patch the level.dat, meaning we'll have to figure out a way to identify our injected biome rules in the midst of the cluster of other biome rules.
Possibility 2
Inject into LevelStorageSource and set a global flag so we know when the level.dat is being saved. If it's being saved, don't add our biome rules. This keeps the biome rule addition solely locked to runtime, removing the possibility of nesting.
The problem now is that we by necessity can't cache the additional biome rules, instead adding them each and every time the method is called. This slows down chunk generation a not-inconsiderable amount.
...
I'm not sure which is the better option, but the current situation is untenable. I'm currently having to manually un-brick my world every few days or so, so we've got to do something.
@ByThePowerOfScience Thank you for helping figure this one out!
I may have a tentative solution, but it'll require some testing to figure out.
I've reverted back to 2.5.x's solution of modifying the surfaceRule in NoiseGeneratorSettings rather than NoiseBasedChunkGenerator for simplicity. I've added a check that compares a seed value generated by a hashed string of all of the new sequenced rules to be added. Hopefully, this code will only run on the occasions:
- level creation
- surface rule seed checksum changed (add/removed mods etc).
As I've said, still requires some testing. I should be able to have a Jar soonish, so if that's something you would like to test yourself hands-on, you can contact me on discord for that.
@ByThePowerOfScience Thank you for helping figure this one out! I may have a tentative solution, but it'll require some testing to figure out.
Citadel/src/main/java/com/github/alexthe666/citadel/mixin/NoiseGeneratorSettingsMixin.java
Line 31 in dd42eb2
if (citadelServerData != null && !citadelServerData.isUsingLatestSurfaceRules()) {I've reverted back to 2.5.x's solution of modifying the surfaceRule in NoiseGeneratorSettings rather than NoiseBasedChunkGenerator for simplicity. I've added a check that compares a seed value generated by a hashed string of all of the new sequenced rules to be added. Hopefully, this code will only run on the occasions:
* level creation * surface rule seed checksum changed (add/removed mods etc).As I've said, still requires some testing. I should be able to have a Jar soonish, so if that's something you would like to test yourself hands-on, you can contact me on discord for that.

Appears to have the unwanted side effect of breaking surface rules on relog for client worlds. Shame, I'll have to try something else.
Possible solution is reverting to the old way of merged rules, with some needed changes:
- wrap all surfaces rules added by citadel with
CitadelSurfaceRuleWrapper - before merging surface rules, strip all
CitadelSurfaceRuleWrappersurface rules.
Will still need to investigate and see if this fixes it or not.
I just thought of another alternative: you could wrap your injected surface rules in a custom tag type with its own identifier, then use a Mixin on the Codec (chain a .or) to make that custom class be recognized as vanilla surface rules.
Then you always know exactly where your injected rules are for change later.
{
"surface_rules": [
{
"citadel": true,
"rules": [
{
"sequence": [
"..."
]
},
]
}
]
}Revised solution:
- If terrablender is installed, add all of Citadel's new surface rules to its SurfaceRuleManager, and do not directly change surface rules itself via mixin.
- If terrablender is not installed, wrap all rules in the new rule type CitadelSurfaceRuleWrapper, and then set the surface rule to this via mixin
- Before the level data is saved to nbt, swap the surface rules with the old, unwrapped ones, then swap back.
