make falling node checks check for protection

[fluxionary]

Goal of the PR

to prevent players from knocking things off the walls or ceiling in protected areas

How does the PR work?

adds a new optional player_name parameter to check_for_falling and check_single_for_falling. if this value is not nil, and minetest.is_protected(pos, player_name), then things won't fall. the builtin on_placenode, on_dignode, and on_puncnode callbacks supply this value if triggered by a player.

Does it resolve any reported issue?

i don't think so

Does this relate to a goal in the roadmap?

i don't think so

If not a bug fix, why is this PR needed? What usecases does it solve?

players on the your-land server occasionally complain about things getting knocked off walls in protected areas, and i can't figure out a way to keep that from happening w/out tweaking the API.

To do

i'm not sure if this won't break someone else's usecase, though i don't know of any. feedback appreciated.

This PR is Ready for Review.

How to test

devtest has testnodes:falling, but you'll need a protection mod (e.g. areas) and a way to place it (worldedit?)

[oong819]

will that check for liquid too?

[fluxionary]

will that check for liquid too?

check for liquid... doing what? flowing? no, this doesn't affect how liquids flow.

[Desour]

~~:-1: ~~ Edit: I'm unsure.

• Falling nodes are not intended to float. (How do players even create floating falling nodes?)
• Mods will still use check_falling without playername, so your checks don't cover everything.
• A less drastic change that still covers your use-case is possible: Make the on_punchnode and co. callbacks overwritable.

[fluxionary]

• Falling nodes are not intended to float.

a valid objection.

(How do players even create floating falling nodes?)

players generally do not, but there are 2 cases that concern me:

• falling nodes which were created via mapgen and are suspended (e.g. sand in the ceiling of a cave), and which a player incorporates into their protected area (via e.g. the areas mod)
• nodes which a player placed which later got listed as attached nodes in an integration mod (most recently, ladders from the "default" mod of minetest_game)

* Mods will still use `check_falling` without playername, so your checks don't cover everything.

of course. but those can't be fixed before some sort of API is created to solve the problem.

* A less drastic change that still covers your use-case is possible: Make the `on_punchnode` and co. callbacks overwritable.

i'm sure not why this would be "less drastic".

it certainly is possible to override minetest.node_punch, minetest.node_dig, and minetest.item_place_node so as to prevent the on_punchnode, on_dignode, and on_placenode callbacks from running, but i don't see a way to use that to prevent a specific callback from running. there's still lots of cases where you want those callbacks to run in protected areas.

do you perhaps mean that we should expose the specific callbacks that are used to check for falling nodes? those are all one-line functions that only call the method which i've extended in this PR.

a post-script:

for those who don't like change, it'd certainly be possible to gate the behavior behind a configuration option (e.g. falling_nodes_are_protected, a boolean value which defaults to false).

[Desour]

nodes which a player placed which later got listed as attached nodes in an integration mod (most recently, ladders from the "default" mod of minetest_game)

Ah, I see the issue now.

do you perhaps mean that we should expose the specific callbacks that are used to check for falling nodes?

Yes. My idea was that there's something like a minetest.check_falling_on_punch that calls check_for_falling, and the registered on_punchnode callback calls this function. Mods can then overwrite minetest.check_falling_on_punch.

(Btw. it looks to me like only the punch case needs change, digging and placing is already blocked by protection (apart from the border, but the protection border has always been less secured anyway).)

a configuration option

I like settings. But some of the core devs think we have too many. Also, settings might be too limited, i.e. maybe you only want to check protection for ladders.

[fluxionary]

Yes. My idea was that there's something like a minetest.check_falling_on_punch that calls check_for_falling, and the registered on_punchnode callback calls this function. Mods can then overwrite minetest.check_falling_on_punch.

after mulling over this issue for a while, i decide i like this approach. this would let me more easily rewrite falling node behavior as a mod.

do others support that? if so, i'll close this and create a new PR.

[SmallJoker]

https://irc.minetest.net/minetest-dev/2023-05-28#i_6087676

Suggestion: Add player_name to core.check_for_falling and let protection mods decide whether or not it's acceptable to let nodes fall.

[savilli]

I don't quite understand how the protected node gets removed. Does the node remove itself on on_punch callback, and the protection mode doesn't prevent on_punch from being called?

[Desour]

I support fixing the issue (= I acknowledge this as bugfix). But I'm not sure if I support the implementation.

[fluxionary]

I support fixing the issue (= I acknowledge this as bugfix). But I'm not sure if I support the implementation.

i'll close this now, and re-open w/ the proposal in this comment, at some future date.