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.

Sign up for GitHub

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 WaveformComparisonDisplay potentially crashing on invalid track length #25016

Merged
merged 2 commits into from
Oct 5, 2023
Merged

Fix WaveformComparisonDisplay potentially crashing on invalid track length #25016

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76cc2f9
Fix `WaveformComparisonDisplay` potentially crashing on invalid track…
peppy Oct 5, 2023
61fd418
Ensure `regenerateDisplay` is eventually performed if originally call…
peppy Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions osu.Game/Screens/Edit/Timing/WaveformComparisonDisplay.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ private void showFromTime(double time, bool animated)

private void regenerateDisplay(bool animated)
{
// Before a track is loaded, it won't have a valid length, which will break things.
if (!beatmap.Value.Track.IsLoaded)
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard is fine and all to address the direct failure, but what are the guarantees the component will function correctly after hitting it? By which I mean: say that the waveform regeneration is requested, but the track isn't loaded yet. This early-guard will prevent the crash, but what is going to ensure that the waveform gets regenerated at some later point?

There is this code in Update():

osu/osu.Game/Screens/Edit/Timing/WaveformComparisonDisplay.cs

Lines 168 to 178 in 0f7093c

protected override void Update()
{
base.Update();
if (!IsHovered && !displayLocked.Value)
{
int currentBeat = (int)Math.Floor((editorClock.CurrentTimeAccurate - selectedGroupStartTime) / timingPoint.BeatLength);
showFromBeat(currentBeat);
}
}

but the guarantees it gives come with a lot of caveats (component needs to be not hovered, not locked, and even then showFromBeat() / showFromTime() have early-return guards that prevent possibly-redundant regenerations from happening etc.)

I'd probably see this as a flow based around Cached or something. Everything that forces the display regeneration would instead invalidate the Cached, and Update() would check if the Cached is invalid and if it's not, run this method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, this issue has actually been a thing for a while as far as I can tell. I've had cases the display doesn't update until I start to play the track (likely because it hit the case in this PR or something very similar but not as fatal).

Was hoping for a quick fix, but will look at doing this more correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used Scheduler.AddOnce as it feels like a pretty simple solution here.


double index = (displayedTime - selectedGroupStartTime) / timingPoint.BeatLength;

// Chosen as a pretty usable number across all BPMs.
Expand Down