diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index ed55793aba..573e77b99b 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -13,6 +13,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Changed +- Remove exceptions and replaced Debug usage by NetcodeLog on `NetworkSpawnManager`. (#3933) - Improve performance of `NetworkBehaviour`. (#3915) - Improve performance of `NetworkTransform`. (#3907) - Improve performance of `NetworkRigidbodyBase`. (#3906) @@ -26,6 +27,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Early return when `ChangeOwnership` is called while not spawned. (#3933) - Fixed issue where attempts to use `NetworkLog` when there is no `NetworkManager` instance would result in an exception. (#3917) ### Security diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index bf4eacbc1b..928133e5f0 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -75,11 +75,11 @@ private void AddPlayerObject(NetworkObject playerObject) { if (!playerObject.IsPlayerObject) { - if (NetworkManager.LogLevel == LogLevel.Normal) + if (NetworkManager.LogLevel <= LogLevel.Normal) { NetworkLog.LogError($"Attempting to register a {nameof(NetworkObject)} as a player object but {nameof(NetworkObject.IsPlayerObject)} is not set!"); - return; } + return; } var cmbService = NetworkManager.CMBServiceConnection; @@ -148,11 +148,11 @@ private void RemovePlayerObject(NetworkObject playerObject, bool destroyingObjec { if (!playerObject.IsPlayerObject) { - if (NetworkManager.LogLevel == LogLevel.Normal) + if (NetworkManager.LogLevel <= LogLevel.Normal) { NetworkLog.LogError($"Attempting to deregister a {nameof(NetworkObject)} as a player object but {nameof(NetworkObject.IsPlayerObject)} is not set!"); - return; } + return; } playerObject.IsPlayerObject = false; m_PlayerObjects.Remove(playerObject); @@ -211,8 +211,10 @@ internal bool RemoveObjectFromShowingTo(NetworkObject networkObject, ulong clien // probably overkill, but deals with multiple entries while (ObjectsToShowToClient[clientId].Contains(networkObject)) { - Debug.LogWarning( - "Object was shown and hidden from the same client in the same Network frame. As a result, the client will _not_ receive a NetworkSpawn"); + if (NetworkManager.LogLevel > LogLevel.Normal) + { + NetworkLog.LogWarning($"Object was shown and hidden from the same client in the same Network frame. As a result, the client will _not_ receive a NetworkSpawn"); + } ObjectsToShowToClient[clientId].Remove(networkObject); ret = true; } @@ -277,7 +279,11 @@ internal void UpdateOwnershipTable(NetworkObject networkObject, ulong newOwner, else { // Really, as long as UpdateOwnershipTable is invoked when ownership is gained or lost this should never happen - throw new Exception($"Client-ID {previousOwner} had a partial {nameof(m_ObjectToOwnershipTable)} entry! Potentially corrupted {nameof(OwnershipToObjectsTable)}?"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"Client-ID {previousOwner} had a partial {nameof(m_ObjectToOwnershipTable)} entry! Potentially corrupted {nameof(OwnershipToObjectsTable)}?"); + } + return; } } @@ -375,7 +381,11 @@ public NetworkObject GetPlayerNetworkObject(ulong clientId) { if (!NetworkManager.IsServer && NetworkManager.LocalClientId != clientId) { - throw new NotServerException("Only the server can find player objects from other clients."); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogErrorServer($"{clientId} Only the server can find player objects from other clients."); + } + return null; } if (TryGetNetworkClient(clientId, out NetworkClient networkClient)) { @@ -389,7 +399,6 @@ public NetworkObject GetPlayerNetworkObject(ulong clientId) return m_PlayerObjectsTable[clientId].First(); } } - return null; } @@ -432,7 +441,10 @@ internal void RemoveOwnership(NetworkObject networkObject) { if (NetworkManager.DistributedAuthorityMode && !NetworkManager.ShutdownInProgress) { - Debug.LogError($"Removing ownership is invalid in Distributed Authority Mode. Use {nameof(ChangeOwnership)} instead."); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"Removing ownership is invalid in Distributed Authority Mode. Use {nameof(ChangeOwnership)} instead."); + } return; } ChangeOwnership(networkObject, NetworkManager.ServerClientId, true); @@ -443,11 +455,20 @@ internal void RemoveOwnership(NetworkObject networkObject) internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool isAuthorized, bool isRequestApproval = false) { + if (!networkObject.IsSpawned) + { + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogErrorServer($"[{networkObject.name}] Cannot change ownership while not spawned."); + } + return; + } + if (clientId == networkObject.OwnerClientId) { if (NetworkManager.LogLevel <= LogLevel.Developer) { - Debug.LogWarning($"[{nameof(NetworkSpawnManager)}][{nameof(ChangeOwnership)}] Attempting to change ownership to Client-{clientId} when the owner is already {networkObject.OwnerClientId}! (Ignoring)"); + NetworkLog.LogWarning($"[{nameof(NetworkSpawnManager)}][{nameof(ChangeOwnership)}] Attempting to change ownership to Client-{clientId} when the owner is already {networkObject.OwnerClientId}! (Ignoring)"); } return; } @@ -526,17 +547,16 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool } else if (!isAuthorized) { - throw new NotServerException("Only the server can change ownership"); - } - - if (!networkObject.IsSpawned) - { - throw new SpawnStateException("Object is not spawned"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError("Only the server can change ownership! (ignoring)"); + } + return; } if (!networkObject.Observers.Contains(clientId)) { - if (NetworkManager.LogLevel == LogLevel.Developer) + if (NetworkManager.LogLevel <= LogLevel.Developer) { NetworkLog.LogWarningServer($"[Invalid Owner] Cannot send Ownership change as client-{clientId} cannot see {networkObject.name}! Use {nameof(NetworkObject.NetworkShow)} first."); } @@ -737,7 +757,10 @@ public NetworkObject InstantiateAndSpawn(NetworkObject networkPrefab, ulong owne { if (networkPrefab == null) { - Debug.LogError(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.NetworkPrefabNull]); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.NetworkPrefabNull]); + } return null; } @@ -745,20 +768,29 @@ public NetworkObject InstantiateAndSpawn(NetworkObject networkPrefab, ulong owne // We only need to check for authority when running in client-server mode if (!NetworkManager.IsServer && !NetworkManager.DistributedAuthorityMode) { - Debug.LogError(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.NotAuthority]); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.NotAuthority]); + } return null; } if (NetworkManager.ShutdownInProgress) { - Debug.LogWarning(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.InvokedWhenShuttingDown]); + if (NetworkManager.LogLevel <= LogLevel.Normal) + { + NetworkLog.LogWarning(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.InvokedWhenShuttingDown]); + } return null; } // Verify it is actually a valid prefab if (!NetworkManager.NetworkConfig.Prefabs.Contains(networkPrefab.gameObject)) { - Debug.LogError(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.NotRegisteredNetworkPrefab]); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError(InstantiateAndSpawnErrors[InstantiateAndSpawnErrorTypes.NotRegisteredNetworkPrefab]); + } return null; } @@ -787,7 +819,10 @@ internal NetworkObject InstantiateAndSpawnNoParameterChecks(NetworkObject networ if (networkObject == null) { - Debug.LogError($"Failed to instantiate and spawn {networkPrefab.name}!"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"Failed to instantiate and spawn {networkPrefab.name}!"); + } return null; } @@ -1043,7 +1078,10 @@ internal void AuthorityLocalSpawn([NotNull] NetworkObject networkObject, ulong n { if (networkObject.IsSpawned) { - Debug.LogError($"{networkObject.name} is already spawned!"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"{networkObject.name} is already spawned!"); + } return; } @@ -1052,7 +1090,10 @@ internal void AuthorityLocalSpawn([NotNull] NetworkObject networkObject, ulong n var networkObjectChildren = networkObject.GetComponentsInChildren(); if (networkObjectChildren.Length > 1) { - Debug.LogError("Spawning NetworkObjects with nested NetworkObjects is only supported for scene objects. Child NetworkObjects will not be spawned over the network!"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError("Spawning NetworkObjects with nested NetworkObjects is only supported for scene objects. Child NetworkObjects will not be spawned over the network!"); + } } } @@ -1095,7 +1136,10 @@ internal void AuthorityLocalSpawn([NotNull] NetworkObject networkObject, ulong n // Itentionally checking as opposed to just assigning in order to generate notification. if (!networkObject.Observers.Contains(ownerClientId)) { - Debug.LogError($"Client-{ownerClientId} is the owner of {networkObject.name} but is not an observer! Adding owner, but there is a bug in observer synchronization!"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"Client-{ownerClientId} is the owner of {networkObject.name} but is not an observer! Adding owner, but there is a bug in observer synchronization!"); + } networkObject.AddObserver(ownerClientId); } } @@ -1121,7 +1165,10 @@ internal void NonAuthorityLocalSpawn([NotNull] NetworkObject networkObject, in N { if (networkObject.IsSpawned) { - Debug.LogError($"[{networkObject.name}] Object-{networkObject.NetworkObjectId} is already spawned!"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"[{networkObject.name}] Object-{networkObject.NetworkObjectId} is already spawned!"); + } return; } @@ -1139,12 +1186,18 @@ internal void SpawnNetworkObjectLocallyCommon(NetworkObject networkObject, ulong { if (networkObject.NetworkManagerOwner == null) { - Debug.LogError("NetworkManagerOwner should not be null!"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"{networkObject.name}'s NetworkManagerOwner should not be null!"); + } } if (SpawnedObjects.ContainsKey(networkId)) { - Debug.LogWarning($"[{NetworkManager.name}] Trying to spawn {networkObject.name} with a {nameof(NetworkObject.NetworkObjectId)} of {networkId} but it is already in the spawned list!"); + if (NetworkManager.LogLevel <= LogLevel.Normal) + { + NetworkLog.LogWarning($"[{NetworkManager.name}] Trying to spawn {networkObject.name} with a {nameof(NetworkObject.NetworkObjectId)} of {networkId} but it is already in the spawned list!"); + } return; } @@ -1311,7 +1364,11 @@ internal void SendSpawnCallForObserverUpdate(ulong[] newObservers, NetworkObject { if (!NetworkManager.DistributedAuthorityMode) { - throw new Exception("[SendSpawnCallForObserverUpdate] Invoking a distributed authority only method when distributed authority is not enabled!"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogErrorServer($"[SendSpawnCallForObserverUpdate] Invoking a distributed authority only method on {networkObject.name} when distributed authority is not enabled!"); + } + return; } var message = new CreateObjectMessage @@ -1352,7 +1409,10 @@ internal void DespawnObject(NetworkObject networkObject, bool destroyObject = fa { if (!NetworkManager.IsServer && !NetworkManager.DistributedAuthorityMode) { - NetworkLog.LogErrorServer("Only server can despawn objects"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogErrorServer("Only server can despawn objects"); + } return; } @@ -1360,7 +1420,10 @@ internal void DespawnObject(NetworkObject networkObject, bool destroyObject = fa { if (!NetworkManager.DAHost || NetworkManager.DAHost && !authorityOverride) { - NetworkLog.LogErrorServer($"In distributed authority mode, only the owner of the NetworkObject can despawn it! Local Client is ({NetworkManager.LocalClientId}) while the owner is ({networkObject.OwnerClientId})"); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogErrorServer($"In distributed authority mode, only the owner of the NetworkObject can despawn it! Local Client is ({NetworkManager.LocalClientId}) while the owner is ({networkObject.OwnerClientId})"); + } return; } } @@ -1536,7 +1599,10 @@ internal void OnDespawnNonAuthorityObject([NotNull] NetworkObject networkObject, { if (networkObject.HasAuthority) { - NetworkLog.LogError($"OnDespawnNonAuthorityObject called on object {networkObject.NetworkObjectId} when is current client {NetworkManager.LocalClientId} has authority on this object."); + if (NetworkManager.LogLevel <= LogLevel.Error) + { + NetworkLog.LogError($"[OnDespawnNonAuthorityObject] called on object {networkObject.NetworkObjectId} when its current client {NetworkManager.LocalClientId} has authority on this object."); + } } if (networkObject.IsSceneObject == false) @@ -1564,7 +1630,10 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec // We have to do this check first as subsequent checks assume we can access NetworkObjectId. if (!networkObject) { - NetworkLog.LogWarning("Trying to destroy network object but it is null"); + if (NetworkManager.LogLevel <= LogLevel.Normal) + { + NetworkLog.LogWarning("Trying to destroy network object but it is null"); + } return; } @@ -1573,7 +1642,10 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec { if (!NetworkManager.ShutdownInProgress && !NetworkManager.SceneManager.IsSceneEventInProgress()) { - NetworkLog.LogWarning($"Trying to destroy object {networkObject.NetworkObjectId} but it doesn't seem to exist anymore!"); + if (NetworkManager.LogLevel <= LogLevel.Normal) + { + NetworkLog.LogWarning($"Trying to destroy object {networkObject.NetworkObjectId} but it doesn't seem to exist anymore!"); + } } return; } @@ -1587,7 +1659,10 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec { if (destroyGameObject && networkObject.IsSceneObject == true && !NetworkManager.SceneManager.IsSceneUnloading(networkObject)) { - NetworkLog.LogWarning("Destroying in-scene network objects can lead to unexpected behavior. It is recommended to use NetworkObject.Despawn(false) instead."); + if (NetworkManager.LogLevel <= LogLevel.Normal) + { + NetworkLog.LogWarning("Destroying in-scene network objects can lead to unexpected behavior. It is recommended to use NetworkObject.Despawn(false) instead."); + } } // Get all child NetworkObjects @@ -1771,9 +1846,9 @@ internal void HandleNetworkObjectShow(bool forceSend = false) return; } - // In distributed authority mode, we send a single message that is broadcasted to all clients - // that will be shown the object (i.e. 1 message to service that then broadcasts that to the - // targeted clients). When using a DAHost, we skip this and send like we do in client-server + // In distributed authority mode, we send a single message to the service, + // which then broadcasts it to all clients that should see the object. + // When using a DAHost, we skip this and send using the client-server approach. if (isDistributedAuthorityClient) { var behaviourUpdater = NetworkManager.BehaviourUpdater; @@ -1781,6 +1856,8 @@ internal void HandleNetworkObjectShow(bool forceSend = false) { if (entry.Key != null && entry.Key.IsSpawned) { + // Try catch due to ensure that if anything throws we keep processing the list. + // This can throw if the GameObject was destroyed on this frame. try { // Always push the most recent deltas when showing a NetworkObject @@ -1810,6 +1887,8 @@ internal void HandleNetworkObjectShow(bool forceSend = false) { if (networkObject != null && networkObject.IsSpawned) { + // Try catch due to ensure that if anything throws we keep processing the list. + // This can throw if the GameObject was destroyed on this frame. try { if (forceSend) @@ -2030,7 +2109,10 @@ internal void DistributeNetworkObjects(ulong clientId) } if (!child.IsOwnershipDistributable || !child.IsOwnershipTransferable) { - NetworkLog.LogWarning($"Sibling {child.name} of root parent {ownerList.Value[i].name} is neither transferable or distributable! Object distribution skipped and could lead to a potentially un-owned or owner-mismatched {nameof(NetworkObject)}!"); + if (NetworkManager.LogLevel <= LogLevel.Normal) + { + NetworkLog.LogWarning($"Sibling {child.name} of root parent {ownerList.Value[i].name} is neither transferable or distributable! Object distribution skipped and could lead to a potentially un-owned or owner-mismatched {nameof(NetworkObject)}!"); + } continue; } // Transfer ownership of all distributable =or= transferable children with the same owner to the same client to preserve the sibling ownership tree. @@ -2194,12 +2276,6 @@ internal void NotifyNetworkObjectsSynchronized() /// internal void ShowHiddenObjectsToNewlyJoinedClient(ulong newClientId) { - if (NetworkManager == null || NetworkManager.ShutdownInProgress && NetworkManager.LogLevel <= LogLevel.Developer) - { - Debug.LogWarning($"[Internal Error] {nameof(ShowHiddenObjectsToNewlyJoinedClient)} invoked while shutdown is in progress!"); - return; - } - if (!NetworkManager.DistributedAuthorityMode) { Debug.LogError($"[Internal Error] {nameof(ShowHiddenObjectsToNewlyJoinedClient)} should only be invoked when using a distributed authority network topology!"); @@ -2222,7 +2298,7 @@ internal void ShowHiddenObjectsToNewlyJoinedClient(ulong newClientId) if (NetworkManager.LogLevel <= LogLevel.Developer) { // Track if there is some other location where the client is being added to the observers list when the object is hidden from the session owner - Debug.LogWarning($"[{networkObject.name}] Has new client as an observer but it is hidden from the session owner!"); + NetworkLog.LogWarning($"[{networkObject.name}] Has new client as an observer but it is hidden from the session owner!"); } // For now, remove the client (impossible for the new client to have an instance since the session owner doesn't) to make sure newly added // code to handle this edge case works. @@ -2235,12 +2311,6 @@ internal void ShowHiddenObjectsToNewlyJoinedClient(ulong newClientId) internal void SynchronizeObjectsToNewlyJoinedClient(ulong newClientId) { - if (NetworkManager == null || NetworkManager.ShutdownInProgress && NetworkManager.LogLevel <= LogLevel.Developer) - { - Debug.LogWarning($"[Internal Error] {nameof(SynchronizeObjectsToNewlyJoinedClient)} invoked while shutdown is in progress!"); - return; - } - if (!NetworkManager.DistributedAuthorityMode) { Debug.LogError($"[Internal Error] {nameof(SynchronizeObjectsToNewlyJoinedClient)} should only be invoked when using a distributed authority network topology!"); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkSpawnManagerTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkSpawnManagerTests.cs index 3039ea298c..a0d51696bb 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkSpawnManagerTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkSpawnManagerTests.cs @@ -61,10 +61,9 @@ public void TestClientCantAccessServerPlayer() return; } // client can't access server player - Assert.Throws(() => - { - m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(serverSideClientId); - }); + string expectedLog = $"[Netcode-Server Sender=0] {serverSideClientId} Only the server can find player objects from other clients."; + LogAssert.Expect(UnityEngine.LogType.Error, expectedLog); + m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(serverSideClientId); } [Test] @@ -100,10 +99,9 @@ public void TestClientCantAccessOtherPlayer() } // client can't access other player - Assert.Throws(() => - { - m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(otherClientSideClientId); - }); + string expectedLog = $"[Netcode-Server Sender=0] {otherClientSideClientId} Only the server can find player objects from other clients."; + LogAssert.Expect(UnityEngine.LogType.Error, expectedLog); + m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(otherClientSideClientId); } [Test]