-
Notifications
You must be signed in to change notification settings - Fork 577
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
Conversation
mods/doors/init.lua
Outdated
if is_doors_upper_node(pos) then | ||
minetest.remove_node(pos) | ||
end | ||
minetest.remove_node(pos) | ||
return {name} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this 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
?
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.