[no sq] LBM-related bug fixes

[sfan5]

see commit messages for more details

closes #15902

To do

This PR is Ready for Review.

How to test

if core.get_modpath("basenodes") and core.get_mapgen_setting("mg_name") == "singlenode" then
	core.register_on_joinplayer(function()
		core.chat_send_all("LBM was registered")
	end)
	core.register_lbm({
		name = ":lab:reliability_test",
		nodenames = {"basenodes:dirt_with_grass"},
		bulk_action = function(pos_list)
			print("swapping grass in " .. core.pos_to_string(vector.floor(vector.divide(pos_list[1], 16))))
			core.bulk_swap_node(pos_list, {name="basenodes:stone"})
		end,
	})
end

1. create a new devtest world
2. exit the game, change mapgen to singlenode
3. rejoin game and see. ensure no grass is left (if you get close)

[ryvnf]

Works. Tested using the reproduction steps in #15902 which uses a mineclonia branch

[ryvnf]

Would you be willing to add the run_on_unknown flag to the LBM definition like you suggested in our previous discussion #15902 (comment)?

I made a workaround for Mineclonia which uses ABMs to detect if a mapblock is likely to be outdated and updates it in bulk if that is the case. It would however be good if we could remove that workaround in the future when people are running fixed Luanti versions. Without such a flag we will not be able to stay compatible with old worlds without keeping the ABM in there forever (which has a constant performance impact even if it is small).

[sfan5]

It's kind of a kludge so I was hoping to avoid it.
Is there any reason why you didn't use an LBM with run_at_every_load? An ABM seems totally unnecessary for this.

[ryvnf]

It's kind of a kludge so I was hoping to avoid it.
Is there any reason why you didn't use an LBM with run_at_every_load? An ABM seems totally unnecessary for this.

My reasons for not using run_at_every_load are:

1. The goal is to add a random "4dir" and "color4dir" rotation to nodes (to make terrain appear less grid-like). Using run_at_every_load would result in nodes being rotated on every activation
2. This needs to be performed on a lot nodes (most notably grass and snow). Using run_at_every_load will have a significant performance impact when players are moving around and activating mapblocks while doing so

The ABM gets around these issues by checking if a node it is triggered on is likely a mapblock for which the LBM has not run correctly (neighbouring nodes all have the same rotation), and updates the whole mapblock if that is the case. It does not need to be triggered for every node and can have a higher chance and interval which makes it more lightweight. My co-maintainer of Mineclonia is still skeptical towards adding it though because the ABM cannot be removed, even after the bug is fixed.

I also expect other games to encounter similar problems. VoxeLibre for example plans to lower the bedrock layer to increase world depth. When we did this in Mineclonia we ended up using ABMs with min/max_y because we found LBMs to be unreliable (at the time we did not investigate if this was due to an engine bug). Having such a flag would allow VoxeLibre be able to do that with LBMs which are more natural. They may also be interested in porting the grass changes I am currently working on in the future. Other games/mods may also want to use similar LBMs to for example add grass/leaves colors depending on heat/humidity. Without a run_on_unknown flag they will be forced to come up with similar workarounds if they want old maps to convert reliably.

So I do think it is worth adding a flag for this, even though it is not pretty and understand being hesitant about it. It does not feel good if games/mods are required to implement workarounds that introduce an ever present performance overhead that cannot ever be removed.

[ryvnf]

I am not sure if this would be preferable, but an alternative would be to add a setting which makes all LBMs run on mapblocks without a timestamp. Games that want this behavior (and are aware of possible consequences) can enable it in their minetest.conf. That avoids having to extend the API, but has the tradeoff that it cannot be enabled on an individual basis.

Maybe it would be possible to hide such a setting from the settings menu (as it does not have much relevance there).

[SmallJoker]

Looks good overall.

[sfan5]

1. Using run_at_every_load would result in nodes being rotated on every activation

I assumed you had a way to detect if a particular grass node has already been rotated.

2. Using run_at_every_load will have a significant performance impact when players are moving around and activating mapblocks while doing so

Bulk LBMs alleviate the per-node overhead to a large degree.
You also have to consider that ABMs will run continuously for all grass nodes in active blocks even if you have already fixed them up. Compared to that checking all grass upon activation once should surely be more efficient.

[SmallJoker]

Tested according to the test script. Works. The mapblocks are updated reliably as they get activated by approaching them.

I also tested the opposite case when the LBM was in theory already run on a mapblock (where its modification date is newer than the LBM introduction).
Instructions:

1. Created a new world, generated some mapblocks
2. Changed mg_name in map_meta.txt to singlenode
3. Enabled the LBM mod and joined to convert a few mapblocks
4. World exit
5. Changed env_meta.txt
   • from lbm_introduction_times = lab:reliability_test~31;
   • to lbm_introduction_times = lab:reliability_test~6; (move to past)
6. World rejoin
7. The other mapblocks don't get updated.

Works as-is.

[ryvnf]

I assumed you had a way to detect if a particular grass node has already been rotated.

Not really. I can only check if it has probably not been rotated by comparing the param2 value to that of its neighbors.

[sfan5]

How does your ABM solution avoid rotating grass multiple times then?

[ryvnf]

How does your ABM solution avoid rotating grass multiple times then?

It does it the same way – it checks if at least 4 neighbours have the same rotation. My thinking was that the ABM is executed pretty rarely and only need to trigger on one node in a mapblock to update the whole thing. My co-maintainer cora found it questionable for something that is purely visual though, so we will not be using that workaround.

One could use bulk LBMs to do a similar check since you get all positions. Basically check if the first 5 positions or so have the same rotation. That may be better than what I proposed with ABMs. It is still a pretty hacky solution though and not ideal.

[ryvnf]

I have also realized that even if we may use a run_on_every_load workaround in this case (I have not check how costly it will be), we will probably get into serious performance issues in the future when adding biome coloring for water. We have planned to do that after #12907 is merged.

If a player rides a boat over a deep ocean they will probably activate several thousand water nodes per second. So I think adding run_on_unknown or what I proposed with a minetest.conf setting may be warranted. I can understand being hesitant to add it as I have been the only one to request it so far, but I am pretty confident other people will benefit too (like VoxeLibre when they do their bedrock lowering).

Would you be opposed to me making a PR for run_on_unknown so it can be discussed? I would be fine if it gets rejected afterwards but I would like to give it a try and do my best to convince the core devs it is worth it.

[sfan5]

If a player rides a boat over a deep ocean they will probably activate several thousand water nodes per second.

Have you tested if an ABM is really faster than a bulk LBM? I think your intuition for what is going to be slow/fast is wrong in this case.

Would you be opposed to me making a PR for run_on_unknown so it can be discussed? I would be fine if it gets rejected afterwards but I would like to give it a try and do my best to convince the core devs it is worth it.

Sure, feel free to contribute a PR. My main concern is that I didn't want to spend time writing the code myself.

[ryvnf]

Have you tested if an ABM is really faster than a bulk LBM? I think your intuition for what is going to be slow/fast is wrong in this case.

I think you are right. My ABM workaround was also made before I realized bulk LBMs could be used instead of individual ones.

Sure, feel free to contribute a PR. My main concern is that I spend time writing the code myself.

I will look into it then. Also thanks for looking into this issue so quickly.