From 814b318a10fc584ae24bd3a4478724a3347502c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 7 Jan 2022 14:00:22 +0100 Subject: [PATCH 1/3] Add test coverage of slider end snapping behaviour --- .../Editor/TestSceneSliderSnapping.cs | 183 ++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs new file mode 100644 index 0000000000..4b4aedcff4 --- /dev/null +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs @@ -0,0 +1,183 @@ +// Copyright (c) ppy Pty Ltd . 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.Input.Events; +using osu.Framework.Testing; +using osu.Framework.Utils; +using osu.Game.Beatmaps; +using osu.Game.Beatmaps.ControlPoints; +using osu.Game.Input.Bindings; +using osu.Game.Rulesets.Objects; +using osu.Game.Rulesets.Osu.Edit; +using osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components; +using osu.Game.Rulesets.Osu.Objects; +using osu.Game.Rulesets.Osu.UI; +using osu.Game.Screens.Edit; +using osu.Game.Screens.Edit.Compose.Components; +using osu.Game.Tests.Beatmaps; +using osu.Game.Tests.Visual; +using osuTK; +using osuTK.Input; + +namespace osu.Game.Rulesets.Osu.Tests.Editor +{ + public class TestSceneSliderSnapping : EditorTestScene + { + private const double beat_length = 1000; + + protected override Ruleset CreateEditorRuleset() => new OsuRuleset(); + + protected override IBeatmap CreateBeatmap(RulesetInfo ruleset) + { + var controlPointInfo = new ControlPointInfo(); + controlPointInfo.Add(0, new TimingControlPoint { BeatLength = beat_length }); + return new TestBeatmap(ruleset, false) + { + ControlPointInfo = controlPointInfo + }; + } + + private Slider slider; + + public override void SetUpSteps() + { + base.SetUpSteps(); + + AddStep("add unsnapped slider", () => EditorBeatmap.Add(slider = new Slider + { + StartTime = 0, + Position = OsuPlayfield.BASE_SIZE / 5, + Path = new SliderPath + { + ControlPoints = + { + new PathControlPoint(Vector2.Zero), + new PathControlPoint(OsuPlayfield.BASE_SIZE * 3 / 5) + } + } + })); + AddStep("set beat divisor to 1/1", () => + { + var beatDivisor = (BindableBeatDivisor)Editor.Dependencies.Get(typeof(BindableBeatDivisor)); + beatDivisor.Value = 1; + }); + } + + [Test] + public void TestMovingUnsnappedSliderNodesSnaps() + { + PathControlPointPiece sliderEnd = null; + + assertSliderSnapped(false); + + AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); + AddStep("select slider end", () => + { + sliderEnd = this.ChildrenOfType().Single(piece => piece.ControlPoint.Position != Vector2.Zero); + InputManager.MoveMouseTo(sliderEnd.ScreenSpaceDrawQuad.Centre); + }); + AddStep("move slider end", () => + { + InputManager.PressButton(MouseButton.Left); + InputManager.MoveMouseTo(sliderEnd.ScreenSpaceDrawQuad.Centre - new Vector2(0, 20)); + InputManager.ReleaseButton(MouseButton.Left); + }); + assertSliderSnapped(true); + } + + [Test] + public void TestResizingUnsnappedSliderSnaps() + { + SelectionBoxScaleHandle handle = null; + + assertSliderSnapped(false); + + AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); + AddStep("move mouse to scale handle", () => + { + handle = this.ChildrenOfType().First(); + InputManager.MoveMouseTo(handle.ScreenSpaceDrawQuad.Centre); + }); + AddStep("scale slider", () => + { + InputManager.PressButton(MouseButton.Left); + InputManager.MoveMouseTo(handle.ScreenSpaceDrawQuad.Centre + new Vector2(20, 20)); + InputManager.ReleaseButton(MouseButton.Left); + }); + assertSliderSnapped(true); + } + + [Test] + public void TestRotatingUnsnappedSliderDoesNotSnap() + { + SelectionBoxRotationHandle handle = null; + + assertSliderSnapped(false); + + AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); + AddStep("move mouse to rotate handle", () => + { + handle = this.ChildrenOfType().First(); + InputManager.MoveMouseTo(handle.ScreenSpaceDrawQuad.Centre); + }); + AddStep("scale slider", () => + { + InputManager.PressButton(MouseButton.Left); + InputManager.MoveMouseTo(handle.ScreenSpaceDrawQuad.Centre + new Vector2(0, 20)); + InputManager.ReleaseButton(MouseButton.Left); + }); + assertSliderSnapped(false); + } + + [Test] + public void TestFlippingSliderDoesNotSnap() + { + OsuSelectionHandler selectionHandler = null; + + assertSliderSnapped(false); + + AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); + AddStep("flip slider horizontally", () => + { + selectionHandler = this.ChildrenOfType().Single(); + selectionHandler.OnPressed(new KeyBindingPressEvent(InputManager.CurrentState, GlobalAction.EditorFlipHorizontally)); + }); + + assertSliderSnapped(false); + + AddStep("flip slider vertically", () => + { + selectionHandler = this.ChildrenOfType().Single(); + selectionHandler.OnPressed(new KeyBindingPressEvent(InputManager.CurrentState, GlobalAction.EditorFlipVertically)); + }); + + assertSliderSnapped(false); + } + + [Test] + public void TestReversingSliderDoesNotSnap() + { + assertSliderSnapped(false); + + AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); + AddStep("reverse slider", () => + { + InputManager.PressKey(Key.ControlLeft); + InputManager.Key(Key.G); + InputManager.ReleaseKey(Key.ControlLeft); + }); + + assertSliderSnapped(false); + } + + private void assertSliderSnapped(bool snapped) + => AddAssert($"slider is {(snapped ? "" : "not ")}snapped", () => + { + double durationInBeatLengths = slider.Duration / beat_length; + double fractionalPart = durationInBeatLengths - (int)durationInBeatLengths; + return Precision.AlmostEquals(fractionalPart, 0) == snapped; + }); + } +} From d2f44813dd9aed366931123b3fca31a25673decb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 7 Jan 2022 14:46:36 +0100 Subject: [PATCH 2/3] Add test coverage for slider snapping when adding/removing control points --- .../Editor/TestSceneSliderSnapping.cs | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs index 4b4aedcff4..b43b2b1461 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderSnapping.cs @@ -54,6 +54,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor ControlPoints = { new PathControlPoint(Vector2.Zero), + new PathControlPoint(OsuPlayfield.BASE_SIZE * 2 / 5), new PathControlPoint(OsuPlayfield.BASE_SIZE * 3 / 5) } } @@ -75,7 +76,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); AddStep("select slider end", () => { - sliderEnd = this.ChildrenOfType().Single(piece => piece.ControlPoint.Position != Vector2.Zero); + sliderEnd = this.ChildrenOfType().Single(piece => piece.ControlPoint == slider.Path.ControlPoints.Last()); InputManager.MoveMouseTo(sliderEnd.ScreenSpaceDrawQuad.Centre); }); AddStep("move slider end", () => @@ -87,6 +88,47 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor assertSliderSnapped(true); } + [Test] + public void TestAddingControlPointToUnsnappedSliderNodesSnaps() + { + assertSliderSnapped(false); + + AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); + AddStep("move mouse to new point location", () => + { + var firstPiece = this.ChildrenOfType().Single(piece => piece.ControlPoint == slider.Path.ControlPoints[0]); + var secondPiece = this.ChildrenOfType().Single(piece => piece.ControlPoint == slider.Path.ControlPoints[1]); + InputManager.MoveMouseTo((firstPiece.ScreenSpaceDrawQuad.Centre + secondPiece.ScreenSpaceDrawQuad.Centre) / 2); + }); + AddStep("move slider end", () => + { + InputManager.PressKey(Key.ControlLeft); + InputManager.Click(MouseButton.Left); + InputManager.ReleaseKey(Key.ControlLeft); + }); + assertSliderSnapped(true); + } + + [Test] + public void TestRemovingControlPointFromUnsnappedSliderNodesSnaps() + { + assertSliderSnapped(false); + + AddStep("select slider", () => EditorBeatmap.SelectedHitObjects.Add(slider)); + AddStep("move mouse to second control point", () => + { + var secondPiece = this.ChildrenOfType().Single(piece => piece.ControlPoint == slider.Path.ControlPoints[1]); + InputManager.MoveMouseTo(secondPiece); + }); + AddStep("quick delete", () => + { + InputManager.PressKey(Key.ShiftLeft); + InputManager.PressButton(MouseButton.Right); + InputManager.ReleaseKey(Key.ShiftLeft); + }); + assertSliderSnapped(true); + } + [Test] public void TestResizingUnsnappedSliderSnaps() { From c09f6ee0522fa038d58810d010d02d8570e9f6bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 7 Jan 2022 15:11:38 +0100 Subject: [PATCH 3/3] Use slider snapping more liberally to match user expectations Previously the slider path length would be snapped using the current beat snap setting on *every* change of the slider path. As it turns out this is unexpected behaviour in some situations (e.g. when reversing a path, which is expected to preserve the previous duration, even though the slider may be technically "unsnapped" at that point in time due to a different beat snap setting being selected afterwards). --- .../Components/PathControlPointVisualiser.cs | 5 +++++ .../Sliders/SliderSelectionBlueprint.cs | 15 +++++++-------- .../Edit/OsuSelectionHandler.cs | 16 +++++++++++++++- .../Rulesets/Objects/SliderPathExtensions.cs | 10 ++++++++++ 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs index 065d4737a5..ae4141073e 100644 --- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs +++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs @@ -283,6 +283,9 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components } } + // Snap the path to the current beat divisor before checking length validity. + slider.SnapTo(snapProvider); + if (!slider.Path.HasValidLength) { for (int i = 0; i < slider.Path.ControlPoints.Count; i++) @@ -290,6 +293,8 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components slider.Position = oldPosition; slider.StartTime = oldStartTime; + // Snap the path length again to undo the invalid length. + slider.SnapTo(snapProvider); return; } diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs index 2aebe05c2f..6cf2a493a9 100644 --- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs +++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs @@ -80,7 +80,7 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders controlPoints.BindTo(HitObject.Path.ControlPoints); pathVersion.BindTo(HitObject.Path.Version); - pathVersion.BindValueChanged(_ => updatePath()); + pathVersion.BindValueChanged(_ => editorBeatmap?.Update(HitObject)); BodyPiece.UpdateFrom(HitObject); } @@ -208,6 +208,8 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders // Move the control points from the insertion index onwards to make room for the insertion controlPoints.Insert(insertionIndex, pathControlPoint); + HitObject.SnapTo(composer); + return pathControlPoint; } @@ -227,7 +229,10 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders controlPoints.Remove(c); } - // If there are 0 or 1 remaining control points, the slider is in a degenerate (single point) form and should be deleted + // Snap the slider to the current beat divisor before checking length validity. + HitObject.SnapTo(composer); + + // If there are 0 or 1 remaining control points, or the slider has an invalid length, it is in a degenerate form and should be deleted if (controlPoints.Count <= 1 || !HitObject.Path.HasValidLength) { placementHandler?.Delete(HitObject); @@ -242,12 +247,6 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders HitObject.Position += first; } - private void updatePath() - { - HitObject.Path.ExpectedDistance.Value = composer?.GetSnappedDistanceFromDistance(HitObject, (float)HitObject.Path.CalculatedDistance) ?? (float)HitObject.Path.CalculatedDistance; - editorBeatmap?.Update(HitObject); - } - private void convertToStream() { if (editorBeatmap == null || changeHandler == null || beatDivisor == null) diff --git a/osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs b/osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs index 071ecf6329..efbac5439c 100644 --- a/osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs +++ b/osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs @@ -1,12 +1,16 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +#nullable enable + using System.Collections.Generic; using System.Linq; +using osu.Framework.Allocation; using osu.Framework.Graphics; using osu.Framework.Graphics.Primitives; using osu.Framework.Utils; using osu.Game.Extensions; +using osu.Game.Rulesets.Edit; using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Osu.Objects; @@ -18,6 +22,9 @@ namespace osu.Game.Rulesets.Osu.Edit { public class OsuSelectionHandler : EditorSelectionHandler { + [Resolved(CanBeNull = true)] + private IPositionSnapProvider? positionSnapProvider { get; set; } + /// /// During a transform, the initial origin is stored so it can be used throughout the operation. /// @@ -27,7 +34,7 @@ namespace osu.Game.Rulesets.Osu.Edit /// During a transform, the initial path types of a single selected slider are stored so they /// can be maintained throughout the operation. /// - private List referencePathTypes; + private List? referencePathTypes; protected override void OnSelectionChanged() { @@ -197,6 +204,10 @@ namespace osu.Game.Rulesets.Osu.Edit for (int i = 0; i < slider.Path.ControlPoints.Count; ++i) slider.Path.ControlPoints[i].Type = referencePathTypes[i]; + // Snap the slider's length to the current beat divisor + // to calculate the final resulting duration / bounding box before the final checks. + slider.SnapTo(positionSnapProvider); + //if sliderhead or sliderend end up outside playfield, revert scaling. Quad scaledQuad = getSurroundingQuad(new OsuHitObject[] { slider }); (bool xInBounds, bool yInBounds) = isQuadInBounds(scaledQuad); @@ -206,6 +217,9 @@ namespace osu.Game.Rulesets.Osu.Edit foreach (var point in slider.Path.ControlPoints) point.Position = oldControlPoints.Dequeue(); + + // Snap the slider's length again to undo the potentially-invalid length applied by the previous snap. + slider.SnapTo(positionSnapProvider); } private void scaleHitObjects(OsuHitObject[] hitObjects, Anchor reference, Vector2 scale) diff --git a/osu.Game/Rulesets/Objects/SliderPathExtensions.cs b/osu.Game/Rulesets/Objects/SliderPathExtensions.cs index 1308fff7ae..ba614900c0 100644 --- a/osu.Game/Rulesets/Objects/SliderPathExtensions.cs +++ b/osu.Game/Rulesets/Objects/SliderPathExtensions.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System.Linq; +using osu.Game.Rulesets.Edit; using osu.Game.Rulesets.Objects.Types; using osuTK; @@ -11,6 +12,15 @@ namespace osu.Game.Rulesets.Objects { public static class SliderPathExtensions { + /// + /// Snaps the provided 's duration using the . + /// + public static void SnapTo(this THitObject hitObject, IPositionSnapProvider? snapProvider) + where THitObject : HitObject, IHasPath + { + hitObject.Path.ExpectedDistance.Value = snapProvider?.GetSnappedDistanceFromDistance(hitObject, (float)hitObject.Path.CalculatedDistance) ?? hitObject.Path.CalculatedDistance; + } + /// /// Reverse the direction of this path. ///