Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't remove node above door if it's not a doors:hidden node #3045

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

fluxionary
Copy link
Contributor

currently, doors will remove the node above the door if the door is destroyed in various ways. however, there's times when the node above the door may not actually be a "doors:hidden" node, because of other mods. in particular, a mesecons sticky piston can be used to destroy an arbitrary node, whether or not it is private (can only be destroyed by a certain player) or protected (can only be modified if minetest.is_protected returns false).

this PR checks that the node above the door is actually a "doors:hidden" node before destroying it.

mods/doors/init.lua Outdated Show resolved Hide resolved
mods/doors/init.lua Outdated Show resolved Hide resolved
if is_doors_upper_node(pos) then
minetest.remove_node(pos)
end
minetest.remove_node(pos)
return {name}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should both of these return {name}? I believe the return {name} here should be removed, otherwise we get varying drops based on a race condition:

  • If the top is blasted first, we get 2 doors.
  • If the bottom is blasted first, the top is removed, and we only get one door.

Also: Am I missing something, or could you duplicate doors by blowing them up with TNT (before this patch)?

And in general, concerning the logic: Shouldn't both nodes remove each other to avoid inconsistent states? A "bottom without hidden top" shouldn't happen either, should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm deliberately not looking into whether return {name} is correct right now, or what the deal is w/ tnt. i'll try to look at it soon though. perhaps that should be a separate issue.

the door and "hidden" node can currently be separated by e.g. a mesecons piston. i really doubt anyone wants the doors mod to depend (optionally) on mesecons, and other mods could introduce that same kind of mechanic in the future, so that wouldn't even solve the problem in general. i suppose mesecons could automatically register all doors as MVPS stoppers? e.g. iterate doors.registered_doors and also override doors.register.

@sfan5 sfan5 added the Bugfix label Aug 3, 2023
@sfan5 sfan5 self-requested a review August 3, 2023 08:18
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely an improvement over removing an arbitrary node. Further improvements can be made later on.

The local above = ...; if is...(above) then remove_node(...) end is repeated three times. Perhaps rather than having is_door_upper_node, extract this into a function try_remove_upper_node?

@sfan5 sfan5 merged commit 08e057b into minetest:master Aug 11, 2023
1 check passed
MoNTE48 pushed a commit to MoNTE48/minetest_game that referenced this pull request Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants