Skip to content
New issue

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

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

Already on GitHub? # to your account

Add support for "freestyle" in multiplayer #31260

Merged
merged 42 commits into from
Feb 4, 2025

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Dec 24, 2024

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

Preamble

I was told to effectively hack this in, and was not given an opportunity to refactor multiplayer to properly support it or even to be able to write tests without pulling my hair out. So here goes.

Have I done some basic testing to see things work? Yes.
Will things break? Definitely.
Do I know what will break? Nope.

Description

2024-12-24.19-46-05.mp4

I've named this "freestyle", because I see a future where this is more than just beatmap/ruleset but also mods and thus include "freemods" in it. Perhaps something like a menu where you can choose what is free.

For the time being, when it's enabled, it will disable mods/freemods. I'm not sure what the intended UX is, but it will definitely be a lot more complicated due to mod types not matching between rulesets, i.e. this will fail a ton of validation everywhere. So I'm keeping it simple.

There's no display in the room for what/how other users have chosen to play, I'm not even sure where I'd add that in because the room design is a whole bunch of mess. There's already no display of other users' freemods either, so /shrug?

@peppy
Copy link
Member

peppy commented Dec 24, 2024

There's already no display of other users' freemods either, so /shrug?

2024-12-24 21 08 35@2x

It's shown here. I think adding the ruleset icon in the same flow area at minimum would be good to have.

@peppy
Copy link
Member

peppy commented Dec 24, 2024

@ppy/team-design able to provide some basic design direction for how ruleset and difficulty selection could be shown per-user here?

@smoogipoo
Copy link
Contributor Author

Had to pretty much rewrite the entire screen and add server-side support to do the above ¯\(ツ)/¯. Added it as a ruleset icon displayed on the panel:

2024-12-25.23-36-05.mp4

@peppy peppy self-requested a review January 24, 2025 09:21
@smoogipoo
Copy link
Contributor Author

Showing freestyle status on lobby listings (filtering to rooms with it enabled?)

image

what causes the server imcompatibility mentioned in the OP

Checked, seems fine.

@peppy
Copy link
Member

peppy commented Jan 29, 2025

I'm going to do some refactoring because the following is very confusing:

  • GetGameplayRuleset is virtual (and overridden by the multiplayer screen), but also reads from UserRuleset which is expected to be set by the multiplayer screen.
  • UserRuleset sits alongside the existing UserMods, but UserMods is never written to outside RoomSubScreen while UserRuleset is.

@peppy
Copy link
Member

peppy commented Jan 29, 2025

@smoogipoo i'm still working through this, but can you have a check on the commits above? i don't think there should be anything too contested. some of the flow has changed / removed, but in testing it seems to still hold up so far.

@smoogipoo
Copy link
Contributor Author

smoogipoo commented Jan 29, 2025

I don't know if you've tested the whole onRoomUpdated() flow, because I know there's a reason it was done that way, but at face value the changes look reasonable.

@peppy
Copy link
Member

peppy commented Feb 3, 2025

I think things here are in a state that we could consider merging this, but mod support is not yet added.

@smoogipoo is this something you are interested in looking at, or would you prefer I take that on?

Either way, I think merging these base PRs is going to make testing and forward development easier. If we don't have the mods setup ready for the next release, we can just hide the footer UI button temporarily.

peppy
peppy previously approved these changes Feb 3, 2025
@smoogipoo
Copy link
Contributor Author

Your latest changes broke tests. Please make sure to fix those.

As I have mentioned in the past, I highly prefer to tackle everything in multiplayer myself.

@peppy peppy self-requested a review February 4, 2025 09:15
To keep things simple, let's not bother debouncing this. The debouncing
was causing spectating handling to fail because of two interdependent
components binding to `BeatmapAvailability`:

Binding to update the screen's `Beatmap` after a download completes:

https://github.com/ppy/osu/blob/58747061171c4ebe70201dfe4d3329ed7f4343f5/osu.Game/Screens/OnlinePlay/Match/RoomSubScreen.cs#L266-L267

Binding to attempt a load request:

https://github.com/ppy/osu/blob/8bb7bea04e56fab9247baa59ae879e16c8b4bd9b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs#L67

The first must update the beatmap before the second runs, else gameplay
will not load due to `Beatmap.IsDefault`.
@peppy
Copy link
Member

peppy commented Feb 4, 2025

Roger, so to confirm you'll be giving priority to getting freemod working when freestyle is on to?

Fixed the test, see the commit message for details.

@smoogipoo
Copy link
Contributor Author

If you consider it to be of utmost importance, then yes, I can look at that as my immediate focus. But it will not be done in this PR.

@peppy
Copy link
Member

peppy commented Feb 4, 2025

If you consider it to be of utmost importance

The rough goal was for the next release (this week) but failing that it can happen the release after.

@smoogipoo
Copy link
Contributor Author

This PR has been in review for over a month, and that hasn't been the blocker given we had an IRL discussion about it. As I've made clear in the past - the quicker you merge my PRs, the faster we can come up with trailing work tasks, and the better I can focus on one specific area.

In the meantime, and in order not to conflict with potential changes here, I've divested my attention to other things that needed addressing - room interop flows, friend states, and user panel status.

Things will need to change if priorities have not been adequately assigned thus far.

@peppy peppy merged commit 15ed029 into ppy:master Feb 4, 2025
7 of 9 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:multiplayer area:playlists notable feature Attach to pull requests which should be highlighted on the changelog. Things users want to read. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to use any difficulty in multiplayer
2 participants