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

Add support for creating, joining, and parting osu!web rooms via interop #265

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

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Jan 23, 2025

This server needs the ability to manage rooms for matchmaking. Because I intend to use the existing MultiplayerHub for matchmaking messages, I took a bit of a detour and made it the single endpoint for creating multiplayer rooms so that the client will no longer need to juggle two endpoints for doing certain things.

I see a future where we move playlists, daily challenge, and even the lounge to this server, but I'm taking small meaningful steps because matchmaking is the goal for now.

Avoid serialising raw details about the exception (e.g. URL) and unwrap
`{ "error": "" }` JSON responses, for instance as returned by the room
creation endpoint.

Client receives:
- `APP_DEBUG=true` -> "422: Unprocessable Entity" (HTTP status code msg)
- `APP_DEBUG=false` -> "beatmaps not found: ..." (osu!web API error msg)
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 base implementation here was pulled from osu-server-beatmap-submission with local adjustments for this server's usage (return value and HubException handling).

Comment on lines -723 to +735
await leaveRoom(state, true);
await base.CleanUpState(state);
await leaveRoom(state, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meaningless change because the base implementation is empty. I just thought I'd make this consistent with other implementations, seeing as leaveRoom can throw an exception.

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.

Looks mostly quaint although I do wonder how much discussion on general direction happened in places I can't see or hear.

osu.Server.Spectator/Services/ILegacyIO.cs Outdated Show resolved Hide resolved
await runLegacyIO(HttpMethod.Delete, $"multiplayer/rooms/{roomId}/users/{userId}");
}

private class CreateRoomRequest : Room
Copy link
Collaborator

Choose a reason for hiding this comment

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

mild teeth pain from this inheritance but I guess anything else would probably be more annoying...

i guess this can stay for now maybe but as soon as this is touched for any reason we should probably decouple from Room ASAP

osu.Server.Spectator/AppSettings.cs Outdated Show resolved Hide resolved
@bdach bdach requested a review from peppy January 24, 2025 09:06
Co-authored-by: Berkan Diler <[email protected]>
@bdach
Copy link
Collaborator

bdach commented Jan 27, 2025

Something is going to need to happen to fix the test failures here. Easiest suggestion would be to not have a static ctor in AppSettings and instead just have getters delegate directly to Environment.GetEnvironmentVariable().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants