Merge pull request #4729 from peppy/fix-control-point-ordering

Fix control points with same timestamp potentially being parsed incorrectly

osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs

@@ -170,27 +170,98 @@ namespace osu.Game.Tests.Beatmaps.Formats
                 var controlPoints = beatmap.ControlPointInfo;
 
                 Assert.AreEqual(4, controlPoints.TimingPoints.Count);
-                var timingPoint = controlPoints.TimingPoints[0];
+                Assert.AreEqual(42, controlPoints.DifficultyPoints.Count);
+                Assert.AreEqual(42, controlPoints.SamplePoints.Count);
+                Assert.AreEqual(42, controlPoints.EffectPoints.Count);
+
+                var timingPoint = controlPoints.TimingPointAt(0);
+                Assert.AreEqual(956, timingPoint.Time);
+                Assert.AreEqual(329.67032967033, timingPoint.BeatLength);
+                Assert.AreEqual(TimeSignatures.SimpleQuadruple, timingPoint.TimeSignature);
+
+                timingPoint = controlPoints.TimingPointAt(48428);
                 Assert.AreEqual(956, timingPoint.Time);
                 Assert.AreEqual(329.67032967033d, timingPoint.BeatLength);
                 Assert.AreEqual(TimeSignatures.SimpleQuadruple, timingPoint.TimeSignature);
 
-                Assert.AreEqual(5, controlPoints.DifficultyPoints.Count);
-                var difficultyPoint = controlPoints.DifficultyPoints[0];
-                Assert.AreEqual(116999, difficultyPoint.Time);
-                Assert.AreEqual(0.75000000000000189d, difficultyPoint.SpeedMultiplier);
+                timingPoint = controlPoints.TimingPointAt(119637);
+                Assert.AreEqual(119637, timingPoint.Time);
+                Assert.AreEqual(659.340659340659, timingPoint.BeatLength);
+                Assert.AreEqual(TimeSignatures.SimpleQuadruple, timingPoint.TimeSignature);
 
-                Assert.AreEqual(34, controlPoints.SamplePoints.Count);
-                var soundPoint = controlPoints.SamplePoints[0];
+                var difficultyPoint = controlPoints.DifficultyPointAt(0);
+                Assert.AreEqual(0, difficultyPoint.Time);
+                Assert.AreEqual(1.0, difficultyPoint.SpeedMultiplier);
+
+                difficultyPoint = controlPoints.DifficultyPointAt(48428);
+                Assert.AreEqual(48428, difficultyPoint.Time);
+                Assert.AreEqual(1.0, difficultyPoint.SpeedMultiplier);
+
+                difficultyPoint = controlPoints.DifficultyPointAt(116999);
+                Assert.AreEqual(116999, difficultyPoint.Time);
+                Assert.AreEqual(0.75, difficultyPoint.SpeedMultiplier, 0.1);
+
+                var soundPoint = controlPoints.SamplePointAt(0);
                 Assert.AreEqual(956, soundPoint.Time);
                 Assert.AreEqual("soft", soundPoint.SampleBank);
                 Assert.AreEqual(60, soundPoint.SampleVolume);
 
-                Assert.AreEqual(8, controlPoints.EffectPoints.Count);
-                var effectPoint = controlPoints.EffectPoints[0];
+                soundPoint = controlPoints.SamplePointAt(53373);
+                Assert.AreEqual(53373, soundPoint.Time);
+                Assert.AreEqual("soft", soundPoint.SampleBank);
+                Assert.AreEqual(60, soundPoint.SampleVolume);
+
+                soundPoint = controlPoints.SamplePointAt(119637);
+                Assert.AreEqual(119637, soundPoint.Time);
+                Assert.AreEqual("soft", soundPoint.SampleBank);
+                Assert.AreEqual(80, soundPoint.SampleVolume);
+
+                var effectPoint = controlPoints.EffectPointAt(0);
+                Assert.AreEqual(0, effectPoint.Time);
+                Assert.IsFalse(effectPoint.KiaiMode);
+                Assert.IsFalse(effectPoint.OmitFirstBarLine);
+
+                effectPoint = controlPoints.EffectPointAt(53703);
                 Assert.AreEqual(53703, effectPoint.Time);
                 Assert.IsTrue(effectPoint.KiaiMode);
                 Assert.IsFalse(effectPoint.OmitFirstBarLine);
+
+                effectPoint = controlPoints.EffectPointAt(119637);
+                Assert.AreEqual(119637, effectPoint.Time);
+                Assert.IsFalse(effectPoint.KiaiMode);
+                Assert.IsFalse(effectPoint.OmitFirstBarLine);
+            }
+        }
+
+        [Test]
+        public void TestDecodeOverlappingTimingPoints()
+        {
+            var decoder = new LegacyBeatmapDecoder { ApplyOffsets = false };
+
+            using (var resStream = TestResources.OpenResource("overlapping-control-points.osu"))
+            using (var stream = new StreamReader(resStream))
+            {
+                var controlPoints = decoder.Decode(stream).ControlPointInfo;
+
+                Assert.That(controlPoints.DifficultyPointAt(500).SpeedMultiplier, Is.EqualTo(1.5).Within(0.1));
+                Assert.That(controlPoints.DifficultyPointAt(1500).SpeedMultiplier, Is.EqualTo(1.5).Within(0.1));
+                Assert.That(controlPoints.DifficultyPointAt(2500).SpeedMultiplier, Is.EqualTo(0.75).Within(0.1));
+                Assert.That(controlPoints.DifficultyPointAt(3500).SpeedMultiplier, Is.EqualTo(1.5).Within(0.1));
+
+                Assert.That(controlPoints.EffectPointAt(500).KiaiMode, Is.True);
+                Assert.That(controlPoints.EffectPointAt(1500).KiaiMode, Is.True);
+                Assert.That(controlPoints.EffectPointAt(2500).KiaiMode, Is.False);
+                Assert.That(controlPoints.EffectPointAt(3500).KiaiMode, Is.True);
+
+                Assert.That(controlPoints.SamplePointAt(500).SampleBank, Is.EqualTo("drum"));
+                Assert.That(controlPoints.SamplePointAt(1500).SampleBank, Is.EqualTo("drum"));
+                Assert.That(controlPoints.SamplePointAt(2500).SampleBank, Is.EqualTo("normal"));
+                Assert.That(controlPoints.SamplePointAt(3500).SampleBank, Is.EqualTo("drum"));
+
+                Assert.That(controlPoints.TimingPointAt(500).BeatLength, Is.EqualTo(500).Within(0.1));
+                Assert.That(controlPoints.TimingPointAt(1500).BeatLength, Is.EqualTo(500).Within(0.1));
+                Assert.That(controlPoints.TimingPointAt(2500).BeatLength, Is.EqualTo(250).Within(0.1));
+                Assert.That(controlPoints.TimingPointAt(3500).BeatLength, Is.EqualTo(500).Within(0.1));
             }
         }
 

osu.Game.Tests/Resources/overlapping-control-points.osu

@@ -0,0 +1,19 @@
+osu file format v14
+
+[TimingPoints]
+
+// Timing then inherited
+0,500,4,2,0,100,1,0
+0,-66.6666666666667,4,3,0,100,0,1
+
+// Inherited then timing (equivalent to previous)
+1000,-66.6666666666667,4,3,0,100,0,1
+1000,500,4,2,0,100,1,0
+
+// Inherited then timing (different to previous)
+2000,-133.333333333333,4,1,0,100,0,0
+2000,250,4,2,0,100,1,0
+
+// Timing then inherited (different to previous)
+3000,500,4,2,0,100,1,0
+3000,-66.6666666666667,4,3,0,100,0,1

osu.Game/Beatmaps/ControlPoints/ControlPoint.cs

@@ -12,17 +12,14 @@ namespace osu.Game.Beatmaps.ControlPoints
         /// </summary>
         public double Time;
 
+        /// <summary>
+        /// Whether this timing point was generated internally, as opposed to parsed from the underlying beatmap.
+        /// </summary>
+        internal bool AutoGenerated;
+
         public int CompareTo(ControlPoint other) => Time.CompareTo(other.Time);
 
-        /// <summary>
-        /// Whether this <see cref="ControlPoint"/> provides the same parametric changes as another <see cref="ControlPoint"/>.
-        /// Basically an equality check without considering the <see cref="Time"/>.
-        /// </summary>
-        /// <param name="other">The <see cref="ControlPoint"/> to compare to.</param>
-        /// <returns>Whether this <see cref="ControlPoint"/> is equivalent to <paramref name="other"/>.</returns>
-        public virtual bool EquivalentTo(ControlPoint other) => true;
-
         public bool Equals(ControlPoint other)
-            => EquivalentTo(other) && Time.Equals(other?.Time);
+            => Time.Equals(other?.Time);
     }
 }

osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs

@@ -1,11 +1,12 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using osuTK;
 
 namespace osu.Game.Beatmaps.ControlPoints
 {
-    public class DifficultyControlPoint : ControlPoint
+    public class DifficultyControlPoint : ControlPoint, IEquatable<DifficultyControlPoint>
     {
         /// <summary>
         /// The speed multiplier at this control point.
@@ -18,9 +19,8 @@ namespace osu.Game.Beatmaps.ControlPoints
 
         private double speedMultiplier = 1;
 
-        public override bool EquivalentTo(ControlPoint other)
-            => base.EquivalentTo(other)
-               && other is DifficultyControlPoint difficulty
-               && SpeedMultiplier.Equals(difficulty.SpeedMultiplier);
+        public bool Equals(DifficultyControlPoint other)
+            => base.Equals(other)
+               && SpeedMultiplier.Equals(other?.SpeedMultiplier);
     }
 }

osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs

@@ -1,9 +1,11 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
+
 namespace osu.Game.Beatmaps.ControlPoints
 {
-    public class EffectControlPoint : ControlPoint
+    public class EffectControlPoint : ControlPoint, IEquatable<EffectControlPoint>
     {
         /// <summary>
         /// Whether this control point enables Kiai mode.
@@ -15,10 +17,8 @@ namespace osu.Game.Beatmaps.ControlPoints
         /// </summary>
         public bool OmitFirstBarLine;
 
-        public override bool EquivalentTo(ControlPoint other)
-            => base.EquivalentTo(other)
-               && other is EffectControlPoint effect
-               && KiaiMode.Equals(effect.KiaiMode)
-               && OmitFirstBarLine.Equals(effect.OmitFirstBarLine);
+        public bool Equals(EffectControlPoint other)
+            => base.Equals(other)
+               && KiaiMode == other?.KiaiMode && OmitFirstBarLine == other.OmitFirstBarLine;
     }
 }

osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs

@@ -1,11 +1,12 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using osu.Game.Audio;
 
 namespace osu.Game.Beatmaps.ControlPoints
 {
-    public class SampleControlPoint : ControlPoint
+    public class SampleControlPoint : ControlPoint, IEquatable<SampleControlPoint>
     {
         public const string DEFAULT_BANK = "normal";
 
@@ -44,10 +45,8 @@ namespace osu.Game.Beatmaps.ControlPoints
             return newSampleInfo;
         }
 
-        public override bool EquivalentTo(ControlPoint other)
-            => base.EquivalentTo(other)
-               && other is SampleControlPoint sample
-               && SampleBank.Equals(sample.SampleBank)
-               && SampleVolume.Equals(sample.SampleVolume);
+        public bool Equals(SampleControlPoint other)
+            => base.Equals(other)
+               && string.Equals(SampleBank, other?.SampleBank) && SampleVolume == other?.SampleVolume;
     }
 }

osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs

@@ -1,12 +1,13 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using osuTK;
 using osu.Game.Beatmaps.Timing;
 
 namespace osu.Game.Beatmaps.ControlPoints
 {
-    public class TimingControlPoint : ControlPoint
+    public class TimingControlPoint : ControlPoint, IEquatable<TimingControlPoint>
     {
         /// <summary>
         /// The time signature at this control point.
@@ -24,10 +25,8 @@ namespace osu.Game.Beatmaps.ControlPoints
 
         private double beatLength = 1000;
 
-        public override bool EquivalentTo(ControlPoint other)
-            => base.EquivalentTo(other)
-               && other is TimingControlPoint timing
-               && TimeSignature.Equals(timing.TimeSignature)
-               && BeatLength.Equals(timing.BeatLength);
+        public bool Equals(TimingControlPoint other)
+            => base.Equals(other)
+               && TimeSignature == other?.TimeSignature && beatLength.Equals(other.beatLength);
     }
 }

osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs

@@ -374,14 +374,16 @@ namespace osu.Game.Beatmaps.Formats
                 handleDifficultyControlPoint(new DifficultyControlPoint
                 {
                     Time = time,
-                    SpeedMultiplier = speedMultiplier
+                    SpeedMultiplier = speedMultiplier,
+                    AutoGenerated = timingChange
                 });
 
                 handleEffectControlPoint(new EffectControlPoint
                 {
                     Time = time,
                     KiaiMode = kiaiMode,
-                    OmitFirstBarLine = omitFirstBarSignature
+                    OmitFirstBarLine = omitFirstBarSignature,
+                    AutoGenerated = timingChange
                 });
 
                 handleSampleControlPoint(new LegacySampleControlPoint
@@ -389,7 +391,8 @@ namespace osu.Game.Beatmaps.Formats
                     Time = time,
                     SampleBank = stringSampleSet,
                     SampleVolume = sampleVolume,
-                    CustomSampleBank = customSampleBank
+                    CustomSampleBank = customSampleBank,
+                    AutoGenerated = timingChange
                 });
             }
             catch (FormatException)
@@ -407,7 +410,14 @@ namespace osu.Game.Beatmaps.Formats
             var existing = beatmap.ControlPointInfo.TimingPointAt(newPoint.Time);
 
             if (existing.Time == newPoint.Time)
+            {
+                // autogenerated points should not replace non-autogenerated.
+                // this allows for incorrectly ordered timing points to still be correctly handled.
+                if (newPoint.AutoGenerated && !existing.AutoGenerated)
+                    return;
+
                 beatmap.ControlPointInfo.TimingPoints.Remove(existing);
+            }
 
             beatmap.ControlPointInfo.TimingPoints.Add(newPoint);
         }
@@ -416,11 +426,15 @@ namespace osu.Game.Beatmaps.Formats
         {
             var existing = beatmap.ControlPointInfo.DifficultyPointAt(newPoint.Time);
 
-            if (newPoint.EquivalentTo(existing))
-                return;
-
             if (existing.Time == newPoint.Time)
+            {
+                // autogenerated points should not replace non-autogenerated.
+                // this allows for incorrectly ordered timing points to still be correctly handled.
+                if (newPoint.AutoGenerated && !existing.AutoGenerated)
+                    return;
+
                 beatmap.ControlPointInfo.DifficultyPoints.Remove(existing);
+            }
 
             beatmap.ControlPointInfo.DifficultyPoints.Add(newPoint);
         }
@@ -429,11 +443,15 @@ namespace osu.Game.Beatmaps.Formats
         {
             var existing = beatmap.ControlPointInfo.EffectPointAt(newPoint.Time);
 
-            if (newPoint.EquivalentTo(existing))
-                return;
-
             if (existing.Time == newPoint.Time)
+            {
+                // autogenerated points should not replace non-autogenerated.
+                // this allows for incorrectly ordered timing points to still be correctly handled.
+                if (newPoint.AutoGenerated && !existing.AutoGenerated)
+                    return;
+
                 beatmap.ControlPointInfo.EffectPoints.Remove(existing);
+            }
 
             beatmap.ControlPointInfo.EffectPoints.Add(newPoint);
         }
@@ -442,11 +460,15 @@ namespace osu.Game.Beatmaps.Formats
         {
             var existing = beatmap.ControlPointInfo.SamplePointAt(newPoint.Time);
 
-            if (newPoint.EquivalentTo(existing))
-                return;
-
             if (existing.Time == newPoint.Time)
+            {
+                // autogenerated points should not replace non-autogenerated.
+                // this allows for incorrectly ordered timing points to still be correctly handled.
+                if (newPoint.AutoGenerated && !existing.AutoGenerated)
+                    return;
+
                 beatmap.ControlPointInfo.SamplePoints.Remove(existing);
+            }
 
             beatmap.ControlPointInfo.SamplePoints.Add(newPoint);
         }

osu.Game/Beatmaps/Formats/LegacyDecoder.cs

@@ -189,7 +189,7 @@ namespace osu.Game.Beatmaps.Formats
             Foreground = 3
         }
 
-        internal class LegacySampleControlPoint : SampleControlPoint
+        internal class LegacySampleControlPoint : SampleControlPoint, IEquatable<LegacySampleControlPoint>
         {
             public int CustomSampleBank;
 
@@ -203,10 +203,9 @@ namespace osu.Game.Beatmaps.Formats
                 return baseInfo;
             }
 
-            public override bool EquivalentTo(ControlPoint other)
-                => base.EquivalentTo(other)
-                   && other is LegacySampleControlPoint legacy
-                   && CustomSampleBank == legacy.CustomSampleBank;
+            public bool Equals(LegacySampleControlPoint other)
+                => base.Equals(other)
+                   && CustomSampleBank == other?.CustomSampleBank;
         }
     }
 }