From 5fcd94880d19f36fba5566f03d3bd0aed6359a36 Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 15:58:21 +0900 Subject: [PATCH 1/9] Fix incorrect inequality with multiple speed adjustments at the same start time. --- osu.Game/Rulesets/UI/ScrollingPlayfield.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs index 0a8469c928..9745e5d098 100644 --- a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs @@ -181,7 +181,7 @@ namespace osu.Game.Rulesets.UI 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 - var previousSpeedAdjustment = speedAdjustments.LastOrDefault(s => s.ControlPoint.StartTime < speedAdjustment.ControlPoint.StartTime); + var previousSpeedAdjustment = speedAdjustments.LastOrDefault(s => s.ControlPoint.StartTime <= speedAdjustment.ControlPoint.StartTime); if (previousSpeedAdjustment == null) return; From b7b8d8b76461b29c9cab011c94460fdbb1e5c29e Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 15:59:38 +0900 Subject: [PATCH 2/9] Let's not construct scrolling containers in load() for now This isn't utilized at the moment, and we should be avoiding this here in the first place. --- osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs b/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs index 5d6c03b9de..0ca6fc84c2 100644 --- a/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs +++ b/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs @@ -51,11 +51,7 @@ namespace osu.Game.Rulesets.Timing { ControlPoint = controlPoint; RelativeSizeAxes = Axes.Both; - } - [BackgroundDependencyLoader] - private void load() - { scrollingContainer = CreateScrollingContainer(); scrollingContainer.ScrollingAxes = ScrollingAxes; From 6f662d721cb47e33dafcd3c9a8b44e9914b9da1f Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 16:00:26 +0900 Subject: [PATCH 3/9] Fix possible CollectionModifiedException while adding new SpeedAdjustmentContainers --- osu.Game/Rulesets/UI/ScrollingPlayfield.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs index 9745e5d098..2678df7d44 100644 --- a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs @@ -185,14 +185,18 @@ namespace osu.Game.Rulesets.UI if (previousSpeedAdjustment == null) return; - foreach (DrawableHitObject h in previousSpeedAdjustment.Children) + for (int i = 0; i < previousSpeedAdjustment.Children.Count; i++) { - var newSpeedAdjustment = adjustmentContainerFor(h); + DrawableHitObject hitObject = previousSpeedAdjustment[i]; + + var newSpeedAdjustment = adjustmentContainerFor(hitObject); if (newSpeedAdjustment == previousSpeedAdjustment) continue; - previousSpeedAdjustment.Remove(h); - newSpeedAdjustment.Add(h); + previousSpeedAdjustment.Remove(hitObject); + newSpeedAdjustment.Add(hitObject); + + i--; } } From 4fc77be62498ae4ae2d998cf38cc8b991a4d11b0 Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 16:00:59 +0900 Subject: [PATCH 4/9] Fix incorrect sorting of hit objects into SpeedAdjustmentContainers --- osu.Game/Rulesets/UI/ScrollingPlayfield.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs index 2678df7d44..09a8e50809 100644 --- a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs @@ -228,7 +228,7 @@ namespace osu.Game.Rulesets.UI /// /// The hit object to find the active for. /// The active at 's start time. Null if there are no speed adjustments. - private SpeedAdjustmentContainer adjustmentContainerFor(DrawableHitObject hitObject) => speedAdjustments.FirstOrDefault(c => c.CanContain(hitObject)) ?? speedAdjustments.LastOrDefault(); + private SpeedAdjustmentContainer adjustmentContainerFor(DrawableHitObject hitObject) => speedAdjustments.LastOrDefault(c => c.CanContain(hitObject)) ?? speedAdjustments.FirstOrDefault(); /// /// Finds the which provides the speed adjustment active at a time. @@ -236,7 +236,7 @@ namespace osu.Game.Rulesets.UI /// /// The time to find the active at. /// The active at . Null if there are no speed adjustments. - private SpeedAdjustmentContainer adjustmentContainerAt(double time) => speedAdjustments.FirstOrDefault(c => c.CanContain(time)) ?? speedAdjustments.LastOrDefault(); + private SpeedAdjustmentContainer adjustmentContainerAt(double time) => speedAdjustments.LastOrDefault(c => c.CanContain(time)) ?? speedAdjustments.FirstOrDefault(); } } } From c3cfad4eb5a009d650a3603093a3afc0b3799ff6 Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 16:01:19 +0900 Subject: [PATCH 5/9] Add RemoveSpeedAdjustment --- osu.Game/Rulesets/UI/ScrollingPlayfield.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs index 09a8e50809..aea224ab9e 100644 --- a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs @@ -200,6 +200,25 @@ namespace osu.Game.Rulesets.UI } } + /// + /// Removes a from this container, re-sorting all hit objects + /// which it contained into new s. + /// + /// The to remove. + public void RemoveSpeedAdjustment(SpeedAdjustmentContainer speedAdjustment) + { + if (!speedAdjustments.Remove(speedAdjustment)) + return; + + while (speedAdjustment.Count > 0) + { + DrawableHitObject hitObject = speedAdjustment[0]; + + speedAdjustment.Remove(hitObject); + Add(hitObject); + } + } + public override IEnumerable Objects => speedAdjustments.SelectMany(s => s.Children); /// From d20ea97e7f8222296b99a21528d66316132cf191 Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 16:05:12 +0900 Subject: [PATCH 6/9] Add NUnit test for ScrollingHitObjectContainer Tests ordering of SpeedAdjustmentContainers and DrawableHitObjects added to the ScrollingHitObjectContainer. --- .../Visual/TestCaseScrollingPlayfield.cs | 67 ++++++++++++++++++- osu.Game/Rulesets/UI/ScrollingPlayfield.cs | 5 +- 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/osu.Desktop.Tests/Visual/TestCaseScrollingPlayfield.cs b/osu.Desktop.Tests/Visual/TestCaseScrollingPlayfield.cs index e9c29d36c2..2d49b2d0f9 100644 --- a/osu.Desktop.Tests/Visual/TestCaseScrollingPlayfield.cs +++ b/osu.Desktop.Tests/Visual/TestCaseScrollingPlayfield.cs @@ -3,19 +3,23 @@ using System; using System.Collections.Generic; +using NUnit.Framework; using OpenTK; using osu.Desktop.Tests.Beatmaps; +using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Shapes; using osu.Framework.Input; using osu.Framework.Timing; using osu.Game.Beatmaps; +using osu.Game.Beatmaps.ControlPoints; using osu.Game.Rulesets.Beatmaps; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Objects.Drawables; using osu.Game.Rulesets.Scoring; +using osu.Game.Rulesets.Timing; using osu.Game.Rulesets.UI; namespace osu.Desktop.Tests.Visual @@ -23,6 +27,7 @@ namespace osu.Desktop.Tests.Visual /// /// The most minimal implementation of a playfield with scrolling hit objects. /// + [TestFixture] public class TestCaseScrollingPlayfield : OsuTestCase { public TestCaseScrollingPlayfield() @@ -64,6 +69,66 @@ namespace osu.Desktop.Tests.Visual }); } + [Test] + public void TestSpeedAdjustmentOrdering() + { + var hitObjectContainer = new ScrollingPlayfield.ScrollingHitObjectContainer(Axes.X); + + var speedAdjustments = new[] + { + new SpeedAdjustmentContainer(new MultiplierControlPoint()), + new SpeedAdjustmentContainer(new MultiplierControlPoint(1000) + { + TimingPoint = new TimingControlPoint { BeatLength = 500 } + }), + new SpeedAdjustmentContainer(new MultiplierControlPoint(2000) + { + TimingPoint = new TimingControlPoint { BeatLength = 1000 }, + DifficultyPoint = new DifficultyControlPoint { SpeedMultiplier = 2} + }), + new SpeedAdjustmentContainer(new MultiplierControlPoint(3000) + { + TimingPoint = new TimingControlPoint { BeatLength = 1000 }, + DifficultyPoint = new DifficultyControlPoint { SpeedMultiplier = 1} + }), + }; + + var hitObjects = new[] + { + new DrawableTestHitObject(Axes.X, new TestHitObject { StartTime = -1000 }), + new DrawableTestHitObject(Axes.X, new TestHitObject()), + new DrawableTestHitObject(Axes.X, new TestHitObject { StartTime = 1000 }), + new DrawableTestHitObject(Axes.X, new TestHitObject { StartTime = 2000 }), + new DrawableTestHitObject(Axes.X, new TestHitObject { StartTime = 3000 }), + new DrawableTestHitObject(Axes.X, new TestHitObject { StartTime = 4000 }), + }; + + hitObjects.ForEach(h => hitObjectContainer.Add(h)); + speedAdjustments.ForEach(hitObjectContainer.AddSpeedAdjustment); + + // The 0th index in hitObjectContainer.SpeedAdjustments is the "default" control point + // Check multiplier of the default speed adjustment + Assert.AreEqual(1, hitObjectContainer.SpeedAdjustments[0].ControlPoint.Multiplier); + Assert.AreEqual(1, speedAdjustments[0].ControlPoint.Multiplier); + Assert.AreEqual(2, speedAdjustments[1].ControlPoint.Multiplier); + Assert.AreEqual(2, speedAdjustments[2].ControlPoint.Multiplier); + Assert.AreEqual(1, speedAdjustments[3].ControlPoint.Multiplier); + + // Check insertion of hit objects + Assert.IsTrue(hitObjectContainer.SpeedAdjustments[0].Contains(hitObjects[0])); + Assert.IsTrue(speedAdjustments[0].Contains(hitObjects[1])); + Assert.IsTrue(speedAdjustments[1].Contains(hitObjects[2])); + Assert.IsTrue(speedAdjustments[2].Contains(hitObjects[3])); + Assert.IsTrue(speedAdjustments[3].Contains(hitObjects[4])); + Assert.IsTrue(speedAdjustments[3].Contains(hitObjects[5])); + + hitObjectContainer.RemoveSpeedAdjustment(speedAdjustments[1]); + + // The hit object contained in this speed adjustment should be resorted into the previous one + + Assert.IsTrue(speedAdjustments[0].Contains(hitObjects[2])); + } + private class TestRulesetContainer : ScrollingRulesetContainer { private readonly Axes scrollingAxes; @@ -158,4 +223,4 @@ namespace osu.Desktop.Tests.Visual public override string MaxResultString { get { throw new NotImplementedException(); } } } } -} \ No newline at end of file +} diff --git a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs index aea224ab9e..bfaacdd08e 100644 --- a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs @@ -62,7 +62,7 @@ namespace osu.Game.Rulesets.UI /// /// The container that contains the s and s. /// - internal new readonly ScrollingHitObjectContainer HitObjects; + public new readonly ScrollingHitObjectContainer HitObjects; /// /// Creates a new . @@ -138,7 +138,7 @@ namespace osu.Game.Rulesets.UI /// /// A container that provides the foundation for sorting s into s. /// - internal class ScrollingHitObjectContainer : HitObjectContainer + public class ScrollingHitObjectContainer : HitObjectContainer { /// /// Gets or sets the range of time that is visible by the length of the scrolling axes. @@ -152,6 +152,7 @@ namespace osu.Game.Rulesets.UI public readonly BindableBool Reversed = new BindableBool(); private readonly Container speedAdjustments; + public IReadOnlyList SpeedAdjustments => speedAdjustments; private readonly Axes scrollingAxes; From a737f5fe0da77583cdbd35f41dac09b7a133faf7 Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 16:05:19 +0900 Subject: [PATCH 7/9] CI fixes. --- osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs b/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs index 0ca6fc84c2..3b13fdf00a 100644 --- a/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs +++ b/osu.Game/Rulesets/Timing/SpeedAdjustmentContainer.cs @@ -1,7 +1,6 @@ // Copyright (c) 2007-2017 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE -using osu.Framework.Allocation; using osu.Framework.Configuration; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -27,7 +26,7 @@ namespace osu.Game.Rulesets.Timing public readonly BindableBool Reversed = new BindableBool(); protected override Container Content => content; - private Container content; + private readonly Container content; /// /// The axes which the content of this container will scroll through. @@ -41,7 +40,7 @@ namespace osu.Game.Rulesets.Timing /// public readonly MultiplierControlPoint ControlPoint; - private ScrollingContainer scrollingContainer; + private readonly ScrollingContainer scrollingContainer; /// /// Creates a new . From 5a3c0de7ada1a3a9726e695e02f88a4015442e44 Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 16:10:58 +0900 Subject: [PATCH 8/9] Fix further incorrect comparison. --- osu.Game/Rulesets/UI/ScrollingPlayfield.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs index bfaacdd08e..e9f6b4f2ed 100644 --- a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs @@ -182,7 +182,7 @@ namespace osu.Game.Rulesets.UI 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 - var previousSpeedAdjustment = speedAdjustments.LastOrDefault(s => s.ControlPoint.StartTime <= speedAdjustment.ControlPoint.StartTime); + var previousSpeedAdjustment = speedAdjustments.LastOrDefault(s => s != speedAdjustment && s.ControlPoint.StartTime <= speedAdjustment.ControlPoint.StartTime); if (previousSpeedAdjustment == null) return; From 0189f014028b9bdcad1d1b43a3a4650294bfed96 Mon Sep 17 00:00:00 2001 From: smoogipooo Date: Tue, 22 Aug 2017 16:15:40 +0900 Subject: [PATCH 9/9] Explicitly store defaultSpeedAdjustment, and make it un-removable --- osu.Game/Rulesets/UI/ScrollingPlayfield.cs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs index e9f6b4f2ed..524665487d 100644 --- a/osu.Game/Rulesets/UI/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/ScrollingPlayfield.cs @@ -154,6 +154,8 @@ namespace osu.Game.Rulesets.UI private readonly Container speedAdjustments; public IReadOnlyList SpeedAdjustments => speedAdjustments; + private readonly SpeedAdjustmentContainer defaultSpeedAdjustment; + private readonly Axes scrollingAxes; /// @@ -167,7 +169,7 @@ namespace osu.Game.Rulesets.UI AddInternal(speedAdjustments = new Container { RelativeSizeAxes = Axes.Both }); // Default speed adjustment - AddSpeedAdjustment(new SpeedAdjustmentContainer(new MultiplierControlPoint(0))); + AddSpeedAdjustment(defaultSpeedAdjustment = new SpeedAdjustmentContainer(new MultiplierControlPoint(0))); } /// @@ -208,6 +210,9 @@ namespace osu.Game.Rulesets.UI /// The to remove. public void RemoveSpeedAdjustment(SpeedAdjustmentContainer speedAdjustment) { + if (speedAdjustment == defaultSpeedAdjustment) + throw new InvalidOperationException($"The default {nameof(SpeedAdjustmentContainer)} must not be removed."); + if (!speedAdjustments.Remove(speedAdjustment)) return; @@ -232,11 +237,7 @@ namespace osu.Game.Rulesets.UI if (!(hitObject is IScrollingHitObject)) throw new InvalidOperationException($"Hit objects added to a {nameof(ScrollingHitObjectContainer)} must implement {nameof(IScrollingHitObject)}."); - var target = adjustmentContainerFor(hitObject); - if (target == null) - throw new InvalidOperationException($"A {nameof(SpeedAdjustmentContainer)} to container {hitObject} could not be found."); - - target.Add(hitObject); + adjustmentContainerFor(hitObject).Add(hitObject); } public override bool Remove(DrawableHitObject hitObject) => speedAdjustments.Any(s => s.Remove(hitObject)); @@ -248,7 +249,7 @@ namespace osu.Game.Rulesets.UI /// /// The hit object to find the active for. /// The active at 's start time. Null if there are no speed adjustments. - private SpeedAdjustmentContainer adjustmentContainerFor(DrawableHitObject hitObject) => speedAdjustments.LastOrDefault(c => c.CanContain(hitObject)) ?? speedAdjustments.FirstOrDefault(); + private SpeedAdjustmentContainer adjustmentContainerFor(DrawableHitObject hitObject) => speedAdjustments.LastOrDefault(c => c.CanContain(hitObject)) ?? defaultSpeedAdjustment; /// /// Finds the which provides the speed adjustment active at a time. @@ -256,7 +257,7 @@ namespace osu.Game.Rulesets.UI /// /// The time to find the active at. /// The active at . Null if there are no speed adjustments. - private SpeedAdjustmentContainer adjustmentContainerAt(double time) => speedAdjustments.LastOrDefault(c => c.CanContain(time)) ?? speedAdjustments.FirstOrDefault(); + private SpeedAdjustmentContainer adjustmentContainerAt(double time) => speedAdjustments.LastOrDefault(c => c.CanContain(time)) ?? defaultSpeedAdjustment; } } }