Adjust slider ends to be more lenient during very fast sliders

[peppy]

• Depends on Tidy up LegacyLastTickOffset usages and stop passing everywhere #24965
• Update conversion tests with new output Locally fix conversion tests for now
• Closes LegacyLastTick is still being used for gameplay and editor displays #22805

I've removed all usage of the legacy tick in the osu! ruleset. This makes things a lot more sane in my eyes:

• The tail is mapped to the tail now
• The judgement logic in DrawableSlider reads better

If we go in this direction, the stable expected-conversion code will need to be updated, as it no longer matches. This can be seen from the test failures on this PR currently.

If this is seen as an overbearing or otherwise unwanted change, then the last two commits (62bcb62 and dd6d091) can be reverted.

Another direction I explored was removing the code from SliderEventGenerator completely – the only thing relying on it as of this PR is osu!catch conversion. You can see how this would look on this branch. I stopped exploring this direction as it feels like this would be less legible (having hidden logic in JuiceStream) but others may have different opinions.

Diff for better visualisation as used for videos below

diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
index 7daab820dd..e81ee84916 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
@@ -8,11 +8,13 @@
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
+using osu.Framework.Graphics.Shapes;
+using osu.Game.Graphics.Sprites;
 using osu.Game.Rulesets.Objects;
 using osu.Game.Rulesets.Objects.Drawables;
-using osu.Game.Rulesets.Objects.Types;
 using osu.Game.Skinning;
 using osuTK;
+using osuTK.Graphics;
 
 namespace osu.Game.Rulesets.Osu.Objects.Drawables
 {
@@ -42,6 +44,8 @@ public partial class DrawableSliderTail : DrawableOsuHitObject, IRequireTracking
 
         private Container scaleContainer;
 
+        private Drawable box;
+
         public DrawableSliderTail()
             : base(null)
         {
@@ -68,7 +72,27 @@ private void load()
                     Children = new Drawable[]
                     {
                         // no default for this; only visible in legacy skins.
-                        CirclePiece = new SkinnableDrawable(new OsuSkinComponentLookup(OsuSkinComponents.SliderTailHitCircle), _ => Empty())
+                        CirclePiece = new SkinnableDrawable(new OsuSkinComponentLookup(OsuSkinComponents.SliderTailHitCircle), _ => box = new Container
+                        {
+                            Size = new Vector2(32),
+                            Children = new Drawable[]
+                            {
+                                new Box
+                                {
+                                    Size = new Vector2(32),
+                                    Anchor = Anchor.Centre,
+                                    Origin = Anchor.Centre,
+                                },
+                                new OsuSpriteText
+                                {
+                                    Text = "LegacyLastTick",
+                                    Anchor = Anchor.BottomCentre,
+                                    Origin = Anchor.TopCentre,
+                                    Scale = new Vector2(2),
+                                    Y = 5,
+                                }
+                            }
+                        })
                     }
                 },
             });
@@ -132,9 +156,15 @@ protected override void CheckForResult(bool userTriggered, double timeOffset)
             // The player needs to have engaged in tracking at any point after the tail leniency cutoff.
             // An actual tick miss should only occur if reaching the tick itself.
             if (timeOffset >= SliderEventGenerator.TAIL_LENIENCY && Tracking)
+            {
+                box.Colour = Color4.LimeGreen;
                 ApplyResult(r => r.Type = r.Judgement.MaxResult);
+            }
             else if (timeOffset >= 0)
+            {
+                box.Colour = Color4.Red;
                 ApplyResult(r => r.Type = r.Judgement.MinResult);
+            }
         }
 
         protected override void OnApply()
@@ -142,7 +172,7 @@ protected override void OnApply()
             base.OnApply();
 
             if (Slider != null)
-                Position = Slider.CurvePositionAt(HitObject.RepeatIndex % 2 == 0 ? 1 : 0);
+                Position = HitObject.Position - DrawableSlider.HitObject.Position;
         }
     }
 }
diff --git a/osu.Game.Rulesets.Osu/Objects/Slider.cs b/osu.Game.Rulesets.Osu/Objects/Slider.cs
index c62659d67a..2126ddf6c3 100644
--- a/osu.Game.Rulesets.Osu/Objects/Slider.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Slider.cs
@@ -209,7 +209,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
                         {
                             RepeatIndex = e.SpanIndex,
                             StartTime = e.Time,
-                            Position = EndPosition,
+                            Position = Position + Path.PositionAt(e.PathProgress),
                             StackHeight = StackHeight
                         });
                         break;
diff --git a/osu.Game/Rulesets/Objects/SliderEventGenerator.cs b/osu.Game/Rulesets/Objects/SliderEventGenerator.cs
index 42c422a637..0b6f258565 100644
--- a/osu.Game/Rulesets/Objects/SliderEventGenerator.cs
+++ b/osu.Game/Rulesets/Objects/SliderEventGenerator.cs
@@ -108,7 +108,7 @@ public static class SliderEventGenerator
                 SpanIndex = finalSpanIndex,
                 SpanStartTime = startTime + (spanCount - 1) * spanDuration,
                 Time = startTime + totalDuration,
-                PathProgress = spanCount % 2,
+                PathProgress = finalProgress,
             };
         }
 

---

On very fast sliders, you now only need to be tracking somewhere in the last 36ms, rather than precisely at the (hidden) position of the last tick.

Miss

osu.Game.Rulesets.Osu.Tests.2023-09-29.at.08.40.29.mp4

Valid hits

osu.Game.Rulesets.Osu.Tests.2023-09-29.at.08.42.37.mp4

osu.Game.Rulesets.Osu.Tests.2023-09-29.at.08.45.44.mp4

osu.Game.Rulesets.Osu.Tests.2023-09-29.at.08.48.43.mp4

[bdach]

I've removed all usage of the legacy tick in the osu! ruleset. This makes things a lot more sane in my eyes

Generally tend to agree on this part for what it's worth.

[Walavouchey]

slider end hitsounds play too early in this branch

broken-hitsounds.mp4

first half of the video is this branch, second half is the latest release

[Natelytle]

2023-10-01.17-27-27.mp4

It looks like it's impossible to hit 300s on fast enough buzz sliders

[peppy]

2023-10-01.17-27-27.mp4

It looks like it's impossible to hit 300s on fast enough buzz sliders

See #24966 (comment) 👍

[peppy]

@Natelytle could you please attach the beatmap, and if possible confirm it works on master? Also can you test without classic mod?

[Natelytle]

Sure, I'll test first thing in the morning tomorrow

[peppy]

I've pushed some tentative fixes and test coverage of short sliders, but the tests still pass without the change. So will wait to hear back on the breaking sliders before investigating further.

[peppy]

slider end hitsounds play too early in this branch

@Walavouchey should be resolved via 07207ff.

[bdach]

I think I fixed the remaining test failures, let's see what CI thinks...

Other than that I think we can try getting this in and seeing what the feedback is. I for one don't personally have any reason to hold this back further.

[smoogipoo]

LGTM, waiting on diffcalc spreadsheet run: https://github.com/ppy/osu/actions/runs/6546185309

[peppy]

Diff calc re-run: https://github.com/ppy/osu/actions/runs/6546185309

[peppy]

These tests are fucked on CI. FWIW I can't repro the failures locally.

Shall we just disable them all for now? Is this going to be blocked further while I refactor framework to allow adjustable test precision or similar?

[bdach]

I'm okay with disable or retry.

[peppy]

Let's see if retry actually works.

[peppy]

Diff calc re-run: https://github.com/ppy/osu/actions/runs/6546185309

Looks like there's still three SR changes (lowest yet). I guess we're going to have to look into the remaining discrepancies? 😮‍💨

[bdach]

Egregious retry also didn't help either somehow... I'm not sure if that works with parameterised testing but at this point probably best to ignore.

[peppy]

The remaining issues are floating point precision loss on a double (around 0.00000000008 difference on 45 sliders in the case of beatmap_id 1398809).

Note the immediate window. The new method uses the "more precise" version.

Here's a diff for the above beatmap: https://gist.github.com/peppy/eb58fe462980a6b543bd06ee0d549647/revisions
Created using this patch:

diff --git a/osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs b/osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs
index 488d1e2e9f..e0526a4136 100644
--- a/osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs
+++ b/osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs
@@ -307,6 +307,8 @@ private void computeSliderCursorPosition(Slider slider)
                 if (i == nestedObjects.Count - 1)
                     slider.LazyEndPosition = currCursorPosition;
             }
+
+            Console.WriteLine($"{slider.StartTime} {slider.LazyTravelTime} {slider.LazyEndPosition}");
         }
 
         private Vector2 getEndCursorPosition(OsuHitObject hitObject)

I'd argue there's no point investigating further and this change is good to go. If there change in PP is seen as an issue, it should be fixed at the diffcalc end by making the algorithm less susceptible to floating point differences like this.

[peppy]

From StanR:

this seems fine-ish imo, if the change is important enough it's fine to break the diffcalc a little - it can get fixed later if the issue is bad or undesirable, as for these changes I think it's just fine, the maps in question are underweighted anyway

I think you shouldn't bother with diffcalc pretty much at all and just ping pp committees when changes affecting diffcalc are being rolled out

So let's go ahead with this as is.