Unity-Technologies / multiplayer-community-contributions

Community contributions to Unity Multiplayer Networking products and services.
MIT License
421 stars 161 forks source link

SteamNetworkingSockets Transport: Unexpected clientId Behavior #200

Open Softdrink117 opened 1 year ago

Softdrink117 commented 1 year ago

Hiya,

I've noticed an unexpected issue when trying to use clientIds with the SteamNetworkingSockets Transport. Unity's NetworkManager APIs use clientIds that appear to start 0-indexed, incrementing for each Client connection. However, within the SteamNetworkingSocketsTransport (and its predecessor), it seems like the connectionMapping Dictionary is actually using the Steam ID as a lookup key.

For example, in SteamNetworkingSocketsTransport.cs:163 clientId = param.m_info.m_identityRemote.GetSteamID64();

I added a simple test to see what the connectionMapping keys actually are; injecting the following lines at SteamNetworkingSocketsTransport.cs:105

string connectedClients = "";
foreach(var entry in connectionMapping)
{
    connectedClients += "clientId " + entry.Key + ": steamId " + entry.Value.id + "\n";
}
UnityEngine.Debug.Log(connectedClients);

This displays key: value pairs that seem to be indexed by Steam IDs and not Unity clientIds.

This creates problems when calling some NetworkManager functions, such as GetCurrentRtt() or DisconnectRemoteClient(), using the clientIds provided by NetworkManager.ConnectedClientsIds.

Here is an example error message, where calling GetCurrentRtt(0ul) in Host mode returns an error message (instead of the expected return value of 0, since it is the local machine):

[Netcode-Server Sender=0] SteamNetworkingSocketsTransport - Can't GetCurrentRtt from client, client not connected, clientId: 0
UnityEngine.Debug:LogError (object)
Unity.Netcode.NetworkLog:LogErrorServerLocal (string,ulong) (at Library/PackageCache/com.unity.netcode.gameobjects@1.1.0/Runtime/Logging/NetworkLog.cs:88)
Unity.Netcode.NetworkLog:LogServer (string,Unity.Netcode.NetworkLog/LogType) (at Library/PackageCache/com.unity.netcode.gameobjects@1.1.0/Runtime/Logging/NetworkLog.cs:68)
Unity.Netcode.NetworkLog:LogErrorServer (string) (at Library/PackageCache/com.unity.netcode.gameobjects@1.1.0/Runtime/Logging/NetworkLog.cs:52)
Netcode.Transports.SteamNetworkingSocketsTransport:GetCurrentRtt (ulong) (at Library/PackageCache/com.community.netcode.transport.steamnetworkingsockets@858f6df327/Runtime/SteamNetworkingSocketsTransport.cs:106)

Perhaps I am misunderstanding the intended use case of these features, but it seems to me like the clientIds used by the NetworkManager.ConnectedClientsIds should map to the SteamNetworkingTransport in an intuitive way. I first noticed this issue with the previous implementation (SteamNetworkingTransport), so I updated to the Sockets implementation as part of troubleshooting. As I'm writing this, I also notice that GetCurrentRtt() is not fully implemented yet, with a rather entertaining TODO note included, so it's clear more work is underway.

Thanks very much for your efforts; this is an excellent Transport option and the direct integration with Unity's Netcode system has made integration quite smooth so far.

JamesMcGhee commented 1 year ago

RTT with Steam Networking Socket isn't' that straightforward as there isn't a true ping for it

As to the connection mapping it maps the CSteamID (which is a ulong value) to the HSteamNetConnection and has nothing to do with various HLAPI's internal concept of a "connection ID" which is usually some index

With Steam the ID is the user's CSteamID, that is the address and the ID. Assuming there was a straightforward way to check the round trip time (which there is not) you would check that against the targer's User ID not NetCodes concept of a connection index

Softdrink117 commented 1 year ago

I understand the logic behind using the SteamID, but I think it's a bit confusing as a user that this specific transport doesn't behave in a way that is consistent with other HLAPI transports or the HLAPI docs.

Perhaps it's worth noting in the documentation that this is happening behind the scenes, since it may otherwise be confusing to use.

Thanks,