From 0ed5f274f6e45ea22401a50e905e2a1d3406f170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 30 Oct 2023 10:48:31 +0100 Subject: [PATCH 1/4] Enable NRT in `TestSceneSliderVelocityAdjust` --- .../Editor/TestSceneSliderVelocityAdjust.cs | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs index bb8c52bdfc..d92c6ebb60 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System.Diagnostics; using System.Linq; using NUnit.Framework; @@ -24,15 +22,15 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor { public partial class TestSceneSliderVelocityAdjust : OsuGameTestScene { - private Screens.Edit.Editor editor => Game.ScreenStack.CurrentScreen as Screens.Edit.Editor; + private Screens.Edit.Editor? editor => Game.ScreenStack.CurrentScreen as Screens.Edit.Editor; - private EditorBeatmap editorBeatmap => editor.ChildrenOfType().FirstOrDefault(); + private EditorBeatmap editorBeatmap => editor.ChildrenOfType().FirstOrDefault()!; - private EditorClock editorClock => editor.ChildrenOfType().FirstOrDefault(); + private EditorClock editorClock => editor.ChildrenOfType().FirstOrDefault()!; - private Slider slider => editorBeatmap.HitObjects.OfType().FirstOrDefault(); + private Slider? slider => editorBeatmap.HitObjects.OfType().FirstOrDefault(); - private TimelineHitObjectBlueprint blueprint => editor.ChildrenOfType().FirstOrDefault(); + private TimelineHitObjectBlueprint blueprint => editor.ChildrenOfType().FirstOrDefault()!; private DifficultyPointPiece difficultyPointPiece => blueprint.ChildrenOfType().First(); @@ -66,7 +64,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddAssert("ensure one slider placed", () => slider != null); - AddStep("store velocity", () => velocity = slider.Velocity); + AddStep("store velocity", () => velocity = slider!.Velocity); if (adjustVelocity) { @@ -76,10 +74,10 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddAssert("velocity adjusted", () => { Debug.Assert(velocity != null); - return Precision.AlmostEquals(velocity.Value * 2, slider.Velocity); + return Precision.AlmostEquals(velocity.Value * 2, slider!.Velocity); }); - AddStep("store velocity", () => velocity = slider.Velocity); + AddStep("store velocity", () => velocity = slider!.Velocity); } AddStep("save", () => InputManager.Keys(PlatformAction.Save)); @@ -88,8 +86,8 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddStep("enter editor (again)", () => Game.ScreenStack.Push(new EditorLoader())); AddUntilStep("wait for editor load", () => editor?.ReadyForUse == true); - AddStep("seek to slider", () => editorClock.Seek(slider.StartTime)); - AddAssert("slider has correct velocity", () => slider.Velocity == velocity); + AddStep("seek to slider", () => editorClock.Seek(slider!.StartTime)); + AddAssert("slider has correct velocity", () => slider!.Velocity == velocity); } } } From e1ff0d12c66db4c2f16bc5d88fdb568dd5341bf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 30 Oct 2023 10:55:26 +0100 Subject: [PATCH 2/4] Update tests to NUnit-style assertions --- .../Editor/TestSceneSliderVelocityAdjust.cs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs index d92c6ebb60..979801bc41 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs @@ -1,7 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System.Diagnostics; using System.Linq; using NUnit.Framework; using osu.Framework.Input; @@ -45,7 +44,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor double? velocity = null; AddStep("enter editor", () => Game.ScreenStack.Push(new EditorLoader())); - AddUntilStep("wait for editor load", () => editor?.ReadyForUse == true); + AddUntilStep("wait for editor load", () => editor?.ReadyForUse, () => Is.True); AddStep("seek to first control point", () => editorClock.Seek(editorBeatmap.ControlPointInfo.TimingPoints.First().Time)); AddStep("enter slider placement mode", () => InputManager.Key(Key.Number3)); @@ -58,11 +57,11 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddStep("exit placement mode", () => InputManager.Key(Key.Number1)); - AddAssert("slider placed", () => slider != null); + AddAssert("slider placed", () => slider, () => Is.Not.Null); AddStep("select slider", () => editorBeatmap.SelectedHitObjects.Add(slider)); - AddAssert("ensure one slider placed", () => slider != null); + AddAssert("ensure one slider placed", () => slider, () => Is.Not.Null); AddStep("store velocity", () => velocity = slider!.Velocity); @@ -71,11 +70,8 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddStep("open velocity adjust panel", () => difficultyPointPiece.TriggerClick()); AddStep("change velocity", () => velocityTextBox.Current.Value = 2); - AddAssert("velocity adjusted", () => - { - Debug.Assert(velocity != null); - return Precision.AlmostEquals(velocity.Value * 2, slider!.Velocity); - }); + AddAssert("velocity adjusted", () => slider!.Velocity, + () => Is.EqualTo(velocity!.Value * 2).Within(Precision.DOUBLE_EPSILON)); AddStep("store velocity", () => velocity = slider!.Velocity); } @@ -84,10 +80,10 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddStep("exit", () => InputManager.Key(Key.Escape)); AddStep("enter editor (again)", () => Game.ScreenStack.Push(new EditorLoader())); - AddUntilStep("wait for editor load", () => editor?.ReadyForUse == true); + AddUntilStep("wait for editor load", () => editor?.ReadyForUse, () => Is.True); AddStep("seek to slider", () => editorClock.Seek(slider!.StartTime)); - AddAssert("slider has correct velocity", () => slider!.Velocity == velocity); + AddAssert("slider has correct velocity", () => slider!.Velocity, () => Is.EqualTo(velocity)); } } } From b3369dbb7b188d99f226894924a70339478c21b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 30 Oct 2023 10:57:48 +0100 Subject: [PATCH 3/4] Add failing test for slider velocity --- .../Editor/TestSceneSliderVelocityAdjust.cs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs index 979801bc41..175cbeca6e 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs @@ -85,5 +85,50 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddStep("seek to slider", () => editorClock.Seek(slider!.StartTime)); AddAssert("slider has correct velocity", () => slider!.Velocity, () => Is.EqualTo(velocity)); } + + [Test] + public void TestVelocityUndo() + { + double? velocityBefore = null; + double? durationBefore = null; + + AddStep("enter editor", () => Game.ScreenStack.Push(new EditorLoader())); + AddUntilStep("wait for editor load", () => editor?.ReadyForUse == true); + + AddStep("seek to first control point", () => editorClock.Seek(editorBeatmap.ControlPointInfo.TimingPoints.First().Time)); + AddStep("enter slider placement mode", () => InputManager.Key(Key.Number3)); + + AddStep("move mouse to centre", () => InputManager.MoveMouseTo(editor.ChildrenOfType().First().ScreenSpaceDrawQuad.Centre)); + AddStep("start placement", () => InputManager.Click(MouseButton.Left)); + + AddStep("move mouse to bottom right", () => InputManager.MoveMouseTo(editor.ChildrenOfType().First().ScreenSpaceDrawQuad.BottomRight - new Vector2(10))); + AddStep("end placement", () => InputManager.Click(MouseButton.Right)); + + AddStep("exit placement mode", () => InputManager.Key(Key.Number1)); + + AddAssert("slider placed", () => slider, () => Is.Not.Null); + AddStep("select slider", () => editorBeatmap.SelectedHitObjects.Add(slider)); + + AddStep("store velocity", () => + { + velocityBefore = slider!.Velocity; + durationBefore = slider.Duration; + }); + + AddStep("open velocity adjust panel", () => difficultyPointPiece.TriggerClick()); + AddStep("change velocity", () => velocityTextBox.Current.Value = 2); + + AddAssert("velocity adjusted", () => slider!.Velocity, () => Is.EqualTo(velocityBefore!.Value * 2).Within(Precision.DOUBLE_EPSILON)); + + AddStep("undo", () => + { + InputManager.PressKey(Key.ControlLeft); + InputManager.Key(Key.Z); + InputManager.ReleaseKey(Key.ControlLeft); + }); + + AddAssert("slider has correct velocity", () => slider!.Velocity, () => Is.EqualTo(velocityBefore)); + AddAssert("slider has correct duration", () => slider!.Duration, () => Is.EqualTo(durationBefore)); + } } } From de89b7e53c305c0bf048b3ee42c028775d2da2cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 30 Oct 2023 10:59:02 +0100 Subject: [PATCH 4/4] Fix slider velocity changes not being undone correctly Closes https://github.com/ppy/osu/issues/25239. `LegacyEditorBeatmapPatcher.processHitObjectLocalData()` was already supposed to be handling changes to hitobjects that will show up neither when comparing the hitobjects themselves or the timing point with "legacy" info stripped - so, in other words, changes to slider velocity and samples. However, a change to slider velocity requires default application to take effect, so just resetting the value would visually fix the timeline marker but not change the actual object. Calling `EditorBeatmap.Update()` fixes this by way of triggering default re-application. This could probably be smarter (by only invoking the update when strictly necessary, etc.) - but I'm not sure it's worth the hassle. This is intended to be a quick fix, rather than a complete solution - the complete solution would indeed likely entail a wholesale restructuring of the editor's change handling. --- osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index fe0d4a7822..bb9f702cb5 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -123,6 +123,8 @@ namespace osu.Game.Screens.Edit oldWithRepeats.NodeSamples.Clear(); oldWithRepeats.NodeSamples.AddRange(newWithRepeats.NodeSamples); } + + editorBeatmap.Update(oldObject); } }