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

Add minetest.generate_biomes #11953

Closed
wants to merge 8 commits into from

Conversation

gaelysam
Copy link
Contributor

@gaelysam gaelysam commented Jan 14, 2022

What it does

Implements #8329. Add function minetest.generate_biomes(vm, pos1, pos2[, noise_filler_depth]) to generate biome nodes in the VoxelManip from Lua mapgen mods.

Also updates heatmap, humiditymap and biomemap, so that they can be reused by minetest.generate_decorations, minetest.generate_ores and minetest.get_mapgen_object.

My approach creates a temporary MapgenBasic object and calls MapgenBasic::generateBiomes(). The class definition is modified to allow such a usecase.

minetest.generate_biomes must be called during mapgen (register_on_generated callback), and the X/Z extent of pos1-pos2 must match mapgen chunk size, for the buffers to work correctly. I have not found a way to overcome these limitations that would not lead to a much more complex code.

Why is it interesting/needed?

  • Easier to add biomes to Lua mapgens, a single line will replace a long and complex code to re-implement in Lua what already exists in the core (this latter has been done by several modders including me).
  • Improves compatibility between Lua mapgens and mods that use the biome system (including biome-dependant decorations/ores). This is critical for a Lua mapgen mod to be interesting for players and server owners. Otherwise it stays an experimental thing that is rarely going to be really used.
  • EDIT: Speeds up biome generation compared to Lua-reimplemented biomes (gain is in the range of 50-100 ms). This is also critical for Lua mapgens.

To do

I consider this PR as Ready for Review.

I initially planned to include a function to generate dust nodes but this may come in a separate PR, this one is already interesting in itself.

How to test

I have made a modified version of Paramat's lvm_example for testing. It generates very simple terrain, with biomes, ores and decorations. You can append other biome mods like Ethereal to see how it reacts.

screenshot_20220114_130702
^ This PR + lvm_example + Ethereal NG

…oise'

If generate_ores/decorations is called after generate_biomes, the biome map
is taken into account.
I think minetest.calc_biome_noise has no use alone.
- Auto-detect water level
- Make noise_filler_depth optional
- Check size of chunk and noise buffer, and throw an error if it does not match
…porary one

Update documentation (and fix mistake)
and calculate max bounds from provided VoxelManip instead
@sfan5 sfan5 added Feature ✨ PRs that add or enhance a feature @ Mapgen @ Script API Concept approved Approved by a core dev: PRs welcomed! labels Jan 14, 2022

c_stone = ndef_p->getId("mapgen_stone");
c_water_source = ndef_p->getId("mapgen_water_source");
c_river_water_source = ndef_p->getId("mapgen_river_water_source");
Copy link
Member

Choose a reason for hiding this comment

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

What about the other class members? Those will contain uninitialized data.

Copy link
Contributor Author

@gaelysam gaelysam Jan 14, 2022

Choose a reason for hiding this comment

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

They will, but I'm only setting what is actually used by generateBiomes (it uses a temporary object).

I set some other class members from l_generate_biomes() but I need this function for the protected ones.

I can make ModApiMapgen a friend class of MapgenBasic, leave the constructor empty and initialize all class members in l_generate_biomes if that's better.

EDIT: Doing that right now, it's clearly better

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't looked at the code changes yet but in general it is better to refactor the code so that such workarounds are not even necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it better with the new commit?

Copy link
Member

Choose a reason for hiding this comment

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

(not yet; continued in another comment)

and group class member assignments in l_generate_biomes
also minor codestyle simplification
@gaelysam
Copy link
Contributor Author

gaelysam commented Jan 15, 2022

I've measured biome generation time of this new function against my Lua re-implementation, biomegen mod, that closely reproduces the algorithm in MapgenBasic::generateBiomes(). The gain of time is around 75 ms per chunk, which is significant.

Test code: lvm_example/timer, and enable/disable biomegen mod to see the difference. Time statistics are displayed at the end (concerning only the biome generation part).

With the Lua mod:

Biomegen calls: 583
Biomegen mean time: 83.899 ms
Biomegen time std: 65.877 ms

With minetest.generate_biomes:

Biomegen calls: 434
Biomegen mean time: 8.565 ms
Biomegen time std: 4.387 ms


const NodeDefManager *ndef = getServer(L)->getNodeDefManager();

MapgenBasic mg;
Copy link
Member

@SmallJoker SmallJoker Jan 22, 2022

Choose a reason for hiding this comment

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

Why couldn't you use mg_current for this? MapgenBasic() = default; does not initialize trivial data types which can quickly result in unpredictable/erroneous code.

How about this instead?

MapgenBasic *mg = dynamic_cast<MapgenBasic *>(mg_current);
FATAL_ERROR_IF(!mg, "Bug! This mapgen does not inherit from MapgenBasic.");

.. and make MapgenSinglenode inherit MapgenBasic like all other mapgens. If that does not work, you could still specify virtual functions in Mapgen which the inherited classes would overwrite.

Copy link
Contributor Author

@gaelysam gaelysam Jan 22, 2022

Choose a reason for hiding this comment

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

I thought redefining MapgenSinglenode was out of question, not sure why I was thinking that, maybe paramat was against it a year or so ago. I tried to stay consistent with what was done for l_generate_ores and l_generate_decorations but I see why it does not make sense to create a temporary object for biomes: it is a strictly mapgen-time method, that ores and decorations are not.

Will try that. MapgenV6 should also inherit MapgenBasic in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing existing mapgens to inherit from a class they have no business with just for this does sounds like a bad idea to me.

@lhofhansl
Copy link
Contributor

Where are we with this? With Lua mapgen being slow (and essentially disabling multi threaded generation due to the env lock) any speed improvement is very welcome!

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 9, 2022
@Zughy
Copy link
Contributor

Zughy commented Apr 30, 2022

@Gael-de-Sailly are you gonna apply the requested changes?

@ChargingBulle
Copy link

What exactly does the Supported by core dev Tag do? is that some sort of BDFL-Verification or am I missing something?

@Zughy
Copy link
Contributor

Zughy commented May 16, 2022

@Rick-W-Storm it means that, even if it's not in the roadmap, some core devs will review it and possibly merge it

@Zughy Zughy added Adoption needed The pull request needs someone to adopt it. Adoption welcomed! and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) Possible close labels May 26, 2022
@Zughy Zughy closed this May 26, 2022
@gaelysam
Copy link
Contributor Author

Rebased. I have in some way rediscovered this PR today (lol), I can start working again on this and I hope this can be reopened. The current version is working but I did not make any change since last commit, except fixups to follow recent changes in minetest.

@wsor4035
Copy link
Contributor

image
cant be reopened, please make a new pr and link to this one @Gael-de-Sailly

@Zughy Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Mapgen @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants