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

Create, join, and part multiplayer rooms only via the multiplayer server #31637

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Jan 23, 2025

Old Server New Server
Old Client 🟢 🟢
New Client 🔴 🟢

osu-server-spectator is now the authority in creating multiplayer rooms. Most of the complexity of this PR is a result of RoomManager no longer having the ability to create/join/part rooms, which is now handled either by the multiplayer server or by API requests (for playlists + daily challenge).

MultiplayerRoomManager no longer exists, and I expect RoomManager to go away in the near future too. I'd actually already started down that path but wanted to keep this PR somewhat lean, and so I've only added an XMLDoc on RoomManager mentioning this.

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Relevant functionality has been removed from `RoomManager` in the
process.
In particular, when the exception is:
`AggregateException { AggregateException { HubException } }`,
then the existing code will only unwrap the first aggregate exception.

The overlay's code was copied from the extension so both have been
adjusted here.
@peppy peppy self-requested a review February 11, 2025 14:30
@peppy
Copy link
Member

peppy commented Feb 11, 2025

As a heads up, I'll review this but won't merge until after the imminent release.

@bdach bdach self-requested a review February 25, 2025 08:19
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Initial pass, have not attempted to run yet

Comment on lines +269 to +270
// Poll for any newly-created rooms (including potentially the user's own).
ListingPollingComponent.PollImmediately();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Why was this change made? Is this moved from MultiplayerLoungeSubScreen, or is this compensating for some new shortcoming?

Copy link
Contributor Author

@smoogipoo smoogipoo Feb 25, 2025

Choose a reason for hiding this comment

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

It was previously done in MultiplayerLoungeSubScreen:

public override void OnResuming(ScreenTransitionEvent e)
{
base.OnResuming(e);
// Upon having left a room, we don't know whether we were the only participant, and whether the room is now closed as a result of leaving it.
// To work around this, temporarily remove the room and trigger an immediate listing poll.
if (e.Last is MultiplayerMatchSubScreen match)
{
RoomManager?.RemoveRoom(match.Room);
ListingPollingComponent.PollImmediately();
}
}

Though it only goes half-way here... I'll look into restoring the RemoveRoom() part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine ignoring it for now if this is going to change further to reduce idle work.

Comment on lines +364 to +371
public void Close(Room room)
{
Debug.Assert(room.RoomID != null);

var request = new ClosePlaylistRequest(room.RoomID.Value);
request.Success += RefreshRooms;
api.Queue(request);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Moderate severity concern: This does not feel correctly layered. This request is only valid in playlists and will hard-fail for realtime multiplayer, as per https://github.com/ppy/osu-web/blob/3c9e99eaf4bd9e73d2712f60d67f5bc95f9dfe2b/app/Models/Multiplayer/Room.php#L683-L685.

I would prefer if this operation was pulled down the inheritance hierarchy to playlists specifically. Even if only by having the other implementations of IOnlinePlayLounge throw NotSupportedException.

@@ -361,14 +361,21 @@ public override bool OnExiting(ScreenExitEvent e)
if (!ensureExitConfirmed())
return true;

RoomManager?.PartRoom();
if (Room.RoomID != null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: What does this null check do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The null check itself is probably because this screen is used to do the setup, prior to the room being created. I foresee a future where we move the setup stage out of this screen, as it unnecessarily complicates things.

@@ -15,6 +15,9 @@ public class CreateRoomRequest : APIRequest<APICreatedRoom>
public CreateRoomRequest(Room room)
{
Room = room;

// Also copy back to the source model, since it is likely to have been stored elsewhere.
Success += r => Room.CopyFrom(r);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remark, action not expected: I get that this was copied from RoomManager, but somehow it feels much dodgier to have things work this way. With this the only thing that guarantees full correctness of behaviour is the order of execution of event handlers in the invocation list.

Possibly the way to resolve that would be to split off "this is a room to be created" from "this is an existing room" in terms of models used, but I can't exactly estimate the scope of such an undertaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this the only thing that guarantees full correctness of behaviour is the order of execution of event handlers in the invocation list.

Which can't really be relied on anyway. For example RoomID is updated first and used to determine when setup is in progress, with Playlist updated later on and used to pick out the initial selected item. This results in some scheduling awkwardness in screens:

private void updateSetupState()
{
if (Room.RoomID != null)
{
// Set the first playlist item.
// This is scheduled since updating the room and playlist may happen in an arbitrary order (via Room.CopyFrom()).
Schedule(() => SelectedItem.Value = Room.Playlist.FirstOrDefault());
}
}

What I would probably do other than this is to manually Room.CopyFrom() on success wherever required, which should only be a handful of places (from memory) - lounge, playlists, playlists creation, and some places in tests. That said, I would like to do that after #31866, which further touches on all of this - see for instance PlaylistsRoomUpdater which does room.CopyFrom() internally rather than relying on the request to do it.

Copy link
Collaborator

@bdach bdach Feb 25, 2025

Choose a reason for hiding this comment

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

For example RoomID is updated first and used to determine when setup is in progress, with Playlist updated later on and used to pick out the initial selected item. This results in some scheduling awkwardness in screens:

That snippet you linked there talks about the order of copying properties though, right? My concern is specifically with the CopyFrom() invocation being the first Success callback to run. If it were to - for whatever reason - run after any other Success callback, the callbacks that would run prior to it would see stale data, which was not previously possible because RoomManager was responsible for presenting the Room instance after CopyFrom().

The manual CopyFrom() plan sounds tolerable though, so I don't see anything else needing follow-up in this review thread.

Comment on lines -31 to -37
// this is done here as a pre-check to avoid clicking on already closed rooms in the lounge from triggering a server join.
// should probably be done at a higher level, but due to the current structure of things this is the easiest place for now.
if (room.HasEnded)
{
onError?.Invoke("Cannot join an ended room.");
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: It appears that this check is completely gone and has not been moved around. Is it no longer required for whatever reason?

I don't see it anywhere in either osu-server-spectator or osu-web either.

if (message.StartsWith(not_found_prefix, StringComparison.Ordinal))
{
ErrorText.Text = "The selected beatmap is not available online.";
room.Playlist.SingleOrDefault()?.MarkInvalid();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slight concern: This change bothers me because it assumes that room.Playlist.SingleOrDefault() is going to be the same object reference as the UI-facing, displayed, playlist item, and that feels like a brittle assumption.

Comment on lines +20 to +21
Debug.Assert(t.Exception != null);
Exception exception = t.Exception.AsSingular();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Why was this change made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the age of this PR, I can't actually remember the path that triggered this :/. Likely to do with MultiplayerClient's task nesting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason why I ask is that the helper has many more consumers, and thus it's a bit of a dicey one to accept at face value without knowing what else precisely might break.


private async Task initRoom(Room room, Func<Room, Task<MultiplayerRoom>> initFunc, CancellationToken cancellationToken)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mild code quality concern, no action expected: I've gotten kinda wary of passing Func<>s in like this, and the nondescript variable name (initFunc) isn't helping me much. What I'd propose doing instead is:

  • Extract the body of the innermost delegate here to a separate method (lines 200 through 228, call it setUpJoinedRoomAsync() or something)
  • Half-inline this method in both call sites (would be something like
await joinOrLeaveTaskChain.Add(async () =>
{
    await JoinRoomInternal(room.RoomID.Value, password ?? room.Password, cancellationSource.Token).ConfigureAwait(false);
    await setUpJoinedRoomAsync(); // add whatever parameters this needs to work
}, cancellationSource.Token).ConfigureAwait(false);

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.

None yet

3 participants