Merge pull request #11710 from peppy/fix-mod-settings-fuckery

Fix incorrect mod copying behaviour when settings arrive from external change

osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs

@@ -87,6 +87,7 @@ namespace osu.Game.Tests.Visual.Multiplayer
             {
                 Ruleset.Value = new OsuRuleset().RulesetInfo;
                 Beatmap.SetDefault();
+                SelectedMods.Value = Array.Empty<Mod>();
             });
 
             AddStep("create song select", () => LoadScreen(songSelect = new TestPlaylistsSongSelect()));
@@ -143,7 +144,6 @@ namespace osu.Game.Tests.Visual.Multiplayer
         /// Tests that the same <see cref="Mod"/> instances are not shared between two playlist items.
         /// </summary>
         [Test]
-        [Ignore("Temporarily disabled due to a non-trivial test failure")]
         public void TestNewItemHasNewModInstances()
         {
             AddStep("set dt mod", () => SelectedMods.Value = new[] { new OsuModDoubleTime() });

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

@@ -39,7 +39,11 @@ namespace osu.Game.Tests.Visual.UserInterface
         }
 
         [SetUp]
-        public void SetUp() => Schedule(() => createDisplay(() => new TestModSelectOverlay()));
+        public void SetUp() => Schedule(() =>
+        {
+            SelectedMods.Value = Array.Empty<Mod>();
+            createDisplay(() => new TestModSelectOverlay());
+        });
 
         [SetUpSteps]
         public void SetUpSteps()
@@ -47,6 +51,24 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddStep("show", () => modSelect.Show());
         }
 
+        [Test]
+        public void TestSettingsResetOnDeselection()
+        {
+            var osuModDoubleTime = new OsuModDoubleTime { SpeedChange = { Value = 1.2 } };
+
+            changeRuleset(0);
+
+            AddStep("set dt mod with custom rate", () => { SelectedMods.Value = new[] { osuModDoubleTime }; });
+
+            AddAssert("selected mod matches", () => (SelectedMods.Value.Single() as OsuModDoubleTime)?.SpeedChange.Value == 1.2);
+
+            AddStep("deselect", () => modSelect.DeselectAllButton.Click());
+            AddAssert("selected mods empty", () => SelectedMods.Value.Count == 0);
+
+            AddStep("reselect", () => modSelect.GetModButton(osuModDoubleTime).Click());
+            AddAssert("selected mod has default value", () => (SelectedMods.Value.Single() as OsuModDoubleTime)?.SpeedChange.IsDefault == true);
+        }
+
         [Test]
         public void TestAnimationFlushOnClose()
         {
@@ -152,6 +174,45 @@ namespace osu.Game.Tests.Visual.UserInterface
             });
         }
 
+        [Test]
+        public void TestSettingsAreRetainedOnReload()
+        {
+            changeRuleset(0);
+
+            AddStep("set customized mod externally", () => SelectedMods.Value = new[] { new OsuModDoubleTime { SpeedChange = { Value = 1.01 } } });
+
+            AddAssert("setting remains", () => (SelectedMods.Value.SingleOrDefault() as OsuModDoubleTime)?.SpeedChange.Value == 1.01);
+
+            AddStep("create overlay", () => createDisplay(() => new TestNonStackedModSelectOverlay()));
+
+            AddAssert("setting remains", () => (SelectedMods.Value.SingleOrDefault() as OsuModDoubleTime)?.SpeedChange.Value == 1.01);
+        }
+
+        [Test]
+        public void TestExternallySetModIsReplacedByOverlayInstance()
+        {
+            Mod external = new OsuModDoubleTime();
+            Mod overlayButtonMod = null;
+
+            changeRuleset(0);
+
+            AddStep("set mod externally", () => { SelectedMods.Value = new[] { external }; });
+
+            AddAssert("ensure button is selected", () =>
+            {
+                var button = modSelect.GetModButton(SelectedMods.Value.Single());
+                overlayButtonMod = button.SelectedMod;
+                return overlayButtonMod.GetType() == external.GetType();
+            });
+
+            // Right now, when an external change occurs, the ModSelectOverlay will replace the global instance with its own
+            AddAssert("mod instance doesn't match", () => external != overlayButtonMod);
+
+            AddAssert("one mod present in global selected", () => SelectedMods.Value.Count == 1);
+            AddAssert("globally selected matches button's mod instance", () => SelectedMods.Value.Contains(overlayButtonMod));
+            AddAssert("globally selected doesn't contain original external change", () => !SelectedMods.Value.Contains(external));
+        }
+
         [Test]
         public void TestNonStacked()
         {
@@ -313,7 +374,6 @@ namespace osu.Game.Tests.Visual.UserInterface
 
         private void createDisplay(Func<TestModSelectOverlay> createOverlayFunc)
         {
-            SelectedMods.Value = Array.Empty<Mod>();
             Children = new Drawable[]
             {
                 modSelect = createOverlayFunc().With(d =>

osu.Game/OsuGameBase.cs

@@ -98,7 +98,14 @@ namespace osu.Game
         [Cached(typeof(IBindable<RulesetInfo>))]
         protected readonly Bindable<RulesetInfo> Ruleset = new Bindable<RulesetInfo>();
 
-        // todo: move this to SongSelect once Screen has the ability to unsuspend.
+        /// <summary>
+        /// The current mod selection for the local user.
+        /// </summary>
+        /// <remarks>
+        /// If a mod select overlay is present, mod instances set to this value are not guaranteed to remain as the provided instance and will be overwritten by a copy.
+        /// In such a case, changes to settings of a mod will *not* propagate after a mod is added to this collection.
+        /// As such, all settings should be finalised before adding a mod to this collection.
+        /// </remarks>
         [Cached]
         [Cached(typeof(IBindable<IReadOnlyList<Mod>>))]
         protected readonly Bindable<IReadOnlyList<Mod>> SelectedMods = new Bindable<IReadOnlyList<Mod>>(Array.Empty<Mod>());

osu.Game/Overlays/Mods/ModButton.cs

@@ -46,8 +46,9 @@ namespace osu.Game.Overlays.Mods
         /// Change the selected mod index of this button.
         /// </summary>
         /// <param name="newIndex">The new index.</param>
+        /// <param name="resetSettings">Whether any settings applied to the mod should be reset on selection.</param>
         /// <returns>Whether the selection changed.</returns>
-        private bool changeSelectedIndex(int newIndex)
+        private bool changeSelectedIndex(int newIndex, bool resetSettings = true)
         {
             if (newIndex == selectedIndex) return false;
 
@@ -69,6 +70,9 @@ namespace osu.Game.Overlays.Mods
 
             Mod newSelection = SelectedMod ?? Mods[0];
 
+            if (resetSettings)
+                newSelection.ResetSettingsToDefaults();
+
             Schedule(() =>
             {
                 if (beforeSelected != Selected)
@@ -209,11 +213,17 @@ namespace osu.Game.Overlays.Mods
             Deselect();
         }
 
-        public bool SelectAt(int index)
+        /// <summary>
+        /// Select the mod at the provided index.
+        /// </summary>
+        /// <param name="index">The index to select.</param>
+        /// <param name="resetSettings">Whether any settings applied to the mod should be reset on selection.</param>
+        /// <returns>Whether the selection changed.</returns>
+        public bool SelectAt(int index, bool resetSettings = true)
         {
             if (!Mods[index].HasImplementation) return false;
 
-            changeSelectedIndex(index);
+            changeSelectedIndex(index, resetSettings);
             return true;
         }
 

osu.Game/Overlays/Mods/ModSection.cs

@@ -197,8 +197,11 @@ namespace osu.Game.Overlays.Mods
                     continue;
 
                 var buttonMod = button.Mods[index];
+
+                // as this is likely coming from an external change, ensure the settings of the mod are in sync.
                 buttonMod.CopyFrom(mod);
-                button.SelectAt(index);
+
+                button.SelectAt(index, false);
                 return;
             }
 

osu.Game/Overlays/Mods/ModSelectOverlay.cs

@@ -372,7 +372,10 @@ namespace osu.Game.Overlays.Mods
             base.LoadComplete();
 
             availableMods.BindValueChanged(_ => updateAvailableMods(), true);
-            SelectedMods.BindValueChanged(_ => updateSelectedButtons(), true);
+
+            // intentionally bound after the above line to avoid a potential update feedback cycle.
+            // i haven't actually observed this happening but as updateAvailableMods() changes the selection it is plausible.
+            SelectedMods.BindValueChanged(_ => updateSelectedButtons());
         }
 
         protected override void PopOut()
@@ -479,10 +482,10 @@ namespace osu.Game.Overlays.Mods
             foreach (var section in ModSectionsContainer.Children)
                 section.UpdateSelectedButtons(selectedMods);
 
-            updateMods();
+            updateMultiplier();
         }
 
-        private void updateMods()
+        private void updateMultiplier()
         {
             var multiplier = 1.0;
 

osu.Game/Rulesets/Mods/Mod.cs

@@ -134,7 +134,7 @@ namespace osu.Game.Rulesets.Mods
         }
 
         /// <summary>
-        /// Copies mod setting values from <paramref name="source"/> into this instance.
+        /// Copies mod setting values from <paramref name="source"/> into this instance, overwriting all existing settings.
         /// </summary>
         /// <param name="source">The mod to copy properties from.</param>
         public void CopyFrom(Mod source)
@@ -147,9 +147,7 @@ namespace osu.Game.Rulesets.Mods
                 var targetBindable = (IBindable)prop.GetValue(this);
                 var sourceBindable = (IBindable)prop.GetValue(source);
 
-                // we only care about changes that have been made away from defaults.
-                if (!sourceBindable.IsDefault)
-                    CopyAdjustedSetting(targetBindable, sourceBindable);
+                CopyAdjustedSetting(targetBindable, sourceBindable);
             }
         }
 
@@ -175,5 +173,10 @@ namespace osu.Game.Rulesets.Mods
         }
 
         public bool Equals(IMod other) => GetType() == other?.GetType();
+
+        /// <summary>
+        /// Reset all custom settings for this mod back to their defaults.
+        /// </summary>
+        public virtual void ResetSettingsToDefaults() => CopyFrom((Mod)Activator.CreateInstance(GetType()));
     }
 }

osu.Game/Rulesets/Mods/ModDifficultyAdjust.cs

@@ -141,5 +141,16 @@ namespace osu.Game.Rulesets.Mods
             ApplySetting(DrainRate, dr => difficulty.DrainRate = dr);
             ApplySetting(OverallDifficulty, od => difficulty.OverallDifficulty = od);
         }
+
+        public override void ResetSettingsToDefaults()
+        {
+            base.ResetSettingsToDefaults();
+
+            if (difficulty != null)
+            {
+                // base implementation potentially overwrite modified defaults that came from a beatmap selection.
+                TransferSettings(difficulty);
+            }
+        }
     }
 }