Merge pull request #12444 from peppy/fix-slider-zero-length

Avoid all scenarios where sliders can become zero length

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

@@ -0,0 +1,198 @@
+// 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.Linq;
+using NUnit.Framework;
+using osu.Framework.Testing;
+using osu.Game.Beatmaps;
+using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Objects.Types;
+using osu.Game.Rulesets.Osu.Objects;
+using osu.Game.Rulesets.Osu.UI;
+using osu.Game.Screens.Edit.Compose.Components;
+using osu.Game.Tests.Beatmaps;
+using osuTK;
+using osuTK.Input;
+
+namespace osu.Game.Rulesets.Osu.Tests.Editor
+{
+    [TestFixture]
+    public class TestSceneSliderLengthValidity : TestSceneOsuEditor
+    {
+        private OsuPlayfield playfield;
+
+        protected override IBeatmap CreateBeatmap(RulesetInfo ruleset) => new TestBeatmap(Ruleset.Value, false);
+
+        public override void SetUpSteps()
+        {
+            base.SetUpSteps();
+            AddStep("get playfield", () => playfield = Editor.ChildrenOfType<OsuPlayfield>().First());
+            AddStep("seek to first timing point", () => EditorClock.Seek(Beatmap.Value.Beatmap.ControlPointInfo.TimingPoints.First().Time));
+        }
+
+        [Test]
+        public void TestDraggingStartingPointRemainsValid()
+        {
+            Slider slider = null;
+
+            AddStep("Add slider", () =>
+            {
+                slider = new Slider { StartTime = EditorClock.CurrentTime, Position = new Vector2(300) };
+
+                PathControlPoint[] points =
+                {
+                    new PathControlPoint(new Vector2(0), PathType.Linear),
+                    new PathControlPoint(new Vector2(100, 0)),
+                };
+
+                slider.Path = new SliderPath(points);
+                EditorBeatmap.Add(slider);
+            });
+
+            AddAssert("ensure object placed", () => EditorBeatmap.HitObjects.Count == 1);
+
+            moveMouse(new Vector2(300));
+            AddStep("select slider", () => InputManager.Click(MouseButton.Left));
+
+            double distanceBefore = 0;
+
+            AddStep("store distance", () => distanceBefore = slider.Path.Distance);
+
+            moveMouse(new Vector2(300, 300));
+
+            AddStep("begin drag", () => InputManager.PressButton(MouseButton.Left));
+            moveMouse(new Vector2(350, 300));
+            moveMouse(new Vector2(400, 300));
+            AddStep("end drag", () => InputManager.ReleaseButton(MouseButton.Left));
+
+            AddAssert("slider length shrunk", () => slider.Path.Distance < distanceBefore);
+            AddAssert("ensure slider still has valid length", () => slider.Path.Distance > 0);
+        }
+
+        [Test]
+        public void TestDraggingEndingPointRemainsValid()
+        {
+            Slider slider = null;
+
+            AddStep("Add slider", () =>
+            {
+                slider = new Slider { StartTime = EditorClock.CurrentTime, Position = new Vector2(300) };
+
+                PathControlPoint[] points =
+                {
+                    new PathControlPoint(new Vector2(0), PathType.Linear),
+                    new PathControlPoint(new Vector2(100, 0)),
+                };
+
+                slider.Path = new SliderPath(points);
+                EditorBeatmap.Add(slider);
+            });
+
+            AddAssert("ensure object placed", () => EditorBeatmap.HitObjects.Count == 1);
+
+            moveMouse(new Vector2(300));
+            AddStep("select slider", () => InputManager.Click(MouseButton.Left));
+
+            double distanceBefore = 0;
+
+            AddStep("store distance", () => distanceBefore = slider.Path.Distance);
+
+            moveMouse(new Vector2(400, 300));
+
+            AddStep("begin drag", () => InputManager.PressButton(MouseButton.Left));
+            moveMouse(new Vector2(350, 300));
+            moveMouse(new Vector2(300, 300));
+            AddStep("end drag", () => InputManager.ReleaseButton(MouseButton.Left));
+
+            AddAssert("slider length shrunk", () => slider.Path.Distance < distanceBefore);
+            AddAssert("ensure slider still has valid length", () => slider.Path.Distance > 0);
+        }
+
+        /// <summary>
+        /// If a control point is deleted which results in the slider becoming so short it can't exist,
+        /// for simplicity delete the slider rather than having it in an invalid state.
+        ///
+        /// Eventually we may need to change this, based on user feedback. I think it's likely enough of
+        /// an edge case that we won't get many complaints, though (and there's always the undo button).
+        /// </summary>
+        [Test]
+        public void TestDeletingPointCausesSliderDeletion()
+        {
+            AddStep("Add slider", () =>
+            {
+                Slider slider = new Slider { StartTime = EditorClock.CurrentTime, Position = new Vector2(300) };
+
+                PathControlPoint[] points =
+                {
+                    new PathControlPoint(new Vector2(0), PathType.PerfectCurve),
+                    new PathControlPoint(new Vector2(100, 0)),
+                    new PathControlPoint(new Vector2(0, 10))
+                };
+
+                slider.Path = new SliderPath(points);
+                EditorBeatmap.Add(slider);
+            });
+
+            AddAssert("ensure object placed", () => EditorBeatmap.HitObjects.Count == 1);
+
+            AddStep("select slider", () => InputManager.Click(MouseButton.Left));
+
+            moveMouse(new Vector2(400, 300));
+            AddStep("delete second point", () =>
+            {
+                InputManager.PressKey(Key.ShiftLeft);
+                InputManager.Click(MouseButton.Right);
+                InputManager.ReleaseKey(Key.ShiftLeft);
+            });
+
+            AddAssert("ensure object deleted", () => EditorBeatmap.HitObjects.Count == 0);
+        }
+
+        /// <summary>
+        /// If a scale operation is performed where a single slider is the only thing selected, the path's shape will change.
+        /// If the scale results in the path becoming too short, further mouse movement in the same direction will not change the shape.
+        /// </summary>
+        [Test]
+        public void TestScalingSliderTooSmallRemainsValid()
+        {
+            Slider slider = null;
+
+            AddStep("Add slider", () =>
+            {
+                slider = new Slider { StartTime = EditorClock.CurrentTime, Position = new Vector2(300, 200) };
+
+                PathControlPoint[] points =
+                {
+                    new PathControlPoint(new Vector2(0), PathType.Linear),
+                    new PathControlPoint(new Vector2(0, 50)),
+                    new PathControlPoint(new Vector2(0, 100))
+                };
+
+                slider.Path = new SliderPath(points);
+                EditorBeatmap.Add(slider);
+            });
+
+            AddAssert("ensure object placed", () => EditorBeatmap.HitObjects.Count == 1);
+
+            moveMouse(new Vector2(300));
+            AddStep("select slider", () => InputManager.Click(MouseButton.Left));
+
+            double distanceBefore = 0;
+
+            AddStep("store distance", () => distanceBefore = slider.Path.Distance);
+
+            AddStep("move mouse to handle", () => InputManager.MoveMouseTo(Editor.ChildrenOfType<SelectionBoxDragHandle>().Skip(1).First()));
+            AddStep("begin drag", () => InputManager.PressButton(MouseButton.Left));
+            moveMouse(new Vector2(300, 300));
+            moveMouse(new Vector2(300, 250));
+            moveMouse(new Vector2(300, 200));
+            AddStep("end drag", () => InputManager.ReleaseButton(MouseButton.Left));
+
+            AddAssert("slider length shrunk", () => slider.Path.Distance < distanceBefore);
+            AddAssert("ensure slider still has valid length", () => slider.Path.Distance > 0);
+        }
+
+        private void moveMouse(Vector2 pos) =>
+            AddStep($"move mouse to {pos}", () => InputManager.MoveMouseTo(playfield.ToScreenSpace(pos)));
+    }
+}

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

@@ -41,9 +41,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor
             addClickStep(MouseButton.Left);
             addClickStep(MouseButton.Right);
 
-            assertPlaced(true);
+            assertPlaced(false);
-            assertLength(0);
-            assertControlPointType(0, PathType.Linear);
         }
 
         [Test]

osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointPiece.cs

@@ -185,6 +185,10 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components
 
         protected override void OnDrag(DragEvent e)
         {
+            Vector2[] oldControlPoints = slider.Path.ControlPoints.Select(cp => cp.Position.Value).ToArray();
+            var oldPosition = slider.Position;
+            var oldStartTime = slider.StartTime;
+
             if (ControlPoint == slider.Path.ControlPoints[0])
             {
                 // Special handling for the head control point - the position of the slider changes which means the snapped position and time have to be taken into account
@@ -202,6 +206,16 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components
             else
                 ControlPoint.Position.Value = dragStartPosition + (e.MousePosition - e.MouseDownPosition);
 
+            if (!slider.Path.HasValidLength)
+            {
+                for (var i = 0; i < slider.Path.ControlPoints.Count; i++)
+                    slider.Path.ControlPoints[i].Position.Value = oldControlPoints[i];
+
+                slider.Position = oldPosition;
+                slider.StartTime = oldStartTime;
+                return;
+            }
+
             // Maintain the path type in case it got defaulted to bezier at some point during the drag.
             PointsInSegment[0].Type.Value = dragPathType;
         }

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

@@ -135,7 +135,7 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders
         private void endCurve()
         {
             updateSlider();
-            EndPlacement(true);
+            EndPlacement(HitObject.Path.HasValidLength);
         }
 
         protected override void Update()

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

@@ -215,7 +215,7 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders
             }
 
             // If there are 0 or 1 remaining control points, the slider is in a degenerate (single point) form and should be deleted
-            if (controlPoints.Count <= 1)
+            if (controlPoints.Count <= 1 || !slider.HitObject.Path.HasValidLength)
             {
                 placementHandler?.Delete(HitObject);
                 return;

osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs

@@ -228,7 +228,7 @@ namespace osu.Game.Rulesets.Osu.Edit
             Quad scaledQuad = getSurroundingQuad(new OsuHitObject[] { slider });
             (bool xInBounds, bool yInBounds) = isQuadInBounds(scaledQuad);
 
-            if (xInBounds && yInBounds)
+            if (xInBounds && yInBounds && slider.Path.HasValidLength)
                 return;
 
             foreach (var point in slider.Path.ControlPoints)

osu.Game/Rulesets/Objects/SliderPath.cs

@@ -30,6 +30,8 @@ namespace osu.Game.Rulesets.Objects
         /// </summary>
         public readonly Bindable<double?> ExpectedDistance = new Bindable<double?>();
 
+        public bool HasValidLength => Distance > 0;
+
         /// <summary>
         /// The control points of the path.
         /// </summary>