From 81485c548c84226d4e6f59e9b7f221b376eb449d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 29 Sep 2023 14:19:26 +0900 Subject: [PATCH 1/2] Move `LegacyLastTickOffset` specification to generation code and stop passing everywhere --- .../Beatmaps/CatchBeatmapConverter.cs | 1 - .../Objects/JuiceStream.cs | 4 +--- .../Editor/TestSceneSliderSplitting.cs | 1 - .../Beatmaps/OsuBeatmapConverter.cs | 1 - .../Sliders/SliderSelectionBlueprint.cs | 1 - .../Mods/OsuModStrictTracking.cs | 5 ++--- osu.Game.Rulesets.Osu/Objects/Slider.cs | 6 ++--- .../Objects/SliderTailCircle.cs | 2 +- .../Beatmaps/SliderEventGenerationTest.cs | 16 +++++++------- .../Rulesets/Objects/Legacy/ConvertSlider.cs | 4 +--- .../Rulesets/Objects/SliderEventGenerator.cs | 22 ++++++++++++++----- .../Objects/Types/IHasLegacyLastTickOffset.cs | 14 ------------ 12 files changed, 32 insertions(+), 45 deletions(-) delete mode 100644 osu.Game/Rulesets/Objects/Types/IHasLegacyLastTickOffset.cs diff --git a/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapConverter.cs b/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapConverter.cs index 5e8a0b121670..6a24c26844c2 100644 --- a/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapConverter.cs +++ b/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapConverter.cs @@ -41,7 +41,6 @@ protected override IEnumerable 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(); diff --git a/osu.Game.Rulesets.Catch/Objects/JuiceStream.cs b/osu.Game.Rulesets.Catch/Objects/JuiceStream.cs index 724bdc34012b..87d316d36d8c 100644 --- a/osu.Game.Rulesets.Catch/Objects/JuiceStream.cs +++ b/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) @@ -162,7 +162,5 @@ public SliderPath Path public double Distance => Path.Distance; public IList> NodeSamples { get; set; } = new List>(); - - public double? LegacyLastTickOffset { get; set; } } } diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSplitting.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSplitting.cs index 8ba97892feb8..7315344295d7 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSplitting.cs +++ b/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), diff --git a/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapConverter.cs b/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapConverter.cs index f3aaf831d339..3c051a6bb1aa 100644 --- a/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapConverter.cs +++ b/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapConverter.cs @@ -44,7 +44,6 @@ protected override IEnumerable 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 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)n.Select(s => s.With()).ToList()).ToList(), diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModStrictTracking.cs b/osu.Game.Rulesets.Osu/Mods/OsuModStrictTracking.cs index 9b5d405025e1..78062a0632fb 100644 --- a/osu.Game.Rulesets.Osu/Mods/OsuModStrictTracking.cs +++ b/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, diff --git a/osu.Game.Rulesets.Osu/Objects/Slider.cs b/osu.Game.Rulesets.Osu/Objects/Slider.cs index e05dbd8ea6fe..9736c69e174d 100644 --- a/osu.Game.Rulesets.Osu/Objects/Slider.cs +++ b/osu.Game.Rulesets.Osu/Objects/Slider.cs @@ -71,8 +71,6 @@ public override Vector2 Position } } - public double? LegacyLastTickOffset { get; set; } - /// /// The position of the cursor at the point of completion of this 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,7 +204,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok }); break; - case SliderEventType.LegacyLastTick: + case SliderEventType.LastTick: // 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. diff --git a/osu.Game.Rulesets.Osu/Objects/SliderTailCircle.cs b/osu.Game.Rulesets.Osu/Objects/SliderTailCircle.cs index b4574791d2d6..fb819368377c 100644 --- a/osu.Game.Rulesets.Osu/Objects/SliderTailCircle.cs +++ b/osu.Game.Rulesets.Osu/Objects/SliderTailCircle.cs @@ -10,7 +10,7 @@ namespace osu.Game.Rulesets.Osu.Objects { /// /// Note that this should not be used for timing correctness. - /// See usage in for more information. + /// See usage in for more information. /// public class SliderTailCircle : SliderEndCircle { diff --git a/osu.Game.Tests/Beatmaps/SliderEventGenerationTest.cs b/osu.Game.Tests/Beatmaps/SliderEventGenerationTest.cs index a26c8121ddc0..37a91c8611de 100644 --- a/osu.Game.Tests/Beatmaps/SliderEventGenerationTest.cs +++ b/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(() => { diff --git a/osu.Game/Rulesets/Objects/Legacy/ConvertSlider.cs b/osu.Game/Rulesets/Objects/Legacy/ConvertSlider.cs index d91ecf956a7a..ab8fd2c66271 100644 --- a/osu.Game/Rulesets/Objects/Legacy/ConvertSlider.cs +++ b/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 { /// /// 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; } } diff --git a/osu.Game/Rulesets/Objects/SliderEventGenerator.cs b/osu.Game/Rulesets/Objects/SliderEventGenerator.cs index 7013d32cbcc1..b3477a5fde4d 100644 --- a/osu.Game/Rulesets/Objects/SliderEventGenerator.cs +++ b/osu.Game/Rulesets/Objects/SliderEventGenerator.cs @@ -10,9 +10,17 @@ namespace osu.Game.Rulesets.Objects { public static class SliderEventGenerator { - // ReSharper disable once MethodOverloadWithOptionalParameter + /// + /// 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. + /// + public const double LAST_TICK_OFFSET = -36; + public static IEnumerable 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 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, + + /// + /// Occurs just before the tail. See . + /// + LastTick, Head, Tail, Repeat diff --git a/osu.Game/Rulesets/Objects/Types/IHasLegacyLastTickOffset.cs b/osu.Game/Rulesets/Objects/Types/IHasLegacyLastTickOffset.cs deleted file mode 100644 index caf22c3023fc..000000000000 --- a/osu.Game/Rulesets/Objects/Types/IHasLegacyLastTickOffset.cs +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. -// See the LICENCE file in the repository root for full licence text. - -namespace osu.Game.Rulesets.Objects.Types -{ - /// - /// A type of which may require the last tick to be offset. - /// This is specific to osu!stable conversion, and should not be used elsewhere. - /// - public interface IHasLegacyLastTickOffset - { - double LegacyLastTickOffset { get; } - } -} From d7119674e8b5063d5350fbed78046aa873865a45 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 29 Sep 2023 14:40:44 +0900 Subject: [PATCH 2/2] Update comments to better explain what `LastTick` is doing --- osu.Game.Rulesets.Catch/Objects/JuiceStream.cs | 4 ++-- osu.Game.Rulesets.Osu/Mods/OsuModSynesthesia.cs | 2 +- .../Objects/Drawables/DrawableSlider.cs | 2 +- osu.Game.Rulesets.Osu/Objects/Slider.cs | 11 +++++++---- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/osu.Game.Rulesets.Catch/Objects/JuiceStream.cs b/osu.Game.Rulesets.Catch/Objects/JuiceStream.cs index 87d316d36d8c..fb1a86d8c05e 100644 --- a/osu.Game.Rulesets.Catch/Objects/JuiceStream.cs +++ b/osu.Game.Rulesets.Catch/Objects/JuiceStream.cs @@ -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) diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModSynesthesia.cs b/osu.Game.Rulesets.Osu/Mods/OsuModSynesthesia.cs index 9537f8b38843..e1123807cd94 100644 --- a/osu.Game.Rulesets.Osu/Mods/OsuModSynesthesia.cs +++ b/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() diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs index 1a6a0a9ecc0f..77e60a1690ec 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs @@ -288,7 +288,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(); } diff --git a/osu.Game.Rulesets.Osu/Objects/Slider.cs b/osu.Game.Rulesets.Osu/Objects/Slider.cs index 9736c69e174d..443e4229d25c 100644 --- a/osu.Game.Rulesets.Osu/Objects/Slider.cs +++ b/osu.Game.Rulesets.Osu/Objects/Slider.cs @@ -205,9 +205,10 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok break; case SliderEventType.LastTick: - // 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. + // 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, @@ -262,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); }