fix: Player removed from observers when player object despawns (#3110)

* fix

Remove legacy observer removal code.

* update

adding change log entry.

* fix

Assure owner is assigned as an observer to the player.
Make sure observers are not removed from despawned players.

* fix

Make sure to take spawn with observers into account when assigning the OwnerClientId as an observer.

* test

adding validation test for the fix.

* update

Adding change log PR numbers to the two entries.
(left one out in PR-3108)

* test-fix

Fixing weird issue with the PlayerSpawnDespawn and ghost NetworkManagers.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -12,7 +12,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
-- Fixed issue where `NetworkAnimator` would statically allocate write buffer space for `Animator` parameters that could cause a write error if the number of parameters exceeded the space allocated.
+- Fixed issue where client is removed as an observer from spawned objects when their player instance is despawned. (#3110)
+- Fixed issue where `NetworkAnimator` would statically allocate write buffer space for `Animator` parameters that could cause a write error if the number of parameters exceeded the space allocated. (#3108)
 
 ### Changed
 

com.unity.netcode.gameobjects/Runtime/Components/NetworkAnimator.cs

@@ -306,10 +306,6 @@ private void ParseStateMachineStates(int layerIndex, ref AnimatorController anim
         private void BuildTransitionStateInfoList()
         {
 #if UNITY_EDITOR
-            if (UnityEditor.EditorApplication.isUpdating || UnityEditor.EditorApplication.isPlayingOrWillChangePlaymode)
-            {
-                return;
-            }
             if (m_Animator == null)
             {
                 return;
@@ -1268,10 +1264,11 @@ internal void UpdateAnimationState(AnimationState animationState)
                         NetworkLog.LogError($"[DestinationState To Transition Info] Layer ({animationState.Layer}) sub-table does not contain destination state ({animationState.DestinationStateHash})!");
                     }
                 }
-                else if (NetworkManager.LogLevel == LogLevel.Developer)
-                {
-                    NetworkLog.LogError($"[DestinationState To Transition Info] Layer ({animationState.Layer}) does not exist!");
-                }
+                // For reference, it is valid to have no transition information 
+                //else if (NetworkManager.LogLevel == LogLevel.Developer)
+                //{
+                //    NetworkLog.LogError($"[DestinationState To Transition Info] Layer ({animationState.Layer}) does not exist!");
+                //}
             }
             else if (animationState.Transition && animationState.CrossFade)
             {

com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

@@ -88,6 +88,13 @@ private void AddPlayerObject(NetworkObject playerObject)
                 }
             }
 
+            // Only if spawn with observers is set or we are using a distributed authority network topology and this is the client's player should we add
+            // the owner as an observer.
+            if (playerObject.SpawnWithObservers || (NetworkManager.DistributedAuthorityMode && NetworkManager.LocalClientId == playerObject.OwnerClientId))
+            {
+                playerObject.Observers.Add(playerObject.OwnerClientId);
+            }
+
             m_PlayerObjects.Add(playerObject);
             if (!m_PlayerObjectsTable.ContainsKey(playerObject.OwnerClientId))
             {
@@ -110,8 +117,9 @@ internal void UpdateNetworkClientPlayer(NetworkObject playerObject)
             if (playerNetworkClient.PlayerObject != null && m_PlayerObjects.Contains(playerNetworkClient.PlayerObject))
             {
                 // Just remove the previous player object but keep the assigned observers of the NetworkObject
-                RemovePlayerObject(playerNetworkClient.PlayerObject, true);
+                RemovePlayerObject(playerNetworkClient.PlayerObject);
             }
+
             // Now update the associated NetworkClient's player object
             NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId].AssignPlayerObject(ref playerObject);
             AddPlayerObject(playerObject);
@@ -120,7 +128,7 @@ internal void UpdateNetworkClientPlayer(NetworkObject playerObject)
         /// <summary>
         /// Removes a player object and updates all other players' observers list
         /// </summary>
-        private void RemovePlayerObject(NetworkObject playerObject, bool keepObservers = false)
+        private void RemovePlayerObject(NetworkObject playerObject, bool destroyingObject = false)
         {
             if (!playerObject.IsPlayerObject)
             {
@@ -141,16 +149,21 @@ private void RemovePlayerObject(NetworkObject playerObject, bool keepObservers =
                 }
             }
 
-            // If we want to keep the observers, then exit early
-            if (keepObservers)
+            if (NetworkManager.ConnectionManager.ConnectedClients.ContainsKey(playerObject.OwnerClientId) && destroyingObject)
             {
-                return;
+                NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId].PlayerObject = null;
             }
 
-            foreach (var player in m_PlayerObjects)
-            {
-                player.Observers.Remove(playerObject.OwnerClientId);
-            }
+            // If we want to keep the observers, then exit early
+            //if (keepObservers)
+            //{
+            //    return;
+            //}
+
+            //foreach (var player in m_PlayerObjects)
+            //{
+            //    player.Observers.Remove(playerObject.OwnerClientId);
+            //}
         }
 
         internal void MarkObjectForShowingTo(NetworkObject networkObject, ulong clientId)
@@ -1550,23 +1563,9 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec
                 SpawnedObjectsList.Remove(networkObject);
             }
 
-            // DANGO-TODO: When we fix the issue with observers not being applied to NetworkObjects,
-            // (client connect/disconnect) we can remove this hacky way of doing this.
-            // Basically, when a player disconnects and/or is destroyed they are removed as an observer from all other client
-            // NetworkOject instances.
-            if (networkObject.IsPlayerObject && !networkObject.IsOwner && networkObject.OwnerClientId != NetworkManager.LocalClientId)
-            {
-                foreach (var netObject in SpawnedObjects)
-                {
-                    if (netObject.Value.Observers.Contains(networkObject.OwnerClientId))
-                    {
-                        netObject.Value.Observers.Remove(networkObject.OwnerClientId);
-                    }
-                }
-            }
             if (networkObject.IsPlayerObject)
             {
-                RemovePlayerObject(networkObject);
+                RemovePlayerObject(networkObject, destroyGameObject);
             }
 
             // Always clear out the observers list when despawned

com.unity.netcode.gameobjects/Tests/Runtime/PlayerObjectTests.cs

@@ -1,4 +1,7 @@
 using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine;
@@ -184,4 +187,150 @@ public IEnumerator PlayerSpawnPosition()
             }
         }
     }
+
+    /// <summary>
+    /// This test validates that when a player is spawned it has all observers
+    /// properly set and the spawned and player object lists are properly populated.
+    /// It also validates that when a player is despawned and the client is still connected
+    /// that the client maintains its observers for other players.
+    /// </summary>
+    [TestFixture(HostOrServer.DAHost)]
+    [TestFixture(HostOrServer.Host)]
+    [TestFixture(HostOrServer.Server)]
+    internal class PlayerSpawnAndDespawnTests : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 4;
+
+        private StringBuilder m_ErrorLog = new StringBuilder();
+
+        public PlayerSpawnAndDespawnTests(HostOrServer hostOrServer) : base(hostOrServer) { }
+
+        private bool ValidateObservers(ulong playerClientId, NetworkObject player, ref List<NetworkManager> networkManagers)
+        {
+            foreach (var networkManager in networkManagers)
+            {
+                if (player != null && player.IsSpawned)
+                {
+                    if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(player.NetworkObjectId))
+                    {
+                        m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} does not contain a spawned object entry {nameof(NetworkObject)}-{player.NetworkObjectId} for Client-{playerClientId}!");
+                        return false;
+                    }
+
+                    var playerClone = networkManager.SpawnManager.SpawnedObjects[player.NetworkObjectId];
+                    foreach (var clientId in networkManager.ConnectedClientsIds)
+                    {
+                        if (!playerClone.IsNetworkVisibleTo(clientId))
+                        {
+                            m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} failed visibility check for Client-{clientId} on {nameof(NetworkObject)}-{player.NetworkObjectId} for Client-{playerClientId}!");
+                            return false;
+                        }
+                    }
+
+                    var foundPlayerClone = (NetworkObject)null;
+                    foreach (var playerObject in networkManager.SpawnManager.PlayerObjects)
+                    {
+                        if (playerObject.OwnerClientId == playerClientId && playerObject.NetworkObjectId == player.NetworkObjectId)
+                        {
+                            foundPlayerClone = playerObject;
+                            break;
+                        }
+                    }
+                    if (!foundPlayerClone)
+                    {
+                        m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} does not contain a player entry for {nameof(NetworkObject)}-{player.NetworkObjectId} for Client-{playerClientId}!");
+                        return false;
+                    }
+
+                }
+                else
+                {
+                    // If the player client in question is despawned, then no NetworkManager instance
+                    // should contain a clone of that (or the client's NetworkManager instance as well)
+                    foreach (var playerClone in networkManager.SpawnManager.PlayerObjects)
+                    {
+                        if (playerClone.OwnerClientId == playerClientId)
+                        {
+                            m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} contains a player {nameof(NetworkObject)}-{playerClone.NetworkObjectId} for Client-{playerClientId} when it should be despawned!");
+                            return false;
+                        }
+                    }
+                }
+            }
+            return true;
+        }
+
+        private bool ValidateAllClientPlayerObservers()
+        {
+            var networkManagers = new List<NetworkManager>();
+
+            if (m_ServerNetworkManager.IsHost)
+            {
+                networkManagers.Add(m_ServerNetworkManager);
+            }
+            foreach (var networkManager in m_ClientNetworkManagers)
+            {
+                networkManagers.Add(networkManager);
+            }
+
+            m_ErrorLog.Clear();
+            var success = true;
+            foreach (var networkManager in networkManagers)
+            {
+                var spawnedOrNot = networkManager.LocalClient.PlayerObject == null ? "despawned" : "spawned";
+                m_ErrorLog.AppendLine($"Validating Client-{networkManager.LocalClientId} {spawnedOrNot} player.");
+                if (networkManager.LocalClient == null)
+                {
+                    m_ErrorLog.AppendLine($"No {nameof(NetworkClient)} found for Client-{networkManager.LocalClientId}!");
+                    success = false;
+                    break;
+                }
+                if (!ValidateObservers(networkManager.LocalClientId, networkManager.LocalClient.PlayerObject, ref networkManagers))
+                {
+                    m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} validation pass failed.");
+                    success = false;
+                    break;
+                }
+            }
+            networkManagers.Clear();
+            return success;
+        }
+
+
+        [UnityTest]
+        public IEnumerator PlayerSpawnDespawn()
+        {
+            // Validate all observers are properly set with all players spawned
+            yield return WaitForConditionOrTimeOut(ValidateAllClientPlayerObservers);
+            AssertOnTimeout($"First Validation Failed:\n {m_ErrorLog}");
+            var selectedClient = m_ClientNetworkManagers[Random.Range(0, m_ClientNetworkManagers.Count() - 1)];
+            var playerSelected = selectedClient.LocalClient.PlayerObject;
+
+            if (m_DistributedAuthority)
+            {
+                playerSelected.Despawn(false);
+            }
+            else
+            {
+                m_ServerNetworkManager.SpawnManager.SpawnedObjects[playerSelected.NetworkObjectId].Despawn(true);
+            }
+
+            // Validate all observers are properly set with one of the players despawned
+            yield return WaitForConditionOrTimeOut(ValidateAllClientPlayerObservers);
+            AssertOnTimeout($"Second Validation Failed:\n {m_ErrorLog}");
+
+            if (m_DistributedAuthority)
+            {
+                playerSelected.SpawnAsPlayerObject(selectedClient.LocalClientId, false);
+            }
+            else
+            {
+                SpawnPlayerObject(m_ServerNetworkManager.NetworkConfig.PlayerPrefab, selectedClient);
+            }
+
+            // Validate all observers are properly set when the client's player is respawned.
+            yield return WaitForConditionOrTimeOut(ValidateAllClientPlayerObservers);
+            AssertOnTimeout($"Third Validation Failed:\n {m_ErrorLog}");
+        }
+    }
 }