diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectScreen.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectScreen.cs index 0bd138275b..bdb423a43c 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectScreen.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectScreen.cs @@ -336,8 +336,8 @@ namespace osu.Game.Tests.Visual.UserInterface 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)); + AddAssert("globally selected matches button's mod instance", () => SelectedMods.Value.Any(mod => ReferenceEquals(mod, overlayButtonMod))); + AddAssert("globally selected doesn't contain original external change", () => !SelectedMods.Value.Any(mod => ReferenceEquals(mod, external))); } [Test] diff --git a/osu.Game/Overlays/Mods/ModColumn.cs b/osu.Game/Overlays/Mods/ModColumn.cs index d480382fcb..f9332dd28b 100644 --- a/osu.Game/Overlays/Mods/ModColumn.cs +++ b/osu.Game/Overlays/Mods/ModColumn.cs @@ -53,9 +53,22 @@ namespace osu.Game.Overlays.Mods } } - public Bindable> SelectedMods = new Bindable>(Array.Empty()); public Bindable Active = new BindableBool(true); + /// + /// List of mods marked as selected in this column. + /// + /// + /// Note that the mod instances returned by this property are owned solely by this column + /// (as in, they are locally-managed clones, to ensure proper isolation from any other external instances). + /// + public IReadOnlyList SelectedMods { get; private set; } = Array.Empty(); + + /// + /// Invoked when a mod panel has been selected interactively by the user. + /// + public event Action? SelectionChangedByUser; + protected override bool ReceivePositionalInputAtSubTree(Vector2 screenSpacePos) => base.ReceivePositionalInputAtSubTree(screenSpacePos) && Active.Value; protected virtual ModPanel CreateModPanel(Mod mod) => new ModPanel(mod); @@ -64,6 +77,15 @@ namespace osu.Game.Overlays.Mods private readonly Bindable>> availableMods = new Bindable>>(); + /// + /// All mods that are available for the current ruleset in this particular column. + /// + /// + /// Note that the mod instances in this list are owned solely by this column + /// (as in, they are locally-managed clones, to ensure proper isolation from any other external instances). + /// + private IReadOnlyList localAvailableMods = Array.Empty(); + private readonly TextFlowContainer headerText; private readonly Box headerBackground; private readonly Container contentContainer; @@ -227,6 +249,9 @@ namespace osu.Game.Overlays.Mods private void load(OsuGameBase game, OverlayColourProvider colourProvider, OsuColour colours) { availableMods.BindTo(game.AvailableMods); + // this `BindValueChanged` callback is intentionally here, to ensure that local available mods are constructed as early as possible. + // this is needed to make sure no external changes to mods are dropped while mod panels are asynchronously loading. + availableMods.BindValueChanged(_ => updateLocalAvailableMods(), true); headerBackground.Colour = accentColour = colours.ForModType(ModType); @@ -240,33 +265,26 @@ namespace osu.Game.Overlays.Mods contentBackground.Colour = colourProvider.Background4; } - protected override void LoadComplete() - { - base.LoadComplete(); - availableMods.BindValueChanged(_ => Scheduler.AddOnce(updateMods)); - SelectedMods.BindValueChanged(_ => - { - // if a load is in progress, don't try to update the selection - the load flow will do so. - if (latestLoadTask == null) - updateActiveState(); - }); - updateMods(); - } - - private CancellationTokenSource? cancellationTokenSource; - - private void updateMods() + private void updateLocalAvailableMods() { var newMods = ModUtils.FlattenMods(availableMods.Value.GetValueOrDefault(ModType) ?? Array.Empty()) .Select(m => m.DeepClone()) .ToList(); - if (newMods.SequenceEqual(panelFlow.Children.Select(p => p.Mod))) + if (newMods.SequenceEqual(localAvailableMods)) return; + localAvailableMods = newMods; + Scheduler.AddOnce(loadPanels); + } + + private CancellationTokenSource? cancellationTokenSource; + + private void loadPanels() + { cancellationTokenSource?.Cancel(); - var panels = newMods.Select(mod => CreateModPanel(mod).With(panel => panel.Shear = new Vector2(-ShearedOverlayContainer.SHEAR, 0))); + var panels = localAvailableMods.Select(mod => CreateModPanel(mod).With(panel => panel.Shear = new Vector2(-ShearedOverlayContainer.SHEAR, 0))); Task? loadTask; @@ -280,20 +298,7 @@ namespace osu.Game.Overlays.Mods foreach (var panel in panelFlow) { - panel.Active.BindValueChanged(_ => - { - updateToggleAllState(); - - var newSelectedMods = SelectedMods.Value; - - var matchingModInstance = SelectedMods.Value.SingleOrDefault(selected => selected.GetType() == panel.Mod.GetType()); - if (matchingModInstance != null && (matchingModInstance != panel.Mod || !panel.Active.Value)) - newSelectedMods = newSelectedMods.Except(matchingModInstance.Yield()).ToArray(); - if (panel.Active.Value) - newSelectedMods = newSelectedMods.Append(panel.Mod).ToArray(); - - SelectedMods.Value = newSelectedMods.ToArray(); - }); + panel.Active.BindValueChanged(_ => panelStateChanged(panel)); } }, (cancellationTokenSource = new CancellationTokenSource()).Token); loadTask.ContinueWith(_ => @@ -306,14 +311,62 @@ namespace osu.Game.Overlays.Mods private void updateActiveState() { foreach (var panel in panelFlow) + panel.Active.Value = SelectedMods.Contains(panel.Mod); + } + + /// + /// This flag helps to determine the source of changes to . + /// If the value is false, then are changing due to a user selection on the UI. + /// If the value is true, then are changing due to an external call. + /// + private bool externalSelectionUpdateInProgress; + + private void panelStateChanged(ModPanel panel) + { + updateToggleAllState(); + + var newSelectedMods = panel.Active.Value + ? SelectedMods.Append(panel.Mod) + : SelectedMods.Except(panel.Mod.Yield()); + + SelectedMods = newSelectedMods.ToArray(); + if (!externalSelectionUpdateInProgress) + SelectionChangedByUser?.Invoke(); + } + + /// + /// Adjusts the set of selected mods in this column to match the passed in . + /// + /// + /// This method exists to be able to receive mod instances that come from potentially-external sources and to copy the changes across to this column's state. + /// uses this to substitute any external mod references in + /// to references that are owned by this column. + /// + internal void SetSelection(IReadOnlyList mods) + { + externalSelectionUpdateInProgress = true; + + var newSelection = new List(); + + foreach (var mod in localAvailableMods) { - var matchingSelectedMod = SelectedMods.Value.SingleOrDefault(selected => selected.GetType() == panel.Mod.GetType()); - panel.Active.Value = matchingSelectedMod != null; - if (panel.Active.Value) - panel.Mod.CopyFrom(matchingSelectedMod); + var matchingSelectedMod = mods.SingleOrDefault(selected => selected.GetType() == mod.GetType()); + + if (matchingSelectedMod != null) + { + mod.CopyFrom(matchingSelectedMod); + newSelection.Add(mod); + } else - panel.Mod.ResetSettingsToDefaults(); + { + mod.ResetSettingsToDefaults(); + } } + + SelectedMods = newSelection; + updateActiveState(); + + externalSelectionUpdateInProgress = false; } #region Bulk select / deselect diff --git a/osu.Game/Overlays/Mods/ModSelectScreen.cs b/osu.Game/Overlays/Mods/ModSelectScreen.cs index 2f6eae0afb..bb12143d7c 100644 --- a/osu.Game/Overlays/Mods/ModSelectScreen.cs +++ b/osu.Game/Overlays/Mods/ModSelectScreen.cs @@ -179,7 +179,7 @@ namespace osu.Game.Overlays.Mods foreach (var column in columnFlow.Columns) { - column.SelectedMods.BindValueChanged(updateBindableFromSelection); + column.SelectionChangedByUser += updateBindableFromSelection; } customisationVisible.BindValueChanged(_ => updateCustomisationVisualState(), true); @@ -250,33 +250,26 @@ namespace osu.Game.Overlays.Mods private void updateSelectionFromBindable() { - // note that selectionBindableSyncInProgress is purposefully not checked here. - // this is because in the case of mod selection in solo gameplay, a user selection of a mod can actually lead to deselection of other incompatible mods. - // to synchronise state correctly, updateBindableFromSelection() computes the final mods (including incompatibility rules) and updates SelectedMods, - // and this method then runs unconditionally again to make sure the new visual selection accurately reflects the final set of selected mods. - // selectionBindableSyncInProgress ensures that mutual infinite recursion does not happen after that unconditional call. + // `SelectedMods` may contain mod references that come from external sources. + // to ensure isolation, first pull in the potentially-external change into the mod columns... foreach (var column in columnFlow.Columns) - column.SelectedMods.Value = SelectedMods.Value.Where(mod => mod.Type == column.ModType).ToArray(); + column.SetSelection(SelectedMods.Value); + + // and then, when done, replace the potentially-external mod references in `SelectedMods` with ones we own. + updateBindableFromSelection(); } - private bool selectionBindableSyncInProgress; - - private void updateBindableFromSelection(ValueChangedEvent> modSelectionChange) + private void updateBindableFromSelection() { - if (selectionBindableSyncInProgress) + var candidateSelection = columnFlow.Columns.SelectMany(column => column.SelectedMods).ToArray(); + + if (candidateSelection.SequenceEqual(SelectedMods.Value)) return; - selectionBindableSyncInProgress = true; - - SelectedMods.Value = ComputeNewModsFromSelection( - modSelectionChange.NewValue.Except(modSelectionChange.OldValue), - modSelectionChange.OldValue.Except(modSelectionChange.NewValue)); - - selectionBindableSyncInProgress = false; + SelectedMods.Value = ComputeNewModsFromSelection(SelectedMods.Value, candidateSelection); } - protected virtual IReadOnlyList ComputeNewModsFromSelection(IEnumerable addedMods, IEnumerable removedMods) - => columnFlow.Columns.SelectMany(column => column.SelectedMods.Value).ToArray(); + protected virtual IReadOnlyList ComputeNewModsFromSelection(IReadOnlyList oldSelection, IReadOnlyList newSelection) => newSelection; protected override void PopIn() { diff --git a/osu.Game/Overlays/Mods/UserModSelectScreen.cs b/osu.Game/Overlays/Mods/UserModSelectScreen.cs index ed0a07521b..ca33d35605 100644 --- a/osu.Game/Overlays/Mods/UserModSelectScreen.cs +++ b/osu.Game/Overlays/Mods/UserModSelectScreen.cs @@ -14,9 +14,12 @@ namespace osu.Game.Overlays.Mods { protected override ModColumn CreateModColumn(ModType modType, Key[] toggleKeys = null) => new UserModColumn(modType, false, toggleKeys); - protected override IReadOnlyList ComputeNewModsFromSelection(IEnumerable addedMods, IEnumerable removedMods) + protected override IReadOnlyList ComputeNewModsFromSelection(IReadOnlyList oldSelection, IReadOnlyList newSelection) { - IEnumerable modsAfterRemoval = SelectedMods.Value.Except(removedMods).ToList(); + var addedMods = newSelection.Except(oldSelection); + var removedMods = oldSelection.Except(newSelection); + + IEnumerable modsAfterRemoval = newSelection.Except(removedMods).ToList(); // the preference is that all new mods should override potential incompatible old mods. // in general that's a bit difficult to compute if more than one mod is added at a time,