Join queue despaghettification #2006
Conversation
SpaceMonkeyy86
commented
Mar 18, 2023
•
SpaceMonkeyy86
commented
- The join queue logic is now centralized in a single async method and no longer relies on fallible callbacks
- It is now impossible for the queue to get stuck even if a player crashes their game while connecting
- Players enter the queue once the base game has finished loading to reduce unnecessary delays
dartasen
requested review from
killzoms,
Sunrunner37 and
tornac1234
and removed request for
killzoms and
Sunrunner37
There was a problem hiding this comment.
Probably some things I've not seen yet and didn't test that many figure cases.
One issue I've found is PlayerDisconnected() doesn't trigger a queue continuation. If you join with 2 players, and when both are in the waiting queue and you suddenly Atl+F4 one of the clients, the server will be put in pause state without resuming for the other one (it waited for 5 minutes tho but that's a "worst case" situation while we would have just been able to resume the queue once we know that the loading player was disconnected).
Also there's no way to tell that you're waiting once you're in the loading. You should get informations with like the number of player in front of you in the queue and maybe a maximum wait time calculated by number of players in queue * InitialSyncTimeout, we should definitely add some more visual elements for the player instead of locking him in there without any information. I think we discussed this once, I proposed that we would have some sort of UI on top of the screen showing the informations we want (those proposed above and the timing passed waiting also) and a way to leave the queue and head back to the main menu.
I would not want this merged without at least some information given to the player, and then we'd set the UI I proposed for a later PR.
NitroxClient/Communication/Packets/Processors/PlayerSyncTimeoutProcessor.cs
Outdated
Show resolved
Hide resolved
|
I agree, this PR needs a dedicated UI screen to be production-ready. I think we should go ahead and add it now because it's bad practice to merge a PR that says "Finish me later with another PR." |
NitroxClient/Communication/Packets/Processors/PlayerSyncTimeoutProcessor.cs
Outdated
Show resolved
Hide resolved
|
Would like to hear about where this is going. Should we try to restrict the scope of this PR (e.g. postpone early loading which needs more work as said above) to at least have the fixes merged ? |
|
I think so, adding new menus sounds more like a beta feature. We do need to add at least some kind of additional UI to the loading screen, however. |
|
How's this going ? |
tornac1234
added
the
Area: netcode
|
I took a very long break from this project because I was busy and my interests had changed, so I haven't been keeping up with things very closely. Actually, I was thinking of getting back into Nitrox, and this is a good time to do it. This PR is functional (last time I checked) but to be "polished" it still needs some sort of UI on the loading screen to show progress, or just to modify the vanilla progress bar. That would be better suited for a separate PR. Other than that, it should work (I just need to fix the conflicts). |
|
The PR is ready for review now. Just one thing - currently, if the client is in the queue, the loading screen disappears and they can play as normal. However, when they start syncing it reappears and anything they did is undone. This is unintentional, but I left it in for now since it's nice to be able to jump/swim around while waiting instead of just looking at a static image on a loading screen. Should we keep this in? Maybe add something to the queue enter message to let the player know what is happening? |
| if (Main.TimedOut) | ||
| { | ||
| int timer = 5; | ||
|
|
||
| while (timer > 0) | ||
| { | ||
| Log.InGame($"Initial sync timed out. Quitting game in {timer} second{(timer > 1 ? "s" : "")}…"); | ||
| yield return new WaitForSecondsRealtime(1); | ||
| timer--; | ||
| } | ||
|
|
||
| IngameMenu.main.QuitGame(false); | ||
| yield break; | ||
| } |
There was a problem hiding this comment.
I'd replace that with simply a modal notifying the user of the time out (with option freeze so that the game doesn't play behind) with a QuitGame on callback
NitroxClient/Communication/Packets/Processors/PlayerSyncTimeoutProcessor.cs
Outdated
Show resolved
Hide resolved
| { | ||
| public int Position { get; } | ||
| public int Timeout { get; } | ||
| public bool ShowMaximumWait { get; } |
There was a problem hiding this comment.
Why would we sometimes not show the maximum wait time ?
There was a problem hiding this comment.
We show the maximum wait time per person when the player enters the queue. This information never changes so there is no need to show it again.
NitroxClient/Communication/Packets/Processors/JoinQueueInfoProcessor.cs
Outdated
Show resolved
Hide resolved
| } | ||
| else | ||
| else if (!joiningManager.GetQueuedPlayers().Contains(connection)) |
There was a problem hiding this comment.
Isn't it too harsh on the connection ? o(n) on packet receiving (a lot per second) might be a load too bad for the server
There was a problem hiding this comment.
I highly doubt this could cause performance problems since it only runs for unauthenticated packets (sent very infrequently compared to normal packets) and there will realistically never be more than a few people in the queue at once. It's best to use a profiler to confirm that a piece of code is actually causing a bottleneck before trying to optimize it.
There was a problem hiding this comment.
When a player is disconnected from the server POV but not on their end, they just send all their packet as unauthenticated packets which will just spam this else condition. This else conditions uses GetQueuedPlayers which creates a new list instance each time (o(n) + memory impact), and uses Contains which is also o(n)
|
The join queue takes place entirely on the loading screen now, so we don't have a way to show UIs to the player besides the in-game log. We can add a UI that also shows more information on loading progress like how pre-2.0 Subnautica did it, but that would be best in another PR. |
| } | ||
| else | ||
| else if (!playerManager.GetQueuedPlayers().Contains(connection)) |
There was a problem hiding this comment.
What do we want to do if it doesn't contain the connection in the queued players for some reason won't we silently fail here?
| private static string MillisToMinutes(int milliseconds) | ||
| { | ||
| double minutes = milliseconds / 60000.0; | ||
| return Math.Round(minutes, 1).ToString(); |
There was a problem hiding this comment.
I find that this could be misleading, why would we not just have it in seconds or a combination of both. If the milliseconds came out to 20 seconds we would round it to 0.3 which would require the user to calculate what this actually means
|
|
||
| if (packet.ShowMaximumWait) | ||
| { | ||
| Log.InGame($"The maximum wait time per person is {MillisToMinutes(packet.Timeout)} minutes."); |
There was a problem hiding this comment.
Would use something like this:
| Log.InGame($"The maximum wait time per person is {MillisToMinutes(packet.Timeout)} minutes."); | |
| TimeSpan time = TimeSpan.FromMilliseconds(packet.Timeout); | |
| Log.InGame($"The maximum wait time per person is {time.ToString(@"mm\:ss")}."); |
NitroxClient/Communication/Packets/Processors/JoinQueueInfoProcessor.cs
Outdated
Show resolved
Hide resolved
| } | ||
| else | ||
| else if (!playerManager.GetQueuedPlayers().Contains(connection)) |
There was a problem hiding this comment.
Would like to add an else with error logging here
| private async Task JoinQueueLoop() | ||
| { | ||
| const int REFRESH_DELAY = 10; | ||
|
|
||
| while (true) | ||
| { | ||
| try | ||
| { | ||
| while (JoinQueue.Count == 0) | ||
| { | ||
| queueIdle = true; | ||
| await Task.Delay(REFRESH_DELAY); | ||
| } |
There was a problem hiding this comment.
Won't it be better to only start this task if a new item is added to the JoinQueue and it's Count is zero? Then you can replace the while true with while(JoinQueue.Count == 0)
