-
Notifications
You must be signed in to change notification settings - Fork 121
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
fix issue 645: prevent pistons from pushing blocks into their current position, deleting them. #659
base: master
Are you sure you want to change the base?
Conversation
if vector.equals(n.pos, behind_pos) then | ||
return | ||
end | ||
end | ||
local success, stack, oldstack = mesecon.mvps_push(pusher_pos, dir, max_push, meta:get_string("owner")) |
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.
Shouldn't mvps_push
return success = false
when the node cannot be pushed? That would seem to be the logical behaviour.
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.
But a piston can be pushed. It just shouldn’t be able to push itself. Unlike a movestone for which that shouldn’t be a problem.
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.
my first thought was swapping the piston for the immoveable extended version before calling mvps_push (then swap it back if it was unsuccessful), which would return false and also wouldn't have any significant performance overhead.
unfortunately, a sticky block on the face of the piston will grab said piston and try to pull it along, even if it is immoveable.
this is different from the behavior or minecraft slime blocks, which have no problems pulling away from immoveable blocks.
however, the purpose of this commit is to fix a bug, not to change behavior, so i opted for this instead, which doesn't change any public apis, ensuring it doesn't break other mods.
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.
Shouldn't
mvps_push
returnsuccess = false
when the node cannot be pushed? That would seem to be the logical behaviour.
the biggest reason i don't implement the new logic in mvps_push is because pistons and moveblocks have fundimentally different behavior:
- pistons get bigger when pushing things
- moveblocks stay the same size
one option would be to add an is_piston
or piston_pos
parameter to mvps_push
, and somehow handle the logic there, but once again, i didn't want to amend the public api on my first contribution.
the simplest change to fix the performance loss would be to add an optional nodes parameter to mvps_push
, then add nodes = nodes or mesecons.mvps_get_stack(<...>)
.
i can implement any of these changes if desired.
this isn't the most efficient fix, but as far as i can tell, anything better would require changing the public api, and i don't want to break any other mods that may depend on mesecons_mvps.
some things i noticed while fixing this bug: