Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace MultiplayerRoomComposite with local bindings #30525

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

smoogipoo
Copy link
Contributor

This is the start of trying to make the multiplayer / online play structure more understandable, and safe in light of the discovery of unsafe bindable bindings that are killing tests and have also been seen in the wild.

A few important parts to look out for:

Scope of changes

I've tried to make as little changes as possible, but...

  • I've continued to bind to RoomUpdated in cases where that could be replaced with a (not yet existing) HostChanged event.
  • #nullable disable has been removed from every touched class.

Scheduling semantics

  • OnRoomUpdated()/OnRoomLoadRequested() use Scheduler.AddOnce(). The semantics have not changed.
  • UserJoined()/UserKicked()/UserLeft() uses Scheduler.Add(). These now use Schedule() which applies the transform time but I don't believe this to be significant going by the originating commit.
  • All other events use Schedule(). The semantics have not changed.

Comment on lines +31 to +32
[Cached(typeof(IBindable<PlaylistItem>))]
private readonly Bindable<PlaylistItem> currentItem = new Bindable<PlaylistItem>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned IRL, I'm kinda against having raw IBindable<> things being cached like this. I know this is an existing usage, but hoping we can refactor as a future pass. For instance, putting the bindables inside a MultiplayerState would resolve the issue for me.

I just really dislike caching of bindables like this (and tracking down where the bindables are cached). It took me quite some minutes to figure out that you weren't adding a new cached bindable here and that it was already being provided in RoomSubScreen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree. The path I'm taking right now is to completely remove bindable caching either directly or ferried via another object, and seeing if a case comes up where it breaks down and the current structure makes more sense (hopefully not).

Basically, I don't know how things will end up yet because I haven't gone the full way 😛 It's entirely possible this PR may be reverted at some point if things don't pan out well.

@peppy peppy merged commit 54288c3 into ppy:master Nov 7, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants