Add possibility to easier override HP and breath engine logic by Lua.

[sfence]

• Add the possibility to disable C++-based HP and breath logic.
• Add per player mask which can be used to enable/disable
• This fixes issue Allow Mods To Disable Breath Regen #8042.

To do

This PR is a Ready for Review

• Get feedback
• Apply feedback
• Add documentation

How to test

Run devtest game and change the engine_mask value via the included item for properties change.

[lhofhansl]

Wasn't there some discussion to move the entire logic to Lua?

[sfence]

Wasn't there some discussion to move the entire logic to Lua?

Yes, in #14125.
But this solution should target to allow full backward compatibility and moving logic to Lua as well.
I also think that the C++ solution has better performance than the Lua alternative. So, why not keep it where it is possible?

[Zughy]

So, why not keep it where it is possible?

Because, contrary to HPs, it's not something the majority of games need. It should be eradicated from the engine imho, it's only a burden for core devs

[sfence]

Because, contrary to HPs, it's not something the majority of games need. It should be eradicated from the engine imho, it's only a burden for core devs

I understand.

But, before some time I created some mod that was doing something like breath in Lua. The problem with it was, that it did not work smoothly because of the non-constant time difference between steps.

So, I think that the use of C++ HP and breath engine whatever is possible will look better from the player's side as a result.

[appgurueu]

I think this PR is overkill for de-hardcoding breath; all you need for that would be a simple (player-only) flag that tells the engine to stop managing breath so that the modder can take over. (In fact, this PR goes a bit in the opposite direction of #14125 even IMO: It adds breath to entities, and complicates it by adding more features such as controllable intervals.)

I would suggest splitting this up into a PR which lets you disable engine breath handling for players, which will basically fix #8042 and half of #14125 and should be rather uncontroversial, and another PR for all the features, esp. adding node damage to entities etc. (which I'm frankly not sure belongs into the engine).

So, I think that the use of C++ HP and breath engine whatever is possible will look better from the player's side as a result.

I'm not so sure about this. If I see this correctly, this PR is entirely serverside and uses no server features not available to Lua, so why would it have clientside prediction advantages over a Lua-only solution, which would be entirely serverside as well?
(By the way, I think breath was moved to the server to combat cheating a rather long while ago?)

[sfence]

So, it looks like the idea of using a mask to add the ability to partially disable SAO damage/breath logic in combination with Lua callback does not sound interesting for Minetest developers.

So, this idea is probably a step aside.

So, the best way is probably to abandon this idea of a solution. Am I right?

[lhofhansl]

So, the best way is probably to abandon this idea of a solution. Am I right?

Not the idea I like that.
Maybe a bit simpler... I really like @appgurueu suggestion above. Add a flag to disable engine's breath management, and separate for the Lua API.
I think for now we can leave the C++ logic in place, and perhaps remove it later.

Please do not be discouraged, we like contributions that make modding easier/possible.

[grorp]

This PR still contains the change to add node damage to entities. That doesn't belong in this PR and breaks backwards compatibility if enabled by default. Please remove it.

[sfence]

This PR still contains the change to add node damage to entities. That doesn't belong in this PR and breaks backwards compatibility if enabled by default. Please remove it.

It is related to bug #9372. But yes, it breaks back compatibility. Will it be satisfying to disable it by default?
So backward compatibility will not be broken and #9372 will be also fixed with this PR.

[grorp]

Well, actually I'd be fine with adding node damage to entities as long as it's disabled by default. It would save modders from having to write their own node damage reimplementations in case they just want entities to behave the same as players. However, this seems to be a controversial idea considering the comments in #9372. Maybe it would be better to keep this PR simple and uncontroversial by just including the "allow disabling hardcoded engine features" change.

[Zughy]

@sfence reminder

[grorp]

The new table-based form for engine_mask isn't documented. Also, is there a reason for exposing the underlying bitmask to the Lua API by allowing to specify engine_mask as a number?

I again suggest the https://dev.minetest.net/Code_style_guidelines, for example this:

When breaking conditionals, indent following lines of the conditional with two tabs and the statement body with one tab.

[grorp]

I'm not sure about the naming, but I currently have no better naming proposal.

Tested. The default value behavior didn't match my expectations. When not specifying engine_mask, all flags default to true. When specifying engine_mask as {breathing = false}, the other flags default to false, not to true as I would expect. Defaulting to true in this case would also result in better forward-compatibility if more flags are added in the future.

[grorp]

Tested. The default value behavior didn't match my expectations. When not specifying engine_mask, all flags default to true. When specifying engine_mask as {breathing = false}, the other flags default to false, not to true as I would expect. Defaulting to true in this case would also result in better forward-compatibility if more flags are added in the future.

Since you replied to everything else, I'm repeating this in case you missed it.

[sfence]

Tested. The default value behavior didn't match my expectations. When not specifying engine_mask, all flags default to true. When specifying engine_mask as {breathing = false}, the other flags default to false, not to true as I would expect. Defaulting to true in this case would also result in better forward-compatibility if more flags are added in the future.

In Lua nil and false are commonly expected to have similar/same effect. So it can be confusing to replace nil with true.
Maybe I can change the logic of the engine_mask table not to enable part of engine logic, but to disable part of engine logic.

engine_logic = {
  no_breathing = true,
   ...
}

Does it sound better?

[sfence]

Ok, nil is used with no change meaning in other parts of the Lua API tool. So it should be understandable. The previous version of read_engine_mask was also potentially buggy, so it should be better now.

[grorp]

Works as expected now.

I think defaulting to the old values makes sense because that's how top-level object properties work too.

[SmallJoker]

I like this PR and was about to submit my review when I got an idea. Wouldn't it be more helpful to control these damage ticks with floats for each? That would allow arbitrary delays, or also disabling it with values < 0.

[sfence]

I like this PR and was about to submit my review when I got an idea. Wouldn't it be more helpful to control these damage ticks with floats for each? That would allow arbitrary delays, or also disabling it with values < 0.

This possibility was included but rejected. So I removed it.

After this PR, I want to create PR for the player on_step callback. It will add the possibility to implement custom delays, breath/damage, and similar logic in the player on_step Lua callback.