Merge pull request #23284 from Hy0tic/multiplier-doesnt-update-with-preset-mod

Fix issue where multiplier show wrong value when adjusting speed on preset

osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs

@@ -14,6 +14,7 @@ using osu.Framework.Localisation;
 using osu.Framework.Testing;
 using osu.Framework.Utils;
 using osu.Game.Graphics.UserInterface;
+using osu.Game.Overlays;
 using osu.Game.Overlays.Mods;
 using osu.Game.Overlays.Settings;
 using osu.Game.Rulesets;
@@ -67,6 +68,19 @@ namespace osu.Game.Tests.Visual.UserInterface
                             }
                         }
                     });
+                    r.Add(new ModPreset
+                    {
+                        Name = "Half Time 0.5x",
+                        Description = "Very slow",
+                        Ruleset = r.Find<RulesetInfo>(OsuRuleset.SHORT_NAME),
+                        Mods = new[]
+                        {
+                            new OsuModHalfTime
+                            {
+                                SpeedChange = { Value = 0.5 }
+                            }
+                        }
+                    });
                 });
             });
         }
@@ -566,6 +580,28 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddAssert("5 columns visible", () => this.ChildrenOfType<ModColumn>().Count(col => col.IsPresent) == 5);
         }
 
+        [Test]
+        public void TestModMultiplierUpdates()
+        {
+            createScreen();
+
+            AddStep("select mod preset with half time", () =>
+            {
+                InputManager.MoveMouseTo(this.ChildrenOfType<ModPresetPanel>().Single(preset => preset.Preset.Value.Name == "Half Time 0.5x"));
+                InputManager.Click(MouseButton.Left);
+            });
+            AddAssert("difficulty multiplier display shows correct value", () => modSelectOverlay.ChildrenOfType<DifficultyMultiplierDisplay>().Single().Current.Value, () => Is.EqualTo(0.5));
+
+            // this is highly unorthodox in a test, but because the `ModSettingChangeTracker` machinery heavily leans on events and object disposal and re-creation,
+            // it is instrumental in the reproduction of the failure scenario that this test is supposed to cover.
+            AddStep("force collection", GC.Collect);
+
+            AddStep("open customisation area", () => modSelectOverlay.CustomisationButton!.TriggerClick());
+            AddStep("reset half time speed to default", () => modSelectOverlay.ChildrenOfType<ModSettingsArea>().Single()
+                                                                              .ChildrenOfType<RestoreDefaultValueButton<double>>().Single().TriggerClick());
+            AddUntilStep("difficulty multiplier display shows correct value", () => modSelectOverlay.ChildrenOfType<DifficultyMultiplierDisplay>().Single().Current.Value, () => Is.EqualTo(0.7));
+        }
+
         private void waitForColumnLoad() => AddUntilStep("all column content loaded",
             () => modSelectOverlay.ChildrenOfType<ModColumn>().Any() && modSelectOverlay.ChildrenOfType<ModColumn>().All(column => column.IsLoaded && column.ItemsLoaded));
 

osu.Game/Overlays/Mods/ModSelectOverlay.cs

@@ -242,17 +242,21 @@ namespace osu.Game.Overlays.Mods
             if (AllowCustomisation)
                 ((IBindable<IReadOnlyList<Mod>>)modSettingsArea.SelectedMods).BindTo(SelectedMods);
 
-            SelectedMods.BindValueChanged(val =>
+            SelectedMods.BindValueChanged(_ =>
             {
-                modSettingChangeTracker?.Dispose();
-
                 updateMultiplier();
                 updateFromExternalSelection();
                 updateCustomisation();
 
+                modSettingChangeTracker?.Dispose();
+
                 if (AllowCustomisation)
                 {
-                    modSettingChangeTracker = new ModSettingChangeTracker(val.NewValue);
+                    // Importantly, use SelectedMods.Value here (and not the ValueChanged NewValue) as the latter can
+                    // potentially be stale, due to complexities in the way change trackers work.
+                    //
+                    // See https://github.com/ppy/osu/pull/23284#issuecomment-1529056988
+                    modSettingChangeTracker = new ModSettingChangeTracker(SelectedMods.Value);
                     modSettingChangeTracker.SettingChanged += _ => updateMultiplier();
                 }
             }, true);