Add more information in PlayerHPChangeReason

[Wuzzy2]

WARNING: This PR currently contains an experimental commit! Do NOT merge yet. Read #16024 (comment) for details.

---

This PR adds more details to the PlayerHPChangeReason and improves the documentation. Specifically, it does the following:

1. Add node and node_pos field for the drown field (analog to node_damage type)
2. Add detail field to allow a standardized way to report more details of the HP change
3. Make builtin report a detail for the HP changes caused by the /kill command and core.do_item_eat
4. Move the documentation of PlayerHPChangeReason to its own section
5. Fill in missing information of said documentation

Read the documentation updates for details.

Rationale

I feel like adding the node and node pos for drowning is a no-brainer, given we already have the same fields for node_damage. This is probably the least controversial change.

I added detail because our current way to report more information was just us saying "mods, invent your custom field" which wasn't ideal. The new documentation requires this to be a string (to keep it simple) and suggests a modname:detailtext naming convention, with builtin using __builtin:kill_command and __builtin:item_eat. detail is of course optional.

More annoyingly, the /kill command and the core.do_item_eat HP change currently do not provide any reason for the HP change. This frustrated me when working on a death messages mod (= a mod printing a death message in chat, along with reason) and there was no way to detect a /kill and core.do_item_eat HP change cleanly without injecting or overwriting stuff. Which works, but is ugly code-wise. Now that the reason is provided by default, this should allow for cleaner mod code.

I believe this PR should now cover all possible HP change events caused by the engine and builtin.

How to test

Create a dummy mod with the following code:

core.register_on_player_hpchange(function(player, hp_change, reason)
       minetest.log("error", "HP CHANGE for "..player:get_player_name().." "..dump(hp_change).." HP; reason="..dump(reason))
end)

Then start a DevTest world with this mod activated. Now go drowning, try the /kill command or eat the apple and look at the output in chat. Then, try out other health-changing events to make sure all the other HP changes still work like they should.

I did some of those tests myself and it looks fine, but please do it on your own as well. Thanks.

[rubenwardy]

detail feels quite vague as a name - how about calling this custom_type?

Or perhaps the type field could be a string rather than enum and allow custom types if they follow the modname:typename convention. This feels quite nice, although idk if this would confuse some existing mods

[sfence]

What about die_reason ?

[rubenwardy]

it's HP change and not necessarily death. As for reason, this entire table is hp change reason

[sfence]

True. So something like change_reason or change_type.

[grorp]

True. So something like change_reason or change_type.

There's an existing field reason.type (where reason is of type PlayerHPChangeReason). Adding a new field reason.change_reason or reason.change_type wouldn't make sense in terms of consistency

[sfence]

reason.event = __builtin:fall" etc...

[grorp]

Thinking about this again: detail has the same purpose as type. If we were the designing a new API, they would be the same field. However, there is backwards compatibility to take into account here.

The ideal option would be to add to new possible enum (string) values, or just fully custom game/mod-provided string values, to PlayerHPChangeReason.type.

If we did that, it would break mods which assume that only the enum values specified in the documentation occur, and that the enum is never extended. I don't think our backwards compatibility guarantee should include "enums are never extended". See also luanti-org/docs.luanti.org#223.

Looking at CTF's kill log and appgurueu's "Kill History", for example, they handle unknown types properly (although the latter is broken with some modlib-related error).

It could also break mods that somehow override core.item_eat or the /kill command to set some internal state, and then check for type == "set_hp" and their custom state in core.register_on_player_hpchange to figure out where damage is coming from. This would be quite hacky, and not something officially supported.

So I think extending type would be acceptable, and is what I suggest as it would be the cleanest API.

The other option would be to keep this as custom_type or similar, say "meant to be used with type == "set_hp"" and "not included in type for legacy reasons".

[Wuzzy2]

I'm waiting for the opinion of other core devs before I change the detail field. I'm open to change the name tho.

But I'm not sure if updating the type field is a good idea.

[sfence]

Maybe __builtin:fall should be also added with this context.

[Wuzzy2]

@sfence: We already have a fall type so I guess it would be redundant. Or what do you mean?

[sfence]

I think we can remove fields as fall, drown, etc., in the future. If there is a type stored as text in some field.
Sounds like a more flexible solution to me, if you can determine the reason from one field without having to check multiple different fields.

[grorp]

Thinking about this again: detail has the same purpose as type. If we were the designing a new API, they would be the same field. However, there is backwards compatibility to take into account here.

The ideal option would be to add to new possible enum (string) values, or just fully custom game/mod-provided string values, to PlayerHPChangeReason.type.

If we did that, it would break mods which assume that only the enum values specified in the documentation occur, and that the enum is never extended. I don't think our backwards compatibility guarantee should include "enums are never extended". See also luanti-org/docs.luanti.org#223.

Looking at CTF's kill log and appgurueu's "Kill History", for example, they handle unknown types properly (although the latter is broken with some modlib-related error).

It could also break mods that somehow override core.item_eat or the /kill command to set some internal state, and then check for type == "set_hp" and their custom state in core.register_on_player_hpchange to figure out where damage is coming from. This would be quite hacky, and not something officially supported.

So I think extending type would be acceptable, and is what I suggest as it would be the cleanest API.

The other option would be to keep this as custom_type or similar, say "meant to be used with type == "set_hp"" and "not included in type for legacy reasons".

[Wuzzy2]

@grorp: So I have thought about your suggestion about extending type.

So one way to do it by just adding the types item_eat and kill_command for the two special builtin events, and delete detail entirely. That would satisfy your request.

In fact, I've pushed an "experimental" commit for just that. Please tell me if this is OK (just for concept approval).

I'm not sure if I'm very happy about that because mods that previously relied on type=="set_hp" to also capture the kill command & core.do_item_eat will no longer capture it. This is precisely the reason why I introduced detail.

I dislike your approach (but pushed an experimental commit anyway) because it effectively changes the meaning of the set_hp type. In comparison, I think the detail doesn't break anything, as it is information that lives on top the regular type without changing it. So existing mods will continue to work perfectly, they will just ignore the detail field.

Frankly, if I have to pick between a slightly more verbose API, and a backwards-compatibiltiy-breaking API (plus, in a normal release!), I pick the verbose API.

I am ok with the suggestion of renaming the detail field tho.

[appgurueu]

Two cents:

1. If we are to make this game-customizable, we can simply let mods set the type. This does not constitute a breaking change: Games will continue to work. Only if you introduce a new mod will old mods have to handle the unexpected type, which is fine: Indeed they might have to decide what to do about it. Either way at that point it's not our problem, but rather a game / mod dev problem as it probably should be.
2. If we are to make the engine set particular types, and want to use the type field for that, that would be a minor breaking change. It may be hard to estimate the impact, but I would be somewhat optimistic that mods deal with unrecognized types properly (as grorp said above), either because they only care about some types or because they use a sensible default catch-all. (And if not they're at least easy to upgrade.) That said I don't deem it as important: Indeed things like item eat or the kill command should be (re)implemented by games. They shouldn't have been in the "engine" to begin with. I don't deem it important to extend them. If a game dev needs a more specific type, they can override them.

Long story short: For the purpose of dehardcoding, it suffices to let modders set the type, which is backwards compatible. I don't consider the engine's game-specific logic (e.g. regarding item eating, /killme command) very important and would prefer that games override it.

[Wuzzy2]

The main reason for this PR is to allow mods to learn when the kill command was used (or do_item_eat) because those are two scenarios the player can get their HP changed without the API telling them.

I don't like the "solution" of "don't do anything, mods have to figure it out on their own" because it doesn't work. What if the builtin kill command was not touched by a mod? Which is completely reasonable btw.

Technically, none of the two approaches being discussed (extending type directly vs adding a detail field) make the engine add new types. It's actually a builtin change.

The argument "kill command and do_item_eat should't have been in engine to begin with" is false. It's in builtin. OK, I know you meant to say 'builtin'. In that case, I agree. However, the fact is, it IS in builtin now and we have to deal with it somehow and provide support. Backwards-compat is very important to me, I don't like the idea of "clean API" (which is COMPLETELY subjective) if we have to sacrifice backwards-compatibility just for nice-looking functions.

Hmm, I'm afraid this PR is currently stuck in limbo. It's not clear how to move forward. Too many conflicting solutions suggested; it's confusing.

My favourite approach still remains the new detail (to be renamed) field because that's the one keeping backwards-compat. The other solutions don't seem to really guarantee that.