Conversation
peppy
force-pushed
the
multiplayer-select-update
branch
from
3340d23 to
f1c0279
Compare
Free mod button doesn't have this behaviour anymore. Should it? Probably. Was it forgotten? Maybe. But with the default being freestyle now, maybe it's also less important. Can still be accessed with one more click
peppy
force-pushed
the
multiplayer-select-update
branch
from
f1c0279 to
d03a6d3
Compare
| beginLooping(); | ||
| } | ||
|
|
||
| Beatmap.BindValueChanged(updateVariousState, true); |
There was a problem hiding this comment.
Moving this from LoadComplete was triggered by attempting to resolve the fact that ApplyToBackground in updateBackgroundDim was being called before the background was set previously.
I took one step further because I found that every one of the state update calls was already reapplied in onArrivingAtScreen, so it made most sense to move everything to happen at one place.
peppy
force-pushed
the
multiplayer-select-update
branch
from
1f4468b to
6e40254
Compare
| Mods.BindValueChanged(_ => updateValidMods()); | ||
| Ruleset.BindValueChanged(onRulesetChanged); | ||
| freestyle.BindValueChanged(onFreestyleChanged); | ||
|
|
||
| freeModSelectOverlayRegistration = OverlayManager?.RegisterBlockingOverlay(freeModSelect); | ||
|
|
||
| updateFooterButtons(); | ||
| updateValidMods(); | ||
|
|
||
| operationInProgress.BindTo(operationTracker.InProgress); | ||
| operationInProgress.BindValueChanged(operation => | ||
| { | ||
| if (operation.NewValue) | ||
| loadingLayer.Show(); | ||
| else | ||
| loadingLayer.Hide(); | ||
| }, true); |
There was a problem hiding this comment.
Why was this chunk of code reordered?
The reason I ask is that this reordering breaks the free mod selection overlay. If a pre-existing playlist item has free mods selected, they will not appear as selected when re-entering the screen.
Screen.Recording.2026-02-25.at.11.28.53.mov
As far as I can tell reordering this chunk causes this because moving the change callbacks to here makes it so
- executes and assigns the correct mods.
-
executes. Because of the logic reordering, this fires the value change callback for
freestyle. - In the aforementioned value change callback, executes. However at this point
osu/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs
Lines 175 to 176 in d6ed124
freeModSelectis inLoadState.Readyrather thanLoaded, meaning thatAllAvailableModsare empty, meaning that all of the mods are discarded.
There was a problem hiding this comment.
I will... check on this with a fresh brain because that all sounds like things which need to be very well documented.
A quick answer would be that I either reordered for style purposes, or because tests were failing with the old ordering (although I think I later resolved such failures in a better way).
There was a problem hiding this comment.
Thanks for the explanation here. I've reverted the ordering changes and added test coverage in 93d12d5.
|
Side note, if you enable all free mods, then select some required mods and deselect them, some free mods get deselected as a byproduct: Screen.Recording.2026-02-25.at.11.51.11.movBut the old song select did this too, so it's probably whatever. |
|
Investigating failing test (of course it didn't fail locally) |
2 participants
Tests pass and seems to work. Need to do a bit more self-testing for higher confidence, but in theory..
Closes #34035.