diff --git a/osu.Game.Tests/Mods/ModSettingsEqualityComparison.cs b/osu.Game.Tests/Mods/ModSettingsEqualityComparison.cs index cd6879cf01..a03b29f7bc 100644 --- a/osu.Game.Tests/Mods/ModSettingsEqualityComparison.cs +++ b/osu.Game.Tests/Mods/ModSettingsEqualityComparison.cs @@ -49,5 +49,31 @@ public void TestMod() 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)); + } } } diff --git a/osu.Game.Tests/Mods/ModSettingsTest.cs b/osu.Game.Tests/Mods/ModSettingsTest.cs index b9ea1f2567..5ec9629dc2 100644 --- a/osu.Game.Tests/Mods/ModSettingsTest.cs +++ b/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 @@ public void TestModSettingsUnboundWhenCopied() } [Test] - public void TestMultiModSettingsUnboundWhenCopied() + public void TestMultiModSettingsUnboundWhenCloned() { var original = new MultiMod(new OsuModDoubleTime()); var copy = (MultiMod)original.DeepClone(); @@ -32,5 +35,67 @@ public void TestMultiModSettingsUnboundWhenCopied() 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 TestSetting { get; } = new BindableDouble(); + } + + private class TestNonMatchingSettingTypeModInt : TestNonMatchingSettingTypeMod + { + public override string Acronym => "NMI"; + public override BindableNumber TestSetting { get; } = new BindableInt(); + } + + private class TestNonMatchingSettingTypeModBool : TestNonMatchingSettingTypeMod + { + public override string Acronym => "NMB"; + public override Bindable 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; } + } } } diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs index f99fe1d8d4..5cf24c1960 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs @@ -22,6 +22,7 @@ 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 @@ public void TestUncommonModsDiscardedOnRulesetChange() 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()! }); + + changeRuleset(0); + AddAssert("ensure mod still selected", () => SelectedMods.Value.SingleOrDefault() is OsuModDifficultyAdjust); + + AddStep("change mod settings", () => + { + var osuMod = getSelectedMod(); + + 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(); + + return taikoMod.ExtendedLimits.Value && + taikoMod.DrainRate.Value == setting_change && + taikoMod.OverallDifficulty.Value == setting_change; + }); + + AddAssert("non-shared settings remain default", () => + { + var taikoMod = getSelectedMod(); + + return taikoMod.ScrollSpeed.IsDefault; + }); + } + [Test] public void TestExternallySetCustomizedMod() { @@ -617,6 +662,8 @@ private void assertCustomisationToggleState(bool disabled, bool active) AddAssert($"customisation toggle is {(active ? "" : "not ")}active", () => modSelectOverlay.CustomisationButton.AsNonNull().Active.Value == active); } + private T getSelectedMod() where T : Mod => SelectedMods.Value.OfType().Single(); + private ModPanel getPanelForMod(Type modType) => modSelectOverlay.ChildrenOfType().Single(panel => panel.Mod.GetType() == modType); diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 34e31b0d61..c55b6c249f 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -58,7 +58,6 @@ 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 @@ private void onRulesetChanged(ValueChangedEvent r) return; } - var previouslySelectedMods = SelectedMods.Value.ToArray(); - - if (!SelectedMods.Disabled) - SelectedMods.Value = Array.Empty(); - AvailableMods.Value = dict; - if (!SelectedMods.Disabled) - SelectedMods.Value = previouslySelectedMods.Select(m => instance.CreateModFromAcronym(m.Acronym)).Where(m => m != null).ToArray(); + if (SelectedMods.Disabled) + 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(); } diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index 04d55bc5fe..787da926ea 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -12,6 +12,7 @@ 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 @@ public virtual string SettingDescription [JsonIgnore] public virtual Type[] IncompatibleMods => Array.Empty(); - private IReadOnlyList? settingsBacking; + private IReadOnlyDictionary? settingsBacking; /// - /// A list of the all settings within this mod. + /// All settings within this mod. /// - internal IReadOnlyList Settings => + /// + /// The settings are returned in ascending key order as per . + /// The ordering is intentionally enforced manually, as ordering of is unspecified. + /// + internal IEnumerable SettingsBindables => SettingsMap.OrderBy(pair => pair.Key).Select(pair => pair.Value); + + /// + /// Provides mapping of names to s of all settings within this mod. + /// + internal IReadOnlyDictionary SettingsMap => settingsBacking ??= this.GetSettingsSourceProperties() - .Select(p => p.Item2.GetValue(this)) - .Cast() - .ToList(); + .Select(p => p.Item2) + .ToDictionary(property => property.Name.ToSnakeCase(), property => (IBindable)property.GetValue(this)!); /// /// Whether all settings in this mod are set to their default state. /// - protected virtual bool UsesDefaultConfiguration => Settings.All(s => s.IsDefault); + protected virtual bool UsesDefaultConfiguration => SettingsBindables.All(s => s.IsDefault); /// /// Creates a copy of this initialised to a default state. @@ -148,15 +157,53 @@ public void CopyFrom(Mod source) 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 sourceBindable = (IBindable)prop.GetValue(source)!; + var targetBindable = (IBindable)property.GetValue(this)!; + var sourceBindable = (IBindable)property.GetValue(source)!; CopyAdjustedSetting(targetBindable, sourceBindable); } } + /// + /// This method copies the values of all settings from 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. + /// + /// + /// 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. + /// + /// The mod to extract settings from. + 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` and `BindableInt`. + if (!targetBindableType.IsAssignableFrom(sourceBindableType) && !sourceBindableType.IsAssignableFrom(targetBindableType)) + continue; + + // TODO: special case for handling number types + + PropertyInfo property = targetSetting.GetType().GetProperty(nameof(Bindable.Value))!; + property.SetValue(targetSetting, property.GetValue(sourceSetting)); + } + } + /// /// When creating copies or clones of a Mod, this method will be called /// to copy explicitly adjusted user settings from . @@ -191,7 +238,7 @@ public bool Equals(Mod? other) 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 @@ public override int GetHashCode() hashCode.Add(GetType()); - foreach (var setting in Settings) + foreach (var setting in SettingsBindables) hashCode.Add(setting.GetUnderlyingSettingValue()); return hashCode.ToHashCode();