[MIRROR] Improves how ai movement checks for if movement is allowed #554
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…MDB IGNORE] (#24920) * Improves how ai movement checks for if movement is allowed (#79620) ## About The Pull Request While looking into the moveloop processing issues I found a place where moveloops were being interacted with while qdeleted. This restructures the code a bit to prevent that being possible and also improves the design overall here so implementers of `allowed_to_move` don't need to be careful about their return value, which is just begging for a bug down the line. `increment_pathing_failure` also wasn't getting called when subtypes stopped movement. The rest of this description is a bit of an informative lecture. There's a pattern you do on occasion when designing public facing functions where you have a public, but non overridden, function with some default behavior that must always run, calling on an internal function that *is* overridden but never called from any other location. This is a good pattern for making sure some core behavior always runs or for making sanity checks that can't be dodged, as the internal function never even gets called when it has been determined that something needs to cancel the whole thing. Previously this code had something that looked kinda like this, there was a proc called `pre_move` that was not supposed to be overridden, and a proc it called called `allowed_to_move`. However, contrary to the usual reason for doing this, `pre_move` had no behavior itself and was meaningless. `allowed_to_move` on the other hand had exactly the sort of sanity check that we'd want using this pattern in the form of `if(!controller.able_to_run())`. This was allowing subtypes access to a qdeleted move loop when it had just been deleted in the parent's version of the proc. This is your yearly reminder to watch out for cargo culting. * Improves how ai movement checks for if movement is allowed --------- Co-authored-by: Emmett Gaines <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mirrored on Skyrat: Skyrat-SS13/Skyrat-tg#24920
Original PR: tgstation/tgstation#79620
About The Pull Request
While looking into the moveloop processing issues I found a place where moveloops were being interacted with while qdeleted. This restructures the code a bit to prevent that being possible and also improves the design overall here so implementers of
allowed_to_movedon't need to be careful about their return value, which is just begging for a bug down the line.increment_pathing_failurealso wasn't getting called when subtypes stopped movement.The rest of this description is a bit of an informative lecture.
There's a pattern you do on occasion when designing public facing functions where you have a public, but non overridden, function with some default behavior that must always run, calling on an internal function that is overridden but never called from any other location. This is a good pattern for making sure some core behavior always runs or for making sanity checks that can't be dodged, as the internal function never even gets called when it has been determined that something needs to cancel the whole thing.
Previously this code had something that looked kinda like this, there was a proc called
pre_movethat was not supposed to be overridden, and a proc it called calledallowed_to_move. However, contrary to the usual reason for doing this,pre_movehad no behavior itself and was meaningless.allowed_to_moveon the other hand had exactly the sort of sanity check that we'd want using this pattern in the form ofif(!controller.able_to_run()). This was allowing subtypes access to a qdeleted move loop when it had just been deleted in the parent's version of the proc.This is your yearly reminder to watch out for cargo culting.