From 5fdeaab7ef6855d64da5551bf03f352402f88d26 Mon Sep 17 00:00:00 2001 From: Emma Date: Wed, 3 Sep 2025 17:18:26 -0400 Subject: [PATCH 1/6] chore: Update the OwnershipPermissionsTests to work with rust server --- .../OwnershipPermissionsTests.cs | 266 +++++++++++------- .../TestHelpers/NetcodeIntegrationTest.cs | 29 +- .../NetcodeIntegrationTestHelpers.cs | 2 +- 3 files changed, 186 insertions(+), 111 deletions(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs index b87b5a529a..7f805a7bee 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs @@ -17,12 +17,6 @@ internal class OwnershipPermissionsTests : IntegrationTestWithApproximation protected override int NumberOfClients => 4; - // TODO: [CmbServiceTests] Adapt to run with the service - daHostInstance will be firstInstance with cmbService - protected override bool UseCMBService() - { - return false; - } - public OwnershipPermissionsTests() : base(HostOrServer.DAHost) { } @@ -44,37 +38,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 +77,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 +110,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 +120,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 +142,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 +153,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 +167,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 +193,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 +208,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 +273,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 +293,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 +372,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); + } + + // 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}!"); - m_ServerNetworkManager.LogLevel = initialLogLevel; + 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..503fddc498 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,32 @@ protected IEnumerator WaitForConditionOrTimeOut(Func checkF }, timeOutHelper); } + /// + /// Waits until the given NetworkObject is spawned on all clients or a timeout occurs. + /// + /// The to watch 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 instanceNetworkObject, TimeoutHelper timeOutHelper = null) + { + var networkObjectId = instanceNetworkObject.GetComponent().NetworkObjectId; + + 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); + } + /// /// 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 +2232,7 @@ private List SpawnObjects(NetworkObject prefabNetworkObject, Network return gameObjectsSpawned; } + /// /// Default constructor /// diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs index b33f47cd01..687753d1bd 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs @@ -197,7 +197,7 @@ internal static string GetCMBServiceEnvironentVariable() #if USE_CMB_SERVICE return "true"; #else - return Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "false"; + return Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "true"; #endif } From c5997cca99d1b42ca4b6d03df65321dbccbdbe8a Mon Sep 17 00:00:00 2001 From: Emma Date: Wed, 3 Sep 2025 20:09:05 -0400 Subject: [PATCH 2/6] chore: Move ChangeOwnership OwnershipMessageTypeFlags to an enum --- .../Runtime/Core/NetworkObject.cs | 16 +- .../Messages/ChangeOwnershipMessage.cs | 326 ++++++++---------- .../Runtime/Spawning/NetworkSpawnManager.cs | 4 +- 3 files changed, 148 insertions(+), 198 deletions(-) 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..54e0b4cc02 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs @@ -1,4 +1,6 @@ +using System.Runtime.CompilerServices; + namespace Unity.Netcode { internal struct ChangeOwnershipMessage : INetworkMessage, INetworkSerializeByMemcpy @@ -25,89 +27,15 @@ internal struct ChangeOwnershipMessage : INetworkMessage, INetworkSerializeByMem internal bool DistributedAuthorityMode; internal ushort OwnershipFlags; internal byte OwnershipRequestResponseStatus; - private byte m_OwnershipMessageTypeFlags; - - 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 ChangeType ChangeMessageType; - internal bool OwnershipFlagsUpdate + internal enum ChangeType : byte { - get - { - return GetFlag(k_OwnershipFlagsUpdate); - } - - set - { - SetFlag(value, k_OwnershipFlagsUpdate); - } - } - - internal bool RequestOwnership - { - 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 = 1, + OwnershipFlagsUpdate = 2, + RequestOwnership = 4, + RequestApproved = 8, + RequestDenied = 10, } public void Serialize(FastBufferWriter writer, int targetVersion) @@ -129,20 +57,21 @@ public void Serialize(FastBufferWriter writer, int targetVersion) } } - writer.WriteValueSafe(m_OwnershipMessageTypeFlags); - if (OwnershipFlagsUpdate || OwnershipIsChanging) + writer.WriteValueSafe(ChangeMessageType); + + if (ChangeMessageType is ChangeType.OwnershipFlagsUpdate or ChangeType.OwnershipChanging or 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 is ChangeType.RequestOwnership or ChangeType.RequestApproved or ChangeType.RequestDenied) { writer.WriteValueSafe(RequestClientId); - if (RequestDenied) + if (ChangeMessageType is ChangeType.RequestDenied) { writer.WriteValueSafe(OwnershipRequestResponseStatus); } @@ -174,25 +103,32 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int } } - reader.ReadValueSafe(out m_OwnershipMessageTypeFlags); - if (OwnershipFlagsUpdate || OwnershipIsChanging) + reader.ReadValueSafe(out ChangeMessageType); + if (ChangeMessageType is ChangeType.OwnershipFlagsUpdate or ChangeType.OwnershipChanging or 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 is ChangeType.RequestOwnership or ChangeType.RequestApproved or 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 is 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 +149,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 is ChangeType.OwnershipChanging or ChangeType.RequestApproved) { HandleOwnershipChange(ref context); } @@ -297,7 +173,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 +181,17 @@ private void HandleExtendedOwnershipUpdate(ref NetworkContext context) // Handle the extended ownership message types var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId]; - if (OwnershipFlagsUpdate) + if (ChangeMessageType is ChangeType.OwnershipFlagsUpdate) { // Just update the ownership flags networkObject.Ownership = (NetworkObject.OwnershipStatus)OwnershipFlags; } - else if (RequestOwnership) + else if (ChangeMessageType is ChangeType.RequestOwnership) { // Requesting ownership, if allowed it will automatically send the ownership change message networkObject.OwnershipRequest(RequestClientId); } - else if (RequestDenied) + else if (ChangeMessageType is ChangeType.RequestDenied) { networkObject.OwnershipRequestResponse((NetworkObject.OwnershipRequestResponseStatus)OwnershipRequestResponseStatus); } @@ -324,7 +200,8 @@ 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,28 +218,29 @@ 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) { @@ -371,19 +249,18 @@ private void HandleOwnershipChange(ref NetworkContext context) networkObject.ChildNetworkBehaviours[i].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 +270,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 +286,78 @@ 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, so exit early + 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, so exit early + return false; + } + // Otherwise, fall through and process the request. + } + else + { + foreach (var clientId in clientList) + { + if (clientId == networkManager.LocalClientId) + { + continue; + } + + switch (ChangeMessageType) + { + // If ownership is changing and this is not an ownership request approval then ignore the SenderId + case ChangeType.OwnershipChanging when senderId == clientId: + // If it is just updating flags then ignore sending to the owner + case ChangeType.OwnershipFlagsUpdate when clientId == OwnerClientId: + // If it is a request approval, then ignore the RequestClientId + case ChangeType.RequestApproved when clientId == RequestClientId: + 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..2d4b173e2c 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -572,11 +572,10 @@ 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, RequestClientId = networkObject.PreviousOwnerId, OwnershipFlags = (ushort)networkObject.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, }; From 6b5158c6cba3afd58a52f54d09d7e896c9e4d7e7 Mon Sep 17 00:00:00 2001 From: Emma Date: Fri, 5 Sep 2025 17:39:33 -0400 Subject: [PATCH 3/6] Remove syntax --- .../Messages/ChangeOwnershipMessage.cs | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs index 54e0b4cc02..83d099bb59 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs @@ -1,4 +1,3 @@ - using System.Runtime.CompilerServices; namespace Unity.Netcode @@ -31,11 +30,11 @@ internal struct ChangeOwnershipMessage : INetworkMessage, INetworkSerializeByMem internal enum ChangeType : byte { - OwnershipChanging = 1, - OwnershipFlagsUpdate = 2, - RequestOwnership = 4, - RequestApproved = 8, - RequestDenied = 10, + OwnershipChanging = 0x01, + OwnershipFlagsUpdate = 0x02, + RequestOwnership = 0x04, + RequestApproved = 0x08, + RequestDenied = 0x10, } public void Serialize(FastBufferWriter writer, int targetVersion) @@ -57,9 +56,7 @@ public void Serialize(FastBufferWriter writer, int targetVersion) } } - writer.WriteValueSafe(ChangeMessageType); - - if (ChangeMessageType is ChangeType.OwnershipFlagsUpdate or ChangeType.OwnershipChanging or ChangeType.RequestApproved) + if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate || ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved) { writer.WriteValueSafe(OwnershipFlags); } @@ -67,7 +64,7 @@ public void Serialize(FastBufferWriter writer, int targetVersion) // When requesting, RequestClientId is the requestor // When approving, RequestClientId is the owner that approved // When denied, RequestClientId is the requestor - if (ChangeMessageType is ChangeType.RequestOwnership or ChangeType.RequestApproved or ChangeType.RequestDenied) + if (ChangeMessageType == ChangeType.RequestOwnership || ChangeMessageType == ChangeType.RequestApproved || ChangeMessageType == ChangeType.RequestDenied) { writer.WriteValueSafe(RequestClientId); @@ -95,16 +92,15 @@ 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 ChangeMessageType); - if (ChangeMessageType is ChangeType.OwnershipFlagsUpdate or ChangeType.OwnershipChanging or ChangeType.RequestApproved) + if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate || ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved) { reader.ReadValueSafe(out OwnershipFlags); } @@ -112,12 +108,12 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int // When requesting, RequestClientId is the requestor // When approving, RequestClientId is the owner that approved // When denied, RequestClientId is the requestor - if (ChangeMessageType is ChangeType.RequestOwnership or ChangeType.RequestApproved or ChangeType.RequestDenied) + 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 (ChangeMessageType is ChangeType.RequestDenied) + if (ChangeMessageType == ChangeType.RequestDenied) { reader.ReadValueSafe(out OwnershipRequestResponseStatus); } @@ -159,7 +155,7 @@ public void Handle(ref NetworkContext context) // 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, ChangeMessageType will always be OwnershipChanging. - if (ChangeMessageType is ChangeType.OwnershipChanging or ChangeType.RequestApproved) + if (ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved) { HandleOwnershipChange(ref context); } @@ -181,17 +177,17 @@ private void HandleExtendedOwnershipUpdate(ref NetworkContext context) // Handle the extended ownership message types var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId]; - if (ChangeMessageType is ChangeType.OwnershipFlagsUpdate) + if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate) { // Just update the ownership flags networkObject.Ownership = (NetworkObject.OwnershipStatus)OwnershipFlags; } - else if (ChangeMessageType is ChangeType.RequestOwnership) + else if (ChangeMessageType == ChangeType.RequestOwnership) { // Requesting ownership, if allowed it will automatically send the ownership change message networkObject.OwnershipRequest(RequestClientId); } - else if (ChangeMessageType is ChangeType.RequestDenied) + else if (ChangeMessageType == ChangeType.RequestDenied) { networkObject.OwnershipRequestResponse((NetworkObject.OwnershipRequestResponseStatus)OwnershipRequestResponseStatus); } @@ -201,7 +197,6 @@ 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; @@ -244,9 +239,9 @@ private void HandleOwnershipChange(ref NetworkContext context) // 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(); } } From c69afe155f46773343fd57dd4ef0fa50a643c6ec Mon Sep 17 00:00:00 2001 From: Emma Date: Tue, 9 Sep 2025 13:48:27 -0400 Subject: [PATCH 4/6] Fix tests --- .../Messages/ChangeOwnershipMessage.cs | 25 +++++++------------ .../Runtime/Spawning/NetworkSpawnManager.cs | 4 +-- .../OwnershipPermissionsTests.cs | 6 +++++ .../NetcodeIntegrationTestHelpers.cs | 2 +- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs index 83d099bb59..c3b9fd559f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs @@ -56,6 +56,8 @@ public void Serialize(FastBufferWriter writer, int targetVersion) } } + writer.WriteValueSafe(ChangeMessageType); + if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate || ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved) { writer.WriteValueSafe(OwnershipFlags); @@ -155,7 +157,7 @@ public void Handle(ref NetworkContext context) // 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, ChangeMessageType will always be OwnershipChanging. - if (ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved) + if (ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved || !networkManager.DistributedAuthorityMode) { HandleOwnershipChange(ref context); } @@ -312,7 +314,8 @@ private bool HandleDAHostMessageForwarding(ref NetworkManager networkManager, ul { 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 + + // We don't want the local DAHost's client to process this message return false; } } @@ -322,31 +325,21 @@ private bool HandleDAHostMessageForwarding(ref NetworkManager networkManager, ul 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 + + // We don't want the local DAHost's client to process this message return false; } - // Otherwise, fall through and process the request. } else { foreach (var clientId in clientList) { - if (clientId == networkManager.LocalClientId) + // Don't forward to self or originating client + if (clientId == networkManager.LocalClientId || clientId == senderId) { continue; } - switch (ChangeMessageType) - { - // If ownership is changing and this is not an ownership request approval then ignore the SenderId - case ChangeType.OwnershipChanging when senderId == clientId: - // If it is just updating flags then ignore sending to the owner - case ChangeType.OwnershipFlagsUpdate when clientId == OwnerClientId: - // If it is a request approval, then ignore the RequestClientId - case ChangeType.RequestApproved when clientId == RequestClientId: - continue; - } - networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, clientId); } } diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index 2d4b173e2c..69376d8ed4 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -575,12 +575,12 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool ChangeMessageType = isRequestApproval ? ChangeOwnershipMessage.ChangeType.RequestApproved : ChangeOwnershipMessage.ChangeType.OwnershipChanging, NetworkObjectId = networkObject.NetworkObjectId, OwnerClientId = networkObject.OwnerClientId, - DistributedAuthorityMode = NetworkManager.DistributedAuthorityMode, + 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 diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs index 7f805a7bee..d4cf1d1f8a 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs @@ -17,6 +17,12 @@ internal class OwnershipPermissionsTests : IntegrationTestWithApproximation protected override int NumberOfClients => 4; + // TODO: [CmbServiceTests] Remove this once MTTB-1570 is resolved. + protected override bool UseCMBService() + { + return false; + } + public OwnershipPermissionsTests() : base(HostOrServer.DAHost) { } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs index 687753d1bd..b33f47cd01 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs @@ -197,7 +197,7 @@ internal static string GetCMBServiceEnvironentVariable() #if USE_CMB_SERVICE return "true"; #else - return Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "true"; + return Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "false"; #endif } From c9a48468d03df3e201751095874c7df55922760a Mon Sep 17 00:00:00 2001 From: Emma Date: Fri, 12 Sep 2025 20:08:44 -0400 Subject: [PATCH 5/6] Apply feedback --- .../TestHelpers/NetcodeIntegrationTest.cs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs index 503fddc498..bb3f29bd7a 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs @@ -1910,13 +1910,11 @@ protected IEnumerator WaitForConditionOrTimeOut(Func checkF /// /// Waits until the given NetworkObject is spawned on all clients or a timeout occurs. /// - /// The to watch for. + /// 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(NetworkObject instanceNetworkObject, TimeoutHelper timeOutHelper = null) + protected IEnumerator WaitForSpawnedOnAllOrTimeOut(ulong networkObjectId, TimeoutHelper timeOutHelper = null) { - var networkObjectId = instanceNetworkObject.GetComponent().NetworkObjectId; - bool ValidateObjectSpawnedOnAllClients(StringBuilder errorLog) { foreach (var client in m_NetworkManagers) @@ -1933,6 +1931,18 @@ bool ValidateObjectSpawnedOnAllClients(StringBuilder errorLog) 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 From 336ed284110d7f1ee2d62158cbfc40b18bf00b43 Mon Sep 17 00:00:00 2001 From: Emma Date: Mon, 15 Sep 2025 11:54:44 -0400 Subject: [PATCH 6/6] Fix tests --- .yamato/package-pack.yml | 3 +-- .yamato/vetting-test.yml | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.yamato/package-pack.yml b/.yamato/package-pack.yml index 8e8cd73d47..c6c0643889 100644 --- a/.yamato/package-pack.yml +++ b/.yamato/package-pack.yml @@ -37,7 +37,6 @@ package_pack_-_ngo_{{ platform.name }}: variables: XRAY_PROFILE: "gold ./pvpExceptions.json" commands: - - python Tools/scripts/release.py # Needed to ensure that CHANGELOG is properly formatted for this test due to the fact that we have bumped package version (to properly perform vetting tests) - upm-pvp pack "com.unity.netcode.gameobjects" --output upm-ci~/packages - upm-pvp xray --packages "upm-ci~/packages/com.unity.netcode.gameobjects*.tgz" --results pvp-results - upm-pvp require {% if platform.name == "win" %}"%XRAY_PROFILE%"{% else %}"$XRAY_PROFILE"{% endif %} --results pvp-results --allow-missing @@ -48,4 +47,4 @@ package_pack_-_ngo_{{ platform.name }}: packages: paths: - "upm-ci~/**" -{% endfor -%} \ No newline at end of file +{% endfor -%} diff --git a/.yamato/vetting-test.yml b/.yamato/vetting-test.yml index 3fbba0edd3..702d91737c 100644 --- a/.yamato/vetting-test.yml +++ b/.yamato/vetting-test.yml @@ -10,7 +10,6 @@ vetting_test: name: MP Tools - Vetting Test (Win, {{editor}} LTS) agent: { type: Unity::VM, flavor: b1.large, image: package-ci/win11:v4 } commands: - - python Tools/scripts/release.py # Needed to ensure that CHANGELOG is properly formatted for this test - npm install -g "upm-ci-utils@stable" --registry https://artifactory.prd.it.unity3d.com/artifactory/api/npm/upm-npm - unity-downloader-cli --fast --wait --unity-version {{ editor }} --components editor --arch x64 - upm-ci package pack --package-path com.unity.netcode.gameobjects @@ -22,4 +21,4 @@ vetting_test: - test-results/** - upm-ci~/test-results/** - upm-ci~/upm-ci.log -{% endfor -%} \ No newline at end of file +{% endfor -%}