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

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Oct 5, 2023

@bdach bdach self-requested a review October 5, 2023 06:19
Comment on lines 194 to 196
// 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():

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.

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Oct 5, 2023
@bdach bdach self-requested a review October 5, 2023 17:48
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Seems okay

@bdach bdach merged commit 4d315cb into ppy:master Oct 5, 2023
@peppy peppy deleted the fix-waveform-comparison-crash branch October 10, 2023 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants