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

Miss count discrepancy on slider heavy maps #25200

Closed
Natelytle opened this issue Oct 21, 2023 · 10 comments · Fixed by #25218
Closed

Miss count discrepancy on slider heavy maps #25200

Natelytle opened this issue Oct 21, 2023 · 10 comments · Fixed by #25218

Comments

@Natelytle
Copy link
Contributor

Type

Crash to desktop

Bug description

When this replay is viewed back on the latest master, the miss count jumps from 3 to 8. May be caused by #24966.

Screenshots or videos

2023-10-21.15-27-58.mp4
2023-10-21.15-28-27.mp4
2023-10-21.15-29-24.mp4
2023-10-21.15-30-01.mp4

Version

d059949

Logs

database.log
input.log
legacy-ipc.log
network.log
performance.log
runtime.log

@bdach
Copy link
Collaborator

bdach commented Oct 22, 2023

May be caused by #24966

Did you bisect this / compare against latest release to say this? Or is this just an educated guess?

@Natelytle
Copy link
Contributor Author

Just an educated guess, because I noticed some similar behaviours when I was testing it out for the first time. I just assumed this was fixed by the followup commits but maybe it wasn't

@Natelytle
Copy link
Contributor Author

Update: Bisected now that I'm at my PC and this issue was caused by 9e1fec0

@bdach
Copy link
Collaborator

bdach commented Oct 23, 2023

Yeah that checks out unfortunately.

The first extra miss on this replay is here:

osu_2023-10-23_11-05-51

This slider is 37.5ms long, so it qualifies for the "legacy last tick is in the middle of the slider" case.

Judgement order before #24966:

[runtime] 2023-10-23 09:28:07 [verbose]: osu.Game.Rulesets.Osu.Objects.SliderTailCircle @ 95143.75 judged @ 95143.75 as SmallTickHit
[runtime] 2023-10-23 09:28:07 [verbose]: osu.Game.Rulesets.Osu.Objects.Slider @ 95125 judged @ 95162.5 as Ok
[runtime] 2023-10-23 09:28:07 [verbose]: osu.Game.Rulesets.Osu.Objects.SliderHeadCircle @ 95125 judged @ 95225.66132676424 as LargeTickMiss

The tail is judged out of order at the midpoint of the slider.

After that pull:

[runtime] 2023-10-23 09:26:36 [verbose]: osu.Game.Rulesets.Osu.Objects.SliderHeadCircle @ 95125 judged @ 95226.7903388258 as LargeTickMiss
[runtime] 2023-10-23 09:26:36 [verbose]: osu.Game.Rulesets.Osu.Objects.SliderTailCircle @ 95162.5 judged @ 95162.5 as SmallTickMiss
[runtime] 2023-10-23 09:26:36 [verbose]: osu.Game.Rulesets.Osu.Objects.Slider @ 95125 judged @ 95162.5 as Miss

The head circle of the slider, due to having a nonzero hitwindow, blocks the tail from being judged (since the aforementioned bisect commit disallows judging nested objects out of order), therefore causing the entire slider to be missed.

I don't even know where to begin with this one. The choices seem to be either to accept this state of affairs or drop the "nested objects must be judged in order" thing and instead either try to hack score to not be broken (#24966 (comment)) or accept score as broken................

@peppy
Copy link
Member

peppy commented Oct 24, 2023

Can we not just change the conditional added in 9e1fec0 to only match ticks? At a glance, that seems to be what it's trying to do. The fact it's matching on a SliderHeadCircle wasn't something I considered when adding that change (forgot they came up as nested objects).

diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
index df55f3e73f..e52451e75d 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
@@ -4,6 +4,7 @@
 #nullable disable
 
 using System.Diagnostics;
+using System.Linq;
 using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
@@ -133,7 +134,8 @@ protected override void CheckForResult(bool userTriggered, double timeOffset)
             //
             // This covers the edge case where the lenience may allow the tail to activate before
             // the last tick, changing ordering of score/combo awarding.
-            if (DrawableSlider.NestedHitObjects.Count > 1 && !DrawableSlider.NestedHitObjects[^2].Judged)
+            var lastTick = DrawableSlider.NestedHitObjects.LastOrDefault(o => o.HitObject is SliderTick || o.HitObject is SliderRepeat);
+            if (lastTick?.Judged == false)
                 return;
 
             // The player needs to have engaged in tracking at any point after the tail leniency cutoff.

@bdach
Copy link
Collaborator

bdach commented Oct 24, 2023

Theoretically, yes, you could do that. But if you do, then that now means there are 2 possibilities:

  • head is judged before tick
  • tick is judged before head

And because the slider head is a 'large tick' and as such affects combo, this also affects score, bringing back the aforementioned issue wherein now it may not be possible to get 1M score just because of unlucky judgement ordering.

@peppy
Copy link
Member

peppy commented Oct 24, 2023

An addition of forcing a miss of the head in such a case could resolve this (and sounds amicable to me). What do you think?

@bdach
Copy link
Collaborator

bdach commented Oct 24, 2023

Maybe? That seems to be conceptually similar to #25129, although that seems to propose forcing the head to be hit rather than missed, and contains a condition of "closeness" to the head that I don't understand.

@peppy
Copy link
Member

peppy commented Oct 24, 2023

#25129 focuses on cases where the head is hit. Correct me if I'm wrong, but the case in this issue doesn't involve the head getting hit by the user.

@bdach
Copy link
Collaborator

bdach commented Oct 24, 2023

Yeah fair enough. You're not wrong.

@peppy peppy changed the title Misscount discrepency on slider heavy maps Miss count discrepancy on slider heavy maps Oct 24, 2023
@peppy peppy self-assigned this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants