-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Join queue despaghettification #2006
base: master
Are you sure you want to change the base?
Changes from 32 commits
b4e462a
2521bac
d6404e6
02ee6c6
e43c176
f161aac
4cb25d9
eddd834
5dc17a9
1717850
ef14264
86010a6
d613ec9
17e9c09
5c9ffef
5fd6aa5
932edb9
708c8cf
2eb654a
1b5920f
708f753
5d618c7
34b2c18
906d1c6
131bac7
eb1ce78
099f8f2
201d221
a8f742f
76e5a9d
efba962
ef9d8d5
a1cf75b
4de4fa7
8043c13
95fc884
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
using System; | ||
using NitroxClient.Communication.Packets.Processors.Abstract; | ||
using NitroxModel.Packets; | ||
|
||
namespace NitroxClient.Communication.Packets.Processors; | ||
|
||
public class JoinQueueInfoProcessor : ClientPacketProcessor<JoinQueueInfo> | ||
{ | ||
public override void Process(JoinQueueInfo packet) | ||
{ | ||
Log.InGame($"You are at position #{packet.Position} in the queue."); | ||
|
||
if (packet.ShowMaximumWait) | ||
{ | ||
Log.InGame($"The maximum wait time per person is {MillisToMinutes(packet.Timeout)} minutes."); | ||
} | ||
} | ||
|
||
private static string MillisToMinutes(int milliseconds) | ||
{ | ||
double minutes = milliseconds / 60000.0; | ||
return Math.Round(minutes, 1).ToString(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using NitroxClient.Communication.Abstract; | ||
using NitroxClient.Communication.Packets.Processors.Abstract; | ||
using NitroxClient.MonoBehaviours; | ||
using NitroxModel.Packets; | ||
|
||
namespace NitroxClient.Communication.Packets.Processors; | ||
|
||
public class PlayerSyncTimeoutProcessor : ClientPacketProcessor<PlayerSyncTimeout> | ||
{ | ||
private readonly IMultiplayerSession session; | ||
|
||
public PlayerSyncTimeoutProcessor(IMultiplayerSession session) | ||
{ | ||
this.session = session; | ||
} | ||
|
||
public override void Process(PlayerSyncTimeout packet) | ||
{ | ||
// This will advance the coroutine in Multiplayer::LoadAsync() which quits to menu | ||
Multiplayer.Main.InitialSyncCompleted = true; | ||
Multiplayer.Main.TimedOut = true; | ||
|
||
session.Disconnect(); | ||
SpaceMonkeyy86 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ public class Multiplayer : MonoBehaviour | |
private GameLogic.Terrain terrain; | ||
|
||
public bool InitialSyncCompleted { get; set; } | ||
public bool TimedOut { get; set; } | ||
|
||
/// <summary> | ||
/// True if multiplayer is loaded and client is connected to a server. | ||
|
@@ -93,6 +94,7 @@ public static void SubnauticaLoadingCompleted() | |
if (Active) | ||
{ | ||
Main.InitialSyncCompleted = false; | ||
Main.TimedOut = false; | ||
Main.StartCoroutine(LoadAsync()); | ||
} | ||
else | ||
|
@@ -113,11 +115,29 @@ public static IEnumerator LoadAsync() | |
|
||
WaitScreen.Remove(worldSettleItem); | ||
|
||
WaitScreen.ManualWaitItem item = WaitScreen.Add(Language.main.Get("Nitrox_JoiningSession")); | ||
WaitScreen.ManualWaitItem joiningItem = WaitScreen.Add(Language.main.Get("Nitrox_JoiningSession")); | ||
yield return Main.StartCoroutine(Main.StartSession()); | ||
WaitScreen.Remove(item); | ||
WaitScreen.Remove(joiningItem); | ||
|
||
WaitScreen.ManualWaitItem waitingItem = WaitScreen.Add(Language.main.Get("Nitrox_Waiting")); | ||
Log.InGame(Language.main.Get("Nitrox_Waiting")); | ||
yield return new WaitUntil(() => Main.InitialSyncCompleted); | ||
WaitScreen.Remove(waitingItem); | ||
|
||
if (Main.TimedOut) | ||
{ | ||
int timer = 5; | ||
|
||
while (timer > 0) | ||
SpaceMonkeyy86 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
SetLoadingComplete(); | ||
OnLoadingComplete?.Invoke(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
using System; | ||
|
||
namespace NitroxModel.Packets; | ||
|
||
[Serializable] | ||
public class JoinQueueInfo : Packet | ||
{ | ||
public int Position { get; } | ||
public int Timeout { get; } | ||
public bool ShowMaximumWait { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would we sometimes not show the maximum wait time ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
public JoinQueueInfo(int position, int timeout, bool showMaximumWait) | ||
{ | ||
Position = position; | ||
Timeout = timeout; | ||
ShowMaximumWait = showMaximumWait; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
using System; | ||
|
||
namespace NitroxModel.Packets; | ||
|
||
[Serializable] | ||
public class PlayerSyncTimeout : Packet { } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
using System; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using NitroxModel.Core; | ||
using NitroxModel.Packets; | ||
using NitroxModel.Packets.Processors.Abstract; | ||
|
@@ -12,26 +13,28 @@ namespace NitroxServer.Communication.Packets | |
public class PacketHandler | ||
{ | ||
private readonly PlayerManager playerManager; | ||
private readonly JoiningManager joiningManager; | ||
private readonly DefaultServerPacketProcessor defaultServerPacketProcessor; | ||
private readonly Dictionary<Type, PacketProcessor> packetProcessorAuthCache = new(); | ||
private readonly Dictionary<Type, PacketProcessor> packetProcessorUnauthCache = new(); | ||
|
||
public PacketHandler(PlayerManager playerManager, DefaultServerPacketProcessor packetProcessor) | ||
public PacketHandler(PlayerManager playerManager, JoiningManager joiningManager, DefaultServerPacketProcessor packetProcessor) | ||
{ | ||
this.playerManager = playerManager; | ||
this.joiningManager = joiningManager; | ||
defaultServerPacketProcessor = packetProcessor; | ||
} | ||
|
||
public void Process(Packet packet, INitroxConnection connection) | ||
{ | ||
Player player = playerManager.GetPlayer(connection); | ||
if (player == null) | ||
if (player != null) | ||
{ | ||
ProcessUnauthenticated(packet, connection); | ||
ProcessAuthenticated(packet, player); | ||
} | ||
else | ||
else if (!joiningManager.GetQueuedPlayers().Contains(connection)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
{ | ||
ProcessAuthenticated(packet, player); | ||
ProcessUnauthenticated(packet, connection); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would use something like this: