refactor: Remove unnecessary exceptions and refactor logging on NetworkSpawnManager and NetworkObject (#3933)

Refactor logging:
- Replace exceptions with log-based error handling
- Replace Debug with NetworkLog
- Move return calls outside log-level checks

Tests:
- Fix failures when running in sequence (NetworkManager singleton not updated)
- Update tests to expect logs instead of exceptions
- Add Netcode prefix to expected logs

Cleanup:
- Remove unreachable code, redundant branches, and unused comments
- General formatting and code cleanup

- Update changelog
- Address review feedback

Author: noellie-velez

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -13,16 +13,18 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
-- Improve performance of `NetworkBehaviour`. (#3915)
-- Improve performance of `NetworkTransform`. (#3907)
-- Improve performance of `NetworkRigidbodyBase`. (#3906)
-- Improve performance of `NetworkAnimator`. (#3905)
+- Replaced Debug usage by NetcodeLog on `NetworkSpawnManager` and `NetworkObject`. (#3933)
+- Improved performance of `NetworkBehaviour`. (#3915)
+- Improved performance of `NetworkTransform`. (#3907)
+- Improved performance of `NetworkRigidbodyBase`. (#3906)
+- Improved performance of `NetworkAnimator`. (#3905)
 
 ### Deprecated
 
 
 ### Removed
 
+- Removed un-needed exceptions on `NetworkSpawnManager`. (#3933)
 
 ### Fixed
 

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

@@ -297,7 +297,7 @@ internal void OnValidate()
                     if (globalId.identifierType != k_SceneObjectType)
                     {
                         // This should never happen, but in the event it does throw and error.
-                        Debug.LogError($"[{gameObject.name}] is detected as an in-scene placed object but its identifier is of type {globalId.identifierType}! **Report this error**");
+                        NetworkLog.LogError($"[{gameObject.name}] is detected as an in-scene placed object but its identifier is of type {globalId.identifierType}! **Report this error**");
                     }
 
                     // If this is a prefab instance, then we want to mark it as having been updated in order for the udpated GlobalObjectIdHash value to be saved.
@@ -1762,23 +1762,25 @@ internal void SpawnInternal(bool destroyWithScene, ulong ownerClientId, bool pla
                     {
                         NetworkLog.LogError($"[{name}] When distributed authority mode is enabled, you can only spawn NetworkObjects that belong to the local instance! Local instance id {NetworkManagerOwner.LocalClientId} is not the same as the assigned owner id: {ownerClientId}!");
                     }
-                    return;
                 }
                 else
                 {
                     if (NetworkManagerOwner.LogLevel <= LogLevel.Error)
                     {
                         NetworkLog.LogError($"[{name}] Only server can spawn {nameof(NetworkObject)}s.");
                     }
-                    return;
                 }
+                return;
             }
 
             if (NetworkManagerOwner.DistributedAuthorityMode)
             {
                 if (NetworkManagerOwner.LocalClient == null || !NetworkManagerOwner.IsConnectedClient || !NetworkManagerOwner.ConnectionManager.LocalClient.IsApproved)
                 {
-                    Debug.LogError($"Cannot spawn {name} until the client is fully connected to the session!");
+                    if (NetworkManagerOwner.LogLevel <= LogLevel.Error)
+                    {
+                        NetworkLog.LogError($"Cannot spawn {name} until the client is fully connected to the session!");
+                    }
                     return;
                 }
                 if (NetworkManagerOwner.NetworkConfig.EnableSceneManagement)
@@ -1834,7 +1836,10 @@ internal void SpawnInternal(bool destroyWithScene, ulong ownerClientId, bool pla
             }
             else
             {
-                NetworkLog.LogWarningServer($"[{name}] Ran into unknown conditional check during spawn when determining distributed authority mode or not");
+                if (NetworkManagerOwner.LogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarningServer($"[{name}] Ran into unknown conditional check during spawn when determining distributed authority mode or not");
+                }
             }
         }
 
@@ -1856,7 +1861,10 @@ public static NetworkObject InstantiateAndSpawn(GameObject networkPrefab, Networ
             var networkObject = networkPrefab.GetComponent<NetworkObject>();
             if (networkObject == null)
             {
-                Debug.LogError($"The {nameof(NetworkPrefab)} {networkPrefab.name} does not have a {nameof(NetworkObject)} component!");
+                if (networkManager.LogLevel <= LogLevel.Error)
+                {
+                    NetworkLog.LogError($"The {nameof(NetworkPrefab)} {networkPrefab.name} does not have a {nameof(NetworkObject)} component!");
+                }
                 return null;
             }
             return networkObject.InstantiateAndSpawn(networkManager, ownerClientId, destroyWithScene, isPlayerObject, forceOverride, position, rotation);
@@ -1878,34 +1886,49 @@ public NetworkObject InstantiateAndSpawn(NetworkManager networkManager, ulong ow
         {
             if (networkManager == null)
             {
-                Debug.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NetworkManagerNull]);
+                if (NetworkManager.LogLevel <= LogLevel.Error)
+                {
+                    NetworkLog.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NetworkManagerNull]);
+                }
                 return null;
             }
 
             if (!networkManager.IsListening)
             {
-                Debug.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NoActiveSession]);
+                if (networkManager.LogLevel <= LogLevel.Error)
+                {
+                    NetworkLog.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NoActiveSession]);
+                }
                 return null;
             }
 
             ownerClientId = networkManager.DistributedAuthorityMode ? networkManager.LocalClientId : ownerClientId;
             // We only need to check for authority when running in client-server mode
             if (!networkManager.IsServer && !networkManager.DistributedAuthorityMode)
             {
-                Debug.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NotAuthority]);
+                if (networkManager.LogLevel <= LogLevel.Error)
+                {
+                    NetworkLog.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NotAuthority]);
+                }
                 return null;
             }
 
             if (networkManager.ShutdownInProgress)
             {
-                Debug.LogWarning(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.InvokedWhenShuttingDown]);
+                if (networkManager.LogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarning(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.InvokedWhenShuttingDown]);
+                }
                 return null;
             }
 
             // Verify it is actually a valid prefab
             if (!networkManager.NetworkConfig.Prefabs.Contains(gameObject))
             {
-                Debug.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NotRegisteredNetworkPrefab]);
+                if (networkManager.LogLevel <= LogLevel.Error)
+                {
+                    NetworkLog.LogError(NetworkSpawnManager.InstantiateAndSpawnErrors[NetworkSpawnManager.InstantiateAndSpawnErrorTypes.NotRegisteredNetworkPrefab]);
+                }
                 return null;
             }
 
@@ -1961,7 +1984,6 @@ public void Despawn(bool destroy = true)
                 {
                     NetworkLog.LogErrorServer($"[{name}][Attempted despawn before {nameof(NetworkObject)} was spawned]");
                 }
-
                 return;
             }
 
@@ -2011,10 +2033,8 @@ public void ChangeOwnership(ulong newOwnerClientId)
                 {
                     NetworkLog.LogErrorServer($"[{name}][Attempted ownership change before {nameof(NetworkObject)} was spawned]");
                 }
-
                 return;
             }
-
             NetworkManagerOwner.SpawnManager.ChangeOwnership(this, newOwnerClientId, HasAuthority);
         }
 
@@ -2030,7 +2050,6 @@ internal void InvokeBehaviourOnOwnershipChanged(ulong originalOwnerClientId, ulo
                 {
                     NetworkLog.LogErrorServer($"[{name}][Attempted behavior invoke on ownership changed before {nameof(NetworkObject)} was spawned]");
                 }
-
                 return;
             }
 
@@ -2061,10 +2080,12 @@ internal void InvokeBehaviourOnOwnershipChanged(ulong originalOwnerClientId, ulo
                 {
                     if (!childBehaviour.gameObject.activeInHierarchy)
                     {
-                        Debug.LogWarning($"[{name}] {childBehaviour.gameObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {childBehaviour.GetType().Name} component was skipped during ownership assignment!");
+                        if (NetworkManagerOwner.LogLevel <= LogLevel.Normal)
+                        {
+                            NetworkLog.LogWarning($"[{name}] {childBehaviour.gameObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {childBehaviour.GetType().Name} component was skipped during ownership assignment!");
+                        }
                         continue;
                     }
-
                     childBehaviour.InternalOnGainedOwnership();
                 }
             }
@@ -2080,7 +2101,10 @@ internal void InvokeOwnershipChanged(ulong previous, ulong next)
                 }
                 else
                 {
-                    Debug.LogWarning($"[{name}] {ChildNetworkBehaviours[i].gameObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {ChildNetworkBehaviours[i].GetType().Name} component was skipped during ownership assignment!");
+                    if (NetworkManagerOwner.LogLevel <= LogLevel.Normal)
+                    {
+                        NetworkLog.LogWarning($"[{name}] {ChildNetworkBehaviours[i].gameObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {ChildNetworkBehaviours[i].GetType().Name} component was skipped during ownership assignment!");
+                    }
                 }
             }
         }
@@ -2271,7 +2295,7 @@ internal bool InternalTrySetParent(NetworkObject parent, bool worldPositionStays
         private void OnTransformParentChanged()
         {
             var networkManager = NetworkManager;
-            if (!AutoObjectParentSync || networkManager.ShutdownInProgress)
+            if (!AutoObjectParentSync || (networkManager != null && networkManager.ShutdownInProgress))
             {
                 return;
             }
@@ -2283,18 +2307,9 @@ private void OnTransformParentChanged()
 
             if (networkManager == null || !networkManager.IsListening)
             {
-                // DANGO-TODO: Review as to whether we want to provide a better way to handle changing parenting of objects when the
-                // object is not spawned. Really, we shouldn't care about these types of changes.
-                if (networkManager.DistributedAuthorityMode && m_CachedParent != null && transform.parent == null)
-                {
-                    m_CachedParent = null;
-                    return;
-                }
                 transform.parent = m_CachedParent;
-                if (NetworkManagerOwner.LogLevel <= LogLevel.Error)
-                {
-                    NetworkLog.LogError($"[{name}] {nameof(networkManager)} is not listening, start a server or host before re-parenting.");
-                }
+                // We want to log at any LogLevel, since we may not have a network manager may here.
+                NetworkLog.LogError($"[{name}] {nameof(networkManager)} is not listening, start a server or host before re-parenting.");
                 return;
             }
 
@@ -2311,7 +2326,7 @@ private void OnTransformParentChanged()
                 else
                 {
                     transform.parent = m_CachedParent;
-                    if (NetworkManagerOwner.LogLevel <= LogLevel.Error)
+                    if (networkManager.LogLevel <= LogLevel.Error)
                     {
                         NetworkLog.LogErrorServer($"[{name}] {nameof(NetworkObject)} can only be re-parented after being spawned!");
                     }
@@ -2348,17 +2363,17 @@ private void OnTransformParentChanged()
                 {
                     transform.parent = m_CachedParent;
                     AuthorityAppliedParenting = false;
-                    if (NetworkManagerOwner.LogLevel <= LogLevel.Error)
+                    if (networkManager.LogLevel <= LogLevel.Error)
                     {
                         NetworkLog.LogErrorServer($"[{name}] Invalid parenting, {nameof(NetworkObject)} moved under a non-{nameof(NetworkObject)} parent");
                     }
                     return;
                 }
-                else if (!parentObject.IsSpawned)
+                if (!parentObject.IsSpawned)
                 {
                     transform.parent = m_CachedParent;
                     AuthorityAppliedParenting = false;
-                    if (NetworkManagerOwner.LogLevel <= LogLevel.Error)
+                    if (networkManager.LogLevel <= LogLevel.Error)
                     {
                         NetworkLog.LogErrorServer($"[{name}] {nameof(NetworkObject)} can only be re-parented under another spawned {nameof(NetworkObject)}.");
                     }
@@ -2494,10 +2509,10 @@ internal bool ApplyNetworkParenting(bool removeParent = false, bool ignoreNotSpa
                 }
             }
 
-            // If we are removing the parent or our latest parent is not set, then remove the parent
+            // If we are removing the parent or our latest parent is not set, then remove the parent.
             // removeParent is only set when:
             //  - The server-side NetworkObject.OnTransformParentChanged is invoked and the parent is being removed
-            //  - The client-side when handling a ParentSyncMessage
+            //  - The client-side is handling a ParentSyncMessage
             // When clients are synchronizing only the m_LatestParent.HasValue will not have a value if there is no parent
             // or a parent was removed prior to the client connecting (i.e. in-scene placed NetworkObjects)
             if (removeParent || !m_LatestParent.HasValue)
@@ -2597,7 +2612,10 @@ internal void InvokeBehaviourNetworkSpawn()
             {
                 if (!childBehaviour.gameObject.activeInHierarchy)
                 {
-                    Debug.LogWarning($"{GenerateDisabledNetworkBehaviourWarning(childBehaviour)}");
+                    if (NetworkManager.LogLevel <= LogLevel.Normal)
+                    {
+                        NetworkLog.LogWarning($"{GenerateDisabledNetworkBehaviourWarning(childBehaviour)}");
+                    }
                     continue;
                 }
                 childBehaviour.NetworkSpawn();
@@ -2709,7 +2727,6 @@ internal List<NetworkBehaviour> ChildNetworkBehaviours
                     }
 #endif
                 }
-
                 return m_ChildNetworkBehaviours;
             }
         }
@@ -2876,9 +2893,7 @@ internal struct SerializedObject
             public bool HasParent;
             public bool IsSceneObject;
             public bool HasTransform;
-
             public bool IsLatestParentSet;
-
             public bool WorldPositionStays;
 
             /// <summary>
@@ -2888,15 +2903,10 @@ internal struct SerializedObject
             /// to the current active scene when its scene is unloaded. (only for dynamically spawned)
             /// </summary>
             public bool DestroyWithScene;
-
             public bool DontDestroyWithOwner;
-
             public bool HasOwnershipFlags;
-
             public bool SyncObservers;
-
             public bool SpawnWithObservers;
-
             public bool HasInstantiationData;
 
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
@@ -2988,9 +2998,6 @@ public struct TransformData : INetworkSerializeByMemcpy
 
             public TransformData Transform;
 
-            //If(Metadata.IsReparented)
-
-            //If(IsLatestParentSet)
             public ulong? LatestParent;
 
             public NetworkObject OwnerObject;
@@ -3310,11 +3317,8 @@ internal static NetworkObject Deserialize(in SerializedObject serializedObject,
                 reader.ReadValueSafe(out instantiationData);
             }
 
-
             // Attempt to create a local NetworkObject
             var networkObject = networkManager.SpawnManager.CreateLocalNetworkObject(serializedObject, instantiationData);
-
-
             if (networkObject == null)
             {
                 // Log the error that the NetworkObject failed to construct
@@ -3336,7 +3340,6 @@ internal static NetworkObject Deserialize(in SerializedObject serializedObject,
                 // We have nothing left to do here.
                 return null;
             }
-
             networkObject.NetworkManagerOwner = networkManager;
 
             // This will get set again when the NetworkObject is spawned locally, but we set it here ahead of spawning
@@ -3625,17 +3628,13 @@ internal uint CheckForGlobalObjectIdHashOverride()
                     {
                         return PrefabGlobalObjectIdHash;
                     }
-                    else
+                    // For legacy manual instantiation and spawning, check the OverrideToNetworkPrefab for a possible match
+                    if (networkManager.NetworkConfig.Prefabs.OverrideToNetworkPrefab.TryGetValue(GlobalObjectIdHash, out var overrideHash))
                     {
-                        // For legacy manual instantiation and spawning, check the OverrideToNetworkPrefab for a possible match
-                        if (networkManager.NetworkConfig.Prefabs.OverrideToNetworkPrefab.ContainsKey(GlobalObjectIdHash))
-                        {
-                            return networkManager.NetworkConfig.Prefabs.OverrideToNetworkPrefab[GlobalObjectIdHash];
-                        }
+                        return overrideHash;
                     }
                 }
             }
-
             return GlobalObjectIdHash;
         }