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

Fix fetching biomes during world generation #11086

Closed
wants to merge 3 commits into from
Closed

Fix fetching biomes during world generation #11086

wants to merge 3 commits into from

Conversation

NewwindServer
Copy link
Contributor

@NewwindServer NewwindServer commented Jul 15, 2024

Fixes #11084

getBiome() on LevelReader calls getNoiseBiome(), which itself calls getChunk() in order to get the biome cached inside the chunk.
However, calling getChunk() on WorldGenRegion will cause a crash if the chunk isn't found: "Requested chunk unavailable during world generation"
This means that features which attempt to fetch biomes during generation (LakeFeature/SnowAndFreezeFeature)
can cause the server to crash.
This patch ensures that these features are fetching biomes in a way that doesn't call getChunk()

@NewwindServer NewwindServer requested a review from a team as a code owner July 15, 2024 17:50
@fucksophie
Copy link

Currently running my server with this exact patch, works perfectly and server no longer crashes on LakeFeature. Would love to see this merged!

@jpenilla
Copy link
Member

jpenilla commented Jul 28, 2024

LakeFeature is deprecated and vanilla has moved away from using it for anything except for lava lakes.
The error for unsafe chunk gets you are removing here is an intentional safeguard against bad code, such as that in the dead path in LakeFeature when creating water lakes. While it may appear to 'fix' the problem at hand it is not a proper solution and will cause other more serious problems to go undetected. This should be reported to Mojang if it's reproducible with a datapack. Given the circumstances I don't really think this is something we should care to fix, but if it is it would need to be a targeted change to LakeFeature for the aforementioned reasons.

relevant disussion

@fucksophie
Copy link

fucksophie commented Jul 29, 2024

This patch works, and stops my datapack heavy server (Terralith, Nullscape, etc...) from crashing. But, at the same time it makes new chunk generation performance x3-x4 worse! Why does it, and how can I make sure performance isn't so bad and it doesn't crash?

@NewwindServer
Copy link
Contributor Author

This patch works, and stops my datapack heavy server (Terralith, Nullscape, etc...) from crashing. But, at the same time it makes new chunk generation performance x3-x4 worse! Why does it, and how can I make sure performance isn't so bad and it doesn't crash?

Can confirm that this changed code is called very often during world gen and could therefore potentially have a negative performance impact, I will make just the two features which call getBiome() during worldgen (LakeFeature and SnowAndFreezeFeature) use the uncached biome instead of overriding getBiome() entirely on WorldGenRegion.

@NewwindServer
Copy link
Contributor Author

@fucksophie I changed the patch to fix the lake feature in a different way which shouldn't slow down world generation. Test it out and lmk

@codebycam
Copy link

Hi @NewwindServer, can you please create a new issue with proper reproduction steps for 1.21? The issue you're linking that this PR fixes (#11084) was closed due to being incomplete and a deleted user.

@NewwindServer NewwindServer closed this by deleting the head repository Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

Lake feature crashes server
4 participants