Fix object:punch(puncher, ... should allow nil for puncher

[sfence]

• Goal of the PR
  Fix object:punch(puncher, ... should allow nil for puncher #3837
• How does the PR work?
  Add support or nil puncher.
• Does it resolve any reported issue?
  Fix object:punch(puncher, ... should allow nil for puncher #3837

To do

This PR is a Ready for Review.

How to test

Add entity callback:callback_puncher and punch it.

[sfan5]

just some minor stuff, looks good otherwise

[SmallJoker]

Why do we need a nil puncher?

---

Discussed in the meeting: https://irc.minetest.net/minetest-dev/2024-03-03#i_6156874

• likely causes problems 3d_armor until it's fixed
• on_rightclick does yet not seem to allow nil ObjectRefs?
• this is generally an OK change to have it consistent

[sfence]

Why do we need a nil puncher?

Why do we need a nil puncher?
Why #3837 has been labeled a bug and not closed to these days?

In #3837 is said: "The API clearly states in several places that nil is allowed. And this would be useful for mods to add "environment" damage that isn't attributable to objects."

[appgurueu]

I agree that we want to support a nil puncher, API-wise. But causing regressions in very popular mods like 3d_armor is a significant concern. We will probably want to run a zipgrep to see which other mods may be affected.

[sfan5]

This might sound obvious, but this only causes problems if some mod uses a nil puncher. So I think it's not that bad.
Delaying this change for the sake of 3d_armor doesn't make much sense (who will fix it and how long do we wait?).

[appgurueu]

So I think it's not that bad.

It's not as bad as mods that outright crash, yes. It requires another mod to use the feature. But once mods start using this feature, there will be worse cross-mod compatibility.

I've opened an issue on the 3d armor repo to notify them - I think they're probably the most popular, potentially affected mod - and additionally asked for a zipgrep of on_punchplayer on ContentDB. Crashing on a nil puncher should be relatively easy to fix (in mods).

I'm fine with merging this for 5.9. I will make an effort to notify at least the maintainers of a couple of the most popular mods.

As far as this PR goes, I think it should make clear that puncher can be nil in register_on_punchplayer (technically this may follow from the docs on :punch, but modders are likely to miss that).

Do we have some kind of place to advertise such changes that may lead to (in this case cross-mod) compatibility issues in the future? Add another section to the changelog?

[SmallJoker]

Why #3837 has been labeled a bug and not closed to these days?

There's so many issues that it's hard to keep track of whether they're still valid.
That issue was opened in 2016. set_hp got support for PlayerHPChangeReason in 2018 and can since be used to specify details of the damage type (as an alternative to punching). I'd assume mods would use different damage groups in ObjectRef:punch to specify the origin of the punch.

The API clearly states in several places that nil is allowed.

(from the issue)
I looked up some callbacks in the documentation and builtin code: (✔ = may be nil, ✘ = only ObjectRef mentioned)

• node can_dig ✔ (used by core.dig_node)
• node register_on_placenode ✔ (used by core.place_node)
• node on_rightclick ✘
• node on_punch ✘ but core.punch_node passes nil anyway
• register_on_punchnode ?? not documented
• node on_dig ?? not documented (core.node_dig accepts and forwards nil, but is not documented either)
• register_on_dignode ?? not documented
• node after_dig_node ✘ but may get nil anyway due to core.node_dig callbacks
• node inventory callbacks: ?? not documented
• object on_rightclick ✘
• register_on_rightclickplayer argument clicker ✘
• object on_punch ✔
• register_on_punchplayer argument hitter ✘ (but may anyway due to this PR)

Whenever something's not documented, I'd assume that ObjectRef is provided.

[appgurueu]

@sfence could you please also document that hitter may be nil in register_on_punchplayer now?

I think we should merge this and

• Keep an eye out for potential issues;
• Highlight this change in the release notes

At least in 3d_armor this should be addressed now.

[appgurueu]

Tested with the provided entity, works.