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

Allow registering, removing and modifying LBMs and ABMs on the fly #13112

Open
kromka-chleba opened this issue Jan 4, 2023 · 12 comments
Open
Labels
Feature request Issues that request the addition or enhancement of a feature @ Script API @ Server / Client / Env.

Comments

@kromka-chleba
Copy link
Contributor

Problem

I'm trying to implement seasonal changes for Exile and for that I would like to have an LBM that updates soil from normal spring form (nice, green) to winter form (brown, dark) when it's winter, but then change it back when spring comes. I can't have two LBMs because they start fighting so I tried overwriting the LBM on the fly which was not successful.
I successfully (partially) overwrote an ABM using the following procedure, but 1. ABMs don't do the job as effectively as LBMs 2. it's quite hacky:

local function override_abm(abm_spec)
    for i = 1, #minetest.registered_abms do
        if minetest.registered_abms[i].name == abm_spec.name then
            minetest.registered_abms[i].name = ""
            minetest.registered_abms[i] = abm_spec
            break
        end
    end
end

This one only manages to overwrite the "action" function of the ABM, but nothing more.
Using this function I was able to kind of silence the ABMs by setting action to an empty function.
Another problem is that the effect is not always reproducible.

Solution

Adding an option to delete, register and/or modify ABMs and LBMs on the fly (during game runtime) would help me achieve the goal and allow for more flexibility in modding generally.

@kromka-chleba kromka-chleba added the Feature request Issues that request the addition or enhancement of a feature label Jan 4, 2023
@NathanSalapat
Copy link
Contributor

Rather than trying to overwrite an LBM or ABM why not introduce some sort of timekeeping mechanic that determines the season, and then run a single LBM that varies what it changes based on the season.
The mymonths mod does something of this nature, storing a calendar with the days/months, and then using that data to run specific parts of code to make seasonal changes.

@kromka-chleba
Copy link
Contributor Author

@NathanSalapat

some sort of timekeeping mechanic that determines the season

I already have this.

and then run a single LBM that varies what it changes based on the season.

I have two groups: spreading (for normal soils) and winter_soil for soils in winter. I tried writing an LBM that has nodenames = {"group:spreading", "group:winter_soil"} and checks the group of the node and the season and changes soil accordingly. But for some reason that didn't work - the LBM was changing entire chunks of soil from spring to winter and back. I don't know if it was a mistake on my side or the engine's weirdness.
Have you tried doing something like this?

The mymonths mod does something of this nature, storing a calendar with the days/months, and then using that data to run specific parts of code to make seasonal changes.

I should take a look then. Still unregistering and changing ABMs and LBMs would be more convenient and probably more efficient.

@kromka-chleba
Copy link
Contributor Author

@NathanSalapat

Hmmm, looking at my code for the fifth time and I spotted that run_at_every_load = false is set for the second one.
I fixed this bug and now these LBMs work, thank you for your suggestion.

I still think that overwriting LBMs would be useful for modding - right now I have one LBM running all the time and checking for season, this isn't efficient.

local winter_soil_lbm = {
    label = "Changes soils to winter soils.",
    name = "nodes_nature:winter_soil_lbm",
    nodenames = {"group:spreading"},
    run_at_every_load = true,
    action = function(pos, node, dtime_s)
        local season_name = seasons.get_season_name()
        if season_name == "winter_early" or
            season_name == "winter_late" then
            minetest.log("error", "winter soil lbm!")
            soil_to_winter(pos, node)
        end
    end,
}

local non_winter_soil_lbm = {
    label = "Changes winter soils to soils.",
    name = "nodes_nature:non_winter_soil_lbm",
    nodenames = {"group:winter_soil"},
    run_at_every_load = false,
    action = function(pos, node, dtime_s)
        local season_name = seasons.get_season_name()
        if not (season_name == "winter_early" or
                season_name == "winter_late") then
            minetest.log("error", "non-winter soil lbm!")
            soil_to_non_winter(pos, node)
        end
    end,
}

@NathanSalapat
Copy link
Contributor

The lbms only run when a mapblock is loaded so a single check for the season isn't all that costly.

@jeremyshannon
Copy link
Contributor

But it does iterate over every node in the group for that mapblock, right? The LBM function runs on one pos at a time, but operates on all qualifying nodes when they're loaded in.

I'd also like to modify Exile's rain_soak ABM to shut off when there's no rain, and apply varying levels of chance and interval depending on rainfall intensity, but since the ABM def can't be modified, I would have to always run the most intensive version of the ABM (highest chance, shortest interval) and just check for rainfall and exit most of the time, which means lots of wasted power looking at dry dirt nodes that are getting 0 rainfall.

ABMs are a major source of bottlenecking in my experience, being able to shut them off or adjust them would be a big help.

@TurkeyMcMac
Copy link
Contributor

Perhaps this could be resolved by adding minetest.register_on_mapblock_activated and minetest.register_on_mapblock_deactivated (or minetest.get_active_mapblocks.) This would allow two things:

  1. More flexible LBMs.
  2. Keeping a list of active blocks to implement more flexible ABMs.

@kromka-chleba
Copy link
Contributor Author

Perhaps this could be resolved by adding minetest.register_on_mapblock_activated and minetest.register_on_mapblock_deactivated (or minetest.get_active_mapblocks.) This would allow two things:

1. More flexible LBMs.

2. Keeping a list of active blocks to implement more flexible ABMs.

As long as this lets me change things dynamically and results in better performance then I'm in.
What kind of arguments would these functions take if they were to be implemented?

@kromka-chleba
Copy link
Contributor Author

@NathanSalapat

The lbms only run when a mapblock is loaded so a single check for the season isn't all that costly.

We actually deployed these LBMs on our dev server and together with the rain soak ABM they cause massive lags so I think it's quite expensive. We had only 5 players on the server. So the issue still stands.
On a side note, the next major release of Exile is going to have these seasons.

@nephele-gh
Copy link

I still think that overwriting LBMs would be useful for modding - right now I have one LBM running all the time and checking for season, this isn't efficient.

Instead you can have the winter function and spring function in two local variables, and have the one you actuall run in a third one. Once the seasons change you simply switch out the variable and then next time the other function will run, which it now points to.

This would be equivalent to changing out the abms I think.

If the season is local data it's probably not that expensive though :)

@jeremyshannon
Copy link
Contributor

The problem there is the same, it's still running the ABM/LBM on every last season-aware node on every interval. You can't tell it "don't give me the summer-themed nodes during summer, please" because that requires altering which nodes the *BM runs on, and the *BM itself is cached and immutable during runtime.
The only way to get it to only run on the nodes that need changing this season is to have an *BM set to only run on that group, which means now you have four ABM/LBM sets, and you can't turn off/tune down the ones that shouldn't be running this season.

I'm also not clear on how "register_on_mapblock_activated" functions would be an improvement performance-wise, as any such function would now have to search each mapblock in Lua for its valid nodes, instead of letting the C side of things pick them out. Best case scenario would seem to be identical performance, with average case being worse. Though perhaps fewer context switches between C++ and Lua would help?

@ancientmarinerdev
Copy link

ancientmarinerdev commented Mar 27, 2023

The problem there is the same, it's still running the ABM/LBM on every last season-aware node on every interval. You can't tell it "don't give me the summer-themed nodes during summer, please" because that requires altering which nodes the *BM runs on, and the *BM itself is cached and immutable during runtime. The only way to get it to only run on the nodes that need changing this season is to have an *BM set to only run on that group, which means now you have four ABM/LBM sets, and you can't turn off/tune down the ones that shouldn't be running this season.

Can you not get the expected value based on season, and check the node is correctly set, only set it, if it isn't correctly set? We had a similar LBM for grass_pallete param2 fixes. We would only set the node, if it wasnt correctly set.

So LBM runs on all nodes, checks season and expected value which is conditional based on your logic, and the code will only reset it if it isn't already set.

ABM's feel the wrong tool for the job, and too frequent. You only want this to happen once on load, not regularly, and ABM's get ditched if too heavy.

@jeremyshannon
Copy link
Contributor

The trouble is with it running on all nodes. And yes LBM is generally what you want, but LBM alone is insufficient to the task, since it only updates newly loaded nodes. (or supposedly reloaded ones as well with always_run, but kromka-chieba ran into some weird behavior with that) That means that nodes near the player will never change season if the they hang around an area (players commonly turtle through winter in Exile, as conditions can be brutal outside) so you need both an LBM AND an ABM to correctly transition to the next season.

The trouble is the LBM and ABM have to run on all spring, summer, autumn, and winter nodes, all year round. We can't turn off the "turn winter to spring" part during winter, or the "summer to autumn" part when it's autumn, all those nodes get checked on every load and continuously by the ABM as the player is walking around. Even if it's only saying "nope, it's currently winter, so we do nothing with this node" it's got to do that on thousands of nodes, all the time, because there's no functional way to switch an *BM off or turn it down.

(It's possible to edit the definition during runtime, but as the definitions are cached and there's no way to update that cache, it does nothing. As a semi-related side note, a great deal of effort could be saved on our end if we could force-update the texture cache to replace node textures with seasonal variants. Not every node needs to change shape or behavior, many just change color.
This would let us turn all grassy nodes at once, everywhere, without fiddling around with *BMs. Just leaving the nodes in place and instructing Minetest to reload the texture for those nodes, would be ideal. Minetest's architecture does not allow for this at the moment.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Issues that request the addition or enhancement of a feature @ Script API @ Server / Client / Env.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants