Skip to content
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 duration fluctuations and one-frame jitters when editing juice streams #29019

Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jul 23, 2024

All of this code is a bit inscrutable and difficult to work with, but these changes appear to help.

Fix EditablePath.UpdateHitObjectFromPath() not automatically updating object

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.

Fix editing juice stream path sometimes changing its duration

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.

Fix tests

These are again, seemingly arbitrary changes to tests, but I could not salvage this otherwise.

  • The added drag end steps are just for proper test cleanup (was causing crosstalk issues)
  • The delete vertex test is adjusted because for whatever reason the juice stream path conversions sometimes spuriously produce extra nodes without affecting the juice stream shape. I'm not super sure why this is, but it's not something I changed, it just fell out when attempting to restore sanity here. I'd ask to look away.
  • The add vertex test is adjusted because:
    • The test attempts to add a control point at the same position as the juice stream's starting position. I'm not sure why this was considered valid, but the long and short of it is that without the EditorBeatmap.Update() call that control point would be allowed to live, but after adding it, the path goes through an extra conversion step that removes the point as it is redundant, and that seems more sane to me.
    • Additionally, the test adds a control point after the juice stream's end to test that it is extended, but the control point would not be correctly snapped to an end time on master. It would be left at 750 time, rather than snapped to 800 (test uses beat length of 100). This is another weird behaviour because if you play with the catch editor on master, you should be able to reproduce the following:
      • Select a juice stream
      • Move its last anchor (that coincides with its last fruit) to an unsnapped position
      • Deselect the juice stream and re-select it again
      • The last anchor will have moved to a snapped position (without the shape of the thing changing)
        This is all weird and strange and I would once again ask to look away because none of this is new breakage.

…ng object

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`.
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.
@peppy peppy self-requested a review July 23, 2024 09:13
@peppy
Copy link
Member

peppy commented Jul 23, 2024

It seems like the reported flicker in the (drawable)hitobject portion is fixed, but there's a couple of remaining quirks I can see (grid lines and blueprint line drawing):

osu.2024-07-23.at.09.20.48.mp4

Is it safe to say these are separate concerns?

@bdach
Copy link
Collaborator Author

bdach commented Jul 23, 2024

Those are probably byproducts of the fact that vertex movement does not trigger the recomputations immediately, but instead schedules them to happen once a frame:

if (editablePath.PathId != lastEditablePathId)
updateHitObjectFromPath();

I'm not sure whether that can be salvaged at all without destroying performance, so I'd generally ask to look away for now yep. The big thing that I'm trying to fix here that on master is you could actually pinpoint the pixel that caused the flicker, and drop the mouse there to get a completely broken juice stream.

@peppy peppy merged commit 40b171f into ppy:master Jul 23, 2024
15 of 17 checks passed
@bdach bdach deleted the catch-juice-stream-editing-weirdness-continued branch July 23, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Any slider velocity change causing glitch to juicestream in catch editor
2 participants