From de0fd5548b6b47b47cca16bbd4700e9fff9f0115 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Thu, 1 May 2025 12:51:06 -0500 Subject: [PATCH 1/6] fix These changes are the backport fix of #3347. --- .../Runtime/Core/NetworkBehaviour.cs | 11 ++++++- .../Runtime/Core/NetworkBehaviourUpdater.cs | 3 -- .../Runtime/Core/NetworkObject.cs | 31 +++++++++++++++++-- .../Messages/ChangeOwnershipMessage.cs | 6 +--- .../NetworkVariable/NetworkVariable.cs | 7 +++++ .../NetworkVariable/NetworkVariableBase.cs | 9 ++++++ .../Runtime/Spawning/NetworkSpawnManager.cs | 20 +++++++++--- 7 files changed, 71 insertions(+), 16 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index 97e446de4b..a1e0278369 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -1091,7 +1091,12 @@ internal void MarkVariablesDirty(bool dirty) } } - internal void MarkOwnerReadVariablesDirty() + /// + /// For owner read permissions, when changing ownership we need to do a full synchronization + /// of all NetworkVariables that are owner read permission based since the owner is the only + /// instance that knows what the most current values are. + /// + internal void MarkOwnerReadDirtyAndCheckOwnerWriteIsDirty() { for (int j = 0; j < NetworkVariableFields.Count; j++) { @@ -1099,6 +1104,10 @@ internal void MarkOwnerReadVariablesDirty() { NetworkVariableFields[j].SetDirty(true); } + if (NetworkVariableFields[j].WritePerm == NetworkVariableWritePermission.Owner) + { + NetworkVariableFields[j].OnCheckIsDirtyState(); + } } } diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs index 22b8c2d09c..524f6a2025 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs @@ -114,9 +114,6 @@ internal void NetworkBehaviourUpdate(bool forceSend = false) { behaviour.PostNetworkVariableWrite(forceSend); } - - // Once done processing, we set the previous owner id to the current owner id - dirtyObj.PreviousOwnerId = dirtyObj.OwnerClientId; } m_DirtyNetworkObjects.Clear(); } diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index d1942f6df0..38491c49d6 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -1485,12 +1485,39 @@ internal void MarkVariablesDirty(bool dirty) } } - internal void MarkOwnerReadVariablesDirty() + /// + /// Used when changing ownership, this will mark any owner read permission base NetworkVariables as dirty + /// and will check if any owner write permission NetworkVariables are dirty (primarily for collections) so + /// the new owner will get a full state update prior to changing ownership. + /// + /// + /// We have to pass in the original owner and previous owner to "reset" back to the current state of this + /// NetworkObject in order to preserve the same ownership change flow. By the time this is invoked, the + /// new and previous owner ids have already been set. + /// + /// the owner prior to beginning the change in ownership change. + /// the previous owner prior to beginning the change in ownership change. + internal void SynchronizeOwnerNetworkVariables(ulong originalOwnerId, ulong originalPreviousOwnerId) { + var currentOwnerId = OwnerClientId; + OwnerClientId = originalOwnerId; + PreviousOwnerId = originalPreviousOwnerId; for (int i = 0; i < ChildNetworkBehaviours.Count; i++) { - ChildNetworkBehaviours[i].MarkOwnerReadVariablesDirty(); + ChildNetworkBehaviours[i].MarkOwnerReadDirtyAndCheckOwnerWriteIsDirty(); } + + // Now set the new owner and previous owner identifiers back to their original new values + // before we run the NetworkBehaviourUpdate. For owner read only permissions this order of + // operations is **particularly important** as we need to first (above) mark things as dirty + // from the context of the original owner and then second (below) we need to send the messages + // which requires the new owner to be set for owner read permission NetworkVariables. + OwnerClientId = currentOwnerId; + PreviousOwnerId = originalOwnerId; + + // Force send a state update for all owner read NetworkVariables and any currently dirty + // owner write NetworkVariables. + NetworkManager.BehaviourUpdater.NetworkBehaviourUpdate(true); } // NGO currently guarantees that the client will receive spawn data for all objects in one network tick. diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs index 43949baedc..e4a5b35a52 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs @@ -62,11 +62,7 @@ public void Handle(ref NetworkContext context) if (originalOwner == networkManager.LocalClientId) { - // Mark any owner read variables as dirty - networkObject.MarkOwnerReadVariablesDirty(); - // Immediately queue any pending deltas and order the message before the - // change in ownership message. - networkManager.BehaviourUpdater.NetworkBehaviourUpdate(true); + networkObject.SynchronizeOwnerNetworkVariables(originalOwner, networkObject.PreviousOwnerId); } networkObject.InvokeOwnershipChanged(originalOwner, OwnerClientId); diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs index 6d1d370316..5df83215b1 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs @@ -166,6 +166,13 @@ public bool CheckDirtyState(bool forceCheck = false) return isDirty; } + /// + internal override void OnCheckIsDirtyState() + { + CheckDirtyState(); + base.OnCheckIsDirtyState(); + } + internal ref T RefValue() { return ref m_InternalValue; diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs index 97b13192d6..377fadeee0 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs @@ -345,6 +345,15 @@ internal ulong OwnerClientId() return m_NetworkBehaviour.NetworkObject.OwnerClientId; } + /// + /// Primarily to check for collections dirty states when doing + /// a fully owner read/write NetworkVariable update. + /// + internal virtual void OnCheckIsDirtyState() + { + + } + /// /// Writes the dirty changes, that is, the changes since the variable was last dirty, to the writer /// diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index c6b6444567..e6dd8879bd 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -250,6 +250,17 @@ internal void RemoveOwnership(NetworkObject networkObject) internal void ChangeOwnership(NetworkObject networkObject, ulong clientId) { + + if (clientId == networkObject.OwnerClientId) + { + if (NetworkManager.LogLevel <= LogLevel.Developer) + { + Debug.LogWarning($"[{nameof(NetworkSpawnManager)}][{nameof(ChangeOwnership)}] Attempting to change ownership to Client-{clientId} when the owner is already {networkObject.OwnerClientId}! (Ignoring)"); + + } + return; + } + // If ownership changes faster than the latency between the client-server and there are NetworkVariables being updated during ownership changes, // then notify the user they could potentially lose state updates if developer logging is enabled. if (m_LastChangeInOwnership.ContainsKey(networkObject.NetworkObjectId) && m_LastChangeInOwnership[networkObject.NetworkObjectId] > Time.realtimeSinceStartup) @@ -278,6 +289,8 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId) { throw new SpawnStateException("Object is not spawned"); } + var originalPreviousOwnerId = networkObject.PreviousOwnerId; + var originalOwner = networkObject.OwnerClientId; // Used to distinguish whether a new owner should receive any currently dirty NetworkVariable updates networkObject.PreviousOwnerId = networkObject.OwnerClientId; @@ -294,13 +307,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId) // Always notify locally on the server when a new owner is assigned networkObject.InvokeBehaviourOnGainedOwnership(); + // If we are the original owner, then we want to synchronize owner read & write NetworkVariables. if (networkObject.PreviousOwnerId == NetworkManager.LocalClientId) { - // Mark any owner read variables as dirty - networkObject.MarkOwnerReadVariablesDirty(); - // Immediately queue any pending deltas and order the message before the - // change in ownership message. - NetworkManager.BehaviourUpdater.NetworkBehaviourUpdate(true); + networkObject.SynchronizeOwnerNetworkVariables(originalOwner, originalPreviousOwnerId); } var message = new ChangeOwnershipMessage From 39f689e23b24e9f61024b4c25a409ca64aa3fcf6 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Thu, 1 May 2025 12:51:44 -0500 Subject: [PATCH 2/6] test This is the backport of the TestAuthorityChangingOwnership test from #3347. --- .../NetworkObjectOwnershipTests.cs | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs index 5fa49158e7..aed25fd216 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Linq; using NUnit.Framework; +using Unity.Netcode.Components; using Unity.Netcode.TestHelpers.Runtime; using UnityEngine; using UnityEngine.TestTools; @@ -23,11 +24,23 @@ public override void OnGainedOwnership() OnGainedOwnershipFired = true; } + protected override void OnOwnershipChanged(ulong previous, ulong current) + { + Assert.True(previous != current, $"[{nameof(OnOwnershipChanged)}][Invalid Parameters] Invoked and the previous ({previous}) equals the current ({current})!"); + base.OnOwnershipChanged(previous, current); + } + public void ResetFlags() { OnLostOwnershipFired = false; OnGainedOwnershipFired = false; } + + [Rpc(SendTo.Server)] + public void ChangeOwnershipRpc(RpcParams rpcParams = default) + { + NetworkObject.ChangeOwnership(rpcParams.Receive.SenderClientId); + } } [TestFixture(HostOrServer.Host)] @@ -52,6 +65,7 @@ protected override void OnServerAndClientsCreated() { m_OwnershipPrefab = CreateNetworkObjectPrefab("OnwershipPrefab"); m_OwnershipPrefab.AddComponent(); + m_OwnershipPrefab.AddComponent(); base.OnServerAndClientsCreated(); } @@ -358,5 +372,75 @@ public IEnumerator TestOwnedObjectCounts() AssertOnTimeout($"Server does not have the correct count for all clients spawned {k_NumberOfSpawnedObjects} {nameof(NetworkObject)}s!"); } + + /// + /// Validates that when changing ownership NetworkTransform does not enter into a bad state + /// because the previous and current owner identifiers are the same. For client-server this + /// ends up always being the server, but for distributed authority the authority changes when + /// ownership changes. + /// + [UnityTest] + public IEnumerator TestAuthorityChangingOwnership() + { + var authorityManager = m_ServerNetworkManager; ; + var allNetworkManagers = m_ClientNetworkManagers.ToList(); + allNetworkManagers.Add(m_ServerNetworkManager); + + m_OwnershipObject = SpawnObject(m_OwnershipPrefab, m_ServerNetworkManager); + m_OwnershipNetworkObject = m_OwnershipObject.GetComponent(); + var ownershipNetworkObjectId = m_OwnershipNetworkObject.NetworkObjectId; + bool WaitForClientsToSpawnNetworkObject() + { + foreach (var clientNetworkManager in m_ClientNetworkManagers) + { + if (!clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(ownershipNetworkObjectId)) + { + return false; + } + } + return true; + } + + yield return WaitForConditionOrTimeOut(WaitForClientsToSpawnNetworkObject); + AssertOnTimeout($"Timed out waiting for all clients to spawn the {m_OwnershipNetworkObject.name} {nameof(NetworkObject)} instance!"); + + var currentTargetOwner = (ulong)0; + bool WaitForAllInstancesToChangeOwnership() + { + foreach (var clientNetworkManager in m_ClientNetworkManagers) + { + if (!clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(ownershipNetworkObjectId)) + { + return false; + } + if (clientNetworkManager.SpawnManager.SpawnedObjects[ownershipNetworkObjectId].OwnerClientId != currentTargetOwner) + { + return false; + } + } + return true; + } + + // Change ownership a few times and as long as the previous and current owners are not the same when + // OnOwnershipChanged is invoked then the test passed. + foreach (var networkManager in allNetworkManagers) + { + if (networkManager == authorityManager) + { + continue; + } + var clonedObject = networkManager.SpawnManager.SpawnedObjects[ownershipNetworkObjectId]; + + if (clonedObject.OwnerClientId == networkManager.LocalClientId) + { + continue; + } + + var testComponent = clonedObject.GetComponent(); + testComponent.ChangeOwnershipRpc(); + yield return WaitForAllInstancesToChangeOwnership(); + AssertOnTimeout($"Timed out waiting for all instances to change ownership to Client-{networkManager.LocalClientId}!"); + } + } } } From a1f5f6d0c63506cdd188027385dd9f6cff6faa04 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Thu, 1 May 2025 12:56:12 -0500 Subject: [PATCH 3/6] update adding changelog entry. --- com.unity.netcode.gameobjects/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 166b9b3710..4e011d8b95 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,6 +12,8 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed issue where when a client changes ownership via RPC the `NetworkBehaviour.OnOwnershipChanged` can result in identical previous and current owner identifiers. (#3434) + ### Changed ## [1.13.0] - 2025-04-29 From e77c1091fa19f622a502cb35446b1bf2d7888e64 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Thu, 1 May 2025 13:01:40 -0500 Subject: [PATCH 4/6] fix Adding inheritdoc for the addition of OnOwnershipChanged. Making ChangeOwnershipRpc internal to avoid required XML API PVP check. --- .../Runtime/NetworkObject/NetworkObjectOwnershipTests.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs index aed25fd216..5ae1110686 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs @@ -14,16 +14,22 @@ public class NetworkObjectOwnershipComponent : NetworkBehaviour public bool OnLostOwnershipFired = false; public bool OnGainedOwnershipFired = false; + + /// public override void OnLostOwnership() { OnLostOwnershipFired = true; } + + /// public override void OnGainedOwnership() { OnGainedOwnershipFired = true; } + + /// protected override void OnOwnershipChanged(ulong previous, ulong current) { Assert.True(previous != current, $"[{nameof(OnOwnershipChanged)}][Invalid Parameters] Invoked and the previous ({previous}) equals the current ({current})!"); @@ -37,7 +43,7 @@ public void ResetFlags() } [Rpc(SendTo.Server)] - public void ChangeOwnershipRpc(RpcParams rpcParams = default) + internal void ChangeOwnershipRpc(RpcParams rpcParams = default) { NetworkObject.ChangeOwnership(rpcParams.Receive.SenderClientId); } From 88c3119e5bc743d1799bdaca4c91f3f1a88a269e Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Thu, 1 May 2025 13:06:36 -0500 Subject: [PATCH 5/6] fix - PVP Adding "returns" for TestAuthorityChangingOwnership --- .../Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs index 5ae1110686..1e869103ec 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs @@ -385,6 +385,7 @@ public IEnumerator TestOwnedObjectCounts() /// ends up always being the server, but for distributed authority the authority changes when /// ownership changes. /// + /// [UnityTest] public IEnumerator TestAuthorityChangingOwnership() { From 1127a3cac4c4846ba0562a2d4754e3ffabb12116 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Thu, 1 May 2025 14:54:07 -0500 Subject: [PATCH 6/6] fix Backporting a fix for DeferredMessagingTest that was already applied to the v2.x branch awhile back. --- .../Tests/Runtime/DeferredMessagingTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DeferredMessagingTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DeferredMessagingTests.cs index 550118b285..24626d8400 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DeferredMessagingTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DeferredMessagingTests.cs @@ -1269,7 +1269,9 @@ public void WhenADeferredMessageIsRemoved_OtherMessagesForDifferentObjectsAreNot Assert.AreEqual(0, manager.DeferredMessageCountForKey(IDeferredNetworkMessageManager.TriggerType.OnSpawn, serverObject2.GetComponent().NetworkObjectId)); } - serverObject2.GetComponent().ChangeOwnership(m_ServerNetworkManager.LocalClientId); + // Changing ownership when the owner specified is already an owner should not send any messages + // The original test was changing ownership to the server when the object was spawned with the server being an owner. + serverObject2.GetComponent().ChangeOwnership(m_ClientNetworkManagers[1].LocalClientId); WaitForAllClientsToReceive(); foreach (var client in m_ClientNetworkManagers)