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 taskbar flashing when a multiplayer game is starting #32180

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

GioSDA
Copy link
Contributor

@GioSDA GioSDA commented Mar 2, 2025

I'm not sure if this was implemented in stable, but it's something that I feel is missing from multiplayer right now. I normally tab out of the lobby while waiting for people to ready up, and so having the indicator makes it easier to do that without the match starting without me.

2025-03-02.00-35-31.mp4

@Joehuu Joehuu self-requested a review March 2, 2025 19:25
@@ -142,6 +145,7 @@ protected override void StartGameplay()

if (client.LocalUser?.State == MultiplayerUserState.Loaded)
{
game?.Window?.Flash();
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be done in PlayerLoader for a bit more time. The beatmap you showcased in the video is not really a good example because it has an intro (it shows a skip button on solo play). Without an intro, circles would show like a second after the flash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're probably right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just unsure how to implement this feature in a way where it only happened when loading into a multiplayer lobby, would it be fine if it just flashed whenever a map begins loading?

Copy link
Member

Choose a reason for hiding this comment

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

MultiplayerPlayerLoader? If you weren't able to find this I have a few concerns though 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! That's what I get for trying to write code so late 😓

Verified

This commit was signed with the committer’s verified signature.
peppy Dean Herbert
Copy link
Member

@Joehuu Joehuu left a comment

Choose a reason for hiding this comment

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

LGTM maybe. Putting it in OnEntering() is another place to consider (arguably more correct as it will be the same time the host presses the button to start the match).

Leaving others to decide.

@smoogipoo
Copy link
Contributor

I think I would probably put this in MultiplayerMatchSubScreen somewhere, because as it is it won't trigger if you're spectating the room. Maybe that's not as important, but feels kinda weird to not be let known that the match started in this other scenario?

That said, I feel like it may be better to delay this until my refactor/rewrite of this screen to separate it from RoomSubScreen where that StartPlay() method currently is.

@peppy
Copy link
Member

peppy commented Mar 6, 2025

Not sure if I'd expect it to trigger on spectating 🤔 debatable for sure.

@smoogipoo
Copy link
Contributor

In that case, I suppose this is fine to go as-is. It's isolated from my changes like this.

@smoogipoo smoogipoo merged commit 1c13c2d into ppy:master Mar 6, 2025
6 of 10 checks passed
@GioSDA GioSDA deleted the multiplayer-taskbar-flash branch March 19, 2025 02:22
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

4 participants