From 3b0ba4b38bc886538d62e56fbbfdf7b081cf5d84 Mon Sep 17 00:00:00 2001 From: Terochi Date: Sat, 29 Apr 2023 19:52:22 +0200 Subject: [PATCH 01/11] Improved logic for `ApplyStateChange` for skin editor --- .../SkinEditor/SkinEditorChangeHandler.cs | 65 ++++++++++++++++--- 1 file changed, 55 insertions(+), 10 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index d1a1850796..151557c9e1 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using System.IO; using System.Linq; @@ -56,20 +57,64 @@ namespace osu.Game.Overlays.SkinEditor if (deserializedContent == null) return; - SerialisedDrawableInfo[] skinnableInfo = deserializedContent.ToArray(); - Drawable[] targetComponents = firstTarget.Components.OfType().ToArray(); + SerialisedDrawableInfo[] skinnableInfos = deserializedContent.ToArray(); + ISerialisableDrawable[] targetComponents = firstTarget.Components.ToArray(); - if (!skinnableInfo.Select(s => s.Type).SequenceEqual(targetComponents.Select(d => d.GetType()))) + // Store indexes based on type for later lookup + + var skinnableInfoIndexes = new Dictionary>(); + var targetComponentsIndexes = new Dictionary>(); + + for (int i = 0; i < skinnableInfos.Length; i++) { - // Perform a naive full reload for now. - firstTarget.Reload(skinnableInfo); + Type lookup = skinnableInfos[i].Type; + + if (!skinnableInfoIndexes.TryGetValue(lookup, out List? infoIndexes)) + skinnableInfoIndexes.Add(lookup, infoIndexes = new List()); + + infoIndexes.Add(i); } - else - { - int i = 0; - foreach (var drawable in targetComponents) - drawable.ApplySerialisedInfo(skinnableInfo[i++]); + for (int i = 0; i < targetComponents.Length; i++) + { + Type lookup = targetComponents[i].GetType(); + + if (!targetComponentsIndexes.TryGetValue(lookup, out List? componentIndexes)) + targetComponentsIndexes.Add(lookup, componentIndexes = new List()); + + componentIndexes.Add(i); + } + + foreach ((Type lookup, List infoIndexes) in skinnableInfoIndexes) + { + if (!targetComponentsIndexes.TryGetValue(lookup, out List? componentIndexes)) + componentIndexes = new List(0); + + int j = 0; + + for (int i = 0; i < infoIndexes.Count; i++) + { + if (i >= componentIndexes.Count) + // Add new component + firstTarget.Add((ISerialisableDrawable)skinnableInfos[infoIndexes[i]].CreateInstance()); + else + // Modify existing component + ((Drawable)targetComponents[componentIndexes[j++]]).ApplySerialisedInfo(skinnableInfos[infoIndexes[i]]); + } + + // Remove extra components + for (; j < componentIndexes.Count; j++) + firstTarget.Remove(targetComponents[componentIndexes[j]], false); + } + + foreach ((Type lookup, List componentIndexes) in targetComponentsIndexes) + { + if (skinnableInfoIndexes.ContainsKey(lookup)) + continue; + + // Remove extra components that weren't removed above + for (int i = 0; i < componentIndexes.Count; i++) + firstTarget.Remove(targetComponents[componentIndexes[i]], false); } } } From 17e4b75dfd5f55756e7d67316566bbb1f195a9dc Mon Sep 17 00:00:00 2001 From: Terochi Date: Sat, 29 Apr 2023 20:54:19 +0200 Subject: [PATCH 02/11] Save first state when editing --- osu.Game/Overlays/SkinEditor/SkinEditor.cs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditor.cs b/osu.Game/Overlays/SkinEditor/SkinEditor.cs index 2b23ce290f..f0ad92d4da 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditor.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditor.cs @@ -235,10 +235,7 @@ namespace osu.Game.Overlays.SkinEditor }, true); canPaste.Current.BindValueChanged(paste => pasteMenuItem.Action.Disabled = !paste.NewValue, true); - SelectedComponents.BindCollectionChanged((_, _) => - { - canCopy.Value = canCut.Value = SelectedComponents.Any(); - }, true); + SelectedComponents.BindCollectionChanged((_, _) => canCopy.Value = canCut.Value = SelectedComponents.Any(), true); clipboard.Content.BindValueChanged(content => canPaste.Value = !string.IsNullOrEmpty(content.NewValue), true); @@ -345,6 +342,7 @@ namespace osu.Game.Overlays.SkinEditor changeHandler = new SkinEditorChangeHandler(skinComponentsContainer); changeHandler.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true); changeHandler.CanRedo.BindValueChanged(v => redoMenuItem.Action.Disabled = !v.NewValue, true); + changeHandler.SaveState(); content.Child = new SkinBlueprintContainer(skinComponentsContainer); @@ -479,12 +477,18 @@ namespace osu.Game.Overlays.SkinEditor protected void Cut() { + if (!canCut.Value) + return; + Copy(); DeleteItems(SelectedComponents.ToArray()); } protected void Copy() { + if (!canCopy.Value) + return; + clipboard.Content.Value = JsonConvert.SerializeObject(SelectedComponents.Cast().Select(s => s.CreateSerialisedInfo()).ToArray()); } @@ -500,6 +504,9 @@ namespace osu.Game.Overlays.SkinEditor protected void Paste() { + if (!canPaste.Value) + return; + changeHandler?.BeginChange(); var drawableInfo = JsonConvert.DeserializeObject(clipboard.Content.Value); From 9a9e02b110431b32bcf9281db2349ef1418c8abe Mon Sep 17 00:00:00 2001 From: Terochi Date: Sun, 30 Apr 2023 02:00:35 +0200 Subject: [PATCH 03/11] Added tests --- .../Visual/Gameplay/TestSceneSkinEditor.cs | 89 ++++++++++++++----- 1 file changed, 69 insertions(+), 20 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs index 119b753d70..71eabd776a 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs @@ -2,12 +2,15 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; +using System.IO; using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; +using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.UserInterface; +using osu.Framework.Input; using osu.Framework.Testing; using osu.Game.Overlays; using osu.Game.Overlays.Settings; @@ -97,15 +100,10 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.PressButton(MouseButton.Left); }); - AddStep("Drag to bottom right", () => - { - InputManager.MoveMouseTo(box3.ScreenSpaceDrawQuad.TopRight + new Vector2(-box3.ScreenSpaceDrawQuad.Width / 8, box3.ScreenSpaceDrawQuad.Height / 4)); - }); + AddStep("Drag to bottom right", + () => { InputManager.MoveMouseTo(box3.ScreenSpaceDrawQuad.TopRight + new Vector2(-box3.ScreenSpaceDrawQuad.Width / 8, box3.ScreenSpaceDrawQuad.Height / 4)); }); - AddStep("Release button", () => - { - InputManager.ReleaseButton(MouseButton.Left); - }); + AddStep("Release button", () => { InputManager.ReleaseButton(MouseButton.Left); }); AddAssert("First two boxes selected", () => skinEditor.SelectedComponents, () => Is.EqualTo(new[] { box1, box2 })); @@ -115,15 +113,9 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.PressButton(MouseButton.Left); }); - AddStep("Drag to top left", () => - { - InputManager.MoveMouseTo(box2.ScreenSpaceDrawQuad.Centre - new Vector2(box2.ScreenSpaceDrawQuad.Width / 4)); - }); + AddStep("Drag to top left", () => { InputManager.MoveMouseTo(box2.ScreenSpaceDrawQuad.Centre - new Vector2(box2.ScreenSpaceDrawQuad.Width / 4)); }); - AddStep("Release button", () => - { - InputManager.ReleaseButton(MouseButton.Left); - }); + AddStep("Release button", () => { InputManager.ReleaseButton(MouseButton.Left); }); AddAssert("Last two boxes selected", () => skinEditor.SelectedComponents, () => Is.EqualTo(new[] { box2, box3 })); @@ -147,10 +139,7 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("Three black boxes added", () => targetContainer.Components.OfType().Count(), () => Is.EqualTo(3)); - AddStep("Store black box blueprints", () => - { - blueprints = skinEditor.ChildrenOfType().Where(b => b.Item is BigBlackBox).ToArray(); - }); + AddStep("Store black box blueprints", () => { blueprints = skinEditor.ChildrenOfType().Where(b => b.Item is BigBlackBox).ToArray(); }); AddAssert("Selection is black box 1", () => skinEditor.SelectedComponents.Single(), () => Is.EqualTo(blueprints[0].Item)); @@ -182,6 +171,48 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("all boxes still selected", () => skinEditor.SelectedComponents, () => Has.Count.EqualTo(2)); } + [Test] + public void TestUndo() + { + SkinComponentsContainer firstTarget = null!; + TestSkinEditorChangeHandler changeHandler = null!; + byte[] defaultState = null!; + IEnumerable testComponents = null!; + + AddStep("Load necessary things", () => + { + firstTarget = Player.ChildrenOfType().First(); + changeHandler = new TestSkinEditorChangeHandler(firstTarget); + changeHandler.SaveState(); + defaultState = changeHandler.GetCurrentState(); + testComponents = new[] { targetContainer.Components.First(), targetContainer.Components[targetContainer.Components.Count / 2], targetContainer.Components.Last() }; + }); + + AddStep("Press undo", () => InputManager.Keys(PlatformAction.Undo)); + AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + + AddStep("Add big black boxes", () => + { + InputManager.MoveMouseTo(skinEditor.ChildrenOfType().First()); + InputManager.Click(MouseButton.Left); + InputManager.Click(MouseButton.Left); + InputManager.Click(MouseButton.Left); + }); + AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); + AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + + AddStep("Select components", () => skinEditor.SelectedComponents.AddRange(testComponents)); + AddStep("Bring to front", () => skinEditor.BringSelectionToFront()); + + AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); + AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + + AddStep("Remove components", () => testComponents.ForEach(c => firstTarget.Remove(c, false))); + + AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); + AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + } + [TestCase(false)] [TestCase(true)] public void TestBringToFront(bool alterSelectionOrder) @@ -269,5 +300,23 @@ namespace osu.Game.Tests.Visual.Gameplay } protected override Ruleset CreatePlayerRuleset() => new OsuRuleset(); + + private partial class TestSkinEditorChangeHandler : SkinEditorChangeHandler + { + public TestSkinEditorChangeHandler(Drawable targetScreen) + : base(targetScreen) + { + } + + public byte[] GetCurrentState() + { + using var stream = new MemoryStream(); + + WriteCurrentStateToStream(stream); + byte[] newState = stream.ToArray(); + + return newState; + } + } } } From 8ec2154965cd79939daa44adb7d0c678606bfae8 Mon Sep 17 00:00:00 2001 From: Terochi Date: Sun, 30 Apr 2023 02:01:18 +0200 Subject: [PATCH 04/11] Fixed `ApplyStateChange` to happen in correct order --- .../SkinEditor/SkinEditorChangeHandler.cs | 48 +++++++------------ 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index 151557c9e1..f38f6b0911 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -62,19 +62,8 @@ namespace osu.Game.Overlays.SkinEditor // Store indexes based on type for later lookup - var skinnableInfoIndexes = new Dictionary>(); var targetComponentsIndexes = new Dictionary>(); - for (int i = 0; i < skinnableInfos.Length; i++) - { - Type lookup = skinnableInfos[i].Type; - - if (!skinnableInfoIndexes.TryGetValue(lookup, out List? infoIndexes)) - skinnableInfoIndexes.Add(lookup, infoIndexes = new List()); - - infoIndexes.Add(i); - } - for (int i = 0; i < targetComponents.Length; i++) { Type lookup = targetComponents[i].GetType(); @@ -85,35 +74,34 @@ namespace osu.Game.Overlays.SkinEditor componentIndexes.Add(i); } - foreach ((Type lookup, List infoIndexes) in skinnableInfoIndexes) + var indexCounting = new Dictionary(); + + var empty = new List(0); + + for (int i = 0; i < skinnableInfos.Length; i++) { + Type lookup = skinnableInfos[i].Type; + if (!targetComponentsIndexes.TryGetValue(lookup, out List? componentIndexes)) - componentIndexes = new List(0); + componentIndexes = empty; - int j = 0; + if (!indexCounting.ContainsKey(lookup)) + indexCounting.Add(lookup, 0); - for (int i = 0; i < infoIndexes.Count; i++) - { - if (i >= componentIndexes.Count) - // Add new component - firstTarget.Add((ISerialisableDrawable)skinnableInfos[infoIndexes[i]].CreateInstance()); - else - // Modify existing component - ((Drawable)targetComponents[componentIndexes[j++]]).ApplySerialisedInfo(skinnableInfos[infoIndexes[i]]); - } - - // Remove extra components - for (; j < componentIndexes.Count; j++) - firstTarget.Remove(targetComponents[componentIndexes[j]], false); + if (i >= componentIndexes.Count) + // Add new component + firstTarget.Add((ISerialisableDrawable)skinnableInfos[i].CreateInstance()); + else + // Modify existing component + ((Drawable)targetComponents[componentIndexes[indexCounting[lookup]++]]).ApplySerialisedInfo(skinnableInfos[i]); } foreach ((Type lookup, List componentIndexes) in targetComponentsIndexes) { - if (skinnableInfoIndexes.ContainsKey(lookup)) - continue; + indexCounting.TryGetValue(lookup, out int i); // Remove extra components that weren't removed above - for (int i = 0; i < componentIndexes.Count; i++) + for (; i < componentIndexes.Count; i++) firstTarget.Remove(targetComponents[componentIndexes[i]], false); } } From 585318400cacf70533aefef88d2880f30d4f909b Mon Sep 17 00:00:00 2001 From: Terochi Date: Sun, 30 Apr 2023 02:32:20 +0200 Subject: [PATCH 05/11] Refactor tests --- .../Visual/Gameplay/TestSceneSkinEditor.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs index 71eabd776a..45cc329f53 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs @@ -183,15 +183,22 @@ namespace osu.Game.Tests.Visual.Gameplay { firstTarget = Player.ChildrenOfType().First(); changeHandler = new TestSkinEditorChangeHandler(firstTarget); + changeHandler.SaveState(); defaultState = changeHandler.GetCurrentState(); - testComponents = new[] { targetContainer.Components.First(), targetContainer.Components[targetContainer.Components.Count / 2], targetContainer.Components.Last() }; + + testComponents = new[] + { + targetContainer.Components.First(), + targetContainer.Components[targetContainer.Components.Count / 2], + targetContainer.Components.Last() + }; }); AddStep("Press undo", () => InputManager.Keys(PlatformAction.Undo)); AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); - AddStep("Add big black boxes", () => + AddStep("Add components", () => { InputManager.MoveMouseTo(skinEditor.ChildrenOfType().First()); InputManager.Click(MouseButton.Left); @@ -203,12 +210,10 @@ namespace osu.Game.Tests.Visual.Gameplay AddStep("Select components", () => skinEditor.SelectedComponents.AddRange(testComponents)); AddStep("Bring to front", () => skinEditor.BringSelectionToFront()); - AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); AddStep("Remove components", () => testComponents.ForEach(c => firstTarget.Remove(c, false))); - AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); } From 949dc1c0f96e31d0a20323d83f3fa8a98ab95592 Mon Sep 17 00:00:00 2001 From: Terochi Date: Sun, 30 Apr 2023 12:05:55 +0200 Subject: [PATCH 06/11] Improved logic --- .../Visual/Gameplay/TestSceneSkinEditor.cs | 25 +++++--- .../SkinEditor/SkinEditorChangeHandler.cs | 57 +++++++++---------- 2 files changed, 46 insertions(+), 36 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs index 45cc329f53..4a0be3bb68 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs @@ -172,7 +172,7 @@ namespace osu.Game.Tests.Visual.Gameplay } [Test] - public void TestUndo() + public void TestUndoEditHistory() { SkinComponentsContainer firstTarget = null!; TestSkinEditorChangeHandler changeHandler = null!; @@ -205,17 +205,28 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.Click(MouseButton.Left); InputManager.Click(MouseButton.Left); }); - AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); - AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + revertAndCheckUnchanged(); + + AddStep("Move components", () => + { + changeHandler.BeginChange(); + testComponents.ForEach(c => ((Drawable)c).Position += Vector2.One); + changeHandler.EndChange(); + }); + revertAndCheckUnchanged(); AddStep("Select components", () => skinEditor.SelectedComponents.AddRange(testComponents)); AddStep("Bring to front", () => skinEditor.BringSelectionToFront()); - AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); - AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + revertAndCheckUnchanged(); AddStep("Remove components", () => testComponents.ForEach(c => firstTarget.Remove(c, false))); - AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); - AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + revertAndCheckUnchanged(); + + void revertAndCheckUnchanged() + { + AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); + AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + } } [TestCase(false)] diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index f38f6b0911..6285080d36 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -60,49 +60,48 @@ namespace osu.Game.Overlays.SkinEditor SerialisedDrawableInfo[] skinnableInfos = deserializedContent.ToArray(); ISerialisableDrawable[] targetComponents = firstTarget.Components.ToArray(); - // Store indexes based on type for later lookup + // Store components based on type for later lookup + var typedComponents = new Dictionary>(); - var targetComponentsIndexes = new Dictionary>(); - - for (int i = 0; i < targetComponents.Length; i++) + foreach (var component in targetComponents) { - Type lookup = targetComponents[i].GetType(); + Type lookup = component.GetType(); - if (!targetComponentsIndexes.TryGetValue(lookup, out List? componentIndexes)) - targetComponentsIndexes.Add(lookup, componentIndexes = new List()); + if (!typedComponents.TryGetValue(lookup, out List? typeComponents)) + typedComponents.Add(lookup, typeComponents = new List()); - componentIndexes.Add(i); + typeComponents.Add(component); } - var indexCounting = new Dictionary(); + // Remove all components + for (int i = targetComponents.Length - 1; i >= 0; i--) + firstTarget.Remove(targetComponents[i], false); - var empty = new List(0); + // Keeps count of how many components for each type were already revived + Dictionary typedComponentCounter = typedComponents.Keys.ToDictionary(t => t, _ => 0); - for (int i = 0; i < skinnableInfos.Length; i++) + foreach (var skinnableInfo in skinnableInfos) { - Type lookup = skinnableInfos[i].Type; + Type lookup = skinnableInfo.Type; - if (!targetComponentsIndexes.TryGetValue(lookup, out List? componentIndexes)) - componentIndexes = empty; + if (!typedComponents.TryGetValue(lookup, out List? typeComponents)) + { + firstTarget.Add((ISerialisableDrawable)skinnableInfo.CreateInstance()); + continue; + } - if (!indexCounting.ContainsKey(lookup)) - indexCounting.Add(lookup, 0); + int typeComponentsUsed = typedComponentCounter[lookup]++; - if (i >= componentIndexes.Count) - // Add new component - firstTarget.Add((ISerialisableDrawable)skinnableInfos[i].CreateInstance()); + ISerialisableDrawable component; + + if (typeComponentsUsed < typeComponents.Count) + // Re-use unused component + ((Drawable)(component = typeComponents[typeComponentsUsed])).ApplySerialisedInfo(skinnableInfo); else - // Modify existing component - ((Drawable)targetComponents[componentIndexes[indexCounting[lookup]++]]).ApplySerialisedInfo(skinnableInfos[i]); - } + // Create new one + component = (ISerialisableDrawable)skinnableInfo.CreateInstance(); - foreach ((Type lookup, List componentIndexes) in targetComponentsIndexes) - { - indexCounting.TryGetValue(lookup, out int i); - - // Remove extra components that weren't removed above - for (; i < componentIndexes.Count; i++) - firstTarget.Remove(targetComponents[componentIndexes[i]], false); + firstTarget.Add(component); } } } From 8f02bd80f967db674c7d1655c3f5f72645b77c76 Mon Sep 17 00:00:00 2001 From: Terochi Date: Sun, 30 Apr 2023 16:10:59 +0200 Subject: [PATCH 07/11] Addressed changes --- .../Visual/Gameplay/TestSceneSkinEditor.cs | 29 +++++++++---- osu.Game/Overlays/SkinEditor/SkinEditor.cs | 15 ++----- .../SkinEditor/SkinEditorChangeHandler.cs | 41 +++++++++++-------- 3 files changed, 48 insertions(+), 37 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs index 4a0be3bb68..f7eb42872d 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs @@ -100,10 +100,15 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.PressButton(MouseButton.Left); }); - AddStep("Drag to bottom right", - () => { InputManager.MoveMouseTo(box3.ScreenSpaceDrawQuad.TopRight + new Vector2(-box3.ScreenSpaceDrawQuad.Width / 8, box3.ScreenSpaceDrawQuad.Height / 4)); }); + AddStep("Drag to bottom right", () => + { + InputManager.MoveMouseTo(box3.ScreenSpaceDrawQuad.TopRight + new Vector2(-box3.ScreenSpaceDrawQuad.Width / 8, box3.ScreenSpaceDrawQuad.Height / 4)); + }); - AddStep("Release button", () => { InputManager.ReleaseButton(MouseButton.Left); }); + AddStep("Release button", () => + { + InputManager.ReleaseButton(MouseButton.Left); + }); AddAssert("First two boxes selected", () => skinEditor.SelectedComponents, () => Is.EqualTo(new[] { box1, box2 })); @@ -113,9 +118,15 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.PressButton(MouseButton.Left); }); - AddStep("Drag to top left", () => { InputManager.MoveMouseTo(box2.ScreenSpaceDrawQuad.Centre - new Vector2(box2.ScreenSpaceDrawQuad.Width / 4)); }); + AddStep("Drag to top left", () => + { + InputManager.MoveMouseTo(box2.ScreenSpaceDrawQuad.Centre - new Vector2(box2.ScreenSpaceDrawQuad.Width / 4)); + }); - AddStep("Release button", () => { InputManager.ReleaseButton(MouseButton.Left); }); + AddStep("Release button", () => + { + InputManager.ReleaseButton(MouseButton.Left); + }); AddAssert("Last two boxes selected", () => skinEditor.SelectedComponents, () => Is.EqualTo(new[] { box2, box3 })); @@ -139,7 +150,10 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("Three black boxes added", () => targetContainer.Components.OfType().Count(), () => Is.EqualTo(3)); - AddStep("Store black box blueprints", () => { blueprints = skinEditor.ChildrenOfType().Where(b => b.Item is BigBlackBox).ToArray(); }); + AddStep("Store black box blueprints", () => + { + blueprints = skinEditor.ChildrenOfType().Where(b => b.Item is BigBlackBox).ToArray(); + }); AddAssert("Selection is black box 1", () => skinEditor.SelectedComponents.Single(), () => Is.EqualTo(blueprints[0].Item)); @@ -184,7 +198,6 @@ namespace osu.Game.Tests.Visual.Gameplay firstTarget = Player.ChildrenOfType().First(); changeHandler = new TestSkinEditorChangeHandler(firstTarget); - changeHandler.SaveState(); defaultState = changeHandler.GetCurrentState(); testComponents = new[] @@ -225,7 +238,7 @@ namespace osu.Game.Tests.Visual.Gameplay void revertAndCheckUnchanged() { AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue)); - AddAssert("Nothing changed", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); + AddAssert("Current state is same as default", () => defaultState.SequenceEqual(changeHandler.GetCurrentState())); } } diff --git a/osu.Game/Overlays/SkinEditor/SkinEditor.cs b/osu.Game/Overlays/SkinEditor/SkinEditor.cs index f0ad92d4da..2b23ce290f 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditor.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditor.cs @@ -235,7 +235,10 @@ namespace osu.Game.Overlays.SkinEditor }, true); canPaste.Current.BindValueChanged(paste => pasteMenuItem.Action.Disabled = !paste.NewValue, true); - SelectedComponents.BindCollectionChanged((_, _) => canCopy.Value = canCut.Value = SelectedComponents.Any(), true); + SelectedComponents.BindCollectionChanged((_, _) => + { + canCopy.Value = canCut.Value = SelectedComponents.Any(); + }, true); clipboard.Content.BindValueChanged(content => canPaste.Value = !string.IsNullOrEmpty(content.NewValue), true); @@ -342,7 +345,6 @@ namespace osu.Game.Overlays.SkinEditor changeHandler = new SkinEditorChangeHandler(skinComponentsContainer); changeHandler.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true); changeHandler.CanRedo.BindValueChanged(v => redoMenuItem.Action.Disabled = !v.NewValue, true); - changeHandler.SaveState(); content.Child = new SkinBlueprintContainer(skinComponentsContainer); @@ -477,18 +479,12 @@ namespace osu.Game.Overlays.SkinEditor protected void Cut() { - if (!canCut.Value) - return; - Copy(); DeleteItems(SelectedComponents.ToArray()); } protected void Copy() { - if (!canCopy.Value) - return; - clipboard.Content.Value = JsonConvert.SerializeObject(SelectedComponents.Cast().Select(s => s.CreateSerialisedInfo()).ToArray()); } @@ -504,9 +500,6 @@ namespace osu.Game.Overlays.SkinEditor protected void Paste() { - if (!canPaste.Value) - return; - changeHandler?.BeginChange(); var drawableInfo = JsonConvert.DeserializeObject(clipboard.Content.Value); diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index 6285080d36..8c7f7bbab7 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -34,7 +34,7 @@ namespace osu.Game.Overlays.SkinEditor return; components = new BindableList { BindTarget = firstTarget.Components }; - components.BindCollectionChanged((_, _) => SaveState()); + components.BindCollectionChanged((_, _) => SaveState(), true); } protected override void WriteCurrentStateToStream(MemoryStream stream) @@ -61,47 +61,52 @@ namespace osu.Game.Overlays.SkinEditor ISerialisableDrawable[] targetComponents = firstTarget.Components.ToArray(); // Store components based on type for later lookup - var typedComponents = new Dictionary>(); + var typedComponents = new Dictionary>(); - foreach (var component in targetComponents) + for (int i = targetComponents.Length - 1; i >= 0; i--) { + Drawable component = (Drawable)targetComponents[i]; Type lookup = component.GetType(); - if (!typedComponents.TryGetValue(lookup, out List? typeComponents)) - typedComponents.Add(lookup, typeComponents = new List()); + if (!typedComponents.TryGetValue(lookup, out Stack? typeComponents)) + typedComponents.Add(lookup, typeComponents = new Stack()); - typeComponents.Add(component); + typeComponents.Push(component); } // Remove all components for (int i = targetComponents.Length - 1; i >= 0; i--) firstTarget.Remove(targetComponents[i], false); - // Keeps count of how many components for each type were already revived - Dictionary typedComponentCounter = typedComponents.Keys.ToDictionary(t => t, _ => 0); - foreach (var skinnableInfo in skinnableInfos) { Type lookup = skinnableInfo.Type; - if (!typedComponents.TryGetValue(lookup, out List? typeComponents)) + if (!typedComponents.TryGetValue(lookup, out Stack? typeComponents)) { firstTarget.Add((ISerialisableDrawable)skinnableInfo.CreateInstance()); continue; } - int typeComponentsUsed = typedComponentCounter[lookup]++; - - ISerialisableDrawable component; - - if (typeComponentsUsed < typeComponents.Count) + if (typeComponents.TryPop(out Drawable? component)) + { // Re-use unused component - ((Drawable)(component = typeComponents[typeComponentsUsed])).ApplySerialisedInfo(skinnableInfo); + component.ApplySerialisedInfo(skinnableInfo); + } else + { // Create new one - component = (ISerialisableDrawable)skinnableInfo.CreateInstance(); + component = skinnableInfo.CreateInstance(); + } - firstTarget.Add(component); + firstTarget.Add((ISerialisableDrawable)component); + } + + foreach ((Type _, Stack typeComponents) in typedComponents) + { + // Dispose extra components + foreach (var component in typeComponents) + component.Dispose(); } } } From fb4b681cc527c1b9430d2537e474d77f9035286c Mon Sep 17 00:00:00 2001 From: Terochi Date: Sun, 30 Apr 2023 16:36:01 +0200 Subject: [PATCH 08/11] Use `Queue` instead of `Stack` --- .../SkinEditor/SkinEditorChangeHandler.cs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index 8c7f7bbab7..18ffdb0edc 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -61,17 +61,16 @@ namespace osu.Game.Overlays.SkinEditor ISerialisableDrawable[] targetComponents = firstTarget.Components.ToArray(); // Store components based on type for later lookup - var typedComponents = new Dictionary>(); + var typedComponents = new Dictionary>(); - for (int i = targetComponents.Length - 1; i >= 0; i--) + foreach (ISerialisableDrawable component in targetComponents) { - Drawable component = (Drawable)targetComponents[i]; Type lookup = component.GetType(); - if (!typedComponents.TryGetValue(lookup, out Stack? typeComponents)) - typedComponents.Add(lookup, typeComponents = new Stack()); + if (!typedComponents.TryGetValue(lookup, out Queue? typeComponents)) + typedComponents.Add(lookup, typeComponents = new Queue()); - typeComponents.Push(component); + typeComponents.Enqueue((Drawable)component); } // Remove all components @@ -82,13 +81,13 @@ namespace osu.Game.Overlays.SkinEditor { Type lookup = skinnableInfo.Type; - if (!typedComponents.TryGetValue(lookup, out Stack? typeComponents)) + if (!typedComponents.TryGetValue(lookup, out Queue? typeComponents)) { firstTarget.Add((ISerialisableDrawable)skinnableInfo.CreateInstance()); continue; } - if (typeComponents.TryPop(out Drawable? component)) + if (typeComponents.TryDequeue(out Drawable? component)) { // Re-use unused component component.ApplySerialisedInfo(skinnableInfo); @@ -102,7 +101,7 @@ namespace osu.Game.Overlays.SkinEditor firstTarget.Add((ISerialisableDrawable)component); } - foreach ((Type _, Stack typeComponents) in typedComponents) + foreach ((Type _, Queue typeComponents) in typedComponents) { // Dispose extra components foreach (var component in typeComponents) From 4d52ce769bcea52ee6e10cbc3826a4d570f48707 Mon Sep 17 00:00:00 2001 From: Terochi Date: Mon, 1 May 2023 12:53:58 +0200 Subject: [PATCH 09/11] Revert `SaveState()` calling on initialization --- osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs | 1 + osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs index f7eb42872d..7b37b6624d 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs @@ -198,6 +198,7 @@ namespace osu.Game.Tests.Visual.Gameplay firstTarget = Player.ChildrenOfType().First(); changeHandler = new TestSkinEditorChangeHandler(firstTarget); + changeHandler.SaveState(); defaultState = changeHandler.GetCurrentState(); testComponents = new[] diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index 18ffdb0edc..3dddf639cc 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -34,7 +34,7 @@ namespace osu.Game.Overlays.SkinEditor return; components = new BindableList { BindTarget = firstTarget.Components }; - components.BindCollectionChanged((_, _) => SaveState(), true); + components.BindCollectionChanged((_, _) => SaveState()); } protected override void WriteCurrentStateToStream(MemoryStream stream) From 27fabd99fbc996705e5f700ebdc96f028c035872 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 3 May 2023 19:21:16 +0200 Subject: [PATCH 10/11] Rename variables for legibility Having `typedComponents` and `typeComponents` next to each other is asking for trouble. --- .../Overlays/SkinEditor/SkinEditorChangeHandler.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index 3dddf639cc..f3c5bc9ce2 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -61,16 +61,16 @@ namespace osu.Game.Overlays.SkinEditor ISerialisableDrawable[] targetComponents = firstTarget.Components.ToArray(); // Store components based on type for later lookup - var typedComponents = new Dictionary>(); + var componentsPerTypeLookup = new Dictionary>(); foreach (ISerialisableDrawable component in targetComponents) { Type lookup = component.GetType(); - if (!typedComponents.TryGetValue(lookup, out Queue? typeComponents)) - typedComponents.Add(lookup, typeComponents = new Queue()); + if (!componentsPerTypeLookup.TryGetValue(lookup, out Queue? componentsOfSameType)) + componentsPerTypeLookup.Add(lookup, componentsOfSameType = new Queue()); - typeComponents.Enqueue((Drawable)component); + componentsOfSameType.Enqueue((Drawable)component); } // Remove all components @@ -81,13 +81,13 @@ namespace osu.Game.Overlays.SkinEditor { Type lookup = skinnableInfo.Type; - if (!typedComponents.TryGetValue(lookup, out Queue? typeComponents)) + if (!componentsPerTypeLookup.TryGetValue(lookup, out Queue? componentsOfSameType)) { firstTarget.Add((ISerialisableDrawable)skinnableInfo.CreateInstance()); continue; } - if (typeComponents.TryDequeue(out Drawable? component)) + if (componentsOfSameType.TryDequeue(out Drawable? component)) { // Re-use unused component component.ApplySerialisedInfo(skinnableInfo); @@ -101,7 +101,7 @@ namespace osu.Game.Overlays.SkinEditor firstTarget.Add((ISerialisableDrawable)component); } - foreach ((Type _, Queue typeComponents) in typedComponents) + foreach ((Type _, Queue typeComponents) in componentsPerTypeLookup) { // Dispose extra components foreach (var component in typeComponents) From 1d4d31e35c8b85eb5104734a25b971ec8c827ad3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 3 May 2023 19:22:52 +0200 Subject: [PATCH 11/11] Trim comments Leaving only the ones that add anything useful and do not restate the code verbatim. --- osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index f3c5bc9ce2..673ba873c4 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -60,7 +60,7 @@ namespace osu.Game.Overlays.SkinEditor SerialisedDrawableInfo[] skinnableInfos = deserializedContent.ToArray(); ISerialisableDrawable[] targetComponents = firstTarget.Components.ToArray(); - // Store components based on type for later lookup + // Store components based on type for later reuse var componentsPerTypeLookup = new Dictionary>(); foreach (ISerialisableDrawable component in targetComponents) @@ -73,7 +73,6 @@ namespace osu.Game.Overlays.SkinEditor componentsOfSameType.Enqueue((Drawable)component); } - // Remove all components for (int i = targetComponents.Length - 1; i >= 0; i--) firstTarget.Remove(targetComponents[i], false); @@ -87,23 +86,22 @@ namespace osu.Game.Overlays.SkinEditor continue; } + // Wherever possible, attempt to reuse existing component instances. if (componentsOfSameType.TryDequeue(out Drawable? component)) { - // Re-use unused component component.ApplySerialisedInfo(skinnableInfo); } else { - // Create new one component = skinnableInfo.CreateInstance(); } firstTarget.Add((ISerialisableDrawable)component); } + // Dispose components which were not reused. foreach ((Type _, Queue typeComponents) in componentsPerTypeLookup) { - // Dispose extra components foreach (var component in typeComponents) component.Dispose(); }