Rewrite playlists to not inherit RoomSubScreen
#31882
Conversation
Reducing the amount of nesting to make things more readable.
If style changes, then we also need to re-validate mods. Thus, we should just update the entire selection anyway, and merge the "gameplay state" into it for simplicity.
If one of these elments is hidden, the following spacing element is expected to be hidden too. Simplest is to use margins. Old implementation already did this.
| // Reset beatmap style when no longer from the same beatmap set. | ||
| if (userBeatmap.Value != null && userBeatmap.Value.BeatmapSet!.OnlineID != item.Beatmap.BeatmapSet!.OnlineID) | ||
| userBeatmap.Value = null; | ||
|
|
||
| IBeatmapInfo gameplayBeatmap = userBeatmap.Value ?? item.Beatmap; | ||
|
|
||
| // Reset ruleset style when no longer valid for the beatmap. | ||
| if (userRuleset.Value != null) | ||
| { | ||
| int beatmapRuleset = gameplayBeatmap.Ruleset.OnlineID; | ||
| if (beatmapRuleset > 0 && userRuleset.Value.OnlineID != beatmapRuleset) | ||
| userRuleset.Value = null; | ||
| } |
There was a problem hiding this comment.
Mild readability concern, action not expected: I'm not super comfortable with this code running regardless of state of item.Freestyle. I get that the null checks on user{Beatmap,Ruleset} are designed to catch that out but I'd still prefer all this to be in a code block explicitly predicated on item.Freestyle.
| public override void OnSuspending(ScreenTransitionEvent e) | ||
| { | ||
| endHandlingTrack(); | ||
| base.OnSuspending(e); | ||
| } |
There was a problem hiding this comment.
Blocker: OnSuspending() should ensure the user mod select overlay is hidden, like RoomSubScreen does. Without it, with some good timing, you can enter PlayerLoader with user mod select still active. (It takes some timing, but I did do it. Press green button to start gameplay and then immediately after, press F1.)
That logic is currently in OnBackButton() but it should not be.
| public override bool OnExiting(ScreenExitEvent e) | ||
| { | ||
| if (!ensureExitConfirmed()) | ||
| return true; | ||
|
|
||
| RoomManager?.PartRoom(); | ||
|
|
||
| endHandlingTrack(); | ||
| return base.OnExiting(e); | ||
| } |
There was a problem hiding this comment.
Blocker: OnExiting() should clear global mods, like RoomSubScreen does. Without doing that, you can have something like the following happen:
2025-02-25.12-04-18.mp4
The issue is the track speeding up again after exiting the room subscreen (where it correctly resets to 1x speed) and then re-entering it to create a new room.
|
Operationally, please advise how to proceed with order of merge of this and the series starting from #31637, as I don't see an explicit dependency listed in the OP, but I also don't foresee a world where the two don't conflict even a little bit. |
|
I worked on these changes separately, so I don't mind them being merged in any order. I'll have to resolve conflicts either way. |
This is a full rewrite of
PlaylistsRoomSubScreenas a standalone screen that no longer inherits fromRoomSubScreen. What I want to achieve primarily is the eventual separation of multiplayer and playlists structures/operation, so I plan to do the same withMultiplayerMatchSubScreennext.It's a bit more than moving around code from the base class, but a bit less than totally redesigning every component. So while some things could be improved such as embedding the background box into
PlaylistsRoomFooter, I haven't done it to keep the changes focused towards my immediate goal.I suggest going through the screen with a fresh set of eyes.