diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index 7a17744b33..bd9787ca8a 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -785,13 +785,13 @@ public OwnershipRequestStatus RequestOwnership() // Otherwise, send the request ownership message var changeOwnership = new ChangeOwnershipMessage { + ChangeMessageType = ChangeOwnershipMessage.ChangeType.RequestOwnership, NetworkObjectId = NetworkObjectId, OwnerClientId = OwnerClientId, ClientIdCount = 1, RequestClientId = NetworkManager.LocalClientId, ClientIds = new ulong[1] { OwnerClientId }, DistributedAuthorityMode = true, - RequestOwnership = true, OwnershipFlags = (ushort)Ownership, }; @@ -866,18 +866,18 @@ internal void OwnershipRequest(ulong clientRequestingOwnership) else { // Otherwise, send back the reason why the ownership request was denied for the clientRequestingOwnership - /// Notes: - /// We always apply the as opposed to to the - /// value as ownership could have changed and the denied requests - /// targeting this instance are because there is a request pending. - /// DANGO-TODO: What happens if the client requesting disconnects prior to responding with the update in request pending? + // Notes: + // We always apply the as opposed to to the + // value as ownership could have changed and the denied requests + // targeting this instance are because there is a request pending. + // DANGO-TODO: What happens if the client requesting disconnects prior to responding with the update in request pending? var changeOwnership = new ChangeOwnershipMessage { + ChangeMessageType = ChangeOwnershipMessage.ChangeType.RequestDenied, NetworkObjectId = NetworkObjectId, OwnerClientId = NetworkManager.LocalClientId, // Always use the local clientId (see above notes) RequestClientId = clientRequestingOwnership, DistributedAuthorityMode = true, - RequestDenied = true, OwnershipRequestResponseStatus = (byte)response, OwnershipFlags = (ushort)Ownership, }; @@ -1063,10 +1063,10 @@ internal void SendOwnershipStatusUpdate() var changeOwnership = new ChangeOwnershipMessage { + ChangeMessageType = ChangeOwnershipMessage.ChangeType.OwnershipFlagsUpdate, NetworkObjectId = NetworkObjectId, OwnerClientId = OwnerClientId, DistributedAuthorityMode = true, - OwnershipFlagsUpdate = true, OwnershipFlags = (ushort)Ownership, }; diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs index 7b18c362d1..c3b9fd559f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs @@ -1,3 +1,4 @@ +using System.Runtime.CompilerServices; namespace Unity.Netcode { @@ -25,89 +26,15 @@ internal struct ChangeOwnershipMessage : INetworkMessage, INetworkSerializeByMem internal bool DistributedAuthorityMode; internal ushort OwnershipFlags; internal byte OwnershipRequestResponseStatus; - private byte m_OwnershipMessageTypeFlags; + internal ChangeType ChangeMessageType; - private const byte k_OwnershipChanging = 0x01; - private const byte k_OwnershipFlagsUpdate = 0x02; - private const byte k_RequestOwnership = 0x04; - private const byte k_RequestApproved = 0x08; - private const byte k_RequestDenied = 0x10; - - // If no flags are set, then ownership is changing - internal bool OwnershipIsChanging - { - get - { - return GetFlag(k_OwnershipChanging); - } - - set - { - SetFlag(value, k_OwnershipChanging); - } - } - - internal bool OwnershipFlagsUpdate - { - get - { - return GetFlag(k_OwnershipFlagsUpdate); - } - - set - { - SetFlag(value, k_OwnershipFlagsUpdate); - } - } - - internal bool RequestOwnership + internal enum ChangeType : byte { - get - { - return GetFlag(k_RequestOwnership); - } - - set - { - SetFlag(value, k_RequestOwnership); - } - } - - internal bool RequestApproved - { - get - { - return GetFlag(k_RequestApproved); - } - - set - { - SetFlag(value, k_RequestApproved); - } - } - - internal bool RequestDenied - { - get - { - return GetFlag(k_RequestDenied); - } - - set - { - SetFlag(value, k_RequestDenied); - } - } - - private bool GetFlag(int flag) - { - return (m_OwnershipMessageTypeFlags & flag) != 0; - } - - private void SetFlag(bool set, byte flag) - { - if (set) { m_OwnershipMessageTypeFlags = (byte)(m_OwnershipMessageTypeFlags | flag); } - else { m_OwnershipMessageTypeFlags = (byte)(m_OwnershipMessageTypeFlags & ~flag); } + OwnershipChanging = 0x01, + OwnershipFlagsUpdate = 0x02, + RequestOwnership = 0x04, + RequestApproved = 0x08, + RequestDenied = 0x10, } public void Serialize(FastBufferWriter writer, int targetVersion) @@ -129,20 +56,21 @@ public void Serialize(FastBufferWriter writer, int targetVersion) } } - writer.WriteValueSafe(m_OwnershipMessageTypeFlags); - if (OwnershipFlagsUpdate || OwnershipIsChanging) + writer.WriteValueSafe(ChangeMessageType); + + if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate || ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved) { writer.WriteValueSafe(OwnershipFlags); } - // When requesting, it is the requestor - // When approving, it is the owner that approved - // When denied, it is the requestor - if (RequestOwnership || RequestApproved || RequestDenied) + // When requesting, RequestClientId is the requestor + // When approving, RequestClientId is the owner that approved + // When denied, RequestClientId is the requestor + if (ChangeMessageType == ChangeType.RequestOwnership || ChangeMessageType == ChangeType.RequestApproved || ChangeMessageType == ChangeType.RequestDenied) { writer.WriteValueSafe(RequestClientId); - if (RequestDenied) + if (ChangeMessageType is ChangeType.RequestDenied) { writer.WriteValueSafe(OwnershipRequestResponseStatus); } @@ -166,33 +94,39 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int if (ClientIdCount > 0) { ClientIds = new ulong[ClientIdCount]; - var clientId = (ulong)0; for (int i = 0; i < ClientIdCount; i++) { - ByteUnpacker.ReadValueBitPacked(reader, out clientId); + ByteUnpacker.ReadValueBitPacked(reader, out ulong clientId); ClientIds[i] = clientId; } } - reader.ReadValueSafe(out m_OwnershipMessageTypeFlags); - if (OwnershipFlagsUpdate || OwnershipIsChanging) + reader.ReadValueSafe(out ChangeMessageType); + if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate || ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved) { reader.ReadValueSafe(out OwnershipFlags); } - // When requesting, it is the requestor - // When approving, it is the owner that approved - // When denied, it is the requestor - if (RequestOwnership || RequestApproved || RequestDenied) + // When requesting, RequestClientId is the requestor + // When approving, RequestClientId is the owner that approved + // When denied, RequestClientId is the requestor + if (ChangeMessageType == ChangeType.RequestOwnership || ChangeMessageType == ChangeType.RequestApproved || ChangeMessageType == ChangeType.RequestDenied) { + // We are receiving a request for ownership, or an approval or denial of our request. reader.ReadValueSafe(out RequestClientId); - if (RequestDenied) + if (ChangeMessageType == ChangeType.RequestDenied) { reader.ReadValueSafe(out OwnershipRequestResponseStatus); } } } + else + { + // The only valid message type in Client/Server is ownership changing. + ChangeMessageType = ChangeType.OwnershipChanging; + } + // If we are not a DAHost instance and the NetworkObject does not exist then defer it as it very likely is not spawned yet. // Otherwise if we are the DAHost and it does not exist then we want to forward this message because when the NetworkObject @@ -213,77 +147,17 @@ public void Handle(ref NetworkContext context) // If we are the DAHost then forward this message if (networkManager.DAHost) { - var clientList = ClientIdCount > 0 ? ClientIds : networkManager.ConnectedClientsIds; - - var message = new ChangeOwnershipMessage() - { - NetworkObjectId = NetworkObjectId, - OwnerClientId = OwnerClientId, - DistributedAuthorityMode = true, - OwnershipFlags = OwnershipFlags, - RequestClientId = RequestClientId, - ClientIdCount = 0, - m_OwnershipMessageTypeFlags = m_OwnershipMessageTypeFlags, - }; - - if (RequestDenied) - { - // If the local DAHost's client is not the target, then forward to the target - if (RequestClientId != networkManager.LocalClientId) - { - message.OwnershipRequestResponseStatus = OwnershipRequestResponseStatus; - networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, RequestClientId); - // We don't want the local DAHost's client to process this message, so exit early - return; - } - } - else if (RequestOwnership) - { - // If the DAHost client is not authority, just forward the message to the authority - if (OwnerClientId != networkManager.LocalClientId) - { - networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, OwnerClientId); - // We don't want the local DAHost's client to process this message, so exit early - return; - } - // Otherwise, fall through and process the request. - } - else - { - for (int i = 0; i < clientList.Count; i++) - { - var clientId = clientList[i]; - if (clientId == networkManager.LocalClientId) - { - continue; - } - - // If ownership is changing and this is not an ownership request approval then ignore the SenderId - if (OwnershipIsChanging && !RequestApproved && context.SenderId == clientId) - { - continue; - } - - // If it is just updating flags then ignore sending to the owner - // If it is a request or approving request, then ignore the RequestClientId - if ((OwnershipFlagsUpdate && clientId == OwnerClientId) || ((RequestOwnership || RequestApproved) && clientId == RequestClientId)) - { - continue; - } - networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, clientId); - } - } - // If the NetworkObject is not visible to the DAHost client, then exit early - if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(NetworkObjectId)) + var shouldProcessLocally = HandleDAHostMessageForwarding(ref networkManager, context.SenderId); + if (!shouldProcessLocally) { return; } } - // If ownership is changing, then run through the ownershipd changed sequence + // If ownership is changing (either a straight change or a request approval), then run through the ownership changed sequence // Note: There is some extended ownership script at the bottom of HandleOwnershipChange - // If not in distributed authority mode, then always go straight to HandleOwnershipChange - if (OwnershipIsChanging || !networkManager.DistributedAuthorityMode) + // If not in distributed authority mode, ChangeMessageType will always be OwnershipChanging. + if (ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved || !networkManager.DistributedAuthorityMode) { HandleOwnershipChange(ref context); } @@ -297,7 +171,7 @@ public void Handle(ref NetworkContext context) /// /// Handle the extended distributed authority ownership updates /// - /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void HandleExtendedOwnershipUpdate(ref NetworkContext context) { var networkManager = (NetworkManager)context.SystemOwner; @@ -305,17 +179,17 @@ private void HandleExtendedOwnershipUpdate(ref NetworkContext context) // Handle the extended ownership message types var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId]; - if (OwnershipFlagsUpdate) + if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate) { // Just update the ownership flags networkObject.Ownership = (NetworkObject.OwnershipStatus)OwnershipFlags; } - else if (RequestOwnership) + else if (ChangeMessageType == ChangeType.RequestOwnership) { // Requesting ownership, if allowed it will automatically send the ownership change message networkObject.OwnershipRequest(RequestClientId); } - else if (RequestDenied) + else if (ChangeMessageType == ChangeType.RequestDenied) { networkObject.OwnershipRequestResponse((NetworkObject.OwnershipRequestResponseStatus)OwnershipRequestResponseStatus); } @@ -324,7 +198,7 @@ private void HandleExtendedOwnershipUpdate(ref NetworkContext context) /// /// Handle the traditional change in ownership message type logic /// - /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void HandleOwnershipChange(ref NetworkContext context) { var networkManager = (NetworkManager)context.SystemOwner; @@ -341,49 +215,49 @@ private void HandleOwnershipChange(ref NetworkContext context) var originalOwner = networkObject.OwnerClientId; networkObject.OwnerClientId = OwnerClientId; + // If in distributed authority mode if (networkManager.DistributedAuthorityMode) { networkObject.Ownership = (NetworkObject.OwnershipStatus)OwnershipFlags; - } - // We are current owner (client-server) or running in distributed authority mode - if (originalOwner == networkManager.LocalClientId || networkManager.DistributedAuthorityMode) - { networkObject.InvokeBehaviourOnLostOwnership(); - } - // If in distributed authority mode - if (networkManager.DistributedAuthorityMode) - { // Always update the network properties in distributed authority mode - for (int i = 0; i < networkObject.ChildNetworkBehaviours.Count; i++) + foreach (var child in networkObject.ChildNetworkBehaviours) { - networkObject.ChildNetworkBehaviours[i].UpdateNetworkProperties(); + child.UpdateNetworkProperties(); } + + networkObject.InvokeBehaviourOnGainedOwnership(); } - else // Otherwise update properties like we would in client-server + else { + // We are initial owner + if (originalOwner == networkManager.LocalClientId) + { + networkObject.InvokeBehaviourOnLostOwnership(); + } + // For all other clients that are neither the former or current owner, update the behaviours' properties if (OwnerClientId != networkManager.LocalClientId && originalOwner != networkManager.LocalClientId) { - for (int i = 0; i < networkObject.ChildNetworkBehaviours.Count; i++) + foreach (var childBehaviour in networkObject.ChildNetworkBehaviours) { - networkObject.ChildNetworkBehaviours[i].UpdateNetworkProperties(); + childBehaviour.UpdateNetworkProperties(); } } - } - - // We are new owner or (client-server) or running in distributed authority mode - if (OwnerClientId == networkManager.LocalClientId || networkManager.DistributedAuthorityMode) - { - networkObject.InvokeBehaviourOnGainedOwnership(); - } + // We are new owner + if (OwnerClientId == networkManager.LocalClientId) + { + networkObject.InvokeBehaviourOnGainedOwnership(); + } - if (originalOwner == networkManager.LocalClientId && !networkManager.DistributedAuthorityMode) - { - // Fully synchronize NetworkVariables with either read or write ownership permissions. - networkObject.SynchronizeOwnerNetworkVariables(originalOwner, networkObject.PreviousOwnerId); + if (originalOwner == networkManager.LocalClientId) + { + // Fully synchronize NetworkVariables with either read or write ownership permissions. + networkObject.SynchronizeOwnerNetworkVariables(originalOwner, networkObject.PreviousOwnerId); + } } // Always invoke ownership change notifications @@ -393,7 +267,7 @@ private void HandleOwnershipChange(ref NetworkContext context) // changes have already been applied if the callback is invoked) if (networkManager.DistributedAuthorityMode && networkManager.LocalClientId == OwnerClientId) { - if (RequestApproved) + if (ChangeMessageType is ChangeType.RequestApproved) { networkObject.OwnershipRequestResponse(NetworkObject.OwnershipRequestResponseStatus.Approved); } @@ -409,5 +283,69 @@ private void HandleOwnershipChange(ref NetworkContext context) networkManager.NetworkMetrics.TrackOwnershipChangeReceived(context.SenderId, networkObject, context.MessageSize); } + + /// + /// [DAHost Only] + /// Forward this message to all other clients who need to receive it. + /// + /// The current NetworkManager from the NetworkContext + /// The sender of the current message from the NetworkContext + /// true if this message should also be processed locally; false if the message should only be forwarded + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private bool HandleDAHostMessageForwarding(ref NetworkManager networkManager, ulong senderId) + { + var clientList = ClientIdCount > 0 ? ClientIds : networkManager.ConnectedClientsIds; + + var message = new ChangeOwnershipMessage() + { + NetworkObjectId = NetworkObjectId, + OwnerClientId = OwnerClientId, + DistributedAuthorityMode = true, + OwnershipFlags = OwnershipFlags, + RequestClientId = RequestClientId, + ClientIdCount = 0, + ChangeMessageType = ChangeMessageType, + }; + + if (ChangeMessageType == ChangeType.RequestDenied) + { + // If the local DAHost's client is not the target, then forward to the target + if (RequestClientId != networkManager.LocalClientId) + { + message.OwnershipRequestResponseStatus = OwnershipRequestResponseStatus; + networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, RequestClientId); + + // We don't want the local DAHost's client to process this message + return false; + } + } + else if (ChangeMessageType == ChangeType.RequestOwnership) + { + // If the DAHost client is not authority, just forward the message to the authority + if (OwnerClientId != networkManager.LocalClientId) + { + networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, OwnerClientId); + + // We don't want the local DAHost's client to process this message + return false; + } + } + else + { + foreach (var clientId in clientList) + { + // Don't forward to self or originating client + if (clientId == networkManager.LocalClientId || clientId == senderId) + { + continue; + } + + networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, clientId); + } + } + + // Return whether to process the message on the DAHost itself (only if object is spawned). + return networkManager.SpawnManager.SpawnedObjects.ContainsKey(NetworkObjectId); + } } } diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index 9df5f5e972..69376d8ed4 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -572,16 +572,15 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool { var message = new ChangeOwnershipMessage { + ChangeMessageType = isRequestApproval ? ChangeOwnershipMessage.ChangeType.RequestApproved : ChangeOwnershipMessage.ChangeType.OwnershipChanging, NetworkObjectId = networkObject.NetworkObjectId, OwnerClientId = networkObject.OwnerClientId, - DistributedAuthorityMode = NetworkManager.DistributedAuthorityMode, - RequestApproved = isRequestApproval, - OwnershipIsChanging = true, + DistributedAuthorityMode = true, RequestClientId = networkObject.PreviousOwnerId, OwnershipFlags = (ushort)networkObject.Ownership, }; - // If we are connected to the CMB service or not the DAHost (i.e. pure DA-Clients only) + // If we are connected to the CMB service or not the DAHost (i.e. pure DA-Clients only) if (NetworkManager.CMBServiceConnection || !NetworkManager.DAHost) { // Always update the network properties in distributed authority mode for the client gaining ownership @@ -618,6 +617,7 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool { var message = new ChangeOwnershipMessage { + ChangeMessageType = ChangeOwnershipMessage.ChangeType.OwnershipChanging, NetworkObjectId = networkObject.NetworkObjectId, OwnerClientId = networkObject.OwnerClientId, }; diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs index b87b5a529a..d4cf1d1f8a 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs @@ -17,7 +17,7 @@ internal class OwnershipPermissionsTests : IntegrationTestWithApproximation protected override int NumberOfClients => 4; - // TODO: [CmbServiceTests] Adapt to run with the service - daHostInstance will be firstInstance with cmbService + // TODO: [CmbServiceTests] Remove this once MTTB-1570 is resolved. protected override bool UseCMBService() { return false; @@ -44,37 +44,19 @@ protected override void OnServerAndClientsCreated() private NetworkObject m_ObjectToValidate; - private bool ValidateObjectSpawnedOnAllClients() - { - m_ErrorLog.Clear(); - - var networkObjectId = m_ObjectToValidate.NetworkObjectId; - var name = m_ObjectToValidate.name; - - foreach (var client in m_NetworkManagers) - { - if (!client.SpawnManager.SpawnedObjects.ContainsKey(networkObjectId)) - { - m_ErrorLog.Append($"Client-{client.LocalClientId} has not spawned {name}!"); - return false; - } - } - return true; - } - private bool ValidatePermissionsOnAllClients() + private bool ValidatePermissionsOnAllClients(StringBuilder errorLog) { var currentPermissions = (ushort)m_ObjectToValidate.Ownership; var networkObjectId = m_ObjectToValidate.NetworkObjectId; var objectName = m_ObjectToValidate.name; - m_ErrorLog.Clear(); foreach (var client in m_NetworkManagers) { var otherPermissions = (ushort)client.SpawnManager.SpawnedObjects[networkObjectId].Ownership; if (currentPermissions != otherPermissions) { - m_ErrorLog.Append($"Client-{client.LocalClientId} permissions for {objectName} is {otherPermissions} when it should be {currentPermissions}!"); + errorLog.Append($"Client-{client.LocalClientId} permissions for {objectName} is {otherPermissions} when it should be {currentPermissions}!"); return false; } } @@ -101,17 +83,19 @@ private bool ValidateAllInstancesAreOwnedByClient(ulong clientId) [UnityTest] public IEnumerator ValidateOwnershipPermissionsTest() { - var firstInstance = SpawnObject(m_PermissionsObject, m_ClientNetworkManagers[0]).GetComponent(); + var firstClient = GetNonAuthorityNetworkManager(0); + var firstInstance = SpawnObject(m_PermissionsObject, firstClient).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}"); + + yield return WaitForSpawnedOnAllOrTimeOut(firstInstance); + AssertOnTimeout($"[Failed To Spawn] Ownership permissions object {firstInstance.name} failed to spawn!"); // 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}"); + AssertOnTimeout($"[Permissions Mismatch] {firstInstance.name} has incorrect ownership permissions!"); ////////////////////////////////////// // Setting & Removing Ownership Flags: @@ -132,7 +116,7 @@ public IEnumerator ValidateOwnershipPermissionsTest() firstInstance.SetOwnershipStatus(permission); // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Add][Permissions Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Add][Permissions Mismatch] {firstInstance.name}"); // Remove the status unless it is None (ignore None). if (permission == NetworkObject.OwnershipStatus.None) @@ -142,7 +126,7 @@ public IEnumerator ValidateOwnershipPermissionsTest() firstInstance.RemoveOwnershipStatus(permission); // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Remove][Permissions Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Remove][Permissions Mismatch] {firstInstance.name}"); } //Add multiple flags at the same time @@ -164,7 +148,7 @@ public IEnumerator ValidateOwnershipPermissionsTest() // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Set Multiple][Permissions Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Set Multiple][Permissions Mismatch] {firstInstance.name}"); ////////////////////// // Changing Ownership: @@ -175,12 +159,13 @@ public IEnumerator ValidateOwnershipPermissionsTest() // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Reset][Permissions Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Reset][Permissions Mismatch] {firstInstance.name}"); - var secondInstance = m_ClientNetworkManagers[1].SpawnManager.SpawnedObjects[networkObjectId]; + var secondClient = GetNonAuthorityNetworkManager(1); + var secondInstance = secondClient.SpawnManager.SpawnedObjects[networkObjectId]; var secondInstanceHelper = secondInstance.GetComponent(); - secondInstance.ChangeOwnership(m_ClientNetworkManagers[1].LocalClientId); + secondInstance.ChangeOwnership(secondClient.LocalClientId); Assert.IsTrue(secondInstanceHelper.OwnershipPermissionsFailureStatus == NetworkObject.OwnershipPermissionsFailureStatus.Locked, $"Expected {secondInstance.name} to return {NetworkObject.OwnershipPermissionsFailureStatus.Locked} but its permission failure" + $" status is {secondInstanceHelper.OwnershipPermissionsFailureStatus}!"); @@ -188,20 +173,25 @@ public IEnumerator ValidateOwnershipPermissionsTest() firstInstance.SetOwnershipLock(false); // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Unlock][Permissions Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Unlock][Permissions Mismatch] {firstInstance.name}"); // Sanity check to assure this client's instance isn't already the owner. - Assert.True(!secondInstance.IsOwner, $"[Ownership Check] Client-{m_ClientNetworkManagers[1].LocalClientId} already is the owner!"); + Assert.True(!secondInstance.IsOwner, $"[Ownership Check] Client-{secondClient.LocalClientId} already is the owner!"); + + // With transferable ownership, the second client shouldn't be able to request ownership + var requestStatus = secondInstance.RequestOwnership(); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestRequiredNotSet, $"Client-{secondClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + // Now try to acquire ownership - secondInstance.ChangeOwnership(m_ClientNetworkManagers[1].LocalClientId); + secondInstance.ChangeOwnership(secondClient.LocalClientId); // Validate the permissions value for all instances are the same yield return WaitForConditionOrTimeOut(() => secondInstance.IsOwner); - AssertOnTimeout($"[Acquire Ownership Failed] Client-{m_ClientNetworkManagers[1].LocalClientId} failed to get ownership!"); + AssertOnTimeout($"[Acquire Ownership Failed] Client-{secondClient.LocalClientId} failed to get ownership!"); m_ObjectToValidate = OwnershipPermissionsTestHelper.CurrentOwnedInstance; // Validate all other client instances are showing the same owner - yield return WaitForConditionOrTimeOut(() => ValidateAllInstancesAreOwnedByClient(m_ClientNetworkManagers[1].LocalClientId)); + yield return WaitForConditionOrTimeOut(() => ValidateAllInstancesAreOwnedByClient(secondClient.LocalClientId)); AssertOnTimeout($"[Ownership Mismatch] {secondInstance.name}: \n {m_ErrorLog}"); // Clear the flags, set the permissions to RequestRequired, and lock ownership in one pass. @@ -209,10 +199,10 @@ public IEnumerator ValidateOwnershipPermissionsTest() // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Unlock][Permissions Mismatch] {secondInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Unlock][Permissions Mismatch] {secondInstance.name}"); // Attempt to acquire ownership by just changing it - firstInstance.ChangeOwnership(firstInstance.NetworkManager.LocalClientId); + firstInstance.ChangeOwnership(firstClient.LocalClientId); // Assure we are denied ownership due to it requiring ownership be requested Assert.IsTrue(firstInstanceHelper.OwnershipPermissionsFailureStatus == NetworkObject.OwnershipPermissionsFailureStatus.RequestRequired, @@ -224,52 +214,64 @@ public IEnumerator ValidateOwnershipPermissionsTest() ////////////////////////////////// // Start with a request for the client we expect to be given ownership - var requestStatus = firstInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{firstInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + requestStatus = firstInstance.RequestOwnership(); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{firstClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); // Get the 3rd client to send a request at the "relatively" same time - var thirdInstance = m_ClientNetworkManagers[2].SpawnManager.SpawnedObjects[networkObjectId]; + var thirdClient = GetNonAuthorityNetworkManager(2); + var thirdInstance = thirdClient.SpawnManager.SpawnedObjects[networkObjectId]; var thirdInstanceHelper = thirdInstance.GetComponent(); // At the same time send a request by the third client. requestStatus = thirdInstance.RequestOwnership(); // We expect the 3rd client's request should be able to be sent at this time as well (i.e. creates the race condition between two clients) - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{thirdInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{thirdClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); // We expect the first requesting client to be given ownership yield return WaitForConditionOrTimeOut(() => firstInstance.IsOwner); - AssertOnTimeout($"[Acquire Ownership Failed] Client-{firstInstance.NetworkManager.LocalClientId} failed to get ownership! ({firstInstanceHelper.OwnershipRequestResponseStatus})(Owner: {OwnershipPermissionsTestHelper.CurrentOwnedInstance.OwnerClientId}"); + AssertOnTimeout($"[Acquire Ownership Failed] Client-{firstClient.LocalClientId} failed to get ownership! ({firstInstanceHelper.OwnershipRequestResponseStatus})(Owner: {OwnershipPermissionsTestHelper.CurrentOwnedInstance.OwnerClientId}"); m_ObjectToValidate = OwnershipPermissionsTestHelper.CurrentOwnedInstance; // Just do a sanity check to assure ownership has changed on all clients. - yield return WaitForConditionOrTimeOut(() => ValidateAllInstancesAreOwnedByClient(firstInstance.NetworkManager.LocalClientId)); + yield return WaitForConditionOrTimeOut(() => ValidateAllInstancesAreOwnedByClient(firstClient.LocalClientId)); AssertOnTimeout($"[Ownership Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); // Now, the third client should get a RequestInProgress returned as their request response yield return WaitForConditionOrTimeOut(() => thirdInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.RequestInProgress); - AssertOnTimeout($"[Request In Progress Failed] Client-{thirdInstanceHelper.NetworkManager.LocalClientId} did not get the right request denied reponse!"); + AssertOnTimeout($"[Request In Progress Failed] Client-{thirdClient.LocalClientId} did not get the right request denied response!"); // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Unlock][Permissions Mismatch] {firstInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Unlock][Permissions Mismatch] {firstInstance.name}"); + + // Check for various permissions denied race conditions with changing permissions and requesting ownership + yield return ValidateRequestAndPermissionsChangeRaceCondition(NetworkObject.OwnershipStatus.Distributable, NetworkObject.OwnershipLockActions.SetAndUnlock, NetworkObject.OwnershipRequestResponseStatus.CannotRequest, firstClient, firstInstance, secondInstance, secondInstanceHelper); + yield return ValidateRequestAndPermissionsChangeRaceCondition(NetworkObject.OwnershipStatus.Transferable, NetworkObject.OwnershipLockActions.SetAndLock, NetworkObject.OwnershipRequestResponseStatus.Locked, firstClient, firstInstance, secondInstance, secondInstanceHelper); + + // Should successfully change ownership to secondClient + yield return ValidateRequestAndPermissionsChangeRaceCondition(NetworkObject.OwnershipStatus.Transferable, NetworkObject.OwnershipLockActions.SetAndUnlock, NetworkObject.OwnershipRequestResponseStatus.Approved, firstClient, firstInstance, secondInstance, secondInstanceHelper); + + // Transfer ownership back to firstClient. ValidateRequestAndPermissionsChangeRaceCondition will reset permissions to RequestRequired + yield return ValidateRequestAndPermissionsChangeRaceCondition(NetworkObject.OwnershipStatus.Transferable, NetworkObject.OwnershipLockActions.None, NetworkObject.OwnershipRequestResponseStatus.Approved, secondClient, secondInstance, firstInstance, firstInstanceHelper, false); /////////////////////////////////////////////// // Test for multiple ownership race conditions: /////////////////////////////////////////////// // Get the 4th client's instance - var fourthInstance = m_ClientNetworkManagers[3].SpawnManager.SpawnedObjects[networkObjectId]; + var fourthClient = GetNonAuthorityNetworkManager(3); + var fourthInstance = fourthClient.SpawnManager.SpawnedObjects[networkObjectId]; var fourthInstanceHelper = fourthInstance.GetComponent(); // Send out a request from three clients at the same time // The first one sent (and received for this test) gets ownership requestStatus = secondInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{secondInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{secondClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); requestStatus = thirdInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{thirdInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{thirdClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); requestStatus = fourthInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{fourthInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{fourthClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); // The 2nd and 3rd client should be denied and the 4th client should be approved yield return WaitForConditionOrTimeOut(() => @@ -277,15 +279,19 @@ public IEnumerator ValidateOwnershipPermissionsTest() (thirdInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.RequestInProgress) && (secondInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Approved) ); - AssertOnTimeout($"[Targeted Owner] Client-{secondInstanceHelper.NetworkManager.LocalClientId} did not get the right request denied reponse: {secondInstanceHelper.OwnershipRequestResponseStatus}!"); + AssertOnTimeout($"[Targeted Owner] A client received an incorrect response." + + $"\n Client-{fourthClient.LocalClientId}: Expected {NetworkObject.OwnershipRequestResponseStatus.RequestInProgress}, and got {fourthInstanceHelper.OwnershipRequestResponseStatus}!" + + $"\n Client-{thirdClient.LocalClientId}: Expected {NetworkObject.OwnershipRequestResponseStatus.RequestInProgress}, and got {thirdInstanceHelper.OwnershipRequestResponseStatus}!" + + $"\n Client-{secondClient.LocalClientId}: Expected {NetworkObject.OwnershipRequestResponseStatus.Denied}, and got {secondInstanceHelper.OwnershipRequestResponseStatus}!"); + m_ObjectToValidate = OwnershipPermissionsTestHelper.CurrentOwnedInstance; // Just do a sanity check to assure ownership has changed on all clients. - yield return WaitForConditionOrTimeOut(() => ValidateAllInstancesAreOwnedByClient(secondInstance.NetworkManager.LocalClientId)); - AssertOnTimeout($"[Ownership Mismatch] {secondInstance.name}: \n {m_ErrorLog}"); + yield return WaitForConditionOrTimeOut(() => ValidateAllInstancesAreOwnedByClient(secondClient.LocalClientId)); + AssertOnTimeout($"[Multiple request race condition][Ownership Mismatch] {secondInstance.name}: \n {m_ErrorLog}"); // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Unlock][Permissions Mismatch] {secondInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Multiple request race condition][Permissions Mismatch] {secondInstance.name}"); /////////////////////////////////////////////// // Test for targeted ownership request: @@ -293,74 +299,78 @@ public IEnumerator ValidateOwnershipPermissionsTest() // Now get the DAHost's client's instance var authority = GetAuthorityNetworkManager(); - var daHostInstance = authority.SpawnManager.SpawnedObjects[networkObjectId]; - var daHostInstanceHelper = daHostInstance.GetComponent(); + var authorityInstance = authority.SpawnManager.SpawnedObjects[networkObjectId]; + var authorityInstanceHelper = authorityInstance.GetComponent(); secondInstanceHelper.AllowOwnershipRequest = true; secondInstanceHelper.OnlyAllowTargetClientId = true; - secondInstanceHelper.ClientToAllowOwnership = daHostInstance.NetworkManager.LocalClientId; + secondInstanceHelper.ClientToAllowOwnership = authority.LocalClientId; // Send out a request from all three clients requestStatus = firstInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{firstInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{firstClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); requestStatus = thirdInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{thirdInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{thirdClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); requestStatus = fourthInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{fourthInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); - requestStatus = daHostInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{daHostInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{fourthClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + requestStatus = authorityInstance.RequestOwnership(); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{authority.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); // Only the client marked as ClientToAllowOwnership (daHost) should be approved. All others should be denied. yield return WaitForConditionOrTimeOut(() => (firstInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Denied) && (thirdInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Denied) && (fourthInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Denied) && - (daHostInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Approved) + (authorityInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Approved) ); - AssertOnTimeout($"[Targeted Owner] Client-{daHostInstance.NetworkManager.LocalClientId} did not get the right request response: {daHostInstanceHelper.OwnershipRequestResponseStatus} Expecting: {NetworkObject.OwnershipRequestResponseStatus.Approved}!"); + AssertOnTimeout($"[Targeted Owner] A client received an incorrect response." + + $"\n Client-{firstClient.LocalClientId}: Expected {NetworkObject.OwnershipRequestResponseStatus.Denied}, and got {firstInstanceHelper.OwnershipRequestResponseStatus}!" + + $"\n Client-{thirdClient.LocalClientId}: Expected {NetworkObject.OwnershipRequestResponseStatus.Denied}, and got {thirdInstanceHelper.OwnershipRequestResponseStatus}!" + + $"\n Client-{fourthClient.LocalClientId}: Expected {NetworkObject.OwnershipRequestResponseStatus.Denied}, and got {fourthInstanceHelper.OwnershipRequestResponseStatus}!" + + $"\n Client-{authority.LocalClientId}: Expected {NetworkObject.OwnershipRequestResponseStatus.Approved}, and got {authorityInstanceHelper.OwnershipRequestResponseStatus}!"); /////////////////////////////////////////////// // Test OwnershipStatus.SessionOwner: /////////////////////////////////////////////// - OwnershipPermissionsTestHelper.CurrentOwnedInstance = daHostInstance; + OwnershipPermissionsTestHelper.CurrentOwnedInstance = authorityInstance; m_ObjectToValidate = OwnershipPermissionsTestHelper.CurrentOwnedInstance; // Add multiple statuses - daHostInstance.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable | NetworkObject.OwnershipStatus.SessionOwner); + authorityInstance.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable | NetworkObject.OwnershipStatus.SessionOwner); // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Add][Permissions Mismatch] {daHostInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Add][Permissions Mismatch] {authorityInstance.name}"); // Trying to set SessionOwner flag should override any other flags. - Assert.IsFalse(daHostInstance.HasOwnershipStatus(NetworkObject.OwnershipStatus.Transferable), $"[Set][SessionOwner flag Failure] Expected: {NetworkObject.OwnershipStatus.Transferable} not to be set!"); + Assert.IsFalse(authorityInstance.HasOwnershipStatus(NetworkObject.OwnershipStatus.Transferable), $"[Set][SessionOwner flag Failure] Expected: {NetworkObject.OwnershipStatus.Transferable} not to be set!"); // Add another status. Should fail as SessionOwner should be exclusive - daHostInstance.SetOwnershipStatus(NetworkObject.OwnershipStatus.Distributable); - Assert.IsFalse(daHostInstance.HasOwnershipStatus(NetworkObject.OwnershipStatus.Distributable), $"[Add][SessionOwner flag Failure] Expected: {NetworkObject.OwnershipStatus.Transferable} not to be set!"); + authorityInstance.SetOwnershipStatus(NetworkObject.OwnershipStatus.Distributable); + Assert.IsFalse(authorityInstance.HasOwnershipStatus(NetworkObject.OwnershipStatus.Distributable), $"[Add][SessionOwner flag Failure] Expected: {NetworkObject.OwnershipStatus.Transferable} not to be set!"); // Request ownership of the SessionOwner flag instance requestStatus = firstInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestRequiredNotSet, $"Client-{firstInstance.NetworkManager.LocalClientId} should not be able to send a request for ownership because object is marked as owned by the session owner. {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestRequiredNotSet, $"Client-{firstClient.LocalClientId} should not be able to send a request for ownership because object is marked as owned by the session owner. {requestStatus}!"); // Set ownership directly on local object. This will allow the request to be sent firstInstance.Ownership = NetworkObject.OwnershipStatus.RequestRequired; requestStatus = firstInstance.RequestOwnership(); - Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{firstInstance.NetworkManager.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{firstClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); // Request should be denied with CannotRequest yield return WaitForConditionOrTimeOut(() => firstInstanceHelper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.CannotRequest); - AssertOnTimeout($"[Targeted Owner] Client-{firstInstance.NetworkManager.LocalClientId} did not get the right request response: {daHostInstanceHelper.OwnershipRequestResponseStatus} Expecting: {NetworkObject.OwnershipRequestResponseStatus.CannotRequest}!"); + AssertOnTimeout($"[Targeted Owner] Client-{firstClient.LocalClientId} did not get the right request response: {firstInstanceHelper.OwnershipRequestResponseStatus} Expecting: {NetworkObject.OwnershipRequestResponseStatus.CannotRequest}!"); // Try changing the ownership explicitly - // Get the cloned daHostInstance instance on a client side - var clientInstance = m_ClientNetworkManagers[2].SpawnManager.SpawnedObjects[daHostInstance.NetworkObjectId]; + // Get the cloned authorityClient instance on a client side + var clientInstance = thirdClient.SpawnManager.SpawnedObjects[authorityInstance.NetworkObjectId]; // Get the client instance of the OwnershipPermissionsTestHelper component var clientInstanceHelper = clientInstance.GetComponent(); // Have the client attempt to change ownership - clientInstance.ChangeOwnership(m_ClientNetworkManagers[2].LocalClientId); + clientInstance.ChangeOwnership(thirdClient.LocalClientId); // Verify the client side gets a permission failure status of NetworkObject.OwnershipPermissionsFailureStatus.SessionOwnerOnly Assert.IsTrue(clientInstanceHelper.OwnershipPermissionsFailureStatus == NetworkObject.OwnershipPermissionsFailureStatus.SessionOwnerOnly, @@ -368,58 +378,102 @@ public IEnumerator ValidateOwnershipPermissionsTest() $" status is {clientInstanceHelper.OwnershipPermissionsFailureStatus}!"); // Have the session owner attempt to change ownership to a non-session owner - daHostInstance.ChangeOwnership(m_ClientNetworkManagers[2].LocalClientId); + authorityInstance.ChangeOwnership(thirdClient.LocalClientId); - // Verify the session owner cannot assign a SessionOwner permission NetworkObject to a non-sessionowner client - Assert.IsTrue(daHostInstanceHelper.OwnershipPermissionsFailureStatus == NetworkObject.OwnershipPermissionsFailureStatus.SessionOwnerOnly, - $"Expected {daHostInstance.name} to return {NetworkObject.OwnershipPermissionsFailureStatus.SessionOwnerOnly} but its permission failure" + - $" status is {daHostInstanceHelper.OwnershipPermissionsFailureStatus}!"); + // Verify the session owner cannot assign a SessionOwner permission NetworkObject to a non-authority client + Assert.IsTrue(authorityInstanceHelper.OwnershipPermissionsFailureStatus == NetworkObject.OwnershipPermissionsFailureStatus.SessionOwnerOnly, + $"Expected {authorityInstance.name} to return {NetworkObject.OwnershipPermissionsFailureStatus.SessionOwnerOnly} but its permission failure" + + $" status is {authorityInstanceHelper.OwnershipPermissionsFailureStatus}!"); // Remove status - daHostInstance.RemoveOwnershipStatus(NetworkObject.OwnershipStatus.SessionOwner); + authorityInstance.RemoveOwnershipStatus(NetworkObject.OwnershipStatus.SessionOwner); // Validate the permissions value for all instances are the same. yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); - AssertOnTimeout($"[Remove][Permissions Mismatch] {daHostInstance.name}: \n {m_ErrorLog}"); + AssertOnTimeout($"[Remove][Permissions Mismatch] {authorityInstance.name}"); } [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; + var authority = GetAuthorityNetworkManager(); + var initialLogLevel = authority.LogLevel; + authority.LogLevel = LogLevel.Developer; + + var authorityInstance = SpawnObject(m_PermissionsObject, authority).GetComponent(); + OwnershipPermissionsTestHelper.CurrentOwnedInstance = authorityInstance; m_ObjectToValidate = OwnershipPermissionsTestHelper.CurrentOwnedInstance; - yield return WaitForConditionOrTimeOut(ValidateObjectSpawnedOnAllClients); - AssertOnTimeout($"[Failed To Spawn] {firstInstance.name}: \n {m_ErrorLog}"); - firstInstance.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable, true); + yield return WaitForSpawnedOnAllOrTimeOut(authorityInstance); + AssertOnTimeout($"[Failed To Spawn] {authorityInstance.name}"); + + authorityInstance.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}"); + AssertOnTimeout($"[Permissions Mismatch] {authorityInstance.name}"); - var secondInstance = m_ClientNetworkManagers[0].SpawnManager.SpawnedObjects[networkObjectId]; + var otherClient = GetNonAuthorityNetworkManager(0); + var otherInstance = otherClient.SpawnManager.SpawnedObjects[authorityInstance.NetworkObjectId]; // Remove the client from the observers list - firstInstance.Observers.Remove(m_ClientNetworkManagers[0].LocalClientId); + authorityInstance.Observers.Remove(otherClient.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!"); + authorityInstance.ChangeOwnership(otherClient.LocalClientId); + var senderId = authority.LocalClientId; + var receiverId = otherClient.LocalClientId; + LogAssert.Expect(LogType.Warning, $"[Session-Owner Sender={senderId}] [Invalid Owner] Cannot send Ownership change as client-{receiverId} cannot see {authorityInstance.name}! Use NetworkShow first."); + Assert.True(authorityInstance.IsOwner, $"[Ownership Check] Client-{senderId} 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); + authorityInstance.Observers.Add(otherClient.LocalClientId); + authorityInstance.ChangeOwnership(otherClient.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!"); + // Validate the non-authority client now owns the object + yield return WaitForConditionOrTimeOut(() => otherInstance.IsOwner); + AssertOnTimeout($"[Acquire Ownership Failed] Client-{otherClient.LocalClientId} failed to get ownership!"); + + authority.LogLevel = initialLogLevel; + } + + private IEnumerator ValidateRequestAndPermissionsChangeRaceCondition(NetworkObject.OwnershipStatus newStatus, NetworkObject.OwnershipLockActions lockActions, NetworkObject.OwnershipRequestResponseStatus expectedResponseStatus, NetworkManager firstClient, NetworkObject firstInstance, NetworkObject secondInstance, OwnershipPermissionsTestHelper secondInstanceHelper, bool setFlagsFirst = true) + { + var secondClientId = secondInstance.NetworkManager.LocalClientId; + + if (setFlagsFirst) + { + firstInstance.SetOwnershipStatus(newStatus, true, lockActions); + } - m_ServerNetworkManager.LogLevel = initialLogLevel; + // Request ownership + var requestStatus = secondInstance.RequestOwnership(); + + if (!setFlagsFirst) + { + firstInstance.SetOwnershipStatus(newStatus, true, lockActions); + + } + + // We expect the request should be able to be sent at this time as well (i.e. creates the race condition between request and permissions change) + Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"[{newStatus}] Client-{firstClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!"); + + yield return WaitForConditionOrTimeOut(() => secondInstanceHelper.OwnershipRequestResponseStatus == expectedResponseStatus); + AssertOnTimeout($"[{newStatus}][Request race condition failed] Client-{secondClientId} did not get the right request response status! Expected: {expectedResponseStatus} Received: {secondInstanceHelper.OwnershipRequestResponseStatus}!"); + + var expectedOwner = expectedResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Approved ? secondClientId : firstClient.LocalClientId; + yield return WaitForConditionOrTimeOut(() => ValidateAllInstancesAreOwnedByClient(expectedOwner)); + AssertOnTimeout($"[{newStatus}][Request race condition][Ownership Mismatch] Expected Client-{expectedOwner} to have ownership: \n {m_ErrorLog}"); + + // Owner permissions should prevail - ownership shouldn't change + yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); + AssertOnTimeout($"[{newStatus}][Unlock][Race condition permissions mismatch] {secondInstance.name}"); + + // Reset the permissions to requestRequired + var finalInstance = expectedResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Approved ? secondInstance : firstInstance; + + finalInstance.SetOwnershipStatus(NetworkObject.OwnershipStatus.RequestRequired, true); + yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients); + AssertOnTimeout($"[{newStatus}][Set RequestRequired][Permissions mismatch] {firstClient.name}"); } internal class OwnershipPermissionsTestHelper : NetworkBehaviour diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs index bc647dffde..bb3f29bd7a 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs @@ -1891,7 +1891,7 @@ public bool WaitForConditionOrTimeOutWithTimeTravel(IConditionalPredicate condit } /// - /// Waits until the specified condition returns true or a timeout occurs, then asserts if the timeout was reached. + /// Waits until the specified condition returns true or a timeout occurs. /// This overload allows the condition to provide additional error details via a . /// /// A delegate that takes a for error details and returns true when the desired condition is met. @@ -1907,6 +1907,42 @@ protected IEnumerator WaitForConditionOrTimeOut(Func checkF }, timeOutHelper); } + /// + /// Waits until the given NetworkObject is spawned on all clients or a timeout occurs. + /// + /// The id of the to wait for. + /// An optional to control the timeout period. If null, the default timeout is used. + /// An for use in Unity coroutines. + protected IEnumerator WaitForSpawnedOnAllOrTimeOut(ulong networkObjectId, TimeoutHelper timeOutHelper = null) + { + bool ValidateObjectSpawnedOnAllClients(StringBuilder errorLog) + { + foreach (var client in m_NetworkManagers) + { + if (!client.SpawnManager.SpawnedObjects.ContainsKey(networkObjectId)) + { + errorLog.Append($"Client-{client.LocalClientId} has not spawned Object-{networkObjectId}!"); + return false; + } + } + return true; + } + + yield return WaitForConditionOrTimeOut(ValidateObjectSpawnedOnAllClients, timeOutHelper); + } + + /// + /// Waits until the given NetworkObject is spawned on all clients or a timeout occurs. + /// + /// The to wait for. + /// An optional to control the timeout period. If null, the default timeout is used. + /// An for use in Unity coroutines. + protected IEnumerator WaitForSpawnedOnAllOrTimeOut(NetworkObject networkObject, TimeoutHelper timeOutHelper = null) + { + var networkObjectId = networkObject.GetComponent().NetworkObjectId; + yield return WaitForSpawnedOnAllOrTimeOut(networkObjectId, timeOutHelper); + } + /// /// Validates that all remote clients (i.e. non-server) detect they are connected /// to the server and that the server reflects the appropriate number of clients @@ -2206,6 +2242,7 @@ private List SpawnObjects(NetworkObject prefabNetworkObject, Network return gameObjectsSpawned; } + /// /// Default constructor ///