Notify users of who is spectating them #259
Conversation
Server-side part of ppy/osu#22087.
|
Don't you need to cleanup the usage on the external |
|
You're right, there has to be cleanup, but a very annoying sort of cleanup because I would need to remove the user that disconnected from any and all spectator lists. Which would mean iterating over every single entry in the store. Not sure how to fix this. I feel like if I write that the naive way then things will inevitably blow up... |
|
The only thing I can immediately think of that would fix the concern would be to make the spectators lists bidirectionally associative, something like so (very untested): diff --git a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs b/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
index fdc5139..b40c467 100644
--- a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
+++ b/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
@@ -221,10 +221,16 @@ namespace osu.Server.Spectator.Hubs.Spectator
Username = watcherUsername,
};
- using (var usage = await spectatorLists.GetForUse(userId, createOnMissing: true))
+ using (var streamerUsage = await spectatorLists.GetForUse(userId, createOnMissing: true))
{
- usage.Item ??= new SpectatorList();
- usage.Item.Spectators[watcherId] = watcher;
+ streamerUsage.Item ??= new SpectatorList();
+ streamerUsage.Item.WatchedBy[watcherId] = watcher;
+ }
+
+ using (var watcherUsage = await spectatorLists.GetForUse(watcherId, createOnMissing: true))
+ {
+ watcherUsage.Item ??= new SpectatorList();
+ watcherUsage.Item.Watching.Add(userId);
}
await Clients.User(userId.ToString()).UserStartedWatching([watcher]);
@@ -241,8 +247,8 @@ namespace osu.Server.Spectator.Hubs.Spectator
if (usage?.Item == null)
return;
- usage.Item.Spectators.Remove(watcherId);
- if (usage.Item.Spectators.Count == 0)
+ usage.Item.WatchedBy.Remove(watcherId);
+ if (usage.Item.WatchedBy.Count == 0 && usage.Item.Watching.Count == 0)
usage.Destroy();
}
@@ -261,7 +267,7 @@ namespace osu.Server.Spectator.Hubs.Spectator
using (var usage = await spectatorLists.TryGetForUse(Context.GetUserId()))
{
if (usage?.Item != null)
- watchers = usage.Item.Spectators.Values.ToArray();
+ watchers = usage.Item.WatchedBy.Values.ToArray();
}
if (watchers != null)
@@ -275,6 +281,31 @@ namespace osu.Server.Spectator.Hubs.Spectator
if (state.State != null)
await endPlaySession(state.UserId, state.State);
+ int userId = Context.GetUserId();
+ var usersWatched = new HashSet<int>();
+
+ using (var usage = await spectatorLists.TryGetForUse(userId))
+ {
+ if (usage?.Item != null)
+ {
+ usersWatched.UnionWith(usage.Item.Watching);
+ usage.Destroy();
+ }
+ }
+
+ foreach (int watchedUser in usersWatched)
+ {
+ using (var usage = await spectatorLists.TryGetForUse(watchedUser))
+ {
+ if (usage?.Item != null)
+ {
+ usage.Item.WatchedBy.Remove(watchedUser);
+ if (usage.Item.WatchedBy.Count == 0 && usage.Item.Watching.Count == 0)
+ usage.Destroy();
+ }
+ }
+ }
+
await base.CleanUpState(state);
}
diff --git a/osu.Server.Spectator/Hubs/Spectator/SpectatorList.cs b/osu.Server.Spectator/Hubs/Spectator/SpectatorList.cs
index 49540bd..952ea97 100644
--- a/osu.Server.Spectator/Hubs/Spectator/SpectatorList.cs
+++ b/osu.Server.Spectator/Hubs/Spectator/SpectatorList.cs
@@ -8,6 +8,7 @@ namespace osu.Server.Spectator.Hubs.Spectator
{
public class SpectatorList
{
- public Dictionary<int, SpectatorUser> Spectators { get; } = new Dictionary<int, SpectatorUser>();
+ public Dictionary<int, SpectatorUser> WatchedBy { get; } = new Dictionary<int, SpectatorUser>();
+ public HashSet<int> Watching { get; } = new HashSet<int>();
}
}
But I dunno if this is a good idea, because the |
|
It seems to me like the entire complexity of this is related to the |
|
I think adding an id of who you are watching to |
Yes, correct, it is incurred by that. Initially I didn't bother, but then realised that, say, if the streamer's connection to spectator drops out and then the streamer reconnects, they won't see their spectators anymore unless the spectators... re-spectate? Even though they are in fact actually spectating, which felt really dodgy to me. That said if we don't care I would be very happy to drop all of that. |
If we're going down that path, users would have to be forcefully removed from the spectator channel, with the client doing the reconnection. I agree that this will look very dodgy otherwise. |
|
Sure, that sounds easier to do given there's precedence (
I would expect this in any case. |
My follow up question to these two remarks would be: should it be at all supported to spectate users that aren't currently online, then? Because currently that is supported, and from this angle it's basically the same scenario too. And it was explicitly intended that you should be able to do that, there are references to this in code... Not sure typing is effective to continue this discussion forward at this point, maybe voice? |
|
It should not be supported, and it's also incorrect that we support that for appear-offline users too. |
|
Stable also doesn't support spectating offline users so I think it's fine to not support that. Although I would hope that the client will sit at the spectating screen if the user you're spectating goes offline, and re-engages if they come back online. |
Being able to remain in a watcher group for an offline user is what facilitates that right now. And it also creates my problem in this pull of having to resend the list of spectators on connect, therefore incurring all of the tricky and annoying tracking. The proposal to simplify this pull was to clear the spectator group on disconnect and not support watching offline users anymore, at which point how are the spectators supposed to know that the user has come back online to re-engage? I'd have to push that responsibility to the client somehow I guess? Which in turn may make #262 tricky or outright impossible to achieve? |
|
We're likely going to need to track specific user states one way or another, as has been mentioned IRL and also ppy/osu#31524 (comment) |
|
I think we're gonna need some common agreement and direction and then implementation coordination here because all of the spectator issues & changes have become so entangled that it is no longer clear to me who should do what to progress any of these PRs. I guess my bad for encroaching on the online systems work with this 🙇 |
|
Subscribing to all user states when at the spectator screen sounds like a valid solution which will not require too much effort. The most complex part I can think of is managing the "should we listen" state across two screens that can potentially be visible at the same time (and that can be solved). |
In discussion it appears that the consensus is that the player should not be able to spectate offline players, and as such the server is allowed to purge the list of spectators on disconnection of the streamer. This in turn obviates the need of doing any external tracking of who's spectating whom in the hub.
|
For now, in the interest of keeping this PR as small, self-contained, and conflict-free as possible, I've removed the problematic tracking logic, with the implicit assumption that the ability of spectating offline players will be removed separately in another PR. It's out of scope of this one either way. If you want me to look into making that change to spectator logic then let me know. |
At very least let's make a tracking issue. |
Done: #263 |
|
One important thing here is that we will now be left with ghost spectators if a user is non-gracefully disconnected (ie. if their client doesn't send Unlike the other case discussed above, I do see this one as a blocker. I think a good way forward may be to do tracking inverse to the original PR's code, and keep it in the existing state just to reduce the complexity of the entity store setups: diff --git a/osu.Server.Spectator/Hubs/Spectator/SpectatorClientState.cs b/osu.Server.Spectator/Hubs/Spectator/SpectatorClientState.cs
index ce6f200..f2b1151 100644
--- a/osu.Server.Spectator/Hubs/Spectator/SpectatorClientState.cs
+++ b/osu.Server.Spectator/Hubs/Spectator/SpectatorClientState.cs
@@ -2,6 +2,7 @@
// See the LICENCE file in the repository root for full licence text.
using System;
+using System.Collections.Generic;
using Newtonsoft.Json;
using osu.Game.Online.Spectator;
using osu.Game.Scoring;
@@ -26,6 +27,11 @@ namespace osu.Server.Spectator.Hubs.Spectator
/// </summary>
public long? ScoreToken;
+ /// <summary>
+ /// If currently spectating other users, this contains a list of all users being watched.
+ /// </summary>
+ public List<int>? WatchingUserIds;
+
[JsonConstructor]
public SpectatorClientState(in string connectionId, in int userId)
: base(connectionId, userId)
diff --git a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs b/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
index c913e16..fe70c9f 100644
--- a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
+++ b/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
@@ -154,7 +154,12 @@ namespace osu.Server.Spectator.Hubs.Spectator
}
finally
{
- usage.Destroy();
+ if (usage.Item != null)
+ {
+ usage.Item.Score = null;
+ usage.Item.ScoreToken = null;
+ usage.Item.State = null;
+ }
}
}
@@ -186,6 +191,8 @@ namespace osu.Server.Spectator.Hubs.Spectator
{
Log($"Watching {userId}");
+ // TODO: create own spectator state and update watcher list.
+
try
{
SpectatorState? spectatorState;
@@ -227,6 +234,8 @@ namespace osu.Server.Spectator.Hubs.Spectator
int watcherId = Context.GetUserId();
+ // TODO: remove id from own spectator state watcher list.
+
await Clients.User(userId.ToString()).UserEndedWatching(watcherId);
}
@@ -245,6 +254,12 @@ namespace osu.Server.Spectator.Hubs.Spectator
if (state.State != null)
await endPlaySession(state.UserId, state.State);
+ if (state.WatchingUserIds != null)
+ {
+ foreach (int id in state.WatchingUserIds)
+ await EndWatchingUser(id);
+ }
+
await base.CleanUpState(state);
}
@bdach thoughts on this one? i'm open to other proposals but using the existing state for dual-purposes does feel simpler than a separate entity store to me. maybe. I won't take this further than the diff above unless you'd prefer I take over. Also as touched on, restricted users are not considered at all here, meaning they will show up in the spectator list (likely a blocking concern). And I don't think blocked users are being considered either. Restricted user handling has been something we've deferred for a while, but I'd propose that it's about time we fix this before it becomes an actual community issue (there are other cases – they also they show up in online users and in friend status updates, basically none of this should exist, i'll open an issue). You mentioned potentially wanting to take that on, so I'll leave some more details in a new issue regarding the scope of what restrictions (and silences) should do. |
I tend to agree, and only did the separate store out of concerns I was going to break something - that said, on closer inspection, I think it should be mostly fine, so I applied your diff with some minor renames and the TODOs filled out. |
Server-side part of ppy/osu#22087.