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
|
@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. public readonly record struct PingValue
{
private readonly int _value;
private PingValue(int value) => _value = value;
/// <summary>
/// An unknown or invalid ping measurement.
/// </summary>
public static PingValue Unknown => new(-1);
/// <summary>
/// Creates a PingValue from a millisecond amount.
/// </summary>
/// <param name="milliseconds">The ping in milliseconds. Must be non-negative.</param>
/// <returns>A valid PingValue.</returns>
/// <exception cref="ArgumentException">Thrown when milliseconds is negative.</exception>
public static PingValue FromMs(int milliseconds)
{
if (milliseconds < 0)
throw new ArgumentException("Ping cannot be negative. Use PingValue.Unknown for unknown pings.", nameof(milliseconds));
return new(milliseconds);
}
/// <summary>
/// Returns true if this ping value is valid (not unknown).
/// </summary>
public bool IsValid() => _value >= 0;
/// <summary>
/// Returns true if this ping value is unknown or invalid.
/// </summary>
public bool IsUnknown() => _value < 0;
/// <summary>
/// Gets the ping in milliseconds.
/// Returns -1 for unknown pings.
/// Prefer using IsValid() to check validity before accessing this value.
/// </summary>
public int Milliseconds => _value;
/// <summary>
/// Returns a string representation of theping value.
/// </summary>
public override string ToString() => IsValid() ? $"{_value}ms" : "Unknown";
} |
|
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.
| 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}"); | |
| } |
There was a problem hiding this comment.
Pull request overview
Adds V3 tunnel support (static + dynamic negotiation) while keeping V2 tunnels supported, including UI/status display and new settings/CTCP messages to coordinate tunnel mode and negotiation state across lobby participants.
Changes:
- Introduces V3 tunnel transport/negotiation components (communicator, per-peer negotiator, and game UDP bridge).
- Adds lobby UX for dynamic tunnel negotiation (status matrix/ping indicators) and new tunnel-mode/negstatus commands.
- Reworks ping representation across multiplayer models to use
PingValue(replacingint+-1sentinel), and updates tunnel list selection/filtering for V2/V3.
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| DXMainClient/Resources/DTA/negotiation-failed.png | New UI asset for failed negotiation indicator. |
| DXMainClient/Resources/DTA/negotiating.png | New UI asset for negotiation-in-progress indicator. |
| DXMainClient/Domain/Multiplayer/PlayerInfo.cs | Switches player ping to PingValue. |
| DXMainClient/Domain/Multiplayer/LAN/LANPlayerInfo.cs | Uses PingValue for LAN ping measurement. |
| DXMainClient/Domain/Multiplayer/LAN/HostedLANGame.cs | Exposes hosted LAN ping as PingValue.Unknown. |
| DXMainClient/Domain/Multiplayer/GenericHostedGame.cs | Changes hosted-game ping abstraction to PingValue. |
| DXMainClient/Domain/Multiplayer/CnCNet/V3TunnelCommunicator.cs | Adds UDP communicator for V3 tunnel packets + handler dispatch. |
| DXMainClient/Domain/Multiplayer/CnCNet/V3PlayerNegotiator.cs | Adds per-peer tunnel negotiation logic and messaging. |
| DXMainClient/Domain/Multiplayer/CnCNet/V3PlayerInfo.cs | Adds per-player V3 negotiation state + tunnel scoring logic. |
| DXMainClient/Domain/Multiplayer/CnCNet/V3GameTunnelBridge.cs | Adds local game <-> tunnel forwarding bridge for V3. |
| DXMainClient/Domain/Multiplayer/CnCNet/TunnelHandler.cs | Supports tunnel versions 2+3, tunnel-failure eventing, V3 bridge start/stop, and ping-by-address deduping. |
| DXMainClient/Domain/Multiplayer/CnCNet/TunnelChosenEventArgs.cs | Adds event args for negotiation outcomes. |
| DXMainClient/Domain/Multiplayer/CnCNet/PingValue.cs | Introduces PingValue value type for ping/unknown handling + localization. |
| DXMainClient/Domain/Multiplayer/CnCNet/NegotiationDataManager.cs | Tracks per-pair negotiation statuses/pings for UI. |
| DXMainClient/Domain/Multiplayer/CnCNet/HostedCnCNetGame.cs | Uses PingValue and supports “dynamic tunnel” hosted-game representation. |
| DXMainClient/Domain/Multiplayer/CnCNet/CnCNetTunnel.cs | Stores ping as PingValue and updates ping-refresh behavior. |
| DXMainClient/DXGUI/Multiplayer/GameLobby/TunnelNegotiationStatusPanel.cs | New negotiation status matrix panel UI. |
| DXMainClient/DXGUI/Multiplayer/GameLobby/MultiplayerGameLobby.cs | Adds negotiation textures and ping indicator support for negotiation states. |
| DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs | Implements tunnel modes (V2/V3 static/V3 dynamic), STARTV2/STARTV3, negotiation broadcast/UX, and renegotiation on tunnel failure. |
| DXMainClient/DXGUI/Multiplayer/GameInformationPanel.cs | Displays “dynamic” ping state and uses PingValue formatting. |
| DXMainClient/DXGUI/Multiplayer/CnCNet/TunnelSelectionWindow.cs | Adds target-version filtering and safer list scrolling behavior. |
| DXMainClient/DXGUI/Multiplayer/CnCNet/TunnelListBox.cs | Filters tunnels by version and updates ping display to PingValue. |
| DXMainClient/DXGUI/Multiplayer/CnCNet/GameCreationWindow.cs | Integrates dynamic/legacy tunnel settings into game creation UX. |
| DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs | Supports dynamic-tunnel broadcast parsing via [DYN]. |
| DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetGameLoadingLobby.cs | Updates ping messaging to use PingValue. |
| DXMainClient/DXGUI/Generic/OptionPanels/CnCNetOptionsPanel.cs | Adds user settings toggles for legacy vs dynamic tunnels. |
| ClientCore/Settings/UserINISettings.cs | Adds UseLegacyTunnels and UseDynamicTunnels INI settings. |
| ClientCore/ProgramConstants.cs | Updates CnCNet protocol revision constant. |
Comments suppressed due to low confidence (1)
DXMainClient/Domain/Multiplayer/CnCNet/TunnelHandler.cs:91
V3TunnelCommunicatoris initialized once and never shut down or refreshed:ConnectionManager_Disconnectedjust disables the component, andInitializeTunnelCommunicator()is a no-op after the first initialization. This leaves the UDP receive thread/socket running across disconnects, and it also prevents updating the tunnel endpoint map when the tunnel list refreshes. Consider adding aStop/Dispose/Reseton disconnect and reinitializing (or updating endpoints) when the tunnel list changes.
private void ConnectionManager_Connected(object sender, EventArgs e)
{
InitializeTunnelCommunicator();
Enabled = true;
}
private void ConnectionManager_ConnectionLost(object sender, Online.EventArguments.ConnectionLostEventArgs e) => Enabled = false;
private void ConnectionManager_Disconnected(object sender, EventArgs e) => Enabled = false;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public CnCNetTunnel ChosenTunnel { get; set; } | ||
| public bool IsLocalDecision { get; set; } | ||
| public string FailureReason { get; set; } |
There was a problem hiding this comment.
TunnelChosenEventArgs defines PlayerName, ChosenTunnel, and FailureReason as non-nullable, but callers pass null for ChosenTunnel/FailureReason on failure and check e.ChosenTunnel != null downstream. Update the property types to be nullable (string?, CnCNetTunnel?) and/or provide safe defaults to avoid null-related runtime errors and clarify the API contract.
| public CnCNetTunnel ChosenTunnel { get; set; } | |
| public bool IsLocalDecision { get; set; } | |
| public string FailureReason { get; set; } | |
| public CnCNetTunnel? ChosenTunnel { get; set; } | |
| public bool IsLocalDecision { get; set; } | |
| public string? FailureReason { get; set; } |
| NegotiationStatus? negotiationStatus, | ||
| string tooltipText) |
There was a problem hiding this comment.
UpdatePlayerPingIndicator(..., string tooltipText) treats tooltipText as optional (tooltipText ?? ...) and callers pass null, but the parameter type is non-nullable string. Mark it as string? (and ideally give it a default value) to match actual usage and avoid nullable warnings/contract confusion.
| NegotiationStatus? negotiationStatus, | |
| string tooltipText) | |
| NegotiationStatus? negotiationStatus = null, | |
| string? tooltipText = null) |
| _tunnelHandler = tunnelHandler; | ||
| _isDecider = localPlayer.Id < remotePlayer.Id; | ||
|
|
There was a problem hiding this comment.
_isDecider is determined as localPlayer.Id < remotePlayer.Id, which makes the lower ID the decider. The PR description says the decider should be the player with the higher ID; either update the code to match the documented rule or update the documentation/spec so both clients agree on the same decider selection rule.
| { | ||
| byte[] gameData = _localGameClient.Receive(ref remoteEndPoint); | ||
| _gameEndpoint = remoteEndPoint; | ||
|
|
There was a problem hiding this comment.
BridgeWorker() reads receiverId from gameData.AsSpan(2) without validating packet length. If the local game sends a shorter UDP datagram (or a malformed one is received), ReadUInt16BigEndian will throw and terminate the bridge thread. Add a minimal length check before parsing the receiver ID (and ideally log+skip invalid packets).
| // Validate that the packet is long enough to contain a receiver ID at offset 2 | |
| if (gameData.Length < 4) | |
| { | |
| Logger.Log($"V3GameTunnelBridge: Ignoring too-short game packet from local game (length={gameData.Length})"); | |
| continue; | |
| } |
| HostedCnCNetGame hostedGame = (HostedCnCNetGame)game; | ||
| string pingText; | ||
|
|
||
| if (hostedGame.TunnelServer == null) | ||
| { | ||
| pingText = "Ping: Dynamic".L10N("Client:Main:GameInfoPingDynamic"); | ||
| } | ||
| else | ||
| { | ||
| pingText = "Ping:".L10N("Client:Main:GameInfoPing") + " " + hostedGame.TunnelServer.Ping.ToString(); |
There was a problem hiding this comment.
SetInfo(GenericHostedGame game) unconditionally casts game to HostedCnCNetGame. GameInformationPanel is used for both CnCNet and LAN games, so this will throw InvalidCastException for HostedLANGame (or any other GenericHostedGame implementation). Use pattern matching (game is HostedCnCNetGame hostedGame) and fall back to game.Ping for non-CnCNet games.
| HostedCnCNetGame hostedGame = (HostedCnCNetGame)game; | |
| string pingText; | |
| if (hostedGame.TunnelServer == null) | |
| { | |
| pingText = "Ping: Dynamic".L10N("Client:Main:GameInfoPingDynamic"); | |
| } | |
| else | |
| { | |
| pingText = "Ping:".L10N("Client:Main:GameInfoPing") + " " + hostedGame.TunnelServer.Ping.ToString(); | |
| string pingText; | |
| if (game is HostedCnCNetGame hostedGame) | |
| { | |
| if (hostedGame.TunnelServer == null) | |
| { | |
| pingText = "Ping: Dynamic".L10N("Client:Main:GameInfoPingDynamic"); | |
| } | |
| else | |
| { | |
| pingText = "Ping:".L10N("Client:Main:GameInfoPing") + " " + hostedGame.TunnelServer.Ping.ToString(); | |
| } | |
| } | |
| else | |
| { | |
| pingText = "Ping:".L10N("Client:Main:GameInfoPing") + " " + game.Ping.ToString(); |
| new StringCommandHandler(NEGOTIATION_INFO_MESSAGE, HandleNegotiationInfoMessage), | ||
| new StringCommandHandler(TUNNEL_RENEGOTIATE_MESSAGE, HandleTunnelRenegotiateMessage), | ||
| new StringCommandHandler(TUNNEL_FAILED_MESSAGE, HandleTunnelFailedMessage), | ||
| new StringCommandHandler(CHANGE_TUNNEL_SERVER_MESSAGE, HandleTunnelServerChangeMessage), |
There was a problem hiding this comment.
ctcpCommandHandlers registers CHANGE_TUNNEL_SERVER_MESSAGE twice. This will cause the tunnel change handler to run twice for every CHTNL message (potentially duplicating state changes / notices). Remove the duplicate registration.
| new StringCommandHandler(CHANGE_TUNNEL_SERVER_MESSAGE, HandleTunnelServerChangeMessage), |
| CnCNetTunnel tunnel = tunnelHandler.Tunnels.Find(t => t.Address == tunnelAddress && t.Port == tunnelPort); | ||
| AddNotice($"{sender} is using tunnel: {tunnel.Name}"); | ||
|
|
||
| if (!IsHost) | ||
| return; | ||
|
|
||
| var v3PlayerInfo = _v3PlayerInfos.FirstOrDefault(p => p.Name == sender); | ||
| if (v3PlayerInfo != null) | ||
| v3PlayerInfo.Tunnel = tunnel; |
There was a problem hiding this comment.
HandlePlayerTunnelMessage assumes the tunnel exists in tunnelHandler.Tunnels and dereferences tunnel.Name without a null-check. If a client reports a tunnel that isn't present in the current tunnel list, this will throw. Handle the tunnel == null case (and avoid assigning a null tunnel to v3PlayerInfo.Tunnel).
| PrintNegotiationResults(); | ||
| _negotiationCompletionSource.TrySetResult(true); | ||
| NegotiationComplete?.Invoke(this, EventArgs.Empty); | ||
| return true; |
There was a problem hiding this comment.
NegotiateAsync() always returns true on the non-exception path and unconditionally TrySetResult(true) on _negotiationCompletionSource, even if the negotiation actually timed out or failed (e.g., non-decider timeout sets the TCS to false). This can cause the caller/UI to treat a failed negotiation as successful; consider awaiting the actual negotiation result (and for deciders, fail if no TunnelAck is received within the retry window).
| PrintNegotiationResults(); | |
| _negotiationCompletionSource.TrySetResult(true); | |
| NegotiationComplete?.Invoke(this, EventArgs.Empty); | |
| return true; | |
| // Await the actual negotiation result, which may be set to true or false | |
| bool negotiationSucceeded = await _negotiationCompletionSource.Task; | |
| PrintNegotiationResults(); | |
| NegotiationComplete?.Invoke(this, EventArgs.Empty); | |
| return negotiationSucceeded; |
| var totalTunnels = _remotePlayer.TunnelResults.Count; | ||
| var completedTunnels = 0; | ||
| var selectionMade = false; | ||
| var completionLock = new object(); | ||
| var selectionTcs = new TaskCompletionSource<bool>(); | ||
|
|
||
| foreach (var kvp in _remotePlayer.TunnelResults) | ||
| { | ||
| var tunnel = kvp.Key; | ||
| var result = kvp.Value; | ||
| _ = WaitForTunnelResultsAsync(result, () => { | ||
| lock (completionLock) | ||
| { | ||
| completedTunnels++; | ||
| if (!selectionMade && completedTunnels >= Math.Max(2, totalTunnels * EARLY_SELECTION_THRESHOLD)) | ||
| { | ||
| selectionMade = true; | ||
| selectionTcs.TrySetResult(true); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Wait for early selection or all completion | ||
| await selectionTcs.Task; | ||
|
|
There was a problem hiding this comment.
PerformDeciderNegotiationAsync() awaits selectionTcs.Task, but selectionTcs is only completed when completedTunnels >= Math.Max(2, totalTunnels * EARLY_SELECTION_THRESHOLD). If totalTunnels is 1 (or if totalTunnels * threshold rounds up above totalTunnels), selectionTcs is never set and negotiation hangs. Consider completing the TCS when completedTunnels == totalTunnels as well, and using Math.Max(1, ...) for the threshold.
| using DTAClient.DXGUI.Multiplayer.GameLobby; | ||
|
|
||
| #nullable enable | ||
|
|
||
| namespace DTAClient.Domain.Multiplayer.CnCNet; | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
NegotiationDataManager (Domain layer) depends on DTAClient.DXGUI.Multiplayer.GameLobby to get NegotiationStatus. This creates an inverted dependency (Domain -> UI) and will make it harder to reuse/test the domain code. Consider moving NegotiationStatus into a non-UI namespace (e.g., DTAClient.Domain.Multiplayer.CnCNet) so both UI and domain can reference it without a circular layering dependency.
| using DTAClient.DXGUI.Multiplayer.GameLobby; | |
| #nullable enable | |
| namespace DTAClient.Domain.Multiplayer.CnCNet; | |
| /// <summary> | |
| #nullable enable | |
| namespace DTAClient.Domain.Multiplayer.CnCNet; | |
| /// <summary> | |
| /// The status of connection/negotiation between two players. | |
| /// </summary> | |
| public enum NegotiationStatus | |
| { | |
| NotStarted = 0, | |
| InProgress = 1, | |
| Succeeded = 2, | |
| Failed = 3 | |
| } | |
| /// <summary> |
Rampastring
left a comment
Rampastring
left a comment
There was a problem hiding this comment.
Reviewed the rest aside from CnCNetGameLobby.cs. Found stylistic issues to fix, and suggested a couple of optimizations to sending packets.
Copilot also seems to have found some valid issues.
| public CnCNetTunnel? Tunnel { get; set; } | ||
| public V3PlayerNegotiator? Negotiator { get; set; } | ||
| public Dictionary<CnCNetTunnel, TunnelTestResult> TunnelResults { get; } = []; | ||
| private const int PACKET_LOSS_WEIGHT = 10; |
There was a problem hiding this comment.
Constants in the client's code are usually put to the top of their class, before properties.
Leaving an empty line between constants and properties helps make them distinct, too.
| public bool HasNegotiated { get; set; } | ||
| public bool IsNegotiating { get; set; } | ||
| public CnCNetTunnel? Tunnel { get; set; } | ||
| public V3PlayerNegotiator? Negotiator { get; set; } |
There was a problem hiding this comment.
Seeing the SetNegotiator method, I suggest either making the setter for this property private, or moving the code of SetNegotiator inside the setter for this property. As-is this has a public setter as a well as separate public set method, and from outside it might be confusing which should be used.
| public bool StartNegotiation(V3PlayerInfo localPlayer, TunnelHandler tunnelHandler, List<CnCNetTunnel> availableTunnels) | ||
| { | ||
| if (this == localPlayer) | ||
| return true; |
There was a problem hiding this comment.
Is this intended to happen? If not, maybe it should throw an exception instead to help catch bugs early?
| // If true, you send ping requests and measure latency. | ||
| // If false, you reply to ping requests | ||
| // This is set based on the ID (player1ID < player2ID) | ||
| // As a negotiator runs for each other player, you may be a decider for | ||
| // some, and a non-decider for others. | ||
| private readonly bool _isDecider; |
There was a problem hiding this comment.
These kinds of comments should be turned into summary comments so they show up when hovering over the variables elsewhere in code in Visual Studio. Also sentences should have proper punctuation (especially ending sentences with dots) to make them easier to read.
| // If true, you send ping requests and measure latency. | |
| // If false, you reply to ping requests | |
| // This is set based on the ID (player1ID < player2ID) | |
| // As a negotiator runs for each other player, you may be a decider for | |
| // some, and a non-decider for others. | |
| private readonly bool _isDecider; | |
| /// <summary> | |
| /// If true, you send ping requests and measure latency. | |
| /// If false, you reply to ping requests. | |
| /// | |
| /// This is set based on the ID (player1ID < player2ID). | |
| /// As a negotiator runs for each other player, you may be a decider for | |
| /// some, and a non-decider for others. | |
| /// </summary> | |
| private readonly bool _isDecider; |
| var totalTunnels = _remotePlayer.TunnelResults.Count; | ||
| var completedTunnels = 0; | ||
| var selectionMade = false; | ||
| var completionLock = new object(); | ||
| var selectionTcs = new TaskCompletionSource<bool>(); |
There was a problem hiding this comment.
From Contributing.md:
Never use
varwith primitive types.
| var totalTunnels = _remotePlayer.TunnelResults.Count; | |
| var completedTunnels = 0; | |
| var selectionMade = false; | |
| var completionLock = new object(); | |
| var selectionTcs = new TaskCompletionSource<bool>(); | |
| int totalTunnels = _remotePlayer.TunnelResults.Count; | |
| int completedTunnels = 0; | |
| bool selectionMade = false; | |
| var completionLock = new object(); | |
| var selectionTcs = new TaskCompletionSource<bool>(); |
| _handlers.TryRemove((localId, remoteId), out _); | ||
| Logger.Log($"V3TunnelCommunicator: Unregistered handler for {localId} <-> {remoteId}"); |
There was a problem hiding this comment.
Currently this logs that the handler was removed even if TryRemove returned false. For more precise logging, could check the return value instead:
| _handlers.TryRemove((localId, remoteId), out _); | |
| Logger.Log($"V3TunnelCommunicator: Unregistered handler for {localId} <-> {remoteId}"); | |
| if (_handlers.TryRemove((localId, remoteId), out _)) | |
| Logger.Log($"V3TunnelCommunicator: Unregistered handler for {localId} <-> {remoteId}"); | |
| else | |
| Logger.Log($"V3TunnelCommunicator: Handler not found for {localId} <-> {remoteId} while attempting unregistration"); |
Or, in case the handler should always be present when this is called, could reinforce it with an exception.
| try | ||
| { | ||
| var packet = CreatePacket(senderId, receiverId, packetType, payload); | ||
| _udpClient!.Send(packet, packet.Length, tunnel.Address, tunnel.Port); |
There was a problem hiding this comment.
This could be optimized a little bit by changing the tunnel server's address into an IPEndPoint instance and caching it to the tunnel instance.
Then, you could use an overload of UdpClient.Send that takes an IPEndPoint instead of address + port, saving the system from having to parse the IP address string when sending each packet.
...looks like InitializeConnection already generates IPEndPoints for each tunnel, could just make use of them.
Another easy optimization would be changing the packet creation to write the packet to a Span allocated in the stack of this method, and then using the one of the UdpClient.Send overloads that take a ReadOnlySpan<Byte> parameter, avoiding the heap allocation of the packet byte array.
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:
