From 42c169054afa0b0aaf6e84002d4f05fd80e63e17 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 8 Feb 2021 17:46:34 +0900 Subject: [PATCH 01/14] Revert "Disable failing test temporarily pending resolution" This reverts commit 10142a44716882a4671d4cae2391a96348bd90ba. --- .../Visual/Multiplayer/TestScenePlaylistsSongSelect.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs b/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs index 1d13c6229c..2f7e59f800 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs @@ -143,7 +143,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() }); From 3133ccacfa79704dbf89faad3cb7c82fb242de32 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 9 Feb 2021 13:09:38 +0900 Subject: [PATCH 02/14] Reset selected mods between each test method This doesn't actually fix or change behaviour, but does seem like something we probably want to do here. --- .../Visual/Multiplayer/TestScenePlaylistsSongSelect.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs b/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsSongSelect.cs index 2f7e59f800..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())); From be379e0e3cc910b7b3cf1fb6b48460aeaa2673d4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 9 Feb 2021 13:44:11 +0900 Subject: [PATCH 03/14] Change CopyFrom to always overwrite all settings with incoming values --- osu.Game/Rulesets/Mods/Mod.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index 3a8717e678..dec72d94e5 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); } } From 8204d360a8d84f5ac3fe2eec40155999c23a5ba2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 9 Feb 2021 13:44:42 +0900 Subject: [PATCH 04/14] Always reset local user settings when a mod is deselected in ModSelectOverlay --- osu.Game/Overlays/Mods/ModButton.cs | 2 ++ osu.Game/Overlays/Mods/ModSection.cs | 4 +++- osu.Game/Rulesets/Mods/Mod.cs | 5 +++++ osu.Game/Rulesets/Mods/ModDifficultyAdjust.cs | 11 +++++++++++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/osu.Game/Overlays/Mods/ModButton.cs b/osu.Game/Overlays/Mods/ModButton.cs index 8e0d1f5bbd..06f2fea43f 100644 --- a/osu.Game/Overlays/Mods/ModButton.cs +++ b/osu.Game/Overlays/Mods/ModButton.cs @@ -69,6 +69,8 @@ namespace osu.Game.Overlays.Mods Mod newSelection = SelectedMod ?? Mods[0]; + newSelection.ResetSettingsToDefaults(); + Schedule(() => { if (beforeSelected != Selected) diff --git a/osu.Game/Overlays/Mods/ModSection.cs b/osu.Game/Overlays/Mods/ModSection.cs index ecbcba7ad3..08bd3f8622 100644 --- a/osu.Game/Overlays/Mods/ModSection.cs +++ b/osu.Game/Overlays/Mods/ModSection.cs @@ -197,8 +197,10 @@ namespace osu.Game.Overlays.Mods continue; var buttonMod = button.Mods[index]; - buttonMod.CopyFrom(mod); button.SelectAt(index); + + // the selection above will reset settings to defaults, but as this is an external change we want to copy the new settings across. + buttonMod.CopyFrom(mod); return; } diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index dec72d94e5..2a11c92223 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -173,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); + } + } } } From 0efad9ded10e03233ef2f14d644260178d9c746d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 9 Feb 2021 13:54:13 +0900 Subject: [PATCH 05/14] Add test coverage of setting reset on deselection --- .../UserInterface/TestSceneModSelectOverlay.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs index 37ebc72984..85350c028c 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs @@ -47,6 +47,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() { From e9ef4aaf88a8908596231461420bc83d5ccc569f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 14:34:45 +0900 Subject: [PATCH 06/14] Add test covering expectations of external mod changes --- .../TestSceneModSelectOverlay.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs index 85350c028c..dec9e319ea 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs @@ -170,6 +170,31 @@ namespace osu.Game.Tests.Visual.UserInterface }); } + [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() { From 52f0f3f3b212fb2d38057073138755b9e73c858b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 14:38:15 +0900 Subject: [PATCH 07/14] Add a note about SelectedMods behavioural quirks --- osu.Game/OsuGameBase.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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()); From de8a60435fca3f4644efa5b268848829b3812ae8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 14:44:37 +0900 Subject: [PATCH 08/14] Add failing test covering reported breaking case --- .../UserInterface/TestSceneModSelectOverlay.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs index dec9e319ea..9ca1d4102a 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs @@ -170,6 +170,20 @@ 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() { From 75bc9f607e30495763ec305fe7f6fae608931754 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 14:55:15 +0900 Subject: [PATCH 09/14] Rename wrongly named method --- osu.Game/Overlays/Mods/ModSelectOverlay.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Overlays/Mods/ModSelectOverlay.cs b/osu.Game/Overlays/Mods/ModSelectOverlay.cs index 93fe693937..21ed9af421 100644 --- a/osu.Game/Overlays/Mods/ModSelectOverlay.cs +++ b/osu.Game/Overlays/Mods/ModSelectOverlay.cs @@ -479,10 +479,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; From a39263423c95dd25344dff4e4a5e56afb0842d3a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 15:12:29 +0900 Subject: [PATCH 10/14] Fix externally changed settings from being reset when ModSelectOverlay is initialised --- osu.Game/Overlays/Mods/ModButton.cs | 16 ++++++++++++---- osu.Game/Overlays/Mods/ModSection.cs | 5 +++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/osu.Game/Overlays/Mods/ModButton.cs b/osu.Game/Overlays/Mods/ModButton.cs index 06f2fea43f..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,7 +70,8 @@ namespace osu.Game.Overlays.Mods Mod newSelection = SelectedMod ?? Mods[0]; - newSelection.ResetSettingsToDefaults(); + if (resetSettings) + newSelection.ResetSettingsToDefaults(); Schedule(() => { @@ -211,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 08bd3f8622..71ecef2b82 100644 --- a/osu.Game/Overlays/Mods/ModSection.cs +++ b/osu.Game/Overlays/Mods/ModSection.cs @@ -197,9 +197,10 @@ namespace osu.Game.Overlays.Mods continue; var buttonMod = button.Mods[index]; - button.SelectAt(index); - // the selection above will reset settings to defaults, but as this is an external change we want to copy the new settings across. + button.SelectAt(index, false); + + // as this is likely coming from an external change, ensure the settings of the mod are in sync. buttonMod.CopyFrom(mod); return; } From 435c85a2e79b8682d093c261054c8d4ad67215b4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 15:13:09 +0900 Subject: [PATCH 11/14] Avoid executing selection twice on ModSelectOverlay load --- osu.Game/Overlays/Mods/ModSelectOverlay.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Overlays/Mods/ModSelectOverlay.cs b/osu.Game/Overlays/Mods/ModSelectOverlay.cs index 21ed9af421..f1bfa26a98 100644 --- a/osu.Game/Overlays/Mods/ModSelectOverlay.cs +++ b/osu.Game/Overlays/Mods/ModSelectOverlay.cs @@ -371,8 +371,8 @@ namespace osu.Game.Overlays.Mods { base.LoadComplete(); + SelectedMods.BindValueChanged(_ => updateSelectedButtons()); availableMods.BindValueChanged(_ => updateAvailableMods(), true); - SelectedMods.BindValueChanged(_ => updateSelectedButtons(), true); } protected override void PopOut() From 98a83722ff0ad8fffdd426a55a32792d8ce8febd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 15:29:55 +0900 Subject: [PATCH 12/14] Move the point at which selected mods are reset in tests to allow mutliple creation test flow --- .../Visual/UserInterface/TestSceneModSelectOverlay.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs index 9ca1d4102a..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() @@ -370,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 => From 67c1c4c1ebdd89e8c63c76e071f9598c1db32250 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 15:30:17 +0900 Subject: [PATCH 13/14] Copy settings before applying selection --- osu.Game/Overlays/Mods/ModSection.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Overlays/Mods/ModSection.cs b/osu.Game/Overlays/Mods/ModSection.cs index 71ecef2b82..c3e56abd05 100644 --- a/osu.Game/Overlays/Mods/ModSection.cs +++ b/osu.Game/Overlays/Mods/ModSection.cs @@ -198,10 +198,10 @@ namespace osu.Game.Overlays.Mods var buttonMod = button.Mods[index]; - button.SelectAt(index, false); - // as this is likely coming from an external change, ensure the settings of the mod are in sync. buttonMod.CopyFrom(mod); + + button.SelectAt(index, false); return; } From b3b0d97354d7c57e554a69ea78f516e0e64833c3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 10 Feb 2021 15:32:57 +0900 Subject: [PATCH 14/14] Avoid potential feedback from bindable event binds --- osu.Game/Overlays/Mods/ModSelectOverlay.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Overlays/Mods/ModSelectOverlay.cs b/osu.Game/Overlays/Mods/ModSelectOverlay.cs index f1bfa26a98..a6de0ad6b1 100644 --- a/osu.Game/Overlays/Mods/ModSelectOverlay.cs +++ b/osu.Game/Overlays/Mods/ModSelectOverlay.cs @@ -371,8 +371,11 @@ namespace osu.Game.Overlays.Mods { base.LoadComplete(); - SelectedMods.BindValueChanged(_ => updateSelectedButtons()); availableMods.BindValueChanged(_ => updateAvailableMods(), 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()