Allow referees to join matches via the lazer client to spectate

[bdach]

• Continuation of Add way of realtime interfacing with multiplayer rooms from outside the game #406

When talking about the refereeing support, one of @ThePooN's remarks was that a referee should be able to join the room inside the client as well as from whatever refereeing client they're using, with the primary reasoning for this being the ability to spectate the refereed room in order to stream it.

Until now this was not possible to do because of this check:

osu-server-spectator/osu.Server.Spectator/Hubs/Multiplayer/MultiplayerRoomController.cs

Lines 76 to 79 in 8cb5c1a

	// this is a sanity check to keep *rooms* in a good state.
	// in theory the connection clean-up code should handle this correctly.
	if (room.Users.Any(u => u.UserID == roomUser.UserID))
	throw new InvalidOperationException($"User {roomUser.UserID} attempted to join room {room.RoomID} they are already present in.");

This is the minimal change required to make this happen in a way that doesn't break things too hard. Read further for explanation what that means.

I am PRing this as a very half-baked RFC for two reasons:

1. I want to see what everyone else's response to the core premise of being able to join the room in "two capacities" like this is going to be.
2. In the way this is written, the bare minimum effort is put in, which means:

• The referee will be able to join the room.
• The referee will be able to spectate the room.
• The referee will not be dropped into gameplay if they are in idle state (which would be the case if they don't join spectate).
• The referee will not be allowed to ready-up.

That said, there are corollaries here that could require further work, such as:

• The referee should possibly be allowed to perform all management functions from either the game or the refereeing client. This will require adjustments to multiplayer hub logic such that it checks referee privileges in a way analogous to the referee hub. As written it'll only work if the referee is simultaneously the room host.
• The referee should possibly not be allowed to perform mostly superfluous but distracting actions, such as: changing team, changing user mods, changing user style in freestyle.
• To match the above, requisite changes will need to be made to client logic to appropriately block inoperable controls in the multiplayer lobby screen as required.

These are subjective and/or labour-costly calls, and as such, if this gets past reason (1), I want to gauge everyone's thoughts on where and how far to take (2) so that I don't waste too much effort.

[peppy]

Ignoring the cost of implementation, I agree with everything listed in the OP.

Would definitely want @ThePooN's thoughts on this before green lighting though.

[ThePooN]

What happens otherwise? or if the user is added as referee while they're joined as a player?

[bdach]

If the user joins as player first they won't be able to join as a referee, the referee hub will flatly refuse it. They'll need to leave the room as player, join as a referee first, and then rejoin as player.

Yes this is finicky, it can also probably be improved, but doing it is annoying and even more work.

I thought this was an acceptable tradeoff because the 80% case is a single referee, wherein you have to create the room first, at which point you already are the referee and the entire point becomes moot.

[ThePooN]

I'm not against this, I think this is acceptable if it prevents major headaches on your end, as long as it's clearly signaled to the user/API consumer.

[bdach]

I have pushed commits here that go in the direction I originally outlined in the OP and that everyone seems to agree with but I am not undrafting this yet. The reason why is I'm not sure of this code myself at this point.

At the start of today's struggle what I wanted to do felt positively quaint and I thought I was being overly pessimistic when I estimated the amount of work required here as usual. However I am no longer sure of this - testing various scenarios locally led me to a bunch of weird alleys and corners such as the last three commits here (ee224e6, d9ba619, 7578eb2). So I definitely want to give this some more once-overs tomorrow before I undraft.

The other thing is that this is going to incur significant complexity in client because places like MatchStartControl or MultiplayerReadyButton already don't have great code quality and my complications from the behaviours I want to put down here would make it worse.

Also I'll say this: you could say "spaceman this is your fault for not writing tests". I'd say... maybe? I will take the L any day if I deserve it, but also to find some of these problems you have to figure out that they're problems. Most of the problems I'm having to solve here come from non-trivial interactions between new referee operations and existing player-only setups and flows. To put it in another way - the referee stuff was (rather deliberately) as separated from multiplayer logic as could be, but that separation is not going to hold up much longer if this sort of spectator-in-client-and-referee-both flow is to be entertained.

I guess if you hate anything about what I said and want to hit bricks and abort this entire excursion then please do let me know (sooner rather than later would be appreciated).

[peppy]

(ee224e6, d9ba619, 7578eb2). So I definitely want to give this some more once-overs tomorrow before I undraft.

basically what you're saying is "these issues got into master in a weird state" correct?

i think with a system this complicated it's fine to have these kinds of oversights. we'll fix them along the way. not sure i'd want more test coverage overhead to catch these. maybe but as long as we're not causing critical security failures these are just low level bugs that i wouldn't even blink an eye at fixing without further thought.

with that said, nothing in your updated commits looks offensive to me. i'm not sure i'll be doing a pass on this to potentially find issues you haven't uncovered yet (nor whether that would be productive).

so, LGTM.

[bdach]

basically what you're saying is "these issues got into master in a weird state" correct?

Not really, master is fine. In master there is a hard split where a user can only connect to the referee hub or only to the multiplayer hub. So none of the issues these three commits I linked are issues in master, you just fundamentally can't get into that state. Letting the referees connect to both hubs is what surfaces the issue.

[peppy]

right, i meant the state of this PR before those commits, not master. have lost track of things.

[bdach]

Alright, after another pass over this wherein I've found some more bugs that a918a8a should handle better, I'm pretty okay with where this is at. Undrafting.

A bit of an involved change all things considered but it's not horrible I think. Another instance that made me really wish EntityStore.GetForUse() was re-entrant wrt lock acquisition but it's fine.

list of tests I've done, for potential future reference

referee can join room as spectator /x
referee kicked as spectator on leaving the room via referee hub /x
referee kicked as spectator on closing the room /x
referee kicked as spectator on getting removed as referee by another referee /x
referee kicked as spectator on disconnecting from referee hub /x
referee can kick players from within game without host /x
referee can change room settings within game without host /x
referee can add / edit / remove playlist items within game without host /x
referee can start match within game without host /x
referee can stop match countdown within game without host /x
referee can abort match within game without host /x
referee can transfer host to any player without host /x
referee cannot change state to anything but spectating or idle /x
referee cannot change user style /x
referee cannot change user mods /x
referee cannot change team /x
referee cannot vote to skip intro /x
close room doesn't fail when the referee ISN'T in the game (and thus not connected to multiplayer hub AT ALL) /x