Inconsistent use of client/transport ids

[FreshlyBrewedCode]

TLDR

Client and transport ids are getting mixed up. NetworkTransport.ServerClientId should be a transport id and NetworkManager.LocalClientId is a client id yet the current implementation uses them interchangebly.

Detailed description with context

I am currently building a custom transport and I ran into some problems regarding client ids. My transport does assign and use custom ids internally (i.e. transport ids) and in my current implementation the transport id for the server is not 0 but 2.
So naturally I used the following implementation in my implementation of NetworkTransport:

public override ulong ServerClientId => 2;

This implementation works fine and the messages are routed correctly to the server. However, I run into problems if a player is hosting a game. When the player prefab for the host is spawned it receives an incorrect owner id (always 0).

In particular the problem seems to be that the ServerClientId is hardcoded to 0 in NetworkManager:

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

Line 274 in 3d2c266

public const ulong ServerClientId = 0;

and then used in several places e.g. when passing the ownerClientId:

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

Line 946 in 3d2c266

HandleConnectionApproval(ServerClientId, response);

on the other hand, NetworkObject uses NetworkManager.LocalClientId when comparing for ownership:

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

Line 86 in 3d2c266

public bool IsOwner => NetworkManager != null && OwnerClientId == NetworkManager.LocalClientId;

and NetworkManager.LocalClientId directly references the ServerClientId implemented by the transport:

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

Lines 284 to 287 in 3d2c266

	public ulong LocalClientId
	{
	get => IsServer ? NetworkConfig.NetworkTransport.ServerClientId : m_LocalClientId;
	internal set => m_LocalClientId = value;

So as far as I can tell, if the transport implementation returns another value than 0 for ServerClientId on the server side there will be an ownership issue with the player prefab for the host.

I think the real issue however is that currently client and transport ids are getting mixed up. NetworkTransport.ServerClientId is a transport id and NetworkManager.LocalClientId is a client id yet the currently implementation uses them interchangebly.

Btw. the workaround is to return a different ServerClientId on the server/client side in the implementation of the transport:

public override ulong ServerClientId => isHost ? 0 : serverTransportId;

OS: Win10
Netcode Version: 1.2.0
Unity Version: 2020.3.41f1

[NoelStephensUnity]

Hello @FreshlyBrewedCode !
Sorry you are experiencing this issue as it is a known issue that has a detailed description of a work around until we can provide a way to access and/or override the transportid to clientid and clientid to transportid mapping that occurs within the NetworkManager.

The above link to the work around for this when implementing your own transport should provide you enough information, but if you run into any further issues please feel free to post them here and I will do my best to help you out.

(marking this issue as a feature request for better transport id to client id mapping capabilities)

[FreshlyBrewedCode]

Thanks for the reply @NoelStephensUnity. I saw #2359 and I agree that having such API would be helpful. But I feel like the issue I described here is different.

Just focusing on NetworkTransport, as I described above, if ServerClientId returns something other than 0 on the serverside it causes problems. Is this intended behavior or am I missing something? Additionally, NetworkTransport seems to work exclusivly with transport ids, e.g. the clientId passed to Send and DisconnectRemoteClient and also the out parameter of PollEvent.
Also the entire concept of client ids vs transport ids is (and should be imo) a hidden implementation detail of NetworkManager. So by mixing them up as described in this issue it becomes another aspect to worry about when e.g. implementing a custom transport.

Finally, none of this is mentioned in the docs (as far as I can tell). I myself only was able to figure it our by crawling through the implementations of NetworkManager and NetworkObject.

I disagree that this issue is a feature request. I would say it is a slight design flaw or possibly a bug. But then again, I have not been working musch with the API so maybe I am also missing something.

[NoelStephensUnity]

@FreshlyBrewedCode
This portion of the code is a bit tricky, but the general idea was to allow custom NetworkTransport implementations to have their own unique identifiers while still preserving a higher abstraction at the NGO layer. For the server it is a bit tricky to follow but let's see if this clears it up for you:

This is used internally to obtain the server's transport identifier:
m_ServerTransportId => NetworkConfig.NetworkTransport?.ServerClientId

In NetowrkTransport you have an abstract ulong for the server's identifier:
public abstract ulong ServerClientId { get; }

A custom transport defines this value so when using m_ServerTransportId at the "NGO Identifier" layer clients will always be able to send to the server (i.e. sending a ServerRpc for example).

At the NGO layer, the "NGO Server Identifier" is always 0 but when sending to a server it will use the ClientIdToTransportId method to get the server's transport identifier:

internal ulong ClientIdToTransportId(ulong clientId)
{
    return clientId == ServerClientId ? m_ServerTransportId : m_ClientIdToTransportIdMap[clientId];
}

In the above code you will see that if I pass "0" to ClientIdToTransportId, then clientId == ServerClientId is true and as such I will return the NetworkTransform.ServerClientId which is defined/set by the NetworkTransport derived class.

NGO registers for the OnTransportEvent, which when receiving a message from the transport layer:

protected void InvokeOnTransportEvent(NetworkEvent eventType, ulong clientId, ArraySegment<byte> payload, float receiveTime)
{
    OnTransportEvent?.Invoke(eventType, clientId, payload, receiveTime);
}

The transport layer should use the "Transport client identifier" (not the NGO mapped client identifier). So, if a message was coming from the server to a client then it would pass the NetworkTransform.ServerClientId in as the "clientId". At the NGO layer, it will use the NetworkManager.TransportIdToClientId method to translate from the transport layer identifier to the NGO layer identifier:

internal ulong TransportIdToClientId(ulong transportId)
{
    return transportId == m_ServerTransportId ? ServerClientId : m_TransportIdToClientIdMap[transportId];
}

Which if it was the NetworkTransform.ServerClientId then the clientId (NGO layer) would translate to zero (0).

Really, think of your custom NetworkTransport as an abstraction layer between transport and NGO. NGO only interfaces with it, but maintains the same "NGO Layer" client and server identifiers to keep it "standard"... so the same "NGO Layer" user code implementations would work whether using your custom NetworkTransport, UnityTransport, or any other user's custom NetworkTransport... as the "NGO Layer" client identifiers and how they are assigned remains the same but the transport identifiers can be radically different... client Identifier 1 at the NGO layer will always be client identifier 1... but could be any value at the transport identifier layer (depending upon the transport used etc).

Where I think we can improve and reduce the confusion in this region of the code is to not use property names at the NetworkTransport layer like "clientId" but rather something like "transportId" or "transportClientId" in order to make it clearer that the transport identifier should be used there...so something like NetworkTransport.InvokeOnTransportEvent may be better expressed as:

protected void InvokeOnTransportEvent(NetworkEvent eventType, ulong transportClientId, ArraySegment<byte> payload, float receiveTime)
{
    OnTransportEvent?.Invoke(eventType, transportClientId, payload, receiveTime);
}

For now, when within your NetworkTranport derived class when you see "clientId" or "ServerClientId" try to remind yourself that this is "transport relative" and not "NGO relative".

Does this explanation help any?
I think the feature request is not only to provide access to the identifier mapping table but to better clarify (via property and parameter names) what "identifier" should be used since the NGO layer and NetworkTransport layer both use the same naming convention it can cause confusion as to which "identifier" should be used.

[FreshlyBrewedCode]

Thanks for the detailed explaination. The explaination makes sense and is what I already assumed after looking at the code.

For now, when within your NetworkTranport derived class when you see "clientId" or "ServerClientId" try to remind yourself that this is "transport relative" and not "NGO relative".

If we assume that all "client ids" used in NetworkTransport are "transport relative" (i.e. "transport ids") then I think this snippet in NetworkManager is mixing up the NGO internal ids and transport ids:

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

Lines 284 to 288 in 3d2c266

	public ulong LocalClientId
	{
	get => IsServer ? NetworkConfig.NetworkTransport.ServerClientId : m_LocalClientId;
	internal set => m_LocalClientId = value;
	}

as far as I can tell the LocalClientId should refer to the NGO client id and not the transport id and still it does return NetworkTransport.ServerClientId on the server side which as we established should be a transport relative id.

As mentioned in the original post, this mix up causes problems with e.g. the ownership of player prefabs if ServerClientId returns a value other than 0 on the server side.

[NoelStephensUnity]

@FreshlyBrewedCode
That does indeed look like it is returning the incorrect value. Let me double check as to why that is not returning the NGO layer client identifier, and if it is a mistake/bug I will get a PR up to fix that issue.
Thank you for pointing this out.

[FreshlyBrewedCode]

Great, I think the issue is solved with that PR. Thank you!