osu!std editor confuses the mods menu in non-standard rulesets

[SherlockWh0]

Type

Cosmetic

Bug description

1. Apply mods in song select (use modes other than std and only std maps)
2. Right click the song and click 'edit'
3. Close the editor
4. In song select, the mods are shown
5. But click the mods menu, and the mods won't be there
6. The mods still work though.

Screenshots or videos

no vid, too big

Version

2023.305.0-lazer

Logs

database.log input.log legacy-ipc.log network.log performance-draw.log performance-update.log performance.log runtime.log

[peppy]

I remember we've had discussion in the past about things like Editor affecting the game-wide SelectedMods, but I don't remember the resolution. While I'm looking to fix this while not changing that behaviour, it feels weird and this whole issue could have been avoided if that didn't happen in the first place.

diff --git a/osu.Game/Overlays/Mods/ModSelectOverlay.cs b/osu.Game/Overlays/Mods/ModSelectOverlay.cs
index 16602db4be..7c6d2c6057 100644
--- a/osu.Game/Overlays/Mods/ModSelectOverlay.cs
+++ b/osu.Game/Overlays/Mods/ModSelectOverlay.cs
@@ -230,18 +230,6 @@ private void load(OsuGameBase game, OsuColour colours, AudioManager audio)

         protected override void LoadComplete()
         {
-            // this is called before base call so that the mod state is populated early, and the transition in `PopIn()` can play out properly.
-            globalAvailableMods.BindValueChanged(_ => createLocalMods(), true);
-
-            base.LoadComplete();
-
-            State.BindValueChanged(_ => samplePlaybackDisabled.Value = State.Value == Visibility.Hidden, true);
-
-            // This is an optimisation to prevent refreshing the available settings controls when it can be
-            // reasonably assumed that the settings panel is never to be displayed (e.g. FreeModSelectOverlay).
-            if (AllowCustomisation)
-                ((IBindable<IReadOnlyList<Mod>>)modSettingsArea.SelectedMods).BindTo(SelectedMods);
-
             SelectedMods.BindValueChanged(val =>
             {
                 modSettingChangeTracker?.Dispose();
@@ -255,7 +243,19 @@ protected override void LoadComplete()
                     modSettingChangeTracker = new ModSettingChangeTracker(val.NewValue);
                     modSettingChangeTracker.SettingChanged += _ => updateMultiplier();
                 }
-            }, true);
+            }); // initial invocation via createLocalMods().
+
+            // this is called before base call so that the mod state is populated early, and the transition in `PopIn()` can play out properly.
+            globalAvailableMods.BindValueChanged(_ => createLocalMods(), true);
+
+            base.LoadComplete();
+
+            State.BindValueChanged(_ => samplePlaybackDisabled.Value = State.Value == Visibility.Hidden, true);
+
+            // This is an optimisation to prevent refreshing the available settings controls when it can be
+            // reasonably assumed that the settings panel is never to be displayed (e.g. FreeModSelectOverlay).
+            if (AllowCustomisation)
+                ((IBindable<IReadOnlyList<Mod>>)modSettingsArea.SelectedMods).BindTo(SelectedMods);

             customisationVisible.BindValueChanged(_ => updateCustomisationVisualState(), true);

@@ -335,6 +335,8 @@ private void createLocalMods()

             foreach (var column in columnFlow.Columns.OfType<ModColumn>())
                 column.AvailableMods = AvailableMods.Value.GetValueOrDefault(column.ModType, Array.Empty<ModState>());
+
+            SelectedMods.TriggerChange();
         }

         private void filterMods()

This will fix this issue, but writing test coverage for this is not so simple due to the bindable flow.

[peppy]

https://github.com/ppy/osu/compare/master...peppy:osu:fix-mod-select-selection-after-editor?expand=1

Attempted to create a test but reproducing the fuckery that's going on in a test is near impossible. The test passes in both cases because the way I'm doing a lease still triggers a change to SelectedMods. I tried decoupling in the test scene (see 24fd07b24983eeb15ecac066b57df3993633ff0f which can be optionally removed) but I still can't get that to work. And at that point, is this not just a flawed system?

I think we can all agree the answer is "yes". I'm going to take a look into removing the decoupled faffery in song select instead.

[peppy]

In hindsight, the test may not have been working because I was leasing SelectedMods instead of Ruleset. But I'm still going to look at fixing this at a higher level.

[peppy]

Okay ignore all the above.

The actual issue is simply that SongSelect unbinds SelectedMods on suspend and rebinds on OnResume, but the rebind will not trigger a ValueChanged if the mods equal what they were before.

https://github.com/ppy/osu/blob/6afa65bd3dbda7f8b6245e0bf24ea92d233ba385/osu.Game/Screens/Select/SongSelect.cs#L628-L627

Meanwhile, AvailableMods is bound to ModSelectOverlay this whole time, triggering its own logic, causing the disconnect in display.