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 crash when resetting offset after a play with no hit events #30620

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

peppy
Copy link
Member

@peppy peppy commented Nov 14, 2024

Closes #30573.

@frenzibyte
Copy link
Member

frenzibyte commented Nov 14, 2024

Coming from #30573 (comment), I think it would be a better fix if the beatmap offset control makes sure the graph won't actually possibly receive less than 10 hit events and display the "play is too short" message.

Right now it doesn't do so for the specific case I mentioned in the comment: A slider at the beginning of a beatmap with such a slow velocity that makes it generate more than 10 ticks, and is played with classic mod enabled. Instead, it shows this broken graph:

CleanShot 2024-11-14 at 03 45 33

i.e.:

diff --git a/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs b/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
index f312fb0ec5..2573c95f12 100644
--- a/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
+++ b/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
@@ -182,7 +182,7 @@ private void scoreChanged(ValueChangedEvent<ScoreInfo?> score)
             if (score.NewValue.Mods.Any(m => !m.UserPlayable || m is IHasNoTimedInputs))
                 return;
 
-            var hitEvents = score.NewValue.HitEvents;
+            var hitEvents = score.NewValue.HitEvents.Where(e => !(e.HitObject.HitWindows is HitWindows.EmptyHitWindows) && e.Result.IsBasic() && e.Result.IsHit()).ToList();
 
             if (!(hitEvents.CalculateAverageHitError() is double average))
                 return;

@peppy
Copy link
Member Author

peppy commented Nov 14, 2024

Sure, that seems like a valid fix in addition to what is here, not replacing it.

@peppy
Copy link
Member Author

peppy commented Nov 14, 2024

Looking into this further, I believe the extra fix is out of scope and should be handled separately. It's weird and I don't want to spend the time on it right now, as I'll need to re-learn hitobjects and stuff.

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.

Now that the circumstances of this are actually understood, I'm fine merging this (but will look into follow-up changes related to the conversation above).

@bdach bdach merged commit d08c8ae into ppy:master Nov 14, 2024
10 checks passed
bdach added a commit to bdach/osu that referenced this pull request Nov 14, 2024
…t enough timed hits

Intended to address concerns raised in
ppy#30620 (comment).
@peppy peppy deleted the fix-offset-adjust-crash branch November 15, 2024 03:19
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.

Crash when resetting offset
3 participants