- Closes https://github.com/ppy/osu/issues/31423.
- Regressed in https://github.com/ppy/osu/pull/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.
I'm not *super* sure why this works, but it appears to, and my educated
guess as to why is that it counteracts the effects of a change in the SV
of the juice stream by artificially increasing or decreasing the
velocity when running the appropriate path conversions and expected
distance calculations. The actual SV change takes effect on the next
default application, which is triggered by the `Update()` call at the
end of the method.
This is important because the editable path conversions heavily depend
on the value of `JuiceStream.Velocity` being correct. The value is only
guaranteed to be correct after an `ApplyDefaults()` call, which is
triggered by updating the object via `EditorBeatmap`.
Until now, these were haphazardly enforce inline in blueprint
implementations. The only thing stopping complete breakage is that
`EndPlacement` wasn't called (too much) from outside the blueprint,
leaving them responsible for their own placement.
By moving this conditional out of the provided paramters to
`EndPlacement`, it allows more flexible usage of that method externally.
Coming in a future PR.
Timeline blueprint is glitched when the hit object has negative duration.
Negative duration is unwanted anyways so placement implementation is fixed instead of supporting it in timline blueprint.
This is quite a breaking change, but I think it is beneficial due to the large amount of usage of this class.
I originally intended just to remove the allocations of the two delegates handling the `Changed` flow internally, but as nothing was really using the bindables for anything more than a general "point has changed" case, this felt like a better direction.
