Merge pull request #22711 from Terochi/keep-shared-settings-ruleset-change

Keep shared mod settings when changing ruleset

osu.Game.Tests/Mods/ModSettingsEqualityComparison.cs

@@ -49,5 +49,31 @@ namespace osu.Game.Tests.Mods
             Assert.That(mod3, Is.EqualTo(mod2));
             Assert.That(doubleConvertedMod3, Is.EqualTo(doubleConvertedMod2));
         }
+
+        [Test]
+        public void TestModWithMultipleSettings()
+        {
+            var ruleset = new OsuRuleset();
+
+            var mod1 = new OsuModDifficultyAdjust { OverallDifficulty = { Value = 10 }, CircleSize = { Value = 0 } };
+            var mod2 = new OsuModDifficultyAdjust { OverallDifficulty = { Value = 10 }, CircleSize = { Value = 6 } };
+            var mod3 = new OsuModDifficultyAdjust { OverallDifficulty = { Value = 10 }, CircleSize = { Value = 6 } };
+
+            var doubleConvertedMod1 = new APIMod(mod1).ToMod(ruleset);
+            var doubleConvertedMod2 = new APIMod(mod2).ToMod(ruleset);
+            var doubleConvertedMod3 = new APIMod(mod3).ToMod(ruleset);
+
+            Assert.That(mod1, Is.Not.EqualTo(mod2));
+            Assert.That(doubleConvertedMod1, Is.Not.EqualTo(doubleConvertedMod2));
+
+            Assert.That(mod2, Is.EqualTo(mod2));
+            Assert.That(doubleConvertedMod2, Is.EqualTo(doubleConvertedMod2));
+
+            Assert.That(mod2, Is.EqualTo(mod3));
+            Assert.That(doubleConvertedMod2, Is.EqualTo(doubleConvertedMod3));
+
+            Assert.That(mod3, Is.EqualTo(mod2));
+            Assert.That(doubleConvertedMod3, Is.EqualTo(doubleConvertedMod2));
+        }
     }
 }

osu.Game.Tests/Mods/ModSettingsTest.cs

@@ -2,6 +2,9 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using NUnit.Framework;
+using osu.Framework.Bindables;
+using osu.Framework.Localisation;
+using osu.Game.Configuration;
 using osu.Game.Rulesets.Mods;
 using osu.Game.Rulesets.Osu.Mods;
 
@@ -10,7 +13,7 @@ namespace osu.Game.Tests.Mods
     public class ModSettingsTest
     {
         [Test]
-        public void TestModSettingsUnboundWhenCopied()
+        public void TestModSettingsUnboundWhenCloned()
         {
             var original = new OsuModDoubleTime();
             var copy = (OsuModDoubleTime)original.DeepClone();
@@ -22,7 +25,7 @@ namespace osu.Game.Tests.Mods
         }
 
         [Test]
-        public void TestMultiModSettingsUnboundWhenCopied()
+        public void TestMultiModSettingsUnboundWhenCloned()
         {
             var original = new MultiMod(new OsuModDoubleTime());
             var copy = (MultiMod)original.DeepClone();
@@ -32,5 +35,67 @@ namespace osu.Game.Tests.Mods
             Assert.That(((OsuModDoubleTime)original.Mods[0]).SpeedChange.Value, Is.EqualTo(2.0));
             Assert.That(((OsuModDoubleTime)copy.Mods[0]).SpeedChange.Value, Is.EqualTo(1.5));
         }
+
+        [Test]
+        public void TestDifferentTypeSettingsKeptWhenCopied()
+        {
+            const double setting_change = 50.4;
+
+            var modDouble = new TestNonMatchingSettingTypeModDouble { TestSetting = { Value = setting_change } };
+            var modBool = new TestNonMatchingSettingTypeModBool { TestSetting = { Default = false, Value = true } };
+            var modInt = new TestNonMatchingSettingTypeModInt { TestSetting = { Value = (int)setting_change / 2 } };
+
+            modDouble.CopyCommonSettingsFrom(modBool);
+            modDouble.CopyCommonSettingsFrom(modInt);
+            modBool.CopyCommonSettingsFrom(modDouble);
+            modBool.CopyCommonSettingsFrom(modInt);
+            modInt.CopyCommonSettingsFrom(modDouble);
+            modInt.CopyCommonSettingsFrom(modBool);
+
+            Assert.That(modDouble.TestSetting.Value, Is.EqualTo(setting_change));
+            Assert.That(modBool.TestSetting.Value, Is.EqualTo(true));
+            Assert.That(modInt.TestSetting.Value, Is.EqualTo((int)setting_change / 2));
+        }
+
+        [Test]
+        public void TestDefaultValueKeptWhenCopied()
+        {
+            var modBoolTrue = new TestNonMatchingSettingTypeModBool { TestSetting = { Default = true, Value = false } };
+            var modBoolFalse = new TestNonMatchingSettingTypeModBool { TestSetting = { Default = false, Value = true } };
+
+            modBoolFalse.CopyCommonSettingsFrom(modBoolTrue);
+
+            Assert.That(modBoolFalse.TestSetting.Default, Is.EqualTo(false));
+            Assert.That(modBoolFalse.TestSetting.Value, Is.EqualTo(modBoolTrue.TestSetting.Value));
+        }
+
+        private class TestNonMatchingSettingTypeModDouble : TestNonMatchingSettingTypeMod
+        {
+            public override string Acronym => "NMD";
+            public override BindableNumber<double> TestSetting { get; } = new BindableDouble();
+        }
+
+        private class TestNonMatchingSettingTypeModInt : TestNonMatchingSettingTypeMod
+        {
+            public override string Acronym => "NMI";
+            public override BindableNumber<int> TestSetting { get; } = new BindableInt();
+        }
+
+        private class TestNonMatchingSettingTypeModBool : TestNonMatchingSettingTypeMod
+        {
+            public override string Acronym => "NMB";
+            public override Bindable<bool> TestSetting { get; } = new BindableBool();
+        }
+
+        private abstract class TestNonMatchingSettingTypeMod : Mod
+        {
+            public override string Name => "Non-matching setting type mod";
+            public override LocalisableString Description => "Description";
+            public override double ScoreMultiplier => 1;
+            public override ModType Type => ModType.Conversion;
+
+            [SettingSource("Test setting")]
+            public abstract IBindable TestSetting { get; }
+        }
     }
 }

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

@@ -22,6 +22,7 @@ using osu.Game.Rulesets.Catch.Mods;
 using osu.Game.Rulesets.Mods;
 using osu.Game.Rulesets.Osu;
 using osu.Game.Rulesets.Osu.Mods;
+using osu.Game.Rulesets.Taiko.Mods;
 using osu.Game.Tests.Mods;
 using osuTK;
 using osuTK.Input;
@@ -385,6 +386,50 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddAssert("no mod selected", () => SelectedMods.Value.Count == 0);
         }
 
+        [Test]
+        public void TestKeepSharedSettingsFromSimilarMods()
+        {
+            const float setting_change = 1.2f;
+
+            createScreen();
+            changeRuleset(0);
+
+            AddStep("select difficulty adjust mod", () => SelectedMods.Value = new[] { Ruleset.Value.CreateInstance().CreateMod<ModDifficultyAdjust>()! });
+
+            changeRuleset(0);
+            AddAssert("ensure mod still selected", () => SelectedMods.Value.SingleOrDefault() is OsuModDifficultyAdjust);
+
+            AddStep("change mod settings", () =>
+            {
+                var osuMod = getSelectedMod<OsuModDifficultyAdjust>();
+
+                osuMod.ExtendedLimits.Value = true;
+                osuMod.CircleSize.Value = setting_change;
+                osuMod.DrainRate.Value = setting_change;
+                osuMod.OverallDifficulty.Value = setting_change;
+                osuMod.ApproachRate.Value = setting_change;
+            });
+
+            changeRuleset(1);
+            AddAssert("taiko variant selected", () => SelectedMods.Value.SingleOrDefault() is TaikoModDifficultyAdjust);
+
+            AddAssert("shared settings preserved", () =>
+            {
+                var taikoMod = getSelectedMod<TaikoModDifficultyAdjust>();
+
+                return taikoMod.ExtendedLimits.Value &&
+                       taikoMod.DrainRate.Value == setting_change &&
+                       taikoMod.OverallDifficulty.Value == setting_change;
+            });
+
+            AddAssert("non-shared settings remain default", () =>
+            {
+                var taikoMod = getSelectedMod<TaikoModDifficultyAdjust>();
+
+                return taikoMod.ScrollSpeed.IsDefault;
+            });
+        }
+
         [Test]
         public void TestExternallySetCustomizedMod()
         {
@@ -617,6 +662,8 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddAssert($"customisation toggle is {(active ? "" : "not ")}active", () => modSelectOverlay.CustomisationButton.AsNonNull().Active.Value == active);
         }
 
+        private T getSelectedMod<T>() where T : Mod => SelectedMods.Value.OfType<T>().Single();
+
         private ModPanel getPanelForMod(Type modType)
             => modSelectOverlay.ChildrenOfType<ModPanel>().Single(panel => panel.Mod.GetType() == modType);
 

osu.Game/OsuGameBase.cs

@@ -58,7 +58,6 @@ using osu.Game.Rulesets.Mods;
 using osu.Game.Scoring;
 using osu.Game.Skinning;
 using osu.Game.Utils;
-using File = System.IO.File;
 using RuntimeInfo = osu.Framework.RuntimeInfo;
 
 namespace osu.Game
@@ -626,15 +625,22 @@ namespace osu.Game
                 return;
             }
 
-            var previouslySelectedMods = SelectedMods.Value.ToArray();
-
-            if (!SelectedMods.Disabled)
-                SelectedMods.Value = Array.Empty<Mod>();
-
             AvailableMods.Value = dict;
 
-            if (!SelectedMods.Disabled)
+            if (SelectedMods.Disabled)
-                SelectedMods.Value = previouslySelectedMods.Select(m => instance.CreateModFromAcronym(m.Acronym)).Where(m => m != null).ToArray();
+                return;
+
+            var convertedMods = SelectedMods.Value.Select(mod =>
+            {
+                var newMod = instance.CreateModFromAcronym(mod.Acronym);
+                newMod?.CopyCommonSettingsFrom(mod);
+                return newMod;
+            }).Where(newMod => newMod != null).ToList();
+
+            if (!ModUtils.CheckValidForGameplay(convertedMods, out var invalid))
+                invalid.ForEach(newMod => convertedMods.Remove(newMod));
+
+            SelectedMods.Value = convertedMods;
 
             void revertRulesetChange() => Ruleset.Value = r.OldValue?.Available == true ? r.OldValue : RulesetStore.AvailableRulesets.First();
         }

osu.Game/Rulesets/Mods/Mod.cs

@@ -12,6 +12,7 @@ using osu.Framework.Graphics.Sprites;
 using osu.Framework.Localisation;
 using osu.Framework.Testing;
 using osu.Game.Configuration;
+using osu.Game.Extensions;
 using osu.Game.Rulesets.UI;
 using osu.Game.Utils;
 
@@ -113,21 +114,29 @@ namespace osu.Game.Rulesets.Mods
         [JsonIgnore]
         public virtual Type[] IncompatibleMods => Array.Empty<Type>();
 
-        private IReadOnlyList<IBindable>? settingsBacking;
+        private IReadOnlyDictionary<string, IBindable>? settingsBacking;
 
         /// <summary>
-        /// A list of the all <see cref="IBindable"/> settings within this mod.
+        /// All <see cref="IBindable"/> settings within this mod.
         /// </summary>
-        internal IReadOnlyList<IBindable> Settings =>
+        /// <remarks>
+        /// The settings are returned in ascending key order as per <see cref="SettingsMap"/>.
+        /// The ordering is intentionally enforced manually, as ordering of <see cref="Dictionary{TKey,TValue}.Values"/> is unspecified.
+        /// </remarks>
+        internal IEnumerable<IBindable> SettingsBindables => SettingsMap.OrderBy(pair => pair.Key).Select(pair => pair.Value);
+
+        /// <summary>
+        /// Provides mapping of names to <see cref="IBindable"/>s of all settings within this mod.
+        /// </summary>
+        internal IReadOnlyDictionary<string, IBindable> SettingsMap =>
             settingsBacking ??= this.GetSettingsSourceProperties()
-                                    .Select(p => p.Item2.GetValue(this))
+                                    .Select(p => p.Item2)
-                                    .Cast<IBindable>()
+                                    .ToDictionary(property => property.Name.ToSnakeCase(), property => (IBindable)property.GetValue(this)!);
-                                    .ToList();
 
         /// <summary>
         /// Whether all settings in this mod are set to their default state.
         /// </summary>
-        protected virtual bool UsesDefaultConfiguration => Settings.All(s => s.IsDefault);
+        protected virtual bool UsesDefaultConfiguration => SettingsBindables.All(s => s.IsDefault);
 
         /// <summary>
         /// Creates a copy of this <see cref="Mod"/> initialised to a default state.
@@ -148,15 +157,53 @@ namespace osu.Game.Rulesets.Mods
             if (source.GetType() != GetType())
                 throw new ArgumentException($"Expected mod of type {GetType()}, got {source.GetType()}.", nameof(source));
 
-            foreach (var (_, prop) in this.GetSettingsSourceProperties())
+            foreach (var (_, property) in this.GetSettingsSourceProperties())
             {
-                var targetBindable = (IBindable)prop.GetValue(this)!;
+                var targetBindable = (IBindable)property.GetValue(this)!;
-                var sourceBindable = (IBindable)prop.GetValue(source)!;
+                var sourceBindable = (IBindable)property.GetValue(source)!;
 
                 CopyAdjustedSetting(targetBindable, sourceBindable);
             }
         }
 
+        /// <summary>
+        /// This method copies the values of all settings from <paramref name="source"/> that share the same names with this mod instance.
+        /// The most frequent use of this is when switching rulesets, in order to preserve values of common settings during the switch.
+        /// </summary>
+        /// <remarks>
+        /// The values are copied directly, without adjusting for possibly different allowed ranges of values.
+        /// If the value of a setting is not valid for this instance due to not falling inside of the allowed range, it will be clamped accordingly.
+        /// </remarks>
+        /// <param name="source">The mod to extract settings from.</param>
+        public void CopyCommonSettingsFrom(Mod source)
+        {
+            if (source.UsesDefaultConfiguration)
+                return;
+
+            foreach (var (name, targetSetting) in SettingsMap)
+            {
+                if (!source.SettingsMap.TryGetValue(name, out IBindable? sourceSetting))
+                    continue;
+
+                if (sourceSetting.IsDefault)
+                    continue;
+
+                var targetBindableType = targetSetting.GetType();
+                var sourceBindableType = sourceSetting.GetType();
+
+                // if either the target is assignable to the source or the source is assignable to the target,
+                // then we presume that the data types contained in both bindables are compatible and we can proceed with the copy.
+                // this handles cases like `Bindable<int>` and `BindableInt`.
+                if (!targetBindableType.IsAssignableFrom(sourceBindableType) && !sourceBindableType.IsAssignableFrom(targetBindableType))
+                    continue;
+
+                // TODO: special case for handling number types
+
+                PropertyInfo property = targetSetting.GetType().GetProperty(nameof(Bindable<bool>.Value))!;
+                property.SetValue(targetSetting, property.GetValue(sourceSetting));
+            }
+        }
+
         /// <summary>
         /// When creating copies or clones of a Mod, this method will be called
         /// to copy explicitly adjusted user settings from <paramref name="target"/>.
@@ -191,7 +238,7 @@ namespace osu.Game.Rulesets.Mods
             if (ReferenceEquals(this, other)) return true;
 
             return GetType() == other.GetType() &&
-                   Settings.SequenceEqual(other.Settings, ModSettingsEqualityComparer.Default);
+                   SettingsBindables.SequenceEqual(other.SettingsBindables, ModSettingsEqualityComparer.Default);
         }
 
         public override int GetHashCode()
@@ -200,7 +247,7 @@ namespace osu.Game.Rulesets.Mods
 
             hashCode.Add(GetType());
 
-            foreach (var setting in Settings)
+            foreach (var setting in SettingsBindables)
                 hashCode.Add(setting.GetUnderlyingSettingValue());
 
             return hashCode.ToHashCode();