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

Mapgen: Don't clear biomes, decorations and ores #2947

Merged
merged 1 commit into from Dec 10, 2023

Conversation

appgurueu
Copy link
Contributor

@appgurueu appgurueu commented Apr 24, 2022

I might be unaware of the intention behind this clearing, but for vanilla MTG it should be a no-op as nothing is registered before. For modded MTG however this had the potential to silently unregister the biomes, decorations and ores of mods that don't depend on default and are thus loaded before it.
For decorations and ores in particular this should most likely not be done; for biomes I don't see the point either, but there might be a point I'm missing.

git blame yields 2e950ac6 which provides no reason for the clearings either. @paramat please explain.

@sfan5
Copy link
Member

sfan5 commented Apr 24, 2022

Does this change the map generated for a given seed in any way?
Does the engine have any defaults here?

@appgurueu
Copy link
Contributor Author

Does this change the map generated for a given seed in any way?

According to very brief testing it does not.

Does the engine have any defaults here?

I don't think so. Inserting

assert(next(minetest.registered_biomes) == nil)
assert(next(minetest.registered_ores) == nil)
assert(next(minetest.registered_decorations) == nil)

throws no error.

@appgurueu appgurueu added this to the 5.6.0 milestone May 6, 2022
@appgurueu appgurueu modified the milestones: 5.6.0, 5.8.0 Sep 16, 2023
@Montandalar
Copy link
Contributor

I feel like changing this would help more mapgen mod authors than it could possibly harm. With no biomes built into the engine, I don't know why you would clear them in MTG.

@appgurueu
Copy link
Contributor Author

@sfan5 @SmallJoker objections to merging this?

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the possibility that a mod that registers biomes/ores/decorations does neither directly nor indirectly depend on default. In that case, it could be loaded before default, thus silently overwritten. After this PR, this would generate new, differently looking mapblocks.
The overall chance for that to happen is negligible. The introduced consequences in that case do however look reasonable and acceptable to me.

@appgurueu appgurueu merged commit 0536755 into minetest:master Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants