Rewrite match subscreen to remove bindables #32669

smoogipoo · 2025-04-04T01:42:14Z

Fixes #32503
Split out from #32250, with minor changes.

This finalises the refactorings that have been going on for a while now.

In a similar fashion to #31882, this is a full rewrite of MultiplayerMatchSubScreen that no longer inherits (and finally removes) RoomSubScreen. The primary goal here is to remove the beatmap/ruleset/mod/"selected item" bindables and use MultiplayerRoom states.

Because it's a rewrite, I suggest going through the screen anew (rather than looking at the diff) and possibly comparing to the implementation of PlaylistsRoomSubScreen.

smoogipoo · 2025-04-04T01:44:47Z

osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs

 using osuTK;
+using Container = osu.Framework.Graphics.Containers.Container;
 using ParticipantsList = osu.Game.Screens.OnlinePlay.Multiplayer.Participants.ParticipantsList;

 namespace osu.Game.Screens.OnlinePlay.Multiplayer
 {
    [Cached]


In order to address #32250 (comment), I've kept the subscreen being cached as-is, simplifying the settings overlay implementation to invoke ShowSongSelection().

smoogipoo · 2025-04-04T01:45:52Z

osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs

+            LoadComponent(userModsSelectOverlay = new MultiplayerUserModSelectOverlay
+            {
+                Beatmap = { BindTarget = Beatmap }
+            });


NB: In #32250, the beatmap wasn't bound which caused the footer to not update.

bdach

Well this one didn't get much easier to review.

Spotted a few things from reading the code, once those are fixed I'll probably just do a test in-game and click buttons if I can't break it.

bdach · 2025-04-07T10:06:21Z

osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs

+        /// Responds to changes in the active room to adjust the visibility of the settings and main content.
+        /// Only the settings overlay is visible while the room isn't created, and only the main content is visible after creation.
+        /// </summary>
+        private void onRoomUpdated() => Scheduler.AddOnce(() =>


Scheduler.AddOnce() does not debounce correctly if it's given an inline lambda like this because it's going to fail an equality check framework-side to determine if it should debounce or not. The two options are to either make the body of this a named method (this is the most idiomatic one we use), or to utilise the closure-eliminating overload of this, and have the inner lambda be a static (MultiplayerMatchSubScreen screen) => { } (don't recommend that one).

I don't get it. Have you actually tested this? This is very idiomatic code in multiplayer, and it would also mean stuff like this is broken:

osu/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomSounds.cs

Lines 42 to 55 in 3852cca

private void onUserJoined(MultiplayerRoomUser user)

=> Scheduler.AddOnce(() => userJoinedSample?.Play());

private void onUserLeft(MultiplayerRoomUser user)

=> Scheduler.AddOnce(() => userLeftSample?.Play());

private void onUserKicked(MultiplayerRoomUser user)

=> Scheduler.AddOnce(() => userKickedSample?.Play());

private void onHostChanged(MultiplayerRoomUser? host)

{

if (host != null)

Scheduler.AddOnce(() => hostChangedSample?.Play());

}

Which was added not by me, here: ef5e37c. So two people apparently believe this method works completely differently than it actually does. If this is the case I will do everything in my power to obliterate this method.

See also this o!f test: https://github.com/ppy/osu-framework/blob/5a7056a434be77ff84ecffddd27ac5617b400dac/osu.Framework.Tests/Threading/SchedulerTest.cs#L427-L439

Edit: I can't repro with:

diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index d464362fda..cb84ae4706 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -416,6 +416,9 @@ protected override void LoadComplete() onRoomUpdated(); updateGameplayState(); updateUserActivity(); + + onRoomUpdated(); + onRoomUpdated(); } /// <summary>

No, I have not tested this specific code with a debugger. I did not remember the test above existed. I wrote something like

[Test] public void TestAddOnce() { var scheduler = new Scheduler(); int invocations = 0; scheduler.AddOnce(() => invocations++); scheduler.AddOnce(() => invocations++); scheduler.Update(); Assert.That(invocations, Is.EqualTo(1)); }

and that fails. Which I guess isn't strictly the same what you have because you're never repeating that lambda again, but as that code comment in the test you linked says, I've at this point internalised that AddOnce() should not be called on inline lambdas because there be dragons and it might or it might not work as intended.

See ppy/osu-framework#6023 (comment) even, which is where that PR comment originates from, and #25076 (comment), which sparked it.

I believe the way it's conceptualised is that the lambda instance itself is always the same. i.e.

static object? a; void m() { a ??= new object(); } static object? b; static object? c; void n() { b ??= new object(); c ??= new object(); }

The usage in multiplayer is always a case of the former.

The usage in multiplayer is always a case of the former.

That's not even it actually. If you pull up the IL display in rider and put it to low level C# (for convenience) you'll see this:

private void onRoomUpdated() { this.Scheduler.AddOnce(new Action((object) this, __methodptr(<onRoomUpdated>b__77_0))); }

This is allocating a delegate instance every time, but the compiler has figured out that it can make a method group delegate out of the body of the lambda, so the delegate equality check inside scheduler doesn't fail (because the lambdas are attached to a named method). This'll only work as long as the compiler does that and not something else.

Are you fine with it as is then? As I said this is pretty standard through all of multiplayer, feels weird if we're applying any changes only to this usage.

I suppose we can keep it until a problem sure.

osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs

bdach

Empirically tested this some. Found only one thing, namely that clicking the countdown button on the bottom bar crashes with

[runtime] 2025-04-08 08:42:03 [verbose]: Too many unhandled exceptions, crashing out.
Unhandled exception. System.InvalidOperationException: Cannot show or hide a popover without a parent PopoverContainer in the hierarchy
   at osu.Framework.Extensions.PopoverExtensions.setTargetOnNearestPopover(Drawable origin, IHasPopover target)
   at osu.Framework.Extensions.PopoverExtensions.ShowPopover(IHasPopover hasPopover)
   at osu.Framework.Graphics.Containers.ClickableContainer.OnClick(ClickEvent e)
   at osu.Game.Graphics.UserInterface.OsuAnimatedButton.OnClick(ClickEvent e) in /Users/bdach/Documents/Work/open-source/osu/osu.Game/Graphics/UserInterface/OsuAnimatedButton.cs:line 117
   at osu.Framework.Input.ButtonEventManager`1.PropagateButtonEvent(IEnumerable`1 drawables, UIEvent e)
   at osu.Framework.Input.MouseButtonEventManager.handleClick(InputState state, List`1 targets)
   at osu.Framework.Input.MouseButtonEventManager.HandleButtonUp(InputState state, List`1 targets)

smoogipoo · 2025-04-08T09:37:40Z

Good catch, didn't have any tests for that.

Beyond isolation, this stuff is also used to test (de-)serialisation. Much-of-a-muchness in this case since it isn't too thoroughly tested, but best do it anyway.

smoogipoo added the area:multiplayer label Apr 4, 2025

pull-request-size bot added the size/XXL label Apr 4, 2025

smoogipoo changed the title ~~Rewrite match subscreen to use full online state~~ Rewrite match subscreen to remove bindables Apr 4, 2025

smoogipoo commented Apr 4, 2025

View reviewed changes

Rewrite match subscreen to use full online state

286b3d9

smoogipoo force-pushed the refactor-match-subscreen branch from cf6d937 to 286b3d9 Compare April 4, 2025 01:49

Remove unnecessary using

bb1cfdc

smoogipoo requested a review from bdach April 4, 2025 01:51

smoogipoo mentioned this pull request Apr 4, 2025

Difficulty attributes in multiplayer and playlist extra mod select are not correctly shown #32503

Closed

Merge branch 'master' into refactor-match-subscreen

f288cfc

This was referenced Apr 4, 2025

Allow mods/freemods in combination with freestyle #32674

Open

Allow viewing results of historical multiplayer items #32680

Merged

bdach requested changes Apr 7, 2025

View reviewed changes

smoogipoo added 2 commits April 7, 2025 20:10

Add back removed hash check

aa8ebf9

Validate state with lesser magic

71af50f

bdach requested changes Apr 8, 2025

View reviewed changes

smoogipoo added 2 commits April 8, 2025 18:35

Add failing countdown start test

968fe6e

Fix crash when trying to start countdown

5769292

Clone countdown objects in test client

874ec49

Beyond isolation, this stuff is also used to test (de-)serialisation. Much-of-a-muchness in this case since it isn't too thoroughly tested, but best do it anyway.

bdach approved these changes Apr 8, 2025

View reviewed changes

bdach merged commit b3626bf into ppy:master Apr 8, 2025
10 checks passed

smoogipoo mentioned this pull request Apr 9, 2025

Fix difficulty stats not showing in playlists #32741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite match subscreen to remove bindables #32669

Rewrite match subscreen to remove bindables #32669

smoogipoo commented Apr 4, 2025 •

edited

Loading

smoogipoo Apr 4, 2025

smoogipoo Apr 4, 2025

bdach left a comment

bdach Apr 7, 2025

smoogipoo Apr 7, 2025 •

edited

Loading

bdach Apr 7, 2025 •

edited

Loading

smoogipoo Apr 7, 2025

bdach Apr 7, 2025 •

edited

Loading

smoogipoo Apr 7, 2025

bdach Apr 7, 2025

bdach left a comment

smoogipoo commented Apr 8, 2025

	private void onUserJoined(MultiplayerRoomUser user)
	=> Scheduler.AddOnce(() => userJoinedSample?.Play());

	private void onUserLeft(MultiplayerRoomUser user)
	=> Scheduler.AddOnce(() => userLeftSample?.Play());

	private void onUserKicked(MultiplayerRoomUser user)
	=> Scheduler.AddOnce(() => userKickedSample?.Play());

	private void onHostChanged(MultiplayerRoomUser? host)
	{
	if (host != null)
	Scheduler.AddOnce(() => hostChangedSample?.Play());
	}

Rewrite match subscreen to remove bindables #32669

Rewrite match subscreen to remove bindables #32669

Conversation

smoogipoo commented Apr 4, 2025 • edited Loading

smoogipoo Apr 4, 2025

Choose a reason for hiding this comment

smoogipoo Apr 4, 2025

Choose a reason for hiding this comment

bdach left a comment

Choose a reason for hiding this comment

bdach Apr 7, 2025

Choose a reason for hiding this comment

smoogipoo Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

bdach Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

smoogipoo Apr 7, 2025

Choose a reason for hiding this comment

bdach Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

smoogipoo Apr 7, 2025

Choose a reason for hiding this comment

bdach Apr 7, 2025

Choose a reason for hiding this comment

bdach left a comment

Choose a reason for hiding this comment

smoogipoo commented Apr 8, 2025

smoogipoo commented Apr 4, 2025 •

edited

Loading

smoogipoo Apr 7, 2025 •

edited

Loading

bdach Apr 7, 2025 •

edited

Loading

bdach Apr 7, 2025 •

edited

Loading