Merge pull request #1170 from smoogipooo/fix-speed-adjustments

Fix speed adjustments and hit objects within them being ordered incorrectly.

osu.Desktop.Tests/Visual/TestCaseScrollingPlayfield.cs

@@ -115,18 +115,18 @@ namespace osu.Desktop.Tests.Visual
             Assert.AreEqual(1, speedAdjustments[3].ControlPoint.Multiplier);
 
             // Check insertion of hit objects
-            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[0].Contains(hitObjects[0]));
+            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[4].Contains(hitObjects[0]));
-            Assert.IsTrue(speedAdjustments[0].Contains(hitObjects[1]));
+            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[3].Contains(hitObjects[1]));
-            Assert.IsTrue(speedAdjustments[1].Contains(hitObjects[2]));
+            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[2].Contains(hitObjects[2]));
-            Assert.IsTrue(speedAdjustments[2].Contains(hitObjects[3]));
+            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[1].Contains(hitObjects[3]));
-            Assert.IsTrue(speedAdjustments[3].Contains(hitObjects[4]));
+            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[0].Contains(hitObjects[4]));
-            Assert.IsTrue(speedAdjustments[3].Contains(hitObjects[5]));
+            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[0].Contains(hitObjects[5]));
 
-            hitObjectContainer.RemoveSpeedAdjustment(speedAdjustments[1]);
+            hitObjectContainer.RemoveSpeedAdjustment(hitObjectContainer.SpeedAdjustments[3]);
 
-            // The hit object contained in this speed adjustment should be resorted into the previous one
+            // The hit object contained in this speed adjustment should be resorted into the one occuring before it
 
-            Assert.IsTrue(speedAdjustments[0].Contains(hitObjects[2]));
+            Assert.IsTrue(hitObjectContainer.SpeedAdjustments[3].Contains(hitObjects[1]));
         }
 
         private class TestRulesetContainer : ScrollingRulesetContainer<TestPlayfield, TestHitObject, TestJudgement>

osu.Game/Rulesets/Timing/LinearScrollingContainer.cs

@@ -10,12 +10,10 @@ namespace osu.Game.Rulesets.Timing
     /// </summary>
     internal class LinearScrollingContainer : ScrollingContainer
     {
-        private readonly Axes scrollingAxes;
         private readonly MultiplierControlPoint controlPoint;
 
-        public LinearScrollingContainer(Axes scrollingAxes, MultiplierControlPoint controlPoint)
+        public LinearScrollingContainer(MultiplierControlPoint controlPoint)
         {
-            this.scrollingAxes = scrollingAxes;
             this.controlPoint = controlPoint;
         }
 
@@ -23,8 +21,8 @@ namespace osu.Game.Rulesets.Timing
         {
             base.Update();
 
-            if ((scrollingAxes & Axes.X) > 0) X = (float)(controlPoint.StartTime - Time.Current);
+            if ((ScrollingAxes & Axes.X) > 0) X = (float)(controlPoint.StartTime - Time.Current);
-            if ((scrollingAxes & Axes.Y) > 0) Y = (float)(controlPoint.StartTime - Time.Current);
+            if ((ScrollingAxes & Axes.Y) > 0) Y = (float)(controlPoint.StartTime - Time.Current);
         }
     }
 }

osu.Game/Rulesets/Timing/ScrollingContainer.cs

@@ -74,8 +74,7 @@ namespace osu.Game.Rulesets.Timing
             // absolutely-sized element along the scrolling axes and adding a corresponding duration value. This introduces a bit of error, but will never under-estimate.ion.
 
             // Find the largest element that is absolutely-sized along ScrollingAxes
-            float maxAbsoluteSize = Children.Where(c => (c.RelativeSizeAxes & ScrollingAxes) == 0)
+            float maxAbsoluteSize = Children.Select(c => (ScrollingAxes & Axes.X) > 0 ? c.DrawWidth : c.DrawHeight)
-                                            .Select(c => (ScrollingAxes & Axes.X) > 0 ? c.Width : c.Height)
                                             .DefaultIfEmpty().Max();
 
             float ourAbsoluteSize = (ScrollingAxes & Axes.X) > 0 ? DrawWidth : DrawHeight;
@@ -96,6 +95,8 @@ namespace osu.Game.Rulesets.Timing
         {
             base.Update();
 
+            RelativeChildOffset = new Vector2((ScrollingAxes & Axes.X) > 0 ? (float)ControlPoint.StartTime : 0, (ScrollingAxes & Axes.Y) > 0 ? (float)ControlPoint.StartTime : 0);
+
             // We want our size and position-space along the scrolling axes to span our duration to completely enclose all the hit objects
             Size = new Vector2((ScrollingAxes & Axes.X) > 0 ? (float)Duration : Size.X, (ScrollingAxes & Axes.Y) > 0 ? (float)Duration : Size.Y);
             // And we need to make sure the hit object's position-space doesn't change due to our resizing

osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs

@@ -31,7 +31,11 @@ namespace osu.Game.Rulesets.Timing
         /// <summary>
         /// The axes which the content of this container will scroll through.
         /// </summary>
-        public Axes ScrollingAxes { get; internal set; }
+        public Axes ScrollingAxes
+        {
+            get { return scrollingContainer.ScrollingAxes; }
+            set { scrollingContainer.ScrollingAxes = value; }
+        }
 
         public override bool RemoveWhenNotAlive => false;
 
@@ -52,11 +56,8 @@ namespace osu.Game.Rulesets.Timing
             RelativeSizeAxes = Axes.Both;
 
             scrollingContainer = CreateScrollingContainer();
-
-            scrollingContainer.ScrollingAxes = ScrollingAxes;
             scrollingContainer.ControlPoint = ControlPoint;
             scrollingContainer.VisibleTimeRange.BindTo(VisibleTimeRange);
-            scrollingContainer.RelativeChildOffset = new Vector2((ScrollingAxes & Axes.X) > 0 ? (float)ControlPoint.StartTime : 0, (ScrollingAxes & Axes.Y) > 0 ? (float)ControlPoint.StartTime : 0);
 
             AddInternal(content = scrollingContainer);
         }
@@ -98,11 +99,6 @@ namespace osu.Game.Rulesets.Timing
             base.Add(drawable);
         }
 
-        /// <summary>
-        /// Whether a <see cref="DrawableHitObject"/> falls within this <see cref="SpeedAdjustmentContainer"/>s affecting timespan.
-        /// </summary>
-        public bool CanContain(DrawableHitObject hitObject) => CanContain(hitObject.HitObject.StartTime);
-
         /// <summary>
         /// Whether a point in time falls within this <see cref="SpeedAdjustmentContainer"/>s affecting timespan.
         /// </summary>
@@ -112,6 +108,6 @@ namespace osu.Game.Rulesets.Timing
         /// Creates the <see cref="ScrollingContainer"/> which contains the scrolling <see cref="DrawableHitObject"/>s of this container.
         /// </summary>
         /// <returns>The <see cref="ScrollingContainer"/>.</returns>
-        protected virtual ScrollingContainer CreateScrollingContainer() => new LinearScrollingContainer(ScrollingAxes, ControlPoint);
+        protected virtual ScrollingContainer CreateScrollingContainer() => new LinearScrollingContainer(ControlPoint);
     }
 }

osu.Game/Rulesets/UI/ScrollingPlayfield.cs

@@ -151,7 +151,7 @@ namespace osu.Game.Rulesets.UI
             /// </summary>
             public readonly BindableBool Reversed = new BindableBool();
 
-            private readonly Container<SpeedAdjustmentContainer> speedAdjustments;
+            private readonly SortedContainer speedAdjustments;
             public IReadOnlyList<SpeedAdjustmentContainer> SpeedAdjustments => speedAdjustments;
 
             private readonly SpeedAdjustmentContainer defaultSpeedAdjustment;
@@ -166,14 +166,15 @@ namespace osu.Game.Rulesets.UI
             {
                 this.scrollingAxes = scrollingAxes;
 
-                AddInternal(speedAdjustments = new Container<SpeedAdjustmentContainer> { RelativeSizeAxes = Axes.Both });
+                AddInternal(speedAdjustments = new SortedContainer { RelativeSizeAxes = Axes.Both });
 
                 // Default speed adjustment
                 AddSpeedAdjustment(defaultSpeedAdjustment = new SpeedAdjustmentContainer(new MultiplierControlPoint(0)));
             }
 
             /// <summary>
-            /// Adds a <see cref="SpeedAdjustmentContainer"/> to this container.
+            /// Adds a <see cref="SpeedAdjustmentContainer"/> to this container, re-sorting all hit objects
+            /// in the last <see cref="SpeedAdjustmentContainer"/> that occurred (time-wise) before it.
             /// </summary>
             /// <param name="speedAdjustment">The <see cref="SpeedAdjustmentContainer"/>.</param>
             public void AddSpeedAdjustment(SpeedAdjustmentContainer speedAdjustment)
@@ -181,28 +182,29 @@ namespace osu.Game.Rulesets.UI
                 speedAdjustment.ScrollingAxes = scrollingAxes;
                 speedAdjustment.VisibleTimeRange.BindTo(VisibleTimeRange);
                 speedAdjustment.Reversed.BindTo(Reversed);
-                speedAdjustments.Add(speedAdjustment);
 
-                // We now need to re-sort the hit objects in the last speed adjustment prior to this one, to see if they need a new parent
+                if (speedAdjustments.Count > 0)
-                var previousSpeedAdjustment = speedAdjustments.LastOrDefault(s => s != speedAdjustment && s.ControlPoint.StartTime <= speedAdjustment.ControlPoint.StartTime);
-                if (previousSpeedAdjustment == null)
-                    return;
-
-                for (int i = 0; i < previousSpeedAdjustment.Children.Count; i++)
                 {
-                    DrawableHitObject hitObject = previousSpeedAdjustment[i];
+                    // We need to re-sort all hit objects in the speed adjustment container prior to figure out if they
+                    // should now lie within this one
+                    var existingAdjustment = adjustmentContainerAt(speedAdjustment.ControlPoint.StartTime);
+                    for (int i = 0; i < existingAdjustment.Count; i++)
+                    {
+                        DrawableHitObject hitObject = existingAdjustment[i];
 
-                    var newSpeedAdjustment = adjustmentContainerFor(hitObject);
+                        if (!speedAdjustment.CanContain(hitObject.HitObject.StartTime))
-                    if (newSpeedAdjustment == previousSpeedAdjustment)
                             continue;
 
-                    previousSpeedAdjustment.Remove(hitObject);
+                        existingAdjustment.Remove(hitObject);
-                    newSpeedAdjustment.Add(hitObject);
+                        speedAdjustment.Add(hitObject);
 
                         i--;
                     }
                 }
 
+                speedAdjustments.Add(speedAdjustment);
+            }
+
             /// <summary>
             /// Removes a <see cref="SpeedAdjustmentContainer"/> from this container, re-sorting all hit objects
             /// which it contained into new <see cref="SpeedAdjustmentContainer"/>s.
@@ -237,27 +239,32 @@ namespace osu.Game.Rulesets.UI
                 if (!(hitObject is IScrollingHitObject))
                     throw new InvalidOperationException($"Hit objects added to a {nameof(ScrollingHitObjectContainer)} must implement {nameof(IScrollingHitObject)}.");
 
-                adjustmentContainerFor(hitObject).Add(hitObject);
+                adjustmentContainerAt(hitObject.HitObject.StartTime).Add(hitObject);
             }
 
             public override bool Remove(DrawableHitObject hitObject) => speedAdjustments.Any(s => s.Remove(hitObject));
 
-            /// <summary>
-            /// Finds the <see cref="SpeedAdjustmentContainer"/> which provides the speed adjustment active at the start time
-            /// of a hit object. If there is no <see cref="SpeedAdjustmentContainer"/> active at the start time of the hit object,
-            /// then the first (time-wise) speed adjustment is returned.
-            /// </summary>
-            /// <param name="hitObject">The hit object to find the active <see cref="SpeedAdjustmentContainer"/> for.</param>
-            /// <returns>The <see cref="SpeedAdjustmentContainer"/> active at <paramref name="hitObject"/>'s start time. Null if there are no speed adjustments.</returns>
-            private SpeedAdjustmentContainer adjustmentContainerFor(DrawableHitObject hitObject) => speedAdjustments.LastOrDefault(c => c.CanContain(hitObject)) ?? defaultSpeedAdjustment;
-
             /// <summary>
             /// Finds the <see cref="SpeedAdjustmentContainer"/> which provides the speed adjustment active at a time.
             /// If there is no <see cref="SpeedAdjustmentContainer"/> active at the time, then the first (time-wise) speed adjustment is returned.
             /// </summary>
             /// <param name="time">The time to find the active <see cref="SpeedAdjustmentContainer"/> at.</param>
             /// <returns>The <see cref="SpeedAdjustmentContainer"/> active at <paramref name="time"/>. Null if there are no speed adjustments.</returns>
-            private SpeedAdjustmentContainer adjustmentContainerAt(double time) => speedAdjustments.LastOrDefault(c => c.CanContain(time)) ?? defaultSpeedAdjustment;
+            private SpeedAdjustmentContainer adjustmentContainerAt(double time) => speedAdjustments.FirstOrDefault(c => c.CanContain(time)) ?? defaultSpeedAdjustment;
+
+            private class SortedContainer : Container<SpeedAdjustmentContainer>
+            {
+                protected override int Compare(Drawable x, Drawable y)
+                {
+                    var sX = (SpeedAdjustmentContainer)x;
+                    var sY = (SpeedAdjustmentContainer)y;
+
+                    int result = sY.ControlPoint.StartTime.CompareTo(sX.ControlPoint.StartTime);
+                    if (result != 0)
+                        return result;
+                    return base.Compare(y, x);
+                }
+            }
         }
     }
 }