Merge pull request #24965 from peppy/legacy-tick-not-so-legacy-after-all

Tidy up `LegacyLastTickOffset` usages and stop passing everywhere

osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapConverter.cs

@@ -41,7 +41,6 @@ protected override IEnumerable<CatchHitObject> ConvertHitObject(HitObject obj, I
                         X = xPositionData?.X ?? 0,
                         NewCombo = comboData?.NewCombo ?? false,
                         ComboOffset = comboData?.ComboOffset ?? 0,
-                        LegacyLastTickOffset = (obj as IHasLegacyLastTickOffset)?.LegacyLastTickOffset ?? 0,
                         LegacyConvertedY = yPositionData?.Y ?? CatchHitObject.DEFAULT_LEGACY_CONVERT_Y,
                         SliderVelocityMultiplier = sliderVelocityData?.SliderVelocityMultiplier ?? 1
                     }.Yield();

osu.Game.Rulesets.Catch/Objects/JuiceStream.cs

@@ -77,7 +77,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
             int nodeIndex = 0;
             SliderEventDescriptor? lastEvent = null;
 
-            foreach (var e in SliderEventGenerator.Generate(StartTime, SpanDuration, Velocity, TickDistance, Path.Distance, this.SpanCount(), LegacyLastTickOffset, cancellationToken))
+            foreach (var e in SliderEventGenerator.Generate(StartTime, SpanDuration, Velocity, TickDistance, Path.Distance, this.SpanCount(), cancellationToken))
             {
                 // generate tiny droplets since the last point
                 if (lastEvent != null)
@@ -104,8 +104,8 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
                     }
                 }
 
-                // this also includes LegacyLastTick and this is used for TinyDroplet generation above.
-                // this means that the final segment of TinyDroplets are increasingly mistimed where LegacyLastTickOffset is being applied.
+                // this also includes LastTick and this is used for TinyDroplet generation above.
+                // this means that the final segment of TinyDroplets are increasingly mistimed where LastTick is being applied.
                 lastEvent = e;
 
                 switch (e.Type)
@@ -162,7 +162,5 @@ public SliderPath Path
         public double Distance => Path.Distance;
 
         public IList<IList<HitSampleInfo>> NodeSamples { get; set; } = new List<IList<HitSampleInfo>>();
-
-        public double? LegacyLastTickOffset { get; set; }
     }
 }

osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSplitting.cs

@@ -163,7 +163,6 @@ public void TestSplitRetainsHitsounds()
                 slider = new Slider
                 {
                     Position = new Vector2(0, 50),
-                    LegacyLastTickOffset = 36, // This is necessary for undo to retain the sample control point
                     Path = new SliderPath(new[]
                     {
                         new PathControlPoint(Vector2.Zero, PathType.PerfectCurve),

osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapConverter.cs

@@ -44,7 +44,6 @@ protected override IEnumerable<OsuHitObject> ConvertHitObject(HitObject original
                         Position = positionData?.Position ?? Vector2.Zero,
                         NewCombo = comboData?.NewCombo ?? false,
                         ComboOffset = comboData?.ComboOffset ?? 0,
-                        LegacyLastTickOffset = (original as IHasLegacyLastTickOffset)?.LegacyLastTickOffset,
                         // prior to v8, speed multipliers don't adjust for how many ticks are generated over the same distance.
                         // this results in more (or less) ticks being generated in <v8 maps for the same time duration.
                         TickDistanceMultiplier = beatmap.BeatmapInfo.BeatmapVersion < 8 ? 1f / ((LegacyControlPointInfo)beatmap.ControlPointInfo).DifficultyPointAt(original.StartTime).SliderVelocity : 1,

osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs

@@ -315,7 +315,6 @@ private void splitControlPoints(List<PathControlPoint> controlPointsToSplitAt)
                     StartTime = HitObject.StartTime,
                     Position = HitObject.Position + splitControlPoints[0].Position,
                     NewCombo = HitObject.NewCombo,
-                    LegacyLastTickOffset = HitObject.LegacyLastTickOffset,
                     Samples = HitObject.Samples.Select(s => s.With()).ToList(),
                     RepeatCount = HitObject.RepeatCount,
                     NodeSamples = HitObject.NodeSamples.Select(n => (IList<HitSampleInfo>)n.Select(s => s.With()).ToList()).ToList(),

osu.Game.Rulesets.Osu/Mods/OsuModStrictTracking.cs

@@ -96,14 +96,13 @@ public StrictTrackingSlider(Slider original)
                 Position = original.Position;
                 NewCombo = original.NewCombo;
                 ComboOffset = original.ComboOffset;
-                LegacyLastTickOffset = original.LegacyLastTickOffset;
                 TickDistanceMultiplier = original.TickDistanceMultiplier;
                 SliderVelocityMultiplier = original.SliderVelocityMultiplier;
             }
 
             protected override void CreateNestedHitObjects(CancellationToken cancellationToken)
             {
-                var sliderEvents = SliderEventGenerator.Generate(StartTime, SpanDuration, Velocity, TickDistance, Path.Distance, this.SpanCount(), LegacyLastTickOffset, cancellationToken);
+                var sliderEvents = SliderEventGenerator.Generate(StartTime, SpanDuration, Velocity, TickDistance, Path.Distance, this.SpanCount(), cancellationToken);
 
                 foreach (var e in sliderEvents)
                 {
@@ -130,7 +129,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
                             });
                             break;
 
-                        case SliderEventType.LegacyLastTick:
+                        case SliderEventType.LastTick:
                             AddNested(TailCircle = new StrictTrackingSliderTailCircle(this)
                             {
                                 RepeatIndex = e.SpanIndex,

osu.Game.Rulesets.Osu/Mods/OsuModSynesthesia.cs

@@ -36,7 +36,7 @@ public void ApplyToDrawableHitObject(DrawableHitObject d)
 
             d.HitObjectApplied += _ =>
             {
-                // slider tails are a painful edge case, as their start time is offset 36ms back (see `LegacyLastTick`).
+                // slider tails are a painful edge case, as their start time is offset 36ms back (see `LastTick`).
                 // to work around this, look up the slider tail's parenting slider's end time instead to ensure proper snap.
                 double snapTime = d is DrawableSliderTail tail
                     ? tail.Slider.GetEndTime()

osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs

@@ -296,7 +296,7 @@ protected override void CheckForResult(bool userTriggered, double timeOffset)
         public override void PlaySamples()
         {
             // rather than doing it this way, we should probably attach the sample to the tail circle.
-            // this can only be done after we stop using LegacyLastTick.
+            // this can only be done if we stop using LastTick.
             if (!TailCircle.SamplePlaysOnlyOnHit || TailCircle.IsHit)
                 base.PlaySamples();
         }

osu.Game.Rulesets.Osu/Objects/Slider.cs

@@ -71,8 +71,6 @@ public override Vector2 Position
             }
         }
 
-        public double? LegacyLastTickOffset { get; set; }
-
         /// <summary>
         /// The position of the cursor at the point of completion of this <see cref="Slider"/> if it was hit
         /// with as few movements as possible. This is set and used by difficulty calculation.
@@ -179,7 +177,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
         {
             base.CreateNestedHitObjects(cancellationToken);
 
-            var sliderEvents = SliderEventGenerator.Generate(StartTime, SpanDuration, Velocity, TickDistance, Path.Distance, this.SpanCount(), LegacyLastTickOffset, cancellationToken);
+            var sliderEvents = SliderEventGenerator.Generate(StartTime, SpanDuration, Velocity, TickDistance, Path.Distance, this.SpanCount(), cancellationToken);
 
             foreach (var e in sliderEvents)
             {
@@ -206,10 +204,11 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
                         });
                         break;
 
-                    case SliderEventType.LegacyLastTick:
-                        // we need to use the LegacyLastTick here for compatibility reasons (difficulty).
-                        // it is *okay* to use this because the TailCircle is not used for any meaningful purpose in gameplay.
-                        // if this is to change, we should revisit this.
+                    case SliderEventType.LastTick:
+                        // Of note, we are directly mapping LastTick (instead of `SliderEventType.Tail`)  to SliderTailCircle.
+                        // It is required as difficulty calculation and gameplay relies on reading this value.
+                        // (although it is displayed in classic skins, which may be a concern).
+                        // If this is to change, we should revisit this.
                         AddNested(TailCircle = new SliderTailCircle(this)
                         {
                             RepeatIndex = e.SpanIndex,
@@ -264,7 +263,9 @@ protected void UpdateNestedSamples()
             if (HeadCircle != null)
                 HeadCircle.Samples = this.GetNodeSamples(0);
 
-            // The samples should be attached to the slider tail, however this can only be done after LegacyLastTick is removed otherwise they would play earlier than they're intended to.
+            // The samples should be attached to the slider tail, however this can only be done if LastTick is removed otherwise they would play earlier than they're intended to.
+            // (see mapping logic in `CreateNestedHitObjects` above)
+            //
             // For now, the samples are played by the slider itself at the correct end time.
             TailSamples = this.GetNodeSamples(repeatCount + 1);
         }

osu.Game.Rulesets.Osu/Objects/SliderTailCircle.cs

@@ -10,7 +10,7 @@ namespace osu.Game.Rulesets.Osu.Objects
 {
     /// <summary>
     /// Note that this should not be used for timing correctness.
-    /// See <see cref="SliderEventType.LegacyLastTick"/> usage in <see cref="Slider"/> for more information.
+    /// See <see cref="SliderEventType.LastTick"/> usage in <see cref="Slider"/> for more information.
     /// </summary>
     public class SliderTailCircle : SliderEndCircle
     {

osu.Game.Tests/Beatmaps/SliderEventGenerationTest.cs

@@ -16,7 +16,7 @@ public class SliderEventGenerationTest
         [Test]
         public void TestSingleSpan()
         {
-            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, span_duration / 2, span_duration, 1, null).ToArray();
+            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, span_duration / 2, span_duration, 1).ToArray();
 
             Assert.That(events[0].Type, Is.EqualTo(SliderEventType.Head));
             Assert.That(events[0].Time, Is.EqualTo(start_time));
@@ -31,7 +31,7 @@ public void TestSingleSpan()
         [Test]
         public void TestRepeat()
         {
-            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, span_duration / 2, span_duration, 2, null).ToArray();
+            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, span_duration / 2, span_duration, 2).ToArray();
 
             Assert.That(events[0].Type, Is.EqualTo(SliderEventType.Head));
             Assert.That(events[0].Time, Is.EqualTo(start_time));
@@ -52,7 +52,7 @@ public void TestRepeat()
         [Test]
         public void TestNonEvenTicks()
         {
-            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, 300, span_duration, 2, null).ToArray();
+            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, 300, span_duration, 2).ToArray();
 
             Assert.That(events[0].Type, Is.EqualTo(SliderEventType.Head));
             Assert.That(events[0].Time, Is.EqualTo(start_time));
@@ -83,12 +83,12 @@ public void TestNonEvenTicks()
         }
 
         [Test]
-        public void TestLegacyLastTickOffset()
+        public void TestLastTickOffset()
         {
-            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, span_duration / 2, span_duration, 1, 100).ToArray();
+            var events = SliderEventGenerator.Generate(start_time, span_duration, 1, span_duration / 2, span_duration, 1).ToArray();
 
-            Assert.That(events[2].Type, Is.EqualTo(SliderEventType.LegacyLastTick));
-            Assert.That(events[2].Time, Is.EqualTo(900));
+            Assert.That(events[2].Type, Is.EqualTo(SliderEventType.LastTick));
+            Assert.That(events[2].Time, Is.EqualTo(span_duration + SliderEventGenerator.LAST_TICK_OFFSET));
         }
 
         [Test]
@@ -97,7 +97,7 @@ public void TestMinimumTickDistance()
             const double velocity = 5;
             const double min_distance = velocity * 10;
 
-            var events = SliderEventGenerator.Generate(start_time, span_duration, velocity, velocity, span_duration, 2, 0).ToArray();
+            var events = SliderEventGenerator.Generate(start_time, span_duration, velocity, velocity, span_duration, 2).ToArray();
 
             Assert.Multiple(() =>
             {

osu.Game/Rulesets/Objects/Legacy/ConvertSlider.cs

@@ -13,7 +13,7 @@
 
 namespace osu.Game.Rulesets.Objects.Legacy
 {
-    internal abstract class ConvertSlider : ConvertHitObject, IHasPathWithRepeats, IHasLegacyLastTickOffset, IHasSliderVelocity
+    internal abstract class ConvertSlider : ConvertHitObject, IHasPathWithRepeats, IHasSliderVelocity
     {
         /// <summary>
         /// Scoring distance with a speed-adjusted beat length of 1 second.
@@ -59,7 +59,5 @@ protected override void ApplyDefaultsToSelf(ControlPointInfo controlPointInfo, I
 
             Velocity = scoringDistance / timingPoint.BeatLength;
         }
-
-        public double LegacyLastTickOffset => 36;
     }
 }

osu.Game/Rulesets/Objects/SliderEventGenerator.cs

@@ -10,9 +10,17 @@ namespace osu.Game.Rulesets.Objects
 {
     public static class SliderEventGenerator
     {
-        // ReSharper disable once MethodOverloadWithOptionalParameter
+        /// <summary>
+        /// Historically, slider's final tick (aka the place where the slider would receive a final judgement) was offset by -36 ms. Originally this was
+        /// done to workaround a technical detail (unimportant), but over the years it has become an expectation of players that you don't need to hold
+        /// until the true end of the slider. This very small amount of leniency makes it easier to jump away from fast sliders to the next hit object.
+        ///
+        /// After discussion on how this should be handled going forward, players have unanimously stated that this lenience should remain in some way.
+        /// </summary>
+        public const double LAST_TICK_OFFSET = -36;
+
         public static IEnumerable<SliderEventDescriptor> Generate(double startTime, double spanDuration, double velocity, double tickDistance, double totalDistance, int spanCount,
-                                                                  double? legacyLastTickOffset, CancellationToken cancellationToken = default)
+                                                                  CancellationToken cancellationToken = default)
         {
             // A very lenient maximum length of a slider for ticks to be generated.
             // This exists for edge cases such as /b/1573664 where the beatmap has been edited by the user, and should never be reached in normal usage.
@@ -76,14 +84,14 @@ public static IEnumerable<SliderEventDescriptor> Generate(double startTime, doub
 
             int finalSpanIndex = spanCount - 1;
             double finalSpanStartTime = startTime + finalSpanIndex * spanDuration;
-            double finalSpanEndTime = Math.Max(startTime + totalDuration / 2, (finalSpanStartTime + spanDuration) - (legacyLastTickOffset ?? 0));
+            double finalSpanEndTime = Math.Max(startTime + totalDuration / 2, (finalSpanStartTime + spanDuration) + LAST_TICK_OFFSET);
             double finalProgress = (finalSpanEndTime - finalSpanStartTime) / spanDuration;
 
             if (spanCount % 2 == 0) finalProgress = 1 - finalProgress;
 
             yield return new SliderEventDescriptor
             {
-                Type = SliderEventType.LegacyLastTick,
+                Type = SliderEventType.LastTick,
                 SpanIndex = finalSpanIndex,
                 SpanStartTime = finalSpanStartTime,
                 Time = finalSpanEndTime,
@@ -173,7 +181,11 @@ public struct SliderEventDescriptor
     public enum SliderEventType
     {
         Tick,
-        LegacyLastTick,
+
+        /// <summary>
+        /// Occurs just before the tail. See <see cref="SliderEventGenerator.LAST_TICK_OFFSET"/>.
+        /// </summary>
+        LastTick,
         Head,
         Tail,
         Repeat