Unity-Technologies / com.unity.netcode.gameobjects

Netcode for GameObjects is a high-level netcode SDK that provides networking capabilities to GameObject/MonoBehaviour workflows within Unity and sits on top of underlying transport layer.
MIT License
2.15k stars 435 forks source link

OnNetworkSpawn race condition in NetworkTransform for Teleport method #2869

Closed zachstronaut closed 7 months ago

zachstronaut commented 7 months ago

NetworkTransform's Teleport()method will throw a string exception if you try to use it before that NetworkTransform's OnNetworkSpawn() has completed, which means that any other code that waits forOnNetworkSpawn() in the user codebase that wants to GetComponent<NetworkTransform>() andTeleport() the object has a potential race condition with NetworkTransform.

The source of the error is m_MessageName not being set yet. This however shouldn't be done in OnNetworkSpawn(). This sort of internal boostrapping by Netcode should be done in a pre-spawn step. NetworkBehaviourhas exactly such as concept: InternalOnNetworkSpawn().

NetworkBehaviour's InternalOnNetworkSpawn() should be made a virtualmethod so that the following can be done in NetworkTransform:

internal override void InternalOnNetworkSpawn()
{
    base.InternalOnNetworkSpawn();

    // Register a custom named message specifically for this instance
    m_MessageName = $"NTU_{NetworkObjectId}_{NetworkBehaviourId}";
    NetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(m_MessageName, TransformStateUpdate);
}

Those lines would be removed from NetworkTransform's OnNetworkSpawn().

zachstronaut commented 7 months ago

It's also possible InitializeVariables() or UpdateNetworkProperties() are better candidates for the virtual method?

zachstronaut commented 7 months ago

See also: https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues/2870

NoelStephensUnity commented 7 months ago

This should not be an issue in v1.8.1 of NGO. NetworkTransform no longer uses named messages.

zachstronaut commented 7 months ago

Thanks, that does seem to be the case!