Allow game to specify first and last mod in mod loading order.

[sfence]

Allow games to do something at the beginning and end of the mod loading process.

• Allows game creators to do something before all other mods are loaded and after all other mods are loaded.
• Use added parameter lines first_mod and last_mod in game.conf to determine which mod has to be loaded first and which has to be loaded last.
• This should fix Support for mod that is loaded before any other mod #8934.
• This should help with the solution of issue Allow to override item definition defaults #13442.

To do

This PR is Ready for Review.

• Get feedback
• Apply feedback
• Add documentation to lua_doc.md

How to test

Switch to this branch and load the devtest game.
In devtest game.conf lines first_mod and last_mod are added. There are added checks in every devtest mod and in last_mod mod to verify if the loading order is right.

[appgurueu]

(Unofficial review, not a core dev.)

Allowing games to do something here makes sense.

I'm not a fan of the repeated

if minetest.global_exists("first_mod") then
	table.insert(first_mod.mods,minetest.get_current_modname())
else
	error("ERROR: Mod "..minetest.get_current_modname().." loaded before first_mod.")
end

boilerplate in every devtest mod. Could you perhaps test this differently, for example by means of C++ unit tests?

[lhofhansl]

Alternatively we can add some hooks before_mod_load, after_mod_load (or something, not good at naming) that are executed at those stages.
Games can then implement these hooks in one or more of their mods. Also that way each mod can early and late things.

[sfence]

Alternatively we can add some hooks before_mod_load, after_mod_load (or something, not good at naming) that are executed at those stages. Games can then implement these hooks in one or more of their mods. Also that way each mod can early and late things.

It will be complicated I think. Because every mod will have to be loaded twice time. At first time check if there are some hooks registration and the second time register nodes etc. So it will result in breaking backward compatibility, I am afraid.

But it will be a more flexible solution to do something like it.

[appgurueu]

Also that way each mod can early and late things.

The problem with this is that it doesn't make much sense: There can't be multiple mods that load first or last. And this has practical implications: Mods that want to load first usually want to do so to, say, override some function. If you have two mods attempting to do this, you can easily get a conflict.

[sfence]

@appgurueu

Thanks for the feedback.

Should be testable in C++ unittests. Lua solutions take less effort to create.
But C++ unittest will be probably better for it. I will look at it if I get in right mod for it.

[wsor4035]

just some a thought, not 100%

relating to the keys in game.conf, would it make sense to have them be something like load_first_mod = modname and load_last_mod = modname taking the syntax from world.mt

[sfence]

just some a thought, not 100%

relating to the keys in game.conf, would it make sense to have them be something like load_first_mod = modname and load_last_mod = modname taking the syntax from world.mt

Not a problem to change key names.
But still waiting for roadmap approval or disapproval. So it does not make sense to continue work on this now.

[lhofhansl]

The problem with this is that it doesn't make much sense: There can't be multiple mods that load first or last. And this has practical implications: Mods that want to load first usually want to do so to, say, override some function. If you have two mods attempting to do this, you can easily get a conflict.

Fair enough. I thoughts since games control their mods, they can control the sets of mods that do that.
If this is a better approach let's do this then.

[sfence]

Fair enough. I thoughts since games control their mods, they can control the sets of mods that do that.
If this is a better approach let's do this then.

I think, that before and after hooks can be implemented with this as well. It only requires the first loaded mod to go through mod lists and in every mod path looks for file before_hook.lua for example. after hook can be done also through callback registration. So it will be on the game developer to allow/disallow it.

[FreeLikeGNU]

Each world has a list of enabled mods. Why not set the order with that list with enabled mods at the top of the list with the user able to change that order to needs?

Slightly off-topic: It would be nice to have mod settings in that list as well instead of a just a boolean it could be something like:

just use the mod's default or game.conf settings:
load_mod_some_mod = mods/some_mod

this world uses this mod in a particular way:
load_mod_some_mod = { path= "mods/some_mod", settings = {good = true, bad = 11, ugly = "welcome" } }

[sfence]

Each world has a list of enabled mods. Why not set the order with that list with enabled mods at the top of the list with the user able to change that order to needs?

Slightly off-topic: It would be nice to have mod settings in that list as well instead of a just a boolean it could be something like:

just use the mod's default or game.conf settings: load_mod_some_mod = mods/some_mod

this world uses this mod in a particular way: load_mod_some_mod = { path= "mods/some_mod", settings = {good = true, bad = 11, ugly = "welcome" } }

I am afraid, it will be extremely complicated for the user to determine the correct loading order to satisfy all dependencies in a manually configured loading order list. In addition, this feature works with an enabled mods list. It changes only the logic of ordering that list by dependencies.
What is the expected advantage of changing which mod will be loaded as 9th in order for example?

[rubenwardy]

You should update src/unittest/test_servermodmanager.cpp to add a test that first_mod and last_mod are in the correct places. If you do this, you could maybe remove all the checks from mods inside devtest

[rubenwardy]

Adding to 5.9.0 as it reduces the impact of #14486

[sfence]

I have compiled Minetest on my MacBook M2 and run ./bin/minetest --run-unittests command and I do not see the problem from the GitHub Macos test.
So hopefully, it is not a real problem.

Will be nice if somebody checks it also on a MacBook with an Intel CPU.

[grorp]

@fluxionary why 👎?

So hopefully, it is not a real problem.

I pressed the "run again" button, who knows. But if that doesn't work, you'll have to fix CI either way.

[appgurueu]

Thanks for your patience.

[sfan5]

fwiw I think it's okay to use an empty string as an implicit null, avoid possible confusion between "" (often not valid) and std::nullopt

[sfence]

Looks like @appgurueu should answer this.

[appgurueu]

I could have left this as it was tbh, both are fine. Though since I already converted some to optionals, it improves consistency to also use optionals here. Strings are just used as mod "IDs" / "names" here. When I see a std::string in the context of a name / ID, I don't know if an empty string is allowed (a comment could help communicate this, but only by convention), and I can easily forget to handle the empty string properly.

std::optional<std::string> makes this pretty clear IMO if you know that the std::string is conceptually a name (and hence not supposed to be empty), though something like std::optional<NonEmptyString> would be better.

If we were to make this even cleaner, we could do std::optional<ModName> and using ModName = std::string;, and if we were to make this completely idiot-proof we could even have ModName be a type that enforces a valid, non-empty mod name (this would be overkill, though).

[sfan5]

std::optional<std::string> makes this pretty clear IMO if you know that the std::string is conceptually a name (and hence not supposed to be empty),

I would have interpreted that exactly opposite: someone went to the effort of adding std:optional<>, so there must be something special about "" that makes it different from a null value. maybe it's valid too?