OnNetworkSpawn race condition in NetworkTransform for CanCommitToTransform

[zachstronaut]

CanCommitToTransformisn't correctly set for a NetworkTransformuntil OnNetworkSpawn(). (This ends up being an issue for client-authoritative / owner-authoritative NetworkTransforms.)

Any user code that waits for its own OnNetworkSpawn() that wants to GetComponent<NetworkTransform>() and call a method that will check CanCommitToTransformhas a potential race condition with NetworkTransform.

Setting CanCommitToTransformvia NetworkTransform's Initialize() method shouldn't be done for the first time in NetworkTransform's OnNetworkSpawn(). This sort of internal boostrapping by Netcode should be done in a pre-spawn step. NetworkBehaviour has exactly such as concept: InternalOnNetworkSpawn().

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

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

    CanCommitToTransform = IsServerAuthoritative() ? IsServer : IsOwner;
}

The CanCommitToTransform would also be left in NetworkTransform's Initialize() to handle any later changes to ownership, etc.

[zachstronaut]

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

[NoelStephensUnity]

Yeah. Currently you have to invoke base.OnNetworkSpawn within any override to assure CanCommitToTransform is set. Your suggested fix would resolve the potential for this issue.

[zachstronaut]

Yeah. Currently you have to invoke base.OnNetworkSpawn within any override to assure CanCommitToTransform is set. Your suggested fix would resolve the potential for this issue.

Well, the race condition I'm describing is when any FooBar:NetworkBehaviour.OnNetworkSpawn() makes a GetComponent() call and then tries to do anything that might check CanCommitToTransform ... so it's nothing to do at all with overriding OnNetworkSpawn in a NetworkTransform descendant.

[NoelStephensUnity]

@zachstronaut Ahh... Have you tried moving the component after the NetworkTransform? The attached script is something I have been meaning to add to our repo when I can spend a little more time on it. The attached script/tool lets you find NetworkObjects as well as displays the invocation order of NetworkBehaviours in order to better handle network spawn dependency related issues (i.e. re-ordering the components in the inspector view changes the order in which OnNetworkSpawn is invoked relative to other NetworkBehaviour components).

So, if something is depending upon NetworkTransform's OnNetworkSpawn to be invoked you can order them (for now) such that NetworkTransform's will be invoked prior to the other NetworkBehaviour.

NetcodeForGameObjectsTools.zip

[zachstronaut]

@NoelStephensUnity Thanks, a way to set order of execution amongst NetworkBehaviours is useful.

I still think there's utility in exposing the PreSpawn and Spawn both rather than just Spawn. I've made the change that I proposed in my local Netcode for this fix.

[NoelStephensUnity]

I still think there's utility in exposing the PreSpawn and Spawn both rather than just Spawn. I've made the change that I proposed in my local Netcode for this fix.

Indeed! This is something I was discussing earlier with a fellow team member. I think other related suggestions you have made relative to that concept could be helpful relative to order of operations. Specifically an internal prespawn and spawn that handles any internal initialization for NGO components (i.e. NetworkTransform for sure) would be useful too.

This is in my "next things to improve" list. 👍

[NoelStephensUnity]

@zachstronaut Just wanted to have you take a look at the PR. The pre and post spawn methods have to be invoked on a per NetworkObject basis, but it should at least give you the ability to handle race conditions where you might be expecting another NetworkBehaviour to have run through the OnNetworkSpawn process (for post).

NetworkTransform will be having some adjustments in the near future that will also take all of this into consideration, but the core fix is to provide the pre and post methods.

[NoelStephensUnity]

@zachstronaut I think #2906 should address all of your suggested improvements and then possibly more. 👍 :godmode: