Fix juice stream placement blueprint being initially visually offset #31453
+5
−14
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
|
Hmm, I do think I prefer the patch version.. feels like it's going to be easier to work with if we need to make further changes to the class (because at least it's very explicit about what does/doesn't need to happen). |
peppy
reviewed
Jan 9, 2025
- Closes ppy#31423. - Regressed in ppy#30411. Admittedly, I don't completely understand all of the pieces here, because code quality of this placement blueprint code is ALL-CAPS ATROCIOUS, but I believe the failure mode to be something along the lines of: - User activates juice stream tool, blueprint gets created in initial state. It reads in a mouse position far outside of the playfield, and sets internal positioning appropriately. - When the user moves the mouse into the bounds of the playfield, some positions update (the ones inside `UpdateTimeAndPosition()`, but the fruit markers are for *nested* objects, and `updateHitObjectFromPath()` is responsible for updating those... however, it only fires if the `editablePath.PathId` changes, which it won't here, because there is only one path vertex until the user commits the starting point of the juice stream and it's always at (0,0). - Therefore the position of the starting fruit marker remains bogus until left click, at which point the path changes and everything returns to *relative* sanity. The solution essentially relies on inlining the broken method and only guarding the relevant part of processing behind the path version check (which is actually updating the path). Everything else that can touch positions of nesteds (like default application, and the drawable piece updates) is allowed to happen unconditionally.
bdach
force-pushed
the
fix-juice-stream-placement-offset
branch
from
2de2c56 to
18f1d62
Compare
|
Sure, have switched to it. |
peppy
approved these changes
Jan 9, 2025
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.
Admittedly, I don't completely understand all of the pieces here, because code quality of this placement blueprint code is ALL-CAPS ATROCIOUS, but I believe the failure mode to be something along the lines of:
UpdateTimeAndPosition(), but the fruit markers are for nested objects, andupdateHitObjectFromPath()is responsible for updating those... however, it only fires if theeditablePath.PathIdchanges, which it won't here, because there is only one path vertex until the user commits the starting point of the juice stream and it's always at (0,0).The part in #30411 that regresses the above scenario is the "blueprint is always created even if not visible / cursor is outside playfield" part.
This PR essentially relies on inlining the broken method and only guarding the relevant part of processing behind the path version check (which is actually updating the path). Everything else that can touch positions of nesteds (like default application, and the drawable piece updates) is allowed to happen unconditionally.