Add ability to invite players to multiplayer rooms #25005
Conversation
I'm like 99% sure that this is a mistake as that PR needs a game package with at least the multiplayer interface changes to go out first to even build properly. So I'd hope this is a mistake and this pair of PRs is not actually circularly dependent, but rather this change is standalone and the spectator change depends on it. |
|
What happens if the notification is dismissed? Is the invite just lost? On stable, invites are chat messages, during matches players can leave and rejoin as needed, a player might lose connection of leave by mistake... Invites need to be stored somewhere. |
| switch (exception.GetHubExceptionMessage()) | ||
| { | ||
| case UserBlockedException.MESSAGE: | ||
| PostNotification?.Invoke(new SimpleErrorNotification | ||
| { | ||
| Text = "User cannot be invited by someone they have blocked or are blocked by." | ||
| }); | ||
| break; | ||
|
|
||
| case UserBlocksPMsException.MESSAGE: | ||
| PostNotification?.Invoke(new SimpleErrorNotification | ||
| { | ||
| Text = "User cannot be invited because they cannot receive private messages from people not on their friends list." | ||
| }); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This is a nice way of handling this 👍
I tend towards not doing that, so I'd also be curious of others' opinions. They aren't stored in most (all?) other games I've played other than stable. I'd favour a "request to join" option instead, though that's hard to make work because we don't have constantly updating user presence like stable has, for now. |
That was bad wording on my part. Removed it |
There was a problem hiding this comment.
In general, UX-wise, I don't see why invites are special enough to get a user avatar in the notification, if PM and mention notifications don't have one. Those either should get an avatar too (maybe in a separate PR), or this should be a plain notification for now.
There was a problem hiding this comment.
Oh yeah, I didn't consider other that there are other notifications that would need an avatar too. I think I prefer splitting that into it's own PR to keep this one simple.
|
I don't really have an opinion on the "invites should be stored" thing, but I'm not putting any more review/test time into this until that is concluded as it is likely to have a major impact on the entirety of the implementation. |
Notifications are supposed to persist, and also be visible on the web and in-game. This bypasses that expectation, which is a bit weird. I don't know how much of an issue this is though. If they were to persist like other notifications and display on the web, would that mean you could invite an offline player to a multiplayer match? And clicking the notification on the web would open the game? It's probably too much complexity to offer initially, so I'm leaning towards being fine with this. |
Not storing invites has some nice features too. It allows finer control for letting in people to your room. |
| // TODO: remove this once online state is being updated more correctly. | ||
| user.IsOnline = true; |
There was a problem hiding this comment.
The only problem I can see with this is that this is mutating an instance that comes from the user cache, so this is "polluting" the cache, in a way. But we're going to have to figure out what proper invalidation there would look like, and it's not like setting this is actively wrong or anything, so I'm willing to look away temporarily...
|
I've made a bunch of changes to bring this up-to-scratch in my eyes. CleanShot.2023-10-12.at.09.15.59.mp4@bdach Can I ask for a review of this as I've made considerable changes. I don't think we need automated test coverage of this until we find issues, as the overhead is quite high (and we'll probably end up adding 10 new flaky tests). Up to you whether you test this practically or just from a code perspective. @minetoblend Thanks again for this submission. Please go through my commits and read each change I made to understand what was left to get this into a good state. Also apologies for not providing you with review coverage of this and taking over the PR. I want to get this in the next release and the time-burden of attempting to review the number of issues I found would be higher than making the changes directly. |
- `virtual` modifier was used in mocking. - The spacing change revert is just mostly to keep it out of the final diff.
|
I've pushed a change here to fix CI but am not done reviewing. Will resume tomorrow. |
| @@ -297,29 +297,26 @@ private void onLeaving() | |||
| popoverContainer.HidePopover(); | |||
| } | |||
|
|
|||
| public void Join(Room room, string password, Action<Room> onSuccess = null, Action<string> onFailure = null) | |||
| public virtual void Join(Room room, string password, Action<Room> onSuccess = null, Action<string> onFailure = null) => Schedule(() => | |||
There was a problem hiding this comment.
ah i guess this virtual is for mocking. at the time i couldn't understand why it was there.
Adds invites for multiplayer matches. Players can be invited by right clicking their profile in the user list (F9 menu).
2023-10-03.21-41-17.mp4
Invites are sent through the spectator/multiplayer server as discussed in #14273 (reply in thread).
The server verifies that the invited user can actually be invited (Not blocking/blocked by inviting user, invited user allowing pms if not friends, invited user not restricted).
The invite notification shows the user's avatar instead of an icon. Unfortunately I was unable to test the avatar with anything but the default user avatar because I could not get image loading to work with my local dev server (Pretty sure it's because I'm running the server without https).