diff --git a/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs b/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs index 1d13c6229c..7d83ba569d 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs +++ b/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(); }); AddStep("create song select", () => LoadScreen(songSelect = new TestPlaylistsSongSelect())); @@ -143,7 +144,6 @@ namespace osu.Game.Tests.Visual.Multiplayer /// Tests that the same instances are not shared between two playlist items. /// [Test] - [Ignore("Temporarily disabled due to a non-trivial test failure")] public void TestNewItemHasNewModInstances() { AddStep("set dt mod", () => SelectedMods.Value = new[] { new OsuModDoubleTime() }); diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs index 37ebc72984..2885dbee00 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs +++ b/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(); + 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 createOverlayFunc) { - SelectedMods.Value = Array.Empty(); Children = new Drawable[] { modSelect = createOverlayFunc().With(d => diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 20d88d33f2..d3936ed27e 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -98,7 +98,14 @@ namespace osu.Game [Cached(typeof(IBindable))] protected readonly Bindable Ruleset = new Bindable(); - // todo: move this to SongSelect once Screen has the ability to unsuspend. + /// + /// The current mod selection for the local user. + /// + /// + /// 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. + /// [Cached] [Cached(typeof(IBindable>))] protected readonly Bindable> SelectedMods = new Bindable>(Array.Empty()); diff --git a/osu.Game/Overlays/Mods/ModButton.cs b/osu.Game/Overlays/Mods/ModButton.cs index 8e0d1f5bbd..5e3733cd5e 100644 --- a/osu.Game/Overlays/Mods/ModButton.cs +++ b/osu.Game/Overlays/Mods/ModButton.cs @@ -46,8 +46,9 @@ namespace osu.Game.Overlays.Mods /// Change the selected mod index of this button. /// /// The new index. + /// Whether any settings applied to the mod should be reset on selection. /// Whether the selection changed. - 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) + /// + /// Select the mod at the provided index. + /// + /// The index to select. + /// Whether any settings applied to the mod should be reset on selection. + /// Whether the selection changed. + public bool SelectAt(int index, bool resetSettings = true) { if (!Mods[index].HasImplementation) return false; - changeSelectedIndex(index); + changeSelectedIndex(index, resetSettings); return true; } diff --git a/osu.Game/Overlays/Mods/ModSection.cs b/osu.Game/Overlays/Mods/ModSection.cs index ecbcba7ad3..c3e56abd05 100644 --- a/osu.Game/Overlays/Mods/ModSection.cs +++ b/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; } diff --git a/osu.Game/Overlays/Mods/ModSelectOverlay.cs b/osu.Game/Overlays/Mods/ModSelectOverlay.cs index 93fe693937..a6de0ad6b1 100644 --- a/osu.Game/Overlays/Mods/ModSelectOverlay.cs +++ b/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; diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index 3a8717e678..2a11c92223 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -134,7 +134,7 @@ namespace osu.Game.Rulesets.Mods } /// - /// Copies mod setting values from into this instance. + /// Copies mod setting values from into this instance, overwriting all existing settings. /// /// The mod to copy properties from. 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(); + + /// + /// Reset all custom settings for this mod back to their defaults. + /// + public virtual void ResetSettingsToDefaults() => CopyFrom((Mod)Activator.CreateInstance(GetType())); } } diff --git a/osu.Game/Rulesets/Mods/ModDifficultyAdjust.cs b/osu.Game/Rulesets/Mods/ModDifficultyAdjust.cs index a531e885db..dbc35569e7 100644 --- a/osu.Game/Rulesets/Mods/ModDifficultyAdjust.cs +++ b/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); + } + } } }