ABMs: Make catch-up behaviour optional

[paramat]

Default is true for backwards compatibility
Update lua_api.txt

Requested by Oldcoder and celeron55, see discussion here http://irc.minetest.ru/minetest-dev/2015-10-04#i_4414381
I agree a bool is needed, in almost every use of ABMs in my mods i find the catch up feature causes problems.
This is intended to be backwards compatible such that old ABM definitions missing the bool have the catch up behaviour enabled. The default is catchup enabled.
Tested and seems to work.

[ShadowNinja]

I'm concerned that modders will forget to disable this -- resulting in performance hits. I think that you can change the default to false, since the only issue that will cause is some things not updating quickly -- which I see as a lesser issue compared to the performance issue.

[BlockMen]

@ShadowNinja since when don't we keep backwards compatibility? Older mods might expect/rely on this behavior.

[est31]

As I've said on IRC, I think we should have an enum for different catch-up types. Right now it should only have "simple" and "none", and with a fixed, proper, mode added later on. It should default to "simple" right now, and the proper catch-up once it get added.

[paramat]

In reply to discussion on IRC http://irc.minetest.ru/minetest-dev/2015-10-14#i_4422768
The bool is called 'simple catch up' because that was suggested by celeron55, it's not meant to imply a precise catch-up is intended for the future, it's because this is only a simple and often partial attempt at catch-up by modifying chance for a single interval cycle:

rubenwardy: catchup is misleading
rubenwardy: because it doesn't run multiple times, it just increases the chance of running
celeron55; if it could run multiple times, the server would practically just hang in many cases :P
////////////////////////////////////////////////////////////
est31: it should be made real catch up

A precise catch-up is not a good idea as that would be extremely heavy in some circumstances, and is not actually needed.

[paramat]

So, i feel if we have an enumeration it should be ABM "type" not catchup type. This would combine a catch-up bool with ABM type selection for new types of ABM in future.
But then, maybe better and more flexible is adding a flags field, this can be used for a catchup bool and expanded in future to select ABM types. It's more useful than enumeration because there can be multiple flags controlling different things.

[est31]

A precise catch-up is not a good idea as that would be extremely heavy in some circumstances, and is not actually needed.

Only because something is bad in some circumstances, we still can add an option for it.

I don't think a flags field is useful, we have a definition table, and can use that. Lua has no binary operators for flags fields. LuaJIT has, but not the builtin lua.

[ShadowNinja]

@BlockMen: Compatability in terms of the mod still running is important, compatability in terms of silghtly different behavior isn't. Besides, re-enabling it is only one line.
@est31: bitwise* :-P And Lua 5.3 has them. (LuaJIT and Lua 5.2 just have bitwise functions in a bundled library)

[est31]

@ShadowNinja I know, but bundled lua is 5.2.

Also, note that there is no real sense in making a flag field on the lua side. Its only read at startup, then converted into an internal structure. You can load hundreds of thousands of ABMs until the speed difference really starts to matter, and then we have more serious problems.

[ShadowNinja]

@est31: No, it's 5.1.

[kwolekr]

I dunno, thinking more about this whole matter, I'd have to say "simple" catch up is fine until there is an actual, demonstrated need for a non-emulated catchup, at which point we'll add that.

From an interface perspective I don't think it's actually too bad to tack on an additional boolean later, like "do_complete_catchup" that would imply "catchup = true" if set.

Let's try to keep this simple, yea?

[est31]

@kwolekr imagine cacti growing, or similar. Many people like to build farms to places close to spawn on servers, just because there they are visited more often, and can instantaneously harvest once they come back. If you disable catch-up, then you lose that feature. Or take furnaces. If you had no actual "real" catch-up here, you'd need to stay close to them the whole time they burn. Also many people want to farm things like flowers or grass, because its needed for crafting. All these things do need catch-up enabled. So one way would be to let modders implement catch up, for every ABM they use. But that would be very bad.

Now @paramat, please tell me why to disable catch-up. Where for example would you want to use it once its there?

[paramat]

Okay, flags fields are used elsewhere in API so i wrongly assumed they could be used here.
Reading IRC it seems an enumeration is messy too, so is this PR okay as it is using a bool?

The problem with true catch-up is for example if an area has been unvisited for a very long time, the attempt at true catchup would be insanely heavy and freeze everything. It's not really needed or even requested, we've only ever had a limited form of catch-up and been happy with that, true catch-up itself would need it's own form of limiting.

For basic lightweight use of ABMs catch-up is fine and wanted, but increasingly ABMs are now used as a trigger for all sorts of complex operations that would be too heavy to run at 100% chance, or would look very wrong to trigger 10s to 100s of times all at once, or that need to be triggered very rarely in all situations. Mob spawning is one as described by Oldcoder in the discussion with celeron55, they even considered if the default should be false (in this PR it is true for compatibility).
Most of the uses of ABMs in my mods are unusual uses where catch-up is not wanted.

[est31]

Can you talk about the uses in your mods? What you say right now is very generic, I have no idea what you mean with "complex operation" for example.

[ShadowNinja]

One way would be to let modders implement catch up, for every ABM they use. But that would be very bad.

It may be more code, but you can't just repeat the ABM because that would cause lockups if you've been away too long, and inaccuracies if you limit the ammount of calls. The only good way to handle this is for the mod to calculate what would have happened. We might be able to do some things to make this easier for modders to implement though.

[est31]

Yup, what I call "real" catchup isn't real catchup, its just tomatoe juice with tons of sugar... lol jk. I just want to call the abm function with an additional param of how often the ABM function would have been called. Things that are inter-dependent like plant growing won't work, but cacti get higher and, furnaces burn.

[paramat]

Rebased and changed field name in the definition from "catchup" to "catch_up".

[kwolekr]

👍

[paramat]

Sorry est31 i'm too busy to specifically describe my own uses.

[paramat]

Attempted a merge but the windows builds failed:

/minetest/src/threading/thread.cpp:170:10: error: expected initializer before '==' token
int ret == WaitForSingleObject(m_thread_handle, INFINITE);

/minetest/src/threading/thread.cpp:172:9: error: 'ret' was not declared in this scope
UNUSED(ret);

/minetest/src/threading/thread.cpp:31:34: note: in definition of macro 'UNUSED'
#define UNUSED(expr) do { (void)(expr); } while (0)

/minetest/src/threading/thread.cpp: In static member function 'static DWORD Thread::threadProc(LPVOID)':

/minetest/src/threading/thread.cpp:282:9: warning: converting to non-pointer type 'DWORD {aka long unsigned int}' from NULL [-Wconversion-null]
return NULL;

Those lines were altered by hmmmm's recent commit.

[paramat]

#3264 is the fix.

[est31]

Please don't merge yet, and wait until you are less busy. Its never good to do something in a rush.

[paramat]

Okay, if you want to discuss it more first.

[paramat]

3b9f99e

[paramat]

Hi @est31 i just can't be bothered to go into detail about my uses, this would mean searching through my mods, if you're interested you can study them yourself =} The PR was discussed for a few days and hmmmm gave approval so i merged it.

[paramat]

What i mean by 'complex operation' is a processing-heavy operation. If a heavy operation uses a high chance value (low chance) and a low interval (which is the best combination for heavy operations) then the catch-up feature will cause a very heavy processing spike as 'chance' is temporarily set to 1.

[est31]

@paramat i was right in the process of discussing a PR, and you just simply merged it. What if I had 👎ed it for some reason? I still was in the process of getting a final opinion on the PR.

About your hint that I can search through your mods: I've done that now, and seen that e.g. for the rain on dirt abm it would perhaps make sense to disable catch-up. Not because it would cause a "heavy processing spike" as you claim, but because it probably will spawn rain everywhere. So I even agree to this PR being merged.

Just PLEASE, next time be less rude and arrogant "i dont have time, search through my mods".

[paramat]

Hi, perhaps i should have explained more, i was tired and busy and couldn't remember specific examples, so it would mean a good half hour or more going through my mods. I felt the PR had more than enough justification and felt you were asking for too much. I added a smiley to try and emphasise i didn't mean it in an unpleasant way, i was just calmly writing the truth both times, it was not rudeness or arrogance. Then you CAPSed and ranted at me on IRC.

The PR was discussed and 2 devs, myself and hmmmm came to agreement on implementation and approved, i almost merged it but at the last minute you asked me to wait, so i waited a day, i was being very reasonable. Many PRs i am not happy with get merged quickly as soon as they have the required approval, it's just something a dev has to accept.

[est31]

As I've asked you to wait, I've meant you wait until you have time to discuss about the PR. It was irrelevant for me whether it's one day, one week, or two hours.

I have CAPSed because I was angry and felt you wanted to avoid discussion. Perhaps that was too extreme, I'm sorry.

If a dev (or anybody else) raises valid points why a PR shouldn't be merged, we should first try to address those before the PR does get merged.