osu icon indicating copy to clipboard operation
osu copied to clipboard

Fix initial skin state being stored wrong to undo history

Open peppy opened this issue 1 year ago • 2 comments

Closes https://github.com/ppy/osu/issues/29429.

Scheduler fixes are always bad, but I don't see a better way to handle this.

peppy avatar Sep 30 '24 10:09 peppy

So I was like "alright this appears to work, but it's a schedule fix, so I'm not going to let it pass review without tests". So I wrote one, using a slightly different scenario to what I was testing with, and... the issue is still there...

diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs
index 5267a57a05..88dffc9979 100644
--- a/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs
+++ b/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs
@@ -5,6 +5,7 @@
 
 using System;
 using System.Linq;
+using Newtonsoft.Json;
 using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Extensions;
@@ -23,6 +24,7 @@
 using osu.Game.Rulesets.Osu.Mods;
 using osu.Game.Screens.Edit.Components;
 using osu.Game.Screens.Play;
+using osu.Game.Screens.Play.HUD;
 using osu.Game.Screens.Play.HUD.HitErrorMeters;
 using osu.Game.Skinning;
 using osu.Game.Tests.Beatmaps.IO;
@@ -101,6 +103,36 @@ public void TestMutateProtectedSkinDuringGameplay()
             AddUntilStep("current skin is mutable", () => !Game.Dependencies.Get<SkinManager>().CurrentSkin.Value.SkinInfo.Value.Protected);
         }
 
+        [Test]
+        public void TestMutateProtectedSkinInitialStateIsCorrect()
+        {
+            AddStep("set default skin", () => Game.Dependencies.Get<SkinManager>().CurrentSkinInfo.SetDefault());
+            AddStep("import beatmap", () => BeatmapImportHelper.LoadQuickOszIntoOsu(Game).WaitSafely());
+
+            openSkinEditor();
+            AddUntilStep("current skin is mutable", () => !Game.Dependencies.Get<SkinManager>().CurrentSkin.Value.SkinInfo.Value.Protected);
+
+            AddUntilStep("wait for player", () =>
+            {
+                DismissAnyNotifications();
+                return Game.ScreenStack.CurrentScreen is Player;
+            });
+
+            string state = string.Empty;
+
+            AddStep("dump state of accuracy meter", () => JsonConvert.SerializeObject(Game.ChildrenOfType<ArgonAccuracyCounter>().First().CreateSerialisedInfo()));
+            AddStep("add any component", () => Game.ChildrenOfType<SkinComponentToolbox.ToolboxComponentButton>().First().TriggerClick());
+            AddStep("undo", () =>
+            {
+                InputManager.PressKey(Key.ControlLeft);
+                InputManager.Key(Key.Z);
+                InputManager.ReleaseKey(Key.ControlLeft);
+            });
+            AddAssert("accuracy meter state unchanged",
+                () => JsonConvert.SerializeObject(Game.ChildrenOfType<ArgonAccuracyCounter>().First().CreateSerialisedInfo()),
+                () => Is.EqualTo(state));
+        }
+
         [Test]
         public void TestComponentsDeselectedOnSkinEditorHide()
         {

It appears that for whatever reason it matters whether you open skin editor from an existing gameplay session (fix works then), or whether you open it at main menu, which will open gameplay for you (fix is broken then)...

bdach avatar Oct 01 '24 11:10 bdach

I tried to revive this and I think I got a bit closer to fixing the remaining issues but the tests I added still fail sometimes and I'm not sure why, so I'm gonna leave this for another day and see if I can power through.

bdach avatar Oct 16 '24 12:10 bdach

I think I fixed the issues.

@peppy can you double-check the changes I made in https://github.com/ppy/osu/pull/30060/commits/66ca7448436e7d66072343a1c4af950da3e0d385 and https://github.com/ppy/osu/pull/30060/commits/936677f56abd22328fc9450d3b529b87a672f440? we can probably get this in if you don't see anything wrong with those

bdach avatar Jan 13 '25 12:01 bdach