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

Support Discord game invites in multiplayer lobbies #27443

Merged
merged 21 commits into from Mar 20, 2024

Conversation

jvyden
Copy link
Contributor

@jvyden jvyden commented Mar 1, 2024

This PR introduces support for Discord's game invites. When in a multiplayer lobby, you can use Discord to send an invite to a channel or user, and other users can click the 'Join' button from that message in Discord to instantly join that user's lobby.

This works whether or not the room has a password, since that's shared as a secret over Discord. It is not possible to get this secret without an invite message existing.

The scope of this PR is fairly limited, so here's a list of things that are not included with this PR but I will be adding in follow-up pull requests:

  • Spectating. This is trivial with the groundwork laid in this PR so this will come shortly after.
  • Handling of the URL protocol Discord uses to launch the game when osu! is not already open. works already
  • A refactoring of the updateStatus function is something I'd like to do as well.
  • Triggering presence updates whenever the number of players in the lobby changes. resolved in b11ae1c

Code quality was complaining about hidden variables so I opted for this solution.
Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. There's too much commentary throughout the code, I've adjusted that a little with a couple of suggestions. However, the secret keys must be encrypted before this feature goes live, as to not cause potential inconvenience. Needs input from the team (see #27443 (comment)).

osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
Co-authored-by: Salman Ahmed <frenzibyte@gmail.com>
@jvyden
Copy link
Contributor Author

jvyden commented Mar 1, 2024

Apologies for the abundance of comments - they do help me, but not everyone I suppose.

Are there any other areas of comments you wanted me to adjust other than the ones in your review?

@frenzibyte
Copy link
Member

Are there any other areas of comments you wanted me to adjust other than the ones in your review?

Nothing else besides what I mentioned.

Co-authored-by: Salman Ahmed <frenzibyte@gmail.com>
osu.Desktop/DiscordRichPresence.cs Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
Also log room secrets for debugging purposes
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 5, 2024
jvyden and others added 2 commits March 5, 2024 18:22
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
@frenzibyte frenzibyte self-requested a review March 10, 2024 21:44
@jvyden jvyden requested a review from bdach March 10, 2024 23:15
@bdach bdach added the blocked Don't merge this. label Mar 11, 2024
@bdach
Copy link
Collaborator

bdach commented Mar 11, 2024

Blocking because the fact of sharing app ID with stable makes this terminally broken (https://discord.com/channels/188630481301012481/188630652340404224/1216663746026410007).

osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
@peppy peppy self-requested a review March 11, 2024 08:54
@peppy
Copy link
Sponsor Member

peppy commented Mar 11, 2024

Blocking because the fact of sharing app ID with stable makes this terminally broken (https://discord.com/channels/188630481301012481/188630652340404224/1216663746026410007).

I've updated the application ID to a lazer-specific one!

The mode assets graphics aren't on the new app yet, I need to find high resolution versions of them (seems like the upload criteria have changed so the old ones are too low resolution). added

Please test, I haven't.

@frenzibyte frenzibyte removed the blocked Don't merge this. label Mar 11, 2024
@frenzibyte
Copy link
Member

Looks to work correctly:

CleanShot.2024-03-11.at.13.25.20.mp4

@bdach
Copy link
Collaborator

bdach commented Mar 13, 2024

What's even going on with this branch now? @frenzibyte pushes a commit, @jvyden reverts it, and there's no justification given for the revert in this thread? Is this an edit war or what?

@frenzibyte
Copy link
Member

I made a mistake and there have been pages of back and fourths regarding the mistake in discord. I cannot find the time to return back to this PR, I expect to look into it tomorrow but no promises. You can leave it for now, I'll sort it out by then.

@jvyden
Copy link
Contributor Author

jvyden commented Mar 13, 2024

I'm planning to sort it out, no need.

Sorry for the confusion @bdach, I guess I should have mentioned over here xD

@frenzibyte
Copy link
Member

I mean to sort out the issue by reviewing and commenting on the issue and such, not forcibly taking control from you or something.

@jvyden
Copy link
Contributor Author

jvyden commented Mar 13, 2024

Oh, no, I thought you were also planning to remedy that issue we discussed over Discord. I don't mind.

Okay, I'll just bring context into this thread now because this PR is starting to go off the rails. An issue came up where multiplayer lobbies end up updating RPC too much, since every room update sets UserActivity. This is a problem because Discord has a fairly strict ratelimit on how many RPC updates you can send, and it can start to appear like RPC isn't updating properly.

The fix is to just compare the lobby size from before and after each update (because that's the only thing that should visibly change during a room update)

I just need to make that change, and then hopefully we can stop bumping into things and just get this merged. 😔

@frenzibyte frenzibyte self-requested a review March 14, 2024 04:10
Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

In addition, you're probably already aware of this, but I need to mention it here for visibility to anyone else following along: Discord RPC presence update is firing about 4 times when entering a room, even on master:

CleanShot 2024-03-14 at 08 31 16

I cannot 100% tell whether this issue should be fixed in this PR or can be left for a separate PR, since Discord RPC died on my alternative machine and I couldn't figure out why.

One thing that looks highly relevant here is that UserActivity does not implement equality comparison, therefore the bindable keeps being updated to another activity instance of the same values triggering an unnecessary presence update. This should be fixed by implementing IEquatable<> on each type of the class.

osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
@jvyden jvyden requested a review from frenzibyte March 19, 2024 20:16
osu.Desktop/DiscordRichPresence.cs Outdated Show resolved Hide resolved
…mode, and use `SkipIdenticalPresence` + scheduling to avoid potential rate-limits
@frenzibyte frenzibyte dismissed their stale review March 20, 2024 04:31

resolved myself

@frenzibyte frenzibyte merged commit 773daee into ppy:master Mar 20, 2024
11 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.

None yet

5 participants