-
Notifications
You must be signed in to change notification settings - Fork 450
fix: Corrected wrong clientId on receive by keeping SDK internal mapping of client ID to transport ID so we can ensure no overlap #1368
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
Conversation
…ing of client ID to transport ID so we can ensure no overlap
@@ -243,6 +245,10 @@ public ulong LocalClientId | |||
|
|||
private Dictionary<ulong, NetworkClient> m_ConnectedClients = new Dictionary<ulong, NetworkClient>(); | |||
|
|||
private ulong m_NextClientId = 1; |
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.
@bendoyon do you know if this breaks any of our assumptions about client ids in the profiler?
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.
I hope not since UNet and UTP client IDs aren't remotely alike... it's not really safe to have any assumptions about client ids, I don't think...
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.
I think the only assumption we make is that 0 is the server, so that we can write "Server" instead of "Client 0", which should work in this case, but wanted a sanity check
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.
Even that's not a safe assumption! With UTP the server is not 0. It's quite a large number, actually! But now it will be safe after this. :D
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.
Interesting, I don't remember seeing that issue when testing with UTP. Glad that our incorrect assumption becomes correct with this change though
m_TransportIdToClientIdMap.Remove(transportId); | ||
m_ClientIdToTransportIdMap.Remove(clientId); | ||
|
||
//if (NetworkLog.CurrentLogLevel <= LogLevel.Developer) |
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.
Was this intentional?
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.
No, no, not at all, thank you.
…ing of client ID to transport ID so we can ensure no overlap (#1368)
…ing of client ID to transport ID so we can ensure no overlap (Unity-Technologies#1368) * fix: Corrected wrong clientId on receive by keeping SDK internal mapping of client ID to transport ID so we can ensure no overlap
…ing of client ID to transport ID so we can ensure no overlap (Unity-Technologies#1368) (Unity-Technologies#1374)
With both UNet and UTP, the following flow happens:
So because LocalClientId and ServerClientId happen to have the same value because the server is giving the client an ID from a separate process's ID space, any checks on senderId == LocalClientId will return true... but senderId == ServerClientId will also return true...
...except in the case of UNet, where it uses a sentinel value of 0 to indicate the server client ID... but when receiving a message, the ID it passes is the actual ID instead of the sentinel value, which makes the problem worse for UNet because senderId == ServerClientId will return false... even though it actually is the server's client ID and if the client were to send a message back to that ID, it would, in fact, go to the server.
This addresses that issue by separating the concepts of "client ID" and "transport ID". The SDK uses client IDs that are mapped to transport IDs when interacting with the transport. This way we can ensure that "ServerClientId" (which is a references a socket on the local process) and "LocalClientId" (which actually references a socket on the remote server process) will always have unique values - ServerClientId is always 0 on the clients, and server's clientId values start at 1 and count up, so they can't overlap.
MTT-1534
Changelog
com.unity.netcode.gameobjects
Testing and Documentation