com.unity.netcode.gameobjects icon indicating copy to clipboard operation
com.unity.netcode.gameobjects copied to clipboard

NetworkAnimator.Cleanup() removes OnClientConnectedCallback only if IsServer is true, but ShutdownInternal() sets it to false beforehand.

Open davidyrgilbert opened this issue 3 years ago • 2 comments

Description

When shutting down a server/host and starting it again with a NetworkAnimator on e.g. the player, an error gets thrown:

NullReferenceException: Object reference not set to an instance of an object
Unity.Netcode.Components.NetworkAnimator.OnClientConnectedCallback (System.UInt64 playerId) (at Library/PackageCache/[email protected]/Components/NetworkAnimator.cs:447)
Unity.Netcode.NetworkManager.InvokeOnClientConnectedCallback (System.UInt64 clientId) (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:379)
Unity.Netcode.NetworkManager.HandleConnectionApproval (System.UInt64 ownerClientId, Unity.Netcode.NetworkManager+ConnectionApprovalResponse response) (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:2077)
Unity.Netcode.NetworkManager.StartHost () (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:1126)

It's pretty clear why that happens:

For the NetworkAnimator, OnNetworkSpawn:

                if (IsServer)
                {
                    NetworkManager.OnClientConnectedCallback += OnClientConnectedCallback;
                }

Now, on Cleanup(), the callback is removed again:

        private void Cleanup()
        {
            ...
            if (IsServer)
            {
                NetworkManager.OnClientConnectedCallback -= OnClientConnectedCallback;
            }
            ...

However, cleanup gets called here:

internal void ShutdownInternal()
{
            ....
            IsConnectedClient = false;
            IsServer = false;
            IsClient = false;

            this.UnregisterAllNetworkUpdates();

            ....            

            if (SpawnManager != null)
            {
                SpawnManager.DespawnAndDestroyNetworkObjects();            <---- this is where Cleanup gets called
                SpawnManager.ServerResetShudownStateForSceneObjects();

                SpawnManager = null;
            }
}

Then, the callback is not being cleaned up, but the object behind it is deleted. Connecting again then tries to execute that cleanup but fails, obviously.

### Reproduce Steps

1. Add NetworkAnimator to e.g. PlayerPrefab
2. StartHost()
3. StopHost()
4. StartHost() again. 

Not 100% sure this is enough as we're doing a few initialization things inbetween, but the issue should be quite clear from the code.

### Actual Outcome

Callback doesn't get removed

### Expected Outcome

Callback gets removed

### Screenshots

If applicable, add screenshots to help explain your problem.

### Environment

- OS: Any
- Unity Version: 2021.3.5
- Netcode Version: 1.0.0
- Netcode Commit: https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/commit/39a244854e323e5e0f444bc17bb7662f66ce28f2#diff-d4e79dceee7c1288ed9747284d2b61057e03cc974cfb21af8afb2704b1fada99R258

### Additional Context

davidyrgilbert avatar Aug 09 '22 16:08 davidyrgilbert

@NoelStephensUnity I think this belongs to you as well :)

ashwinimurt avatar Aug 11 '22 18:08 ashwinimurt

Backlog MTT-4385

ashwinimurt avatar Aug 12 '22 00:08 ashwinimurt

Hi @davidyrgilbert! I am closing this out as it was fixed in PR-2074 and the fix will be included in the next update. If this is blocking your progress, you can always use the develop branch that has the most recent fixes applied to it until the package is updated.

NoelStephensUnity avatar Aug 16 '22 19:08 NoelStephensUnity