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 override for biomes, decorations and ores and some other help script functions #14191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Dec 30, 2023

To do

This PR is a Ready for Review.

How to test

Run devtest.
The test code is in mod mapgen in the the init.lua file.

There are added decoration and ore. If everything works fine, you should be able to find the desert biome with desert sand on top, you should be able to find spread decoration jungle grass in the grassland biome and the new basenodes:test_ore node in the stone of grassland and grassland_under biomes.

@wsor4035 wsor4035 added @ Script API @ Mapgen Feature ✨ PRs that add or enhance a feature labels Dec 30, 2023
@Zughy Zughy added the Concept approved Approved by a core dev: PRs welcomed! label Dec 31, 2023
`name` from `minetest.registered_decorations`.
* `minetest.reregister_ores()`
* Clears all ores currently registered and register them again.
* Can be used after overriding/unregistering biome/bimes.
Copy link
Member

Choose a reason for hiding this comment

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

Such re-registering functions should not need to exist. Mods would end up calling it all the time after modifying a registration. Couldn't you do a direct call to C++ side with each minetest.override_ore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function should be used after some changes in registered biomes.
There is no need to call it after changes in registered ores.

I can remove it, and call it after override_biome and unregister_biome calls. Does it sound ok?

Copy link
Member

Choose a reason for hiding this comment

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

Do biomes change their internal ID ( biome->index) when they're modified? I cannot see any other internal relation of ores and biomes aside from that. For example, node names are resolved after on_mods_loaded, so that cannot be the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing or unregistering the biome is possible only by clearing all biomes and registering them now.
So there is no guarantee here, that biome ID will be the same after that action.

It is documented:

minetest/doc/lua_api.md

Lines 5528 to 5532 in 6952bab

* `minetest.clear_registered_biomes()`
* Clears all biomes currently registered.
* Warning: Clearing and re-registering biomes alters the biome to biome ID
correspondences, so any decorations or ores using the 'biomes' field must
afterwards be cleared and re-registered.

It is the reason for reregistering functions to be implemented.

Copy link
Member

Choose a reason for hiding this comment

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

So it appears that the ore registration should not resolve the biome name --> ID mapping immediately but together this the node resolver. That way such workarounds would no longer be 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.

At the actual master branch registration of ore and decoration process everything, also node resolve.

ndef->pendNodeResolve(deco);

ndef->pendNodeResolve(ore);

Both core.registration_ore and core.registration_decoration also returned the handle of registered ore/decoration.

If I register biomes, ores, and decorations to C++ mapgen after on_mods_loaded call, it will not be possible for core.registration_ore and core.registration_decoration to return a valid handler.

It will break backward compatibility probably.

Is it acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

The only problem that I can see with master is the following code: (EDIT: also the few lines above that for single biomes)

// returns number of failed resolutions
size_t fail_count = 0;
for (lua_pushnil(L); lua_next(L, index); lua_pop(L, 1)) {
Biome *biome = get_or_load_biome(L, -1, biomemgr);
if (!biome) {
fail_count++;
infostream << "get_biome_list: failed to get biome '"
<< (lua_isstring(L, -1) ? lua_tostring(L, -1) : "")
<< "'" << std::endl;
continue;
}
biome_id_list->insert(biome->index);
}
return fail_count;

It should yet not map ores to biome IDs. Instead, use the ObjDef name for identification until resolving the IDs somewhere next to the NodeResolver step. Result:

  1. can continue using handles
  2. modify, delete and create biomes without worrying about ores

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see it now. Thank you for your help. I will try to simplest BiomeResolver class.

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that biomes may also have an empty name, which somewhat breaks my plans. In that case we'd need another reliable identifier...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem can be also this:

minetest/doc/lua_api.md

Lines 9908 to 9912 in 57de599

biomes = {"desert", "rainforest"},
-- List of biomes in which this ore occurs.
-- Occurs in all biomes if this is omitted, and ignored if the Mapgen
-- being used does not support biomes.
-- Can be a list of (or a single) biome names, IDs, or definitions.

Looks like it is allowed in the definition of ore to use biome IDs also. Similar to the definition of decoration.

We should mention this in doc, it should be the modder's responsibility to fix biome IDs in ore/decorations after removing/changing the biome.

Comment on lines +5471 to +5473
* `minetest.unregister_ore(name)`
* Unregisters the decoration from the engine, and deletes the entry with key
`name` from `minetest.registered_decorations`.
Copy link
Member

Choose a reason for hiding this comment

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

Function not necessary if we'd accept an explicit nil in minetest.override_ore to remove it. Or is that bad practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already functions like minetest.unregister_item(name) in Lua API.
So keeping of same Lua API style is a good idea, I believe.

@@ -5466,21 +5466,44 @@ Call these functions only at load time!
* Returns an integer object handle uniquely identifying the registered
ore on success.
* The order of ore registrations determines the order of ore generation.
* `minetest.override_ore(name, ore definition)`
* Override existing ore.
Copy link
Member

Choose a reason for hiding this comment

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

Unclear whether it overrides only the specified fields (like minetest.override_item) or whether it is replaced entirely by the specified table.

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.

None yet

4 participants