Merge pull request #23344 from Terochi/skin-editor-change-handler-improvement

Implement better logic for `ApplyStateChange` in skin editor

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;
@@ -182,6 +185,64 @@ namespace osu.Game.Tests.Visual.Gameplay
             AddAssert("all boxes still selected", () => skinEditor.SelectedComponents, () => Has.Count.EqualTo(2));
         }
 
+        [Test]
+        public void TestUndoEditHistory()
+        {
+            SkinComponentsContainer firstTarget = null!;
+            TestSkinEditorChangeHandler changeHandler = null!;
+            byte[] defaultState = null!;
+            IEnumerable<ISerialisableDrawable> testComponents = null!;
+
+            AddStep("Load necessary things", () =>
+            {
+                firstTarget = Player.ChildrenOfType<SkinComponentsContainer>().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 components", () =>
+            {
+                InputManager.MoveMouseTo(skinEditor.ChildrenOfType<BigBlackBox>().First());
+                InputManager.Click(MouseButton.Left);
+                InputManager.Click(MouseButton.Left);
+                InputManager.Click(MouseButton.Left);
+            });
+            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());
+            revertAndCheckUnchanged();
+
+            AddStep("Remove components", () => testComponents.ForEach(c => firstTarget.Remove(c, false)));
+            revertAndCheckUnchanged();
+
+            void revertAndCheckUnchanged()
+            {
+                AddStep("Revert changes", () => changeHandler.RestoreState(int.MinValue));
+                AddAssert("Current state is same as default", () => defaultState.SequenceEqual(changeHandler.GetCurrentState()));
+            }
+        }
+
         [TestCase(false)]
         [TestCase(true)]
         public void TestBringToFront(bool alterSelectionOrder)
@@ -269,5 +330,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;
+            }
+        }
     }
 }

osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs

@@ -1,6 +1,7 @@
 // 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;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
@@ -56,20 +57,53 @@ namespace osu.Game.Overlays.SkinEditor
             if (deserializedContent == null)
                 return;
 
-            SerialisedDrawableInfo[] skinnableInfo = deserializedContent.ToArray();
+            SerialisedDrawableInfo[] skinnableInfos = deserializedContent.ToArray();
-            Drawable[] targetComponents = firstTarget.Components.OfType<Drawable>().ToArray();
+            ISerialisableDrawable[] targetComponents = firstTarget.Components.ToArray();
 
-            if (!skinnableInfo.Select(s => s.Type).SequenceEqual(targetComponents.Select(d => d.GetType())))
+            // Store components based on type for later reuse
+            var componentsPerTypeLookup = new Dictionary<Type, Queue<Drawable>>();
+
+            foreach (ISerialisableDrawable component in targetComponents)
             {
-                // Perform a naive full reload for now.
+                Type lookup = component.GetType();
-                firstTarget.Reload(skinnableInfo);
+
+                if (!componentsPerTypeLookup.TryGetValue(lookup, out Queue<Drawable>? componentsOfSameType))
+                    componentsPerTypeLookup.Add(lookup, componentsOfSameType = new Queue<Drawable>());
+
+                componentsOfSameType.Enqueue((Drawable)component);
+            }
+
+            for (int i = targetComponents.Length - 1; i >= 0; i--)
+                firstTarget.Remove(targetComponents[i], false);
+
+            foreach (var skinnableInfo in skinnableInfos)
+            {
+                Type lookup = skinnableInfo.Type;
+
+                if (!componentsPerTypeLookup.TryGetValue(lookup, out Queue<Drawable>? componentsOfSameType))
+                {
+                    firstTarget.Add((ISerialisableDrawable)skinnableInfo.CreateInstance());
+                    continue;
+                }
+
+                // Wherever possible, attempt to reuse existing component instances.
+                if (componentsOfSameType.TryDequeue(out Drawable? component))
+                {
+                    component.ApplySerialisedInfo(skinnableInfo);
                 }
                 else
                 {
-                int i = 0;
+                    component = skinnableInfo.CreateInstance();
+                }
 
-                foreach (var drawable in targetComponents)
+                firstTarget.Add((ISerialisableDrawable)component);
-                    drawable.ApplySerialisedInfo(skinnableInfo[i++]);
+            }
+
+            // Dispose components which were not reused.
+            foreach ((Type _, Queue<Drawable> typeComponents) in componentsPerTypeLookup)
+            {
+                foreach (var component in typeComponents)
+                    component.Dispose();
             }
         }
     }