Add support for V3 tunnels #845
Conversation
Add V3GameTunnelBridge Add V3TunnelCommunicator Add V3TunnelNegotiator Add TunnelFailed event Update supported tunnel versions
Fixup GameCreationWindow control visibility Remove unnecessary tunnel parameter in V3PlayerInfo Add NegotiationStatusPanel Add tunnel negotiations to CnCNetGameLobby Add V3 tunnel support to CnCNetGameLobby
Move to file-scoped namespaces Fix missing texture on status panel Fix issue with decider seeing OK instead of ping results
Fix an issue with automatic tunnel selection Style fixups Split TunnelChosenEventArgs into new file
Remove IDisposable on bridge
Remove unecessary code
Better updating of player ping indicators
|
Nightly build for this pull request:
|
|
Perhaps the ability to add a button within one of the clients ini files to toggle between V2 and V3 would be useful. Just a thought |
| @@ -0,0 +1,248 @@ | |||
| using System; | |||
There was a problem hiding this comment.
write #nullable enable since you are using the nullable syntax. Same for other new files
| tunnel.PingInMs = pingResult; | ||
| } | ||
|
|
||
| if (previousPing > 0 && (tunnel.PingInMs <= 0 || tunnel.PingInMs > TUNNEL_FAILED_PING_AMOUNT)) |
There was a problem hiding this comment.
Does 0 ms indicate a failure for now? Would be better using -1, in case some one runs a tunnel server AND plays the game, so the distance between tunnel server and the gaming computer is so close
There was a problem hiding this comment.
There was a problem hiding this comment.
I suggest fully go through the code to ensure the ping value successfully deals with 0 and -1. Alternatively to simplify the procedure and reduce error you might also want to wrap the ping int as a readonly record (so we can use something like ping.IsValid()), if needed
| if (GameTunnelBridge != null) | ||
| StopGameBridge(); |
There was a problem hiding this comment.
is the if statement necessary?
There was a problem hiding this comment.
I've removed that. Thanks.
| /// <summary> | ||
| /// Retrieves the <see cref="TunnelTestResult"/> for the specified tunnel, or null if not found. | ||
| /// </summary> | ||
| public TunnelTestResult GetTunnelResult(CnCNetTunnel tunnel) => TunnelResults.TryGetValue(tunnel, out var result) ? result : null; |
There was a problem hiding this comment.
Just wondering why you didn't continue using nullable syntax.
| public double GetBestPing() | ||
| { | ||
| var best = SelectBestTunnel(); | ||
| return best != null ? TunnelResults[best].AverageRtt : double.NaN; |
There was a problem hiding this comment.
Why not using Double.MaxValue? It seems that any comparison involving NaN (e.g., NaN > 5, NaN < 5, NaN == NaN) will result in false
There was a problem hiding this comment.
Sure, I've changed that.
| _localPlayer.Id, | ||
| _remotePlayer.Id, | ||
| TunnelPacketType.PingRequest, | ||
| BitConverter.GetBytes(i) |
There was a problem hiding this comment.
Suggest crash if BitConverter.IsLittleEndian does not match our expected value, in case some extremely rare and weird machines break the protocol
I saw you are using BinaryPrimitives later. Maybe it is better than the machine-specific BitConverter methods here
There was a problem hiding this comment.
Good call, sorted now thanks.
| _negotiationCts.Dispose(); | ||
| _tunnelAckReceived.TrySetCanceled(); | ||
| _negotiationCompletionSource.TrySetCanceled(); | ||
| _tunnelHandler.UnregisterV3PacketHandler(_localPlayer.Id, _remotePlayer.Id); |
There was a problem hiding this comment.
You have used _tunnelHandler?UnregisterV3PacketHandler syntax but here you assume the field is not null. Perhaps it's better to enable nullable for all new files.
Fix V2 tunnels not matching
|
Checked PR with 2 players in lobby total. YR config works fine, have no issues. |
|
That's good to hear, and thanks for testing! |
|
@11EJDE11 I suggest fully go through the code to ensure we successfully deal with 0 and -1 ping values. Currently there seems to be some mix usages between 0 and -1, e.g., Alternatively to simplify the procedure and reduce error you might also want to wrap the ping int as a readonly record (so we can use something like ping.IsValid() to enhance readability), if needed |
|
Sure, that makes sense, SP. I will take another look in the next day or two. |
|
@SadPencil, is the below what you had envisioned? If so, I will update the lobbies and players to all use this. There will be minor changes to a lot of files. If not, I can stick to int and there would be fewer changes required. Either is OK with me although I do prefer below. |
|
Yes！ Also, it's "0 ms" instead of "0ms" in your ToString implementation |
| 0, | ||
| CELL_WIDTH, | ||
| HEADER_HEIGHT); | ||
| headerLabel.TextAnchor = LabelTextAnchorInfo.CENTER; |
There was a problem hiding this comment.
If you set TextAnchor, you should also set AnchorPoint. The intention is probably centering the names on the cells - based on the screenshot that doesn't happen right now, and it would be an improvement to the interface.
See https://github.com/Rampastring/Rampastring.XNAUI/blob/master/XNAControls/XNALabel.cs#L75
| SearchAllGameModes = new BoolSetting(iniFile, MULTIPLAYER, "SearchAllGameModes", false); | ||
|
|
||
| UseLegacyTunnels = new BoolSetting(iniFile, MULTIPLAYER, "UseLegacyTunnels", false); | ||
| UseDynamicTunnels = new BoolSetting(iniFile, MULTIPLAYER, "UseDynamicTunnels", true); |
There was a problem hiding this comment.
Since these are mutually exclusive, making a single enum based (integer or string) TunnelMode setting instead could be a worthy idea. But this is OK too - doesn't significantly increase code complexity.
ClientCore/ProgramConstants.cs
Outdated
| public const string QRES_EXECUTABLE = "qres.dat"; | ||
|
|
||
| public const string CNCNET_PROTOCOL_REVISION = "R13"; | ||
| public const string CNCNET_PROTOCOL_REVISION = "R15"; |
There was a problem hiding this comment.
We could save a character from GameSurge traffic by using alphabetics for the number instead.
RF for example. We could also drop the R to save two bytes - it used to be short for Rampastring to differentiate my protocol from Funky's client, but I don't think that's a major consideration anymore.
| // reportingPlayer -> targetPlayer -> status | ||
| // This tracks what each player reports about their negotiation with each other player |
There was a problem hiding this comment.
Is there a reason these aren't /// <summary> type comments? It'd make IntelliSense pick them up and show them when you hover over the variable anywhere in code.
Same for _playerPingMatrix.
| if (player1 == player2) | ||
| return NegotiationStatus.NotStarted; |
There was a problem hiding this comment.
Can or should this ever happen? Should this be a condition for an exception instead for catching potential bugs?
There was a problem hiding this comment.
It won't happen, but never know what people will feed it in future. Changed 5b9f62b
| if (CurrentTunnel == null && otherPreviousPing.IsValid() && | ||
| (pingResult.IsUnknown() || pingResult.Milliseconds > TUNNEL_FAILED_PING_AMOUNT)) | ||
| TunnelFailed?.Invoke(this, otherTunnel); |
There was a problem hiding this comment.
From the contribution guideline (Contributing.md):
Braceless code block bodies should be made only when both code block head and body are single line.
| if (CurrentTunnel == null && otherPreviousPing.IsValid() && | |
| (pingResult.IsUnknown() || pingResult.Milliseconds > TUNNEL_FAILED_PING_AMOUNT)) | |
| TunnelFailed?.Invoke(this, otherTunnel); | |
| if (CurrentTunnel == null && otherPreviousPing.IsValid() && | |
| (pingResult.IsUnknown() || pingResult.Milliseconds > TUNNEL_FAILED_PING_AMOUNT)) | |
| { | |
| TunnelFailed?.Invoke(this, otherTunnel); | |
| } |
| foreach (var player in _otherPlayers) | ||
| if (player.Tunnel != null) | ||
| Logger.Log($" Player {player.Name}: {player.Tunnel.Address}:{player.Tunnel.Port}"); |
There was a problem hiding this comment.
From the contribution guideline:
nested braceless blocks are not allowed within braceless blocks
| foreach (var player in _otherPlayers) | |
| if (player.Tunnel != null) | |
| Logger.Log($" Player {player.Name}: {player.Tunnel.Address}:{player.Tunnel.Port}"); | |
| foreach (var player in _otherPlayers) | |
| { | |
| if (player.Tunnel != null) | |
| Logger.Log($" Player {player.Name}: {player.Tunnel.Address}:{player.Tunnel.Port}"); | |
| } |
| /// Stops the game tunnel bridge, unregistering packet handlers, | ||
| /// and closed the local/game UDP socket. |
There was a problem hiding this comment.
| /// Stops the game tunnel bridge, unregistering packet handlers, | |
| /// and closed the local/game UDP socket. | |
| /// Stops the game tunnel bridge, unregisters packet handlers, | |
| /// and closes the local/game UDP socket. |
| } | ||
| _tunnelHandler.SendPacket(recipient.Tunnel, _localId, recipient.Id, | ||
| TunnelPacketType.GameData, gameData); |
There was a problem hiding this comment.
| } | |
| _tunnelHandler.SendPacket(recipient.Tunnel, _localId, recipient.Id, | |
| TunnelPacketType.GameData, gameData); | |
| } | |
| _tunnelHandler.SendPacket(recipient.Tunnel, _localId, recipient.Id, | |
| TunnelPacketType.GameData, gameData); |
| else | ||
| Logger.Log($"V3GameTunnelBridge: No matching recipient found for receiverId={receiverId}"); |
There was a problem hiding this comment.
From the contribution guidelines:
If you use if-else you should either have all of the code blocks braced or braceless to keep things consistent.
| else | |
| Logger.Log($"V3GameTunnelBridge: No matching recipient found for receiverId={receiverId}"); | |
| else | |
| { | |
| Logger.Log($"V3GameTunnelBridge: No matching recipient found for receiverId={receiverId}"); | |
| } |
5 participants
Closes #349
New
UserINISettingson: games use V2 tunnels (overridesUseDynamicTunnels).If
off: games use V3 tunnels.on: lobbies start with no predefined tunnel; tunnel negotiations occurs as players join.If
off: games use static V3 tunnels (unless legacy mode is enabled).New Lobby Commands
/tunnelmode [<mode>]Modes:
0 - V3 (static)
1 - V3 (dynamic)
2 - V2 (legacy)
/negstatus/tunnelinfoNew CTCP Messages
PLYTNL <address>:<port>NEGINFO <target_player>;status[;<ping>]TNLRENEG <failed_address>:<failed_port>TNLFAIL <tunnel_name>STARTV2/STARTV3STARThas been versioned intoSTARTV2andSTARTV3.How V3 Tunnels Work
In V2, all players share a single tunnel.
Example:
In V3 dynamic mode, each pair of players negotiates the best tunnel route:
The client now acts as an intermediary: the game connects to the client, which then routes packets to the negotiated tunnels. This is for both static and dynamic V3.
Negotiation Process
Negotiations occur automatically when players join a lobby and take a few seconds once the player list has arrived.
RTT, packet loss, and average latency are measured.
Connectedpackets and sendsPingRequestpackets.PingResponsepackets.If
Ping unofficial CnCNet tunnelsis disabled, you will only negotiate over official tunnels.Example Log Output (truncated)
Tunnel Failure Handling
A new
TunnelFailedevent has been added to the TunnelHandler.Dynamic tunnels:
Automatically trigger renegotiation when a connection goes down (only affected players).
Static tunnels:
Still to come...
Showing the negotiation in the lobby, and the negotiation status panel (available for all players).
Showing a change to the game creation window and a game's ping:
New settings:
