Remove `CachedModelDependencyContainer` and `OnlinePlayComposite` from multiplayer #30634

smoogipoo · 2024-11-15T07:01:06Z

Prereqs:

Make RearrangeableListContainer<> only replace differences osu-framework#6425
framework package bump

Please read previous PRs #30633, #30631, #30598, and #30592 for a gradual explanation of the important changes made.

Most important changes are to `RoomSubScreen` and `PlaylistsRoomSubScreen`, because those are the only two cases that now bind to the event instead.

peppy

Posting partial review, 60/103 files in.

peppy · 2024-11-15T07:14:06Z

osu.Game/Beatmaps/Drawables/UpdateableBeatmapBackgroundSprite.cs

+        /// <summary>
+        /// Delay before the background is loaded while on-screen.
+        /// </summary>
+        public double BackgroundLoadDelay = 500;


Minor gripe but I think these feel better as explicit properties. I see fields like this and expect them to be constants.

diff --git a/osu.Game/Beatmaps/Drawables/UpdateableBeatmapBackgroundSprite.cs b/osu.Game/Beatmaps/Drawables/UpdateableBeatmapBackgroundSprite.cs index 8ddda485b5..be6ca43f4b 100644 --- a/osu.Game/Beatmaps/Drawables/UpdateableBeatmapBackgroundSprite.cs +++ b/osu.Game/Beatmaps/Drawables/UpdateableBeatmapBackgroundSprite.cs @@ -19,12 +19,12 @@ public partial class UpdateableBeatmapBackgroundSprite : ModelBackedDrawable<IBe /// <summary> /// Delay before the background is loaded while on-screen. /// </summary> - public double BackgroundLoadDelay = 500; + public double BackgroundLoadDelay { get; set; } = 500; /// <summary> /// Delay before the background is unloaded while off-screen. /// </summary> - public double BackgroundUnloadDelay = 10000; + public double BackgroundUnloadDelay { get; set; } = 10000; [Resolved] private BeatmapManager beatmaps { get; set; } = null!;

peppy · 2024-11-15T07:19:32Z

osu.Game/Online/Multiplayer/MultiplayerClient.cs


-                    throw new AggregateException(exceptionText.ToString());
-                }
+                Room.Playlist[Room.Playlist.IndexOf(Room.Playlist.Single(existing => existing.ID == item.ID))] = item;


This is still a mouthful to parse. But I guess apart from splitting out variables there's not much to be done.

diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index 998a34931d..7105a3fc91 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -771,8 +771,16 @@ public Task PlaylistItemChanged(MultiplayerPlaylistItem item) Debug.Assert(APIRoom != null); - Room.Playlist[Room.Playlist.IndexOf(Room.Playlist.Single(existing => existing.ID == item.ID))] = item; - APIRoom.Playlist = APIRoom.Playlist.Select((pi, i) => pi.ID == item.ID ? new PlaylistItem(item) : APIRoom.Playlist[i]).ToArray(); + var existingItem = Room.Playlist.Single(existing => existing.ID == item.ID); + Room.Playlist[Room.Playlist.IndexOf(existingItem)] = item; + + APIRoom.Playlist = APIRoom.Playlist.Select((pi, i) => + { + if (pi.ID == item.ID) + return new PlaylistItem(item); + + return APIRoom.Playlist[i]; + }).ToArray(); ItemChanged?.Invoke(item); RoomUpdated?.Invoke();

Marginally better maybe?

Can you just think of this as temporary? I really don't want to duplicate 10 lines of code across 3 files, and in one case where existingItem is already a local variable so there's even going to be different syntactical structures to the code...

peppy · 2024-11-15T07:33:48Z

osu.Game/Screens/OnlinePlay/Components/MatchBeatmapDetailArea.cs

+        private void updateRoomPlaylist()
+            => playlist.Items.ReplaceRange(0, playlist.Items.Count, room.Playlist);


I think this is going to require some better handling. It can be pretty slow to do a full reload of this list:

osu.2024-11-15.at.07.33.18.mp4

Should be fixed by ppy/osu-framework#6425

peppy · 2024-11-15T07:52:04Z

osu.Game/Screens/OnlinePlay/Lounge/Components/DrawableRoomParticipantsList.cs

+                if (a.User == null)
+                    return false;


Should this not be removed (ie. return true) if it's unwanted?

I've done my best to preserve existing semantics in this PR to not cause unexpected side-effects. Unless you can prove to me otherwise, this will not be changing.

peppy

First pass completed.

peppy · 2024-11-15T09:02:34Z

osu.Game/Screens/OnlinePlay/Match/RoomSubScreen.cs

@@ -164,7 +164,7 @@ private void load(AudioManager audio)
                                                    new DrawableMatchRoom(Room, allowEdit)
                                                    {
                                                        OnEdit = () => settingsOverlay.Show(),
-                                                        SelectedItem = { BindTarget = SelectedItem }
+                                                        SelectedItem = SelectedItem


I'm really hesitant about this one. Assigning directly to a bindable always feels wrong. Reading back on the previous PR, the rationale seems to be that "this means we won't forget to set it"? I dunno how valid that is. Would keeping the old BindTarget method still be feasible?

And yes, we use BindableWithCurrent elsewhere like this. The difference I see is that in all those usages (to my knowledge), the property is named Current, which signifies that it has the special underlying behaviour.

In addition, we're passing this multiple levels down (see MultiplayerMatchFooter / MatchStartControl as one example). We usually tend to use DI in such a case. Could we create a MultiplayerState cached class for containing things like this as an alternative? Potentially also containing Room which seems to be passed everywhere?

I think this is one of the most important parts of this series of PRs...

the rationale seems to be that "this means we won't forget to set it"?

It enforces a contract, in the same way a ctor enforces a contract. BindTarget/BindTo and all that work, but it must always be assigned. At compile time it says that if you haven't assigned this you're definitely doing something wrong, so think about what you want to do now.
In previous code this would've been an ambient non-nullable resolved bindable that enforced the same contract at run-time, which imo is very hidden, especially for something that potentially has multiple sources (I've only applied this to SelectedItem, which is exactly this case).

the property is named Current, which signifies that it has the special underlying behaviour.

This is not significance to me because I made no effort to use or reproduce IHasCurrentValue<>. I only wanted to enforce the contract.

Could we create a MultiplayerState cached class for containing things like this as an alternative?

To be honest, if you want that then this series of PRs has no need to exist. Because, the natural follow up is - if we BDL Room, then we might as well keep every property in Room also bindable. Most of the complexity of this PR goes away I believe.

It's a different take on things. I can't say whether that would be a bad idea, so I'm going to block this PR as I investigate that path instead.

As discussed IRL, my proposed direction is likely not where we want to go. I can agree that we should avoid caching "states" to DI, especially when those states are not singleton (smoogi mentioned that if we have states here, there will need to be multiple layers to handle lobby/room).

So let's leave things how they are for now.

peppy · 2024-11-15T09:05:26Z

osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs

+                updateRoomName();
+                updateRoomType();
+                updateRoomQueueMode();
+                updateRoomPassword();
+                updateRoomAutoSkip();
+                updateRoomMaxParticipants();
+                updateRoomAutoStartDuration();
+                updateRoomPlaylist();


a bit painful..

I'm aware and mentioned as such in #30631.

peppy · 2024-11-15T09:51:22Z

FWIW I've run tests locally for over an hour

TestSceneMultiplayer
TestSceneMultiplayerMatchSubScreen

No failures.. promising 👍

bdach

Not sure if it'll be useful but I gave this a review pass.

Generally I will say I don't have the warmest of feelings towards INotifyPropertyChanged as an interface from prior WPF exposure to it, and it doesn't feel great to basically use yet another method of "notify changed" given we have bindables which are already intended for this expressly. Another thing is that lists don't mix super well with INotifyPropertyChanged because you can't raise a collection changed event, and so all consumers, if wanting to be efficient, will have to do their local diffing.

That said, from prior experience with daily challenge, multiplayer did feel a little "too boxed in" abstraction-wise. I'm not 100% sure whether this PR helps much in that respect since it seems to be focused more on pure mechanics of change notifications, but I'm willing to try it in pursuit of new features.

Overall I'm neutral on this change. Willing to let this chain run its paces and see what happens.

osu.Game/Screens/OnlinePlay/Components/StatusColouredContainer.cs

bdach · 2024-11-19T08:15:23Z

osu.Game/Screens/OnlinePlay/Lounge/Components/DrawableRoom.cs

+        private void updateRoomName()
+        {
+            if (roomName != null)
+                roomName.Text = Room.Name;
+        }
+
+        private void updateRoomCategory()
+        {
+            if (Room.Category > RoomCategory.Normal)
+                specialCategoryPill?.Show();
+            else
+                specialCategoryPill?.Hide();
+        }
+
+        private void updateRoomType()
+        {
+            if (endDateInfo != null)
+                endDateInfo.Alpha = Room.Type == MatchType.Playlists ? 1 : 0;
+        }
+
+        private void updateRoomHasPassword()
+        {
+            if (passwordIcon != null)
+                passwordIcon.Alpha = Room.HasPassword ? 1 : 0;
        }


Related to comment above - I probably would have merged these into one method, from experience. I'm talking about every case where these groups of small updateX() methods have materialised; in my experience the efficiency tradeoff has rarely been worth the mental overhead.

I expected these PRs to be reviewed part by part and not in the final thing. I won't do the NRT changes next time and keep it to one PR, my bad.

In each prior PR I wrote a little explanation of what I believe to be the important parts. I mentioned that I'm aware of this separation in #30631, and am also not convinced with it/will clean it up.

I expected these PRs to be reviewed part by part and not in the final thing

I would have done so, but given that seemingly a full review of this diff was done already here, it felt like the wrong thing to review piece-by-piece now...?

smoogipoo · 2024-11-19T09:43:19Z

Closed all other PRs and linked them in the description here because it seems everyone is reading this PR. Apologies for the size :/

smoogipoo · 2024-11-19T10:10:20Z

My thinking going into INotifyPropertyChanged is that MultiplayerClient already does the same thing, and for it to use bindables would be very annoying.

In #30592 I mentioned that I'm not fully sold on the current implementation/location but this is a first step.

Didn't really work to fix these tests due to the sticky nature of the failure. Also I can no longer reproduce locally, so the hope is that these are fixed by ppy#30634.

smoogipoo added 28 commits November 12, 2024 18:17

Make CurrentPlaylistItem not a bindable

72564b5

Fix inspection

8d5cd2b

Make DrawableMatchRoom structurally match other implementations

f3fd87a

Reorder public property vs private field

db025d8

Make RoomID non-bindable

99762da

Most important changes are to `RoomSubScreen` and `PlaylistsRoomSubScreen`, because those are the only two cases that now bind to the event instead.

Fix cases where the bindable is resolved directly

5b2568d

Fix CI issues

a2a930a

Make Room.Name non-bindable

6160df1

Make Room.Host non-bindable

8694f7e

Make Room.Category non-bindable

bde7b8e

Make Room.Type non-bindable

81e4cb3

Make Room.Status non-bindable

5d4838a

Make Room.Availability non-bindable

7e3e520

Make Room.QueueMode non-bindable

198681e

Make Room.Password & Room.HasPassword non-bindable

ec5be6d

Make Room.AutoSkip non-bindable

f001cce

Make Room.ParticipantCount & Room.MaxParticipants non-bindable

b8bae30

Make Room.AutoStartDuration non-bindable

89de4f0

Make Room.Duration & Room.StartDate & Room.EndDate non-bindable

0ceaafe

Make Room.MaxAttempts non-bindable

6c84e42

Make Room.ChannelId non-bindable

80b3e33

Make Room.PlaylistItemStats non-bindable

487a010

Make Room.DifficultyRange non-bindable

c4f8fd1

Make Room.UserScore non-bindable

dc5337d

Make Room.RecentParticipants non-bindable

b16edbb

Make Room.Playlist non-bindable

34c0f72

Remove OnlinePlayComposite

bfbae94

Remove CachedModelDependencyContainer usages from online play

1a656d0

smoogipoo added type:code-quality area:multiplayer labels Nov 15, 2024

pull-request-size bot added the size/XXL label Nov 15, 2024

Remove IDependencyInjectionCandidate interface from Room

db7def9

peppy self-requested a review November 15, 2024 07:07

peppy reviewed Nov 15, 2024

View reviewed changes

smoogipoo added the blocked Don't merge this. label Nov 15, 2024

smoogipoo mentioned this pull request Nov 16, 2024

Leaving a multiplayer lobby now plays the "host leaving" sound #30665

Open

bdach reviewed Nov 19, 2024

View reviewed changes

Use properties instead of fields

a5327aa

smoogipoo removed the blocked Don't merge this. label Nov 19, 2024

Fix inspection issues

9ac8776

peppy self-requested a review November 20, 2024 07:34

peppy added 4 commits November 20, 2024 16:40

Update framework

50089c0

Fix missing updateRoomPlaylist call

03de518

Remove MultiplayerMatchSubScreen flaky test attribute

c6d08da

Didn't really work to fix these tests due to the sticky nature of the failure. Also I can no longer reproduce locally, so the hope is that these are fixed by ppy#30634.

Merge branch 'master' into multiplayer-remove-cmc-and-composite

6fba6de

peppy approved these changes Nov 20, 2024

View reviewed changes

peppy merged commit fabeb6f into ppy:master Nov 20, 2024
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `CachedModelDependencyContainer` and `OnlinePlayComposite` from multiplayer #30634

Remove `CachedModelDependencyContainer` and `OnlinePlayComposite` from multiplayer #30634

smoogipoo commented Nov 15, 2024 •

edited by peppy

Loading

peppy left a comment

peppy Nov 15, 2024

peppy Nov 15, 2024

smoogipoo Nov 19, 2024 •

edited

Loading

peppy Nov 20, 2024

peppy Nov 15, 2024

smoogipoo Nov 15, 2024

peppy Nov 15, 2024

smoogipoo Nov 19, 2024

peppy left a comment

peppy Nov 15, 2024

smoogipoo Nov 15, 2024 •

edited

Loading

peppy Nov 20, 2024

peppy Nov 15, 2024

smoogipoo Nov 19, 2024

peppy commented Nov 15, 2024

bdach left a comment •

edited

Loading

bdach Nov 19, 2024

smoogipoo Nov 19, 2024 •

edited

Loading

bdach Nov 19, 2024 •

edited

Loading

smoogipoo commented Nov 19, 2024

smoogipoo commented Nov 19, 2024

		private void updateRoomPlaylist()
		=> playlist.Items.ReplaceRange(0, playlist.Items.Count, room.Playlist);

Remove CachedModelDependencyContainer and OnlinePlayComposite from multiplayer #30634

Remove CachedModelDependencyContainer and OnlinePlayComposite from multiplayer #30634

Conversation

smoogipoo commented Nov 15, 2024 • edited by peppy Loading

peppy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy commented Nov 15, 2024

bdach left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

bdach Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

smoogipoo commented Nov 19, 2024

smoogipoo commented Nov 19, 2024

Remove `CachedModelDependencyContainer` and `OnlinePlayComposite` from multiplayer #30634

Remove `CachedModelDependencyContainer` and `OnlinePlayComposite` from multiplayer #30634

smoogipoo commented Nov 15, 2024 •

edited by peppy

Loading

smoogipoo Nov 19, 2024 •

edited

Loading

smoogipoo Nov 15, 2024 •

edited

Loading

bdach left a comment •

edited

Loading

smoogipoo Nov 19, 2024 •

edited

Loading

bdach Nov 19, 2024 •

edited

Loading