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

sfence · 2023-12-28T15:48:26Z

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 · 2023-12-28T17:17:56Z

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

sfence · 2023-12-28T17:30:43Z

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 · 2023-12-28T17:42:09Z

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 · 2023-12-28T18:10:01Z

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?)

src/object_properties.cpp

src/object_properties.h

src/script/cpp_api/s_player.cpp

src/server/player_sao.cpp

sfence · 2023-12-29T10:46:11Z

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 · 2023-12-29T23:58:32Z

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.

src/script/cpp_api/s_player.cpp

sfence · 2024-02-05T18:06:50Z

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 · 2024-03-31T17:08:51Z

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.

doc/lua_api.md

Zughy · 2024-04-28T13:16:02Z

@sfence reminder

…e hurt.

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.

src/script/common/c_content.cpp

src/object_properties.h

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.

src/script/common/c_content.cpp

doc/lua_api.md

src/script/common/c_content.cpp

Co-authored-by: grorp <[email protected]>

grorp · 2024-05-08T18:48:18Z

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 · 2024-05-09T03:03:47Z

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 · 2024-05-09T06:07:33Z

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 · 2024-05-29T18:14:44Z

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 · 2024-05-29T18:34:14Z

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.

appgurueu

Tested, works:

enabling/disabling drowning works
enabling/disabling breathing works
enabling/disabling node damage works

I believe it would be cleaner for this to be a player-SAO field rather than an object property. I've sent you a pull request to refactor this appropriately (sfence#3). I'd also like to hear what @grorp thinks.

Besides this, I only have minor comments.

(There's an edge case I consider slightly odd, but which I don't think needs to be addressed (in particular because modders can just turn off the system entirely if they are unhappy with it): When you disable breathing and enable drowning, you only lose health while you are actively drowning. When you "stop drowning" - but stay at zero air - you do not lose health. Intuitively, this doesn't really make sense to me: When you have no air, shouldn't you continue to lose health?)

doc/lua_api.md

src/object_properties.cpp

src/object_properties.h

src/script/common/c_content.cpp

appgurueu · 2024-06-09T11:55:04Z

src/server/player_sao.cpp

@@ -156,7 +158,10 @@ void PlayerSAO::getStaticData(std::string * result) const

 void PlayerSAO::step(float dtime, bool send_recommended)
 {
- if (!isImmortal() && m_drowning_interval.step(dtime, 2.0f)) {
+ bool not_immortal = !isImmortal();


How about mortal? :)

sfence · 2024-06-10T03:12:11Z

@appgurueu

I believe it would be cleaner for this to be a player-SAO field rather than an object property. I've sent you a pull request to refactor this appropriately (sfence#3). I'd also like to hear what @grorp thinks.

I am planning to use the same field for fix #9372. In that case, is it better to have two separate SAO fields in player-SAO and entity-SAO?

appgurueu · 2024-06-10T11:30:35Z

I am planning to use the same field for fix #9372. In that case, is it better to have two separate SAO fields in player-SAO and entity-SAO?

Commented. I do not support #9372. I'd like to wait for other opinions on it so we can make a decision.

Breathing & drowning is exclusive (and I would say should be exclusive to) players; if modders want similar mechanics for their entities, they can and should implement them themselves. The breathing & drowning flags at least should be player-only. Entities having but not using them is hacky.

Even if it was decided that we support #9372, this would, unless client-side prediction is added (which it probably won't and shouldn't be), still make more sense as a SAO-only property. It does not belong in object properties. It's better not to bloat object properties unnecessarily.

Refactor (from object property to PlayerSAO flags)

sfence · 2024-06-19T17:22:03Z

@grorp I take @appgurueu ideas into this PR. So please recheck your approval.

appgurueu · 2024-06-19T20:29:02Z

I'm happy with this now; it has my approval. grorp can mark this as "two approvals" if he's happy too. Since we're in feature freeze, I've put this on the 5.10 milestone.

grorp · 2024-06-23T13:00:21Z

Looks alright.

appgurueu reviewed Dec 28, 2023

View reviewed changes

src/object_properties.cpp Outdated Show resolved Hide resolved

src/object_properties.h Outdated Show resolved Hide resolved

src/script/cpp_api/s_player.cpp Outdated Show resolved Hide resolved

src/server/player_sao.cpp Outdated Show resolved Hide resolved

wsor4035 added the @ Script API label Dec 28, 2023

Zughy added Action / change needed Code still needs changes (PR) / more information requested (Issues) Feature ✨ PRs that add or enhance a feature Concept approved Approved by a core dev: PRs welcomed! labels Jan 22, 2024

sfence force-pushed the sfence_breath_hp branch from 19fc22a to b8ed6bd Compare January 27, 2024 22:16

grorp requested changes Feb 4, 2024

View reviewed changes

src/script/cpp_api/s_player.cpp Outdated Show resolved Hide resolved

grorp self-requested a review February 14, 2024 15:19

Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 14, 2024

grorp reviewed Mar 31, 2024

View reviewed changes

doc/lua_api.md Outdated Show resolved Hide resolved

grorp self-assigned this Mar 31, 2024

grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 31, 2024

sfence added 2 commits May 6, 2024 06:44

Allow to disable parts of engine logic as breathing, drowning and nod…

eb3fef7

…e hurt.

More user/moder friendly engine mask.

e2955f1

sfence force-pushed the sfence_breath_hp branch from b8ed6bd to e2955f1 Compare May 6, 2024 07:22

Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 6, 2024

grorp self-requested a review May 6, 2024 17:53

grorp reviewed May 7, 2024

View reviewed changes

grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 7, 2024

sfence added 2 commits May 7, 2024 16:39

Apply feedback.

72aea4b

Apply feedback.

88f2eb1

Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 7, 2024

grorp reviewed May 8, 2024

View reviewed changes

src/script/common/c_content.cpp Outdated Show resolved Hide resolved

doc/lua_api.md Outdated Show resolved Hide resolved

src/script/common/c_content.cpp Outdated Show resolved Hide resolved

grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 8, 2024

sfence and others added 2 commits May 8, 2024 16:20

Update doc/lua_api.md

df6a1fb

Co-authored-by: grorp <[email protected]>

Apply review.

b3b89bc

Update read_engine_mask logic.

4a2cd38

grorp approved these changes May 9, 2024

View reviewed changes

grorp added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels May 9, 2024

Refactor (from object property to PlayerSAO flags)

05af188

appgurueu reviewed Jun 9, 2024

View reviewed changes

appgurueu mentioned this pull request Jun 10, 2024

Enable node damage for LuaEntities #9372

Open

Merge pull request #3 from appgurueu/sfence_breath_hp++

f1c6a38

Refactor (from object property to PlayerSAO flags)

appgurueu removed the One approval ✅ ◻️ label Jun 19, 2024

appgurueu added this to the 5.10.0 milestone Jun 19, 2024

Bye blank line

68a4df7

appgurueu added the One approval ✅ ◻️ label Jun 19, 2024

grorp added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sfence commented Dec 28, 2023 •

edited

Loading

lhofhansl commented Dec 28, 2023

sfence commented Dec 28, 2023

Zughy commented Dec 28, 2023

sfence commented Dec 28, 2023

appgurueu left a comment •

edited

Loading

sfence commented Dec 29, 2023

lhofhansl commented Dec 29, 2023 •

edited

Loading

grorp left a comment

sfence commented Feb 5, 2024

grorp commented Mar 31, 2024

Zughy commented Apr 28, 2024

grorp left a comment

grorp left a comment

grorp commented May 8, 2024

sfence commented May 9, 2024

sfence commented May 9, 2024

grorp left a comment

SmallJoker commented May 29, 2024

sfence commented May 29, 2024

appgurueu left a comment

appgurueu Jun 9, 2024

sfence commented Jun 10, 2024

appgurueu commented Jun 10, 2024

sfence commented Jun 19, 2024

appgurueu commented Jun 19, 2024

grorp commented Jun 23, 2024

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

Are you sure you want to change the base?

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

Conversation

sfence commented Dec 28, 2023 • edited Loading

To do

How to test

lhofhansl commented Dec 28, 2023

sfence commented Dec 28, 2023

Zughy commented Dec 28, 2023

sfence commented Dec 28, 2023

appgurueu left a comment • edited Loading

Choose a reason for hiding this comment

sfence commented Dec 29, 2023

lhofhansl commented Dec 29, 2023 • edited Loading

grorp left a comment

Choose a reason for hiding this comment

sfence commented Feb 5, 2024

grorp commented Mar 31, 2024

Zughy commented Apr 28, 2024

grorp left a comment

Choose a reason for hiding this comment

grorp left a comment

Choose a reason for hiding this comment

grorp commented May 8, 2024

sfence commented May 9, 2024

sfence commented May 9, 2024

grorp left a comment

Choose a reason for hiding this comment

SmallJoker commented May 29, 2024

sfence commented May 29, 2024

appgurueu left a comment

Choose a reason for hiding this comment

appgurueu Jun 9, 2024

Choose a reason for hiding this comment

sfence commented Jun 10, 2024

appgurueu commented Jun 10, 2024

sfence commented Jun 19, 2024

appgurueu commented Jun 19, 2024

grorp commented Jun 23, 2024

sfence commented Dec 28, 2023 •

edited

Loading

appgurueu left a comment •

edited

Loading

lhofhansl commented Dec 29, 2023 •

edited

Loading