From c958150666a5840daf93b19052d599d042011bdc Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 11 Jul 2023 14:29:45 -0500 Subject: [PATCH 1/8] fix This resolves a few ownership related issues specifically noticed when using an owner authoritative NetworkTransform and ownership is removed. This also assures NetworkVariables will be updated if ownership is removed just like they are when ownership is changed. This also fixes an issue where NetworkManager's ShutdownInProgress was not true upon the application quitting which was causing an exception to occur within NetworkVariableBase under specific conditions. --- .../Runtime/Core/NetworkManager.cs | 2 + .../Runtime/Spawning/NetworkSpawnManager.cs | 39 +++++++++++++------ 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs index b08cb74561..9763930acf 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs @@ -1029,6 +1029,8 @@ internal void ShutdownInternal() // Ensures that the NetworkManager is cleaned up before OnDestroy is run on NetworkObjects and NetworkBehaviours when quitting the application. private void OnApplicationQuit() { + // Make sure ShutdownInProgress returns true during this time + m_ShuttingDown = true; OnDestroy(); } diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index a82aa6b90d..fe9d8ca44e 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -113,12 +113,6 @@ internal void UpdateOwnershipTable(NetworkObject networkObject, ulong newOwner, // Remove the previous owner's entry OwnershipToObjectsTable[previousOwner].Remove(networkObject.NetworkObjectId); - // Server or Host alway invokes the lost ownership notification locally - if (NetworkManager.IsServer) - { - networkObject.InvokeBehaviourOnLostOwnership(); - } - // If we are removing the entry (i.e. despawning or client lost ownership) if (isRemoving) { @@ -143,12 +137,6 @@ internal void UpdateOwnershipTable(NetworkObject networkObject, ulong newOwner, { // Add the new ownership entry OwnershipToObjectsTable[newOwner].Add(networkObject.NetworkObjectId, networkObject); - - // Server or Host always invokes the gained ownership notification locally - if (NetworkManager.IsServer) - { - networkObject.InvokeBehaviourOnGainedOwnership(); - } } else if (isRemoving) { @@ -248,9 +236,23 @@ internal void RemoveOwnership(NetworkObject networkObject) networkObject.OwnerClientId = NetworkManager.ServerClientId; + // If there is no BehaviourUpdater and we are shutting down then return early + if (NetworkManager.BehaviourUpdater == null && NetworkManager.ShutdownInProgress) + { + return; + } + + // This was not being done when ownership was removed but was being done when ownership changed + // Mark variables as dirty and add the NetworkObject for a delta update + networkObject.MarkVariablesDirty(true); + NetworkManager.BehaviourUpdater.AddForUpdate(networkObject); + // Server removes the entry and takes over ownership before notifying UpdateOwnershipTable(networkObject, NetworkManager.ServerClientId, true); + //Notify the local server that it gained ownership + networkObject.InvokeBehaviourOnGainedOwnership(); + var message = new ChangeOwnershipMessage { NetworkObjectId = networkObject.NetworkObjectId, @@ -301,14 +303,27 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId) throw new SpawnStateException("Object is not spawned"); } + // Determine if the server is gong to lose ownership + var serverWillLoseOwnership = networkObject.OwnerClientId == NetworkManager.LocalClientId; + + // Assign the new owner networkObject.OwnerClientId = clientId; + // Notify locally that the server lost ownership + if (serverWillLoseOwnership) + { + networkObject.InvokeBehaviourOnLostOwnership(); + } + networkObject.MarkVariablesDirty(true); NetworkManager.BehaviourUpdater.AddForUpdate(networkObject); // Server adds entries for all client ownership UpdateOwnershipTable(networkObject, networkObject.OwnerClientId); + // Always notify locally on the server when a new owner is assigned + networkObject.InvokeBehaviourOnGainedOwnership(); + var message = new ChangeOwnershipMessage { NetworkObjectId = networkObject.NetworkObjectId, From 90a1b0d505e0b2e5d809068304a5a8c43ed8c268 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 11 Jul 2023 14:41:38 -0500 Subject: [PATCH 2/8] update adding change log entry --- com.unity.netcode.gameobjects/CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 46cc459577..c322f0d249 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -5,6 +5,18 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). Additional documentation and release notes are available at [Multiplayer Documentation](https://docs-multiplayer.unity3d.com). +## [Unreleased] + +### Added + + +### Fixed + +- Fixed issue where removing ownership would not notify the server that it gained ownership. This also resolves the issue where an owner authoritative NetworkTransform would not properly initialize upon removing ownership from a remote client. (#2618) + + +### Changed + ## [1.5.1] - 2023-06-07 From c7503f0fa3811b4af4081956f5c3fba04f2f5aee Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 11 Jul 2023 17:21:13 -0500 Subject: [PATCH 3/8] test Removing a warning expect check that is no longer needed. Adding additional checks to validate RemoveOwnership generates an OnOwnershipChanged notification. --- .../Runtime/NetworkBehaviourGenericTests.cs | 3 +- .../NetworkObjectOwnershipTests.cs | 36 ++++++++++++++++--- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs index cea941a9b4..0f38e697aa 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs @@ -66,8 +66,7 @@ public IEnumerator ValidatedDisableddNetworkBehaviourWarning() // Set the child object to be inactive in the hierarchy childObject.SetActive(false); - - LogAssert.Expect(LogType.Warning, $"{childObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {childBehaviour.GetType().Name} component was skipped during ownership assignment!"); + LogAssert.Expect(LogType.Warning, $"{childObject.name} is disabled! Netcode for GameObjects does not support spawning disabled NetworkBehaviours! The {childBehaviour.GetType().Name} component was skipped during spawn!"); parentNetworkObject.Spawn(); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs index 1f4e3c7471..2aa99a491c 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs @@ -5,6 +5,7 @@ using Unity.Netcode.TestHelpers.Runtime; using UnityEngine; using UnityEngine.TestTools; +using static Unity.Netcode.RuntimeTests.NetworkObjectOwnershipTests; namespace Unity.Netcode.RuntimeTests { @@ -42,6 +43,12 @@ public class NetworkObjectOwnershipTests : NetcodeIntegrationTest public NetworkObjectOwnershipTests(HostOrServer hostOrServer) : base(hostOrServer) { } + public enum OwnershipChecks + { + Change, + Remove + } + protected override void OnServerAndClientsCreated() { m_OwnershipPrefab = CreateNetworkObjectPrefab("OnwershipPrefab"); @@ -62,7 +69,7 @@ public void TestPlayerIsOwned() } [UnityTest] - public IEnumerator TestOwnershipCallbacks() + public IEnumerator TestOwnershipCallbacks([Values] OwnershipChecks ownershipChecks) { m_OwnershipObject = SpawnObject(m_OwnershipPrefab, m_ServerNetworkManager); m_OwnershipNetworkObject = m_OwnershipObject.GetComponent(); @@ -109,7 +116,17 @@ public IEnumerator TestOwnershipCallbacks() serverComponent.ResetFlags(); clientComponent.ResetFlags(); - serverObject.ChangeOwnership(NetworkManager.ServerClientId); + if (ownershipChecks == OwnershipChecks.Change) + { + // Validates that when ownership is changed back to the server it will get an OnGainedOwnership notification + serverObject.ChangeOwnership(NetworkManager.ServerClientId); + } + else + { + // Validates that when ownership is removed the server gets an OnGainedOwnership notification + serverObject.RemoveOwnership(); + } + yield return s_DefaultWaitForTick; Assert.That(serverComponent.OnGainedOwnershipFired); @@ -125,7 +142,7 @@ public IEnumerator TestOwnershipCallbacks() /// Verifies that switching ownership between several clients works properly /// [UnityTest] - public IEnumerator TestOwnershipCallbacksSeveralClients() + public IEnumerator TestOwnershipCallbacksSeveralClients([Values] OwnershipChecks ownershipChecks) { // Build our message hook entries tables so we can determine if all clients received spawn or ownership messages var messageHookEntriesForSpawn = new List(); @@ -247,8 +264,17 @@ bool WaitForClientsToSpawnNetworkObject() previousClientComponent = currentClientComponent; } - // Now change ownership back to the server - serverObject.ChangeOwnership(NetworkManager.ServerClientId); + if (ownershipChecks == OwnershipChecks.Change) + { + // Validates that when ownership is changed back to the server it will get an OnGainedOwnership notification + serverObject.ChangeOwnership(NetworkManager.ServerClientId); + } + else + { + // Validates that when ownership is removed the server gets an OnGainedOwnership notification + serverObject.RemoveOwnership(); + } + yield return WaitForConditionOrTimeOut(ownershipMessageHooks); Assert.False(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for all clients to receive the {nameof(ChangeOwnershipMessage)} message (back to server)."); From 00aba354a801a11f06540cd92b42ec7ccd49c274 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 11 Jul 2023 17:26:40 -0500 Subject: [PATCH 4/8] style removing some whacked auto-inserted using statement that VS 2022 "helped me" with. --- .../Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs | 1 - 1 file changed, 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 2aa99a491c..319415d782 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs @@ -5,7 +5,6 @@ using Unity.Netcode.TestHelpers.Runtime; using UnityEngine; using UnityEngine.TestTools; -using static Unity.Netcode.RuntimeTests.NetworkObjectOwnershipTests; namespace Unity.Netcode.RuntimeTests { From 0be67ed59ba37b8714ded65ee072b69e5fe06f54 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 11 Jul 2023 18:14:52 -0500 Subject: [PATCH 5/8] style removing whitespaces --- .../Tests/Runtime/NetworkBehaviourGenericTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs index 0f38e697aa..ab48aaab09 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs @@ -66,7 +66,7 @@ public IEnumerator ValidatedDisableddNetworkBehaviourWarning() // Set the child object to be inactive in the hierarchy childObject.SetActive(false); - + LogAssert.Expect(LogType.Warning, $"{childObject.name} is disabled! Netcode for GameObjects does not support spawning disabled NetworkBehaviours! The {childBehaviour.GetType().Name} component was skipped during spawn!"); parentNetworkObject.Spawn(); From acd315432cc6dc4074dd49a7e6a57709efed3025 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Wed, 12 Jul 2023 15:27:19 -0500 Subject: [PATCH 6/8] update removing entry --- com.unity.netcode.gameobjects/CHANGELOG.md | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index a7f0ac4e71..5da81ec026 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). Additional documentation and release notes are available at [Multiplayer Documentation](https://docs-multiplayer.unity3d.com). + ## [Unreleased] ### Added @@ -18,15 +19,6 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Changed -## [Unreleased] - -### Added - -### Fixed -- Fixed a failing UTP test that was failing when you install Unity Transport package 2.0.0 or newer. - -## Changed - ## [1.5.1] - 2023-06-07 ### Added From 98a25466f2ef7788a4a98ef183c180e4b311cc73 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Wed, 12 Jul 2023 16:33:58 -0500 Subject: [PATCH 7/8] update Always notify when ownership is lost on the server side. --- .../Runtime/Spawning/NetworkSpawnManager.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index fe9d8ca44e..a2d3366d9f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -303,17 +303,11 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId) throw new SpawnStateException("Object is not spawned"); } - // Determine if the server is gong to lose ownership - var serverWillLoseOwnership = networkObject.OwnerClientId == NetworkManager.LocalClientId; - // Assign the new owner networkObject.OwnerClientId = clientId; - // Notify locally that the server lost ownership - if (serverWillLoseOwnership) - { - networkObject.InvokeBehaviourOnLostOwnership(); - } + // Always notify locally on the server when ownership is lost + networkObject.InvokeBehaviourOnLostOwnership(); networkObject.MarkVariablesDirty(true); NetworkManager.BehaviourUpdater.AddForUpdate(networkObject); From 26554b768d291b5505f23c9577090480214f69eb Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Wed, 12 Jul 2023 16:57:36 -0500 Subject: [PATCH 8/8] update making RemoveOwnership mirror ChangeOwnership's behavior. --- .../Runtime/Spawning/NetworkSpawnManager.cs | 56 ++----------------- 1 file changed, 5 insertions(+), 51 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index a2d3366d9f..f5302cb886 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -215,57 +215,6 @@ public NetworkObject GetPlayerNetworkObject(ulong clientId) return null; } - internal void RemoveOwnership(NetworkObject networkObject) - { - if (!NetworkManager.IsServer) - { - throw new NotServerException("Only the server can change ownership"); - } - - if (!networkObject.IsSpawned) - { - throw new SpawnStateException("Object is not spawned"); - } - - // If we made it here then we are the server and if the server is determined to already be the owner - // then ignore the RemoveOwnership invocation. - if (networkObject.OwnerClientId == NetworkManager.ServerClientId) - { - return; - } - - networkObject.OwnerClientId = NetworkManager.ServerClientId; - - // If there is no BehaviourUpdater and we are shutting down then return early - if (NetworkManager.BehaviourUpdater == null && NetworkManager.ShutdownInProgress) - { - return; - } - - // This was not being done when ownership was removed but was being done when ownership changed - // Mark variables as dirty and add the NetworkObject for a delta update - networkObject.MarkVariablesDirty(true); - NetworkManager.BehaviourUpdater.AddForUpdate(networkObject); - - // Server removes the entry and takes over ownership before notifying - UpdateOwnershipTable(networkObject, NetworkManager.ServerClientId, true); - - //Notify the local server that it gained ownership - networkObject.InvokeBehaviourOnGainedOwnership(); - - var message = new ChangeOwnershipMessage - { - NetworkObjectId = networkObject.NetworkObjectId, - OwnerClientId = networkObject.OwnerClientId - }; - var size = NetworkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, NetworkManager.ConnectedClientsIds); - - foreach (var client in NetworkManager.ConnectedClients) - { - NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject, size); - } - } - /// /// Helper function to get a network client for a clientId from the NetworkManager. /// On the server this will check the list. @@ -291,6 +240,11 @@ private bool TryGetNetworkClient(ulong clientId, out NetworkClient networkClient return false; } + internal void RemoveOwnership(NetworkObject networkObject) + { + ChangeOwnership(networkObject, NetworkManager.ServerClientId); + } + internal void ChangeOwnership(NetworkObject networkObject, ulong clientId) { if (!NetworkManager.IsServer)