diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index d3417214a5..abc7731841 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed `ChangeOwnership` changing ownership to clients that are not observers. This also happened with automated object distribution. (#3323) - Fixed issue where `AnticipatedNetworkVariable` previous value returned by `AnticipatedNetworkVariable.OnAuthoritativeValueChanged` is updated correctly on the non-authoritative side. (#3306) - Fixed `OnClientConnectedCallback` passing incorrect `clientId` when scene management is disabled. (#3312) - Fixed issue where the `NetworkObject.Ownership` custom editor did not take the default "Everything" flag into consideration. (#3305) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index fd418d34c2..f4f44301ae 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -1122,18 +1122,16 @@ internal void OnClientDisconnectFromServer(ulong clientId) { if (!playerObject.DontDestroyWithOwner) { - // DANGO-TODO: This is something that would be best for CMB Service to handle as it is part of the disconnection process // If a player NetworkObject is being despawned, make sure to remove all children if they are marked to not be destroyed // with the owner. - if (NetworkManager.DistributedAuthorityMode && NetworkManager.DAHost) + if (NetworkManager.DAHost) { // Remove any children from the player object if they are not going to be destroyed with the owner var childNetworkObjects = playerObject.GetComponentsInChildren(); foreach (var child in childNetworkObjects) { - // TODO: We have always just removed all children, but we might think about changing this to preserve the nested child - // hierarchy. - if (child.DontDestroyWithOwner && child.transform.transform.parent != null) + // TODO: We have always just removed all children, but we might think about changing this to preserve the nested child hierarchy. + if (child.DontDestroyWithOwner && child.transform.transform.parent) { // If we are here, then we are running in DAHost mode and have the authority to remove the child from its parent child.AuthorityAppliedParenting = true; @@ -1158,10 +1156,7 @@ internal void OnClientDisconnectFromServer(ulong clientId) } else if (!NetworkManager.ShutdownInProgress) { - if (!NetworkManager.ShutdownInProgress) - { - playerObject.RemoveOwnership(); - } + playerObject.RemoveOwnership(); } } @@ -1175,7 +1170,7 @@ internal void OnClientDisconnectFromServer(ulong clientId) for (int i = clientOwnedObjects.Count - 1; i >= 0; i--) { var ownedObject = clientOwnedObjects[i]; - if (ownedObject != null) + if (ownedObject) { // If destroying with owner, then always despawn and destroy (or defer destroying to prefab handler) if (!ownedObject.DontDestroyWithOwner) @@ -1196,68 +1191,93 @@ internal void OnClientDisconnectFromServer(ulong clientId) } else if (!NetworkManager.ShutdownInProgress) { + // DANGO-TODO: We will want to match how the CMB service handles this. For now, we just try to evenly distribute + // ownership. // NOTE: All of the below code only handles ownership transfer. // For client-server, we just remove the ownership. // For distributed authority, we need to change ownership based on parenting if (NetworkManager.DistributedAuthorityMode) { - // Only NetworkObjects that have the OwnershipStatus.Distributable flag set and no parent - // (ownership is transferred to all children) will have their ownership redistributed. - if (ownedObject.IsOwnershipDistributable && ownedObject.GetCachedParent() == null && !ownedObject.IsOwnershipSessionOwner) + // Only NetworkObjects that have the OwnershipStatus.Distributable flag set and are not OwnershipSessionOwner are distributed. + // If the object has a parent - skip it for now, it will be distributed when its root parent is distributed. + if (!ownedObject.IsOwnershipDistributable || ownedObject.IsOwnershipSessionOwner || ownedObject.GetCachedParent()) + { + continue; + } + + if (ownedObject.IsOwnershipLocked) + { + ownedObject.SetOwnershipLock(false); + } + + var targetOwner = NetworkManager.ServerClientId; + // Cycle through the full count of clients to find + // the next viable owner. If none are found, then + // the DAHost defaults to the owner. + for (int j = 0; j < remainingClients.Count; j++) { - if (ownedObject.IsOwnershipLocked) + clientCounter++; + clientCounter = clientCounter % predictedClientCount; + if (ownedObject.Observers.Contains(remainingClients[clientCounter].ClientId)) { - ownedObject.SetOwnershipLock(false); + targetOwner = remainingClients[clientCounter].ClientId; + break; } + } + if (EnableDistributeLogging) + { + Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); + } - // DANGO-TODO: We will want to match how the CMB service handles this. For now, we just try to evenly distribute - // ownership. - var targetOwner = NetworkManager.ServerClientId; - if (predictedClientCount > 1) + NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true); + + // Ownership gets passed down to all children that have the same owner. + var childNetworkObjects = ownedObject.GetComponentsInChildren(); + foreach (var childObject in childNetworkObjects) + { + // We already changed ownership for this + if (childObject == ownedObject) { - clientCounter++; - clientCounter = clientCounter % predictedClientCount; - targetOwner = remainingClients[clientCounter].ClientId; + continue; } - if (EnableDistributeLogging) + // If the client owner disconnected, it is ok to unlock this at this point in time. + if (childObject.IsOwnershipLocked) { - Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); + childObject.SetOwnershipLock(false); } - NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true); - // DANGO-TODO: Should we try handling inactive NetworkObjects? - // Ownership gets passed down to all children that have the same owner. - var childNetworkObjects = ownedObject.GetComponentsInChildren(); - foreach (var childObject in childNetworkObjects) + + // Ignore session owner marked objects + if (childObject.IsOwnershipSessionOwner) { - // We already changed ownership for this - if (childObject == ownedObject) - { - continue; - } - // If the client owner disconnected, it is ok to unlock this at this point in time. - if (childObject.IsOwnershipLocked) - { - childObject.SetOwnershipLock(false); - } + continue; + } - // Ignore session owner marked objects - if (childObject.IsOwnershipSessionOwner) - { - continue; - } + // If the child's owner is not the client disconnected and the objects are marked with either distributable or transferable, then + // do not change ownership. + if (childObject.OwnerClientId != clientId && (childObject.IsOwnershipDistributable || childObject.IsOwnershipTransferable)) + { + continue; + } - // If the child's owner is not the client disconnected and the objects are marked with either distributable or transferable, then - // do not change ownership. - if (childObject.OwnerClientId != clientId && (childObject.IsOwnershipDistributable || childObject.IsOwnershipTransferable)) + var childOwner = targetOwner; + if (!childObject.Observers.Contains(childOwner)) + { + for (int j = 0; j < remainingClients.Count; j++) { - continue; + clientCounter++; + clientCounter = clientCounter % predictedClientCount; + if (ownedObject.Observers.Contains(remainingClients[clientCounter].ClientId)) + { + childOwner = remainingClients[clientCounter].ClientId; + break; + } } + } - NetworkManager.SpawnManager.ChangeOwnership(childObject, targetOwner, true); - if (EnableDistributeLogging) - { - Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); - } + NetworkManager.SpawnManager.ChangeOwnership(childObject, childOwner, true); + if (EnableDistributeLogging) + { + Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); } } } diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index 9f219e4c35..d2319cb96d 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -435,21 +435,16 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool // For client-server: // If ownership changes faster than the latency between the client-server and there are NetworkVariables being updated during ownership changes, // then notify the user they could potentially lose state updates if developer logging is enabled. - if (!NetworkManager.DistributedAuthorityMode && m_LastChangeInOwnership.ContainsKey(networkObject.NetworkObjectId) && m_LastChangeInOwnership[networkObject.NetworkObjectId] > Time.realtimeSinceStartup) + if (NetworkManager.LogLevel == LogLevel.Developer && !NetworkManager.DistributedAuthorityMode && m_LastChangeInOwnership.ContainsKey(networkObject.NetworkObjectId) && m_LastChangeInOwnership[networkObject.NetworkObjectId] > Time.realtimeSinceStartup) { - var hasNetworkVariables = false; for (int i = 0; i < networkObject.ChildNetworkBehaviours.Count; i++) { - hasNetworkVariables = networkObject.ChildNetworkBehaviours[i].NetworkVariableFields.Count > 0; - if (hasNetworkVariables) + if (networkObject.ChildNetworkBehaviours[i].NetworkVariableFields.Count > 0) { + NetworkLog.LogWarningServer($"[Rapid Ownership Change Detected][Potential Loss in State] Detected a rapid change in ownership that exceeds a frequency less than {k_MaximumTickOwnershipChangeMultiplier}x the current network tick rate! Provide at least {k_MaximumTickOwnershipChangeMultiplier}x the current network tick rate between ownership changes to avoid NetworkVariable state loss."); break; } } - if (hasNetworkVariables && NetworkManager.LogLevel == LogLevel.Developer) - { - NetworkLog.LogWarningServer($"[Rapid Ownership Change Detected][Potential Loss in State] Detected a rapid change in ownership that exceeds a frequency less than {k_MaximumTickOwnershipChangeMultiplier}x the current network tick rate! Provide at least {k_MaximumTickOwnershipChangeMultiplier}x the current network tick rate between ownership changes to avoid NetworkVariable state loss."); - } } if (NetworkManager.DistributedAuthorityMode) @@ -517,7 +512,6 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool throw new SpawnStateException("Object is not spawned"); } - if (networkObject.OwnerClientId == clientId && networkObject.PreviousOwnerId == clientId) { if (NetworkManager.LogLevel == LogLevel.Developer) @@ -527,6 +521,15 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool return; } + if (!networkObject.Observers.Contains(clientId)) + { + 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."); + } + return; + } + // Used to distinguish whether a new owner should receive any currently dirty NetworkVariable updates networkObject.PreviousOwnerId = networkObject.OwnerClientId; @@ -1877,6 +1880,12 @@ internal void GetObjectDistribution(ref Dictionary + /// Distributes an even portion of spawned NetworkObjects to the given client. + /// This is called as part of the client connection process to ensure that all clients have a fair share of the spawned NetworkObjects. + /// DANGO-TODO: This will be handled by the CMB Service in the future. + /// + /// Client to distribute NetworkObjects to internal void DistributeNetworkObjects(ulong clientId) { if (!NetworkManager.DistributedAuthorityMode) @@ -1889,7 +1898,6 @@ internal void DistributeNetworkObjects(ulong clientId) return; } - // DA-NGO CMB SERVICE NOTES: // The most basic object distribution should be broken up into a table of spawned object types // where each type contains a list of each client's owned objects of that type that can be @@ -1956,6 +1964,11 @@ internal void DistributeNetworkObjects(ulong clientId) { if ((i % offsetCount) == 0) { + while (!ownerList.Value[i].Observers.Contains(clientId)) + { + i++; + } + var children = ownerList.Value[i].GetComponentsInChildren(); // Since the ownerList.Value[i] has to be distributable, then transfer all child NetworkObjects // with the same owner clientId and are marked as distributable also to the same client to keep @@ -1969,13 +1982,10 @@ internal void DistributeNetworkObjects(ulong clientId) } if (!child.IsOwnershipDistributable || !child.IsOwnershipTransferable) { - if (NetworkManager.LogLevel == LogLevel.Developer) - { - NetworkLog.LogWarning($"Sibling {child.name} of root parent {ownerList.Value[i].name} is neither transferrable or distributable! Object distribution skipped and could lead to a potentially un-owned or owner-mismatched {nameof(NetworkObject)}!"); - } + 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= transferrable children with the same owner to the same client to preserve the sibling ownership tree. + // Transfer ownership of all distributable =or= transferable children with the same owner to the same client to preserve the sibling ownership tree. ChangeOwnership(child, clientId, true); // Note: We don't increment the distributed count for these children as they are skipped when getting the object distribution } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs index dcf44f7d8f..4f70a864ad 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs @@ -400,6 +400,46 @@ public IEnumerator ValidateOwnershipPermissionsTest() AssertOnTimeout($"[Remove][Permissions Mismatch] {daHostInstance.name}: \n {m_ErrorLog}"); } + + [UnityTest] + public IEnumerator ChangeOwnershipWithoutObservers() + { + var initialLogLevel = m_ServerNetworkManager.LogLevel; + m_ServerNetworkManager.LogLevel = LogLevel.Developer; + var firstInstance = SpawnObject(m_PermissionsObject, m_ServerNetworkManager).GetComponent(); + OwnershipPermissionsTestHelper.CurrentOwnedInstance = firstInstance; + var firstInstanceHelper = firstInstance.GetComponent(); + var networkObjectId = firstInstance.NetworkObjectId; + m_ObjectToValidate = OwnershipPermissionsTestHelper.CurrentOwnedInstance; + yield return WaitForConditionOrTimeOut(ValidateObjectSpawnedOnAllClients); + AssertOnTimeout($"[Failed To Spawn] {firstInstance.name}: \n {m_ErrorLog}"); + + firstInstance.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable, true); + // Validate the base non-assigned permissions value for all instances are the same. + yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); + AssertOnTimeout($"[Permissions Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); + + var secondInstance = m_ClientNetworkManagers[0].SpawnManager.SpawnedObjects[networkObjectId]; + + // Remove the client from the observers list + firstInstance.Observers.Remove(m_ClientNetworkManagers[0].LocalClientId); + + // ChangeOwnership should fail + firstInstance.ChangeOwnership(m_ClientNetworkManagers[0].LocalClientId); + LogAssert.Expect(LogType.Warning, "[Session-Owner Sender=0] [Invalid Owner] Cannot send Ownership change as client-1 cannot see PermObject{2}-OnServer{0}! Use NetworkShow first."); + Assert.True(firstInstance.IsOwner, $"[Ownership Check] Client-{m_ServerNetworkManager.LocalClientId} should still own this object!"); + + // Now re-add the client to the Observers list and try to change ownership + firstInstance.Observers.Add(m_ClientNetworkManagers[0].LocalClientId); + firstInstance.ChangeOwnership(m_ClientNetworkManagers[0].LocalClientId); + + // Validate the second client now owns the object + yield return WaitForConditionOrTimeOut(() => secondInstance.IsOwner); + AssertOnTimeout($"[Acquire Ownership Failed] Client-{m_ClientNetworkManagers[0].LocalClientId} failed to get ownership!"); + + m_ServerNetworkManager.LogLevel = initialLogLevel; + } + internal class OwnershipPermissionsTestHelper : NetworkBehaviour { public static NetworkObject CurrentOwnedInstance;