From d53de60fbebcfb4fd5c58564582fdfd8ef9ee406 Mon Sep 17 00:00:00 2001 From: "M. Fatih MAR" Date: Wed, 25 Aug 2021 23:05:27 +0100 Subject: [PATCH] fix: change OwnerClientId before firing OnGainedOwnership() callback --- .../Messaging/InternalMessageHandler.cs | 60 ++++--- .../NetworkObjectOwnershipTests.cs | 153 ++++++++++++++++++ .../NetworkObjectOwnershipTests.cs.meta | 11 ++ 3 files changed, 199 insertions(+), 25 deletions(-) create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs.meta diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs index 82bb82026f..6bce802f67 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs @@ -120,19 +120,23 @@ public void HandleAddObject(ulong clientId, Stream stream) public void HandleDestroyObject(ulong clientId, Stream stream) { - using (var reader = PooledNetworkReader.Get(stream)) + using var reader = PooledNetworkReader.Get(stream); + ulong networkObjectId = reader.ReadUInt64Packed(); + + if (!NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkObjectId, out var networkObject)) { - ulong networkId = reader.ReadUInt64Packed(); - if (!NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkId, out NetworkObject networkObject)) + // This is the same check and log message that happens inside OnDespawnObject, but we have to do it here + // while we still have access to the network ID, otherwise the log message will be less useful. + if (NetworkLog.CurrentLogLevel <= LogLevel.Normal) { - // This is the same check and log message that happens inside OnDespawnObject, but we have to do it here - // while we still have access to the network ID, otherwise the log message will be less useful. - Debug.LogWarning($"Trying to destroy object {networkId} but it doesn't seem to exist anymore!"); - return; + NetworkLog.LogWarning($"Trying to destroy {nameof(NetworkObject)} #{networkObjectId} but it does not exist in {nameof(NetworkSpawnManager.SpawnedObjects)} anymore!"); } - m_NetworkManager.NetworkMetrics.TrackObjectDestroyReceived(clientId, networkId, networkObject.name, stream.Length); - NetworkManager.SpawnManager.OnDespawnObject(networkObject, true); + + return; } + + m_NetworkManager.NetworkMetrics.TrackObjectDestroyReceived(clientId, networkObjectId, networkObject.name, stream.Length); + NetworkManager.SpawnManager.OnDespawnObject(networkObject, true); } /// @@ -145,31 +149,37 @@ public void HandleSceneEvent(ulong clientId, Stream stream) NetworkManager.SceneManager.HandleSceneEvent(clientId, stream); } - public void HandleChangeOwner(ulong clientId, Stream stream) { - using (var reader = PooledNetworkReader.Get(stream)) - { - ulong networkId = reader.ReadUInt64Packed(); - ulong ownerClientId = reader.ReadUInt64Packed(); + using var reader = PooledNetworkReader.Get(stream); + ulong networkObjectId = reader.ReadUInt64Packed(); + ulong ownerClientId = reader.ReadUInt64Packed(); - var networkObject = NetworkManager.SpawnManager.SpawnedObjects[networkId]; - if (networkObject.OwnerClientId == NetworkManager.LocalClientId) + if (!NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkObjectId, out var networkObject)) + { + if (NetworkLog.CurrentLogLevel <= LogLevel.Normal) { - //We are current owner. - networkObject.InvokeBehaviourOnLostOwnership(); + NetworkLog.LogWarning($"Trying to handle owner change but {nameof(NetworkObject)} #{networkObjectId} does not exist in {nameof(NetworkSpawnManager.SpawnedObjects)} anymore!"); } - if (ownerClientId == NetworkManager.LocalClientId) - { - //We are new owner. - networkObject.InvokeBehaviourOnGainedOwnership(); - } + return; + } - networkObject.OwnerClientId = ownerClientId; + if (networkObject.OwnerClientId == NetworkManager.LocalClientId) + { + //We are current owner. + networkObject.InvokeBehaviourOnLostOwnership(); + } + + networkObject.OwnerClientId = ownerClientId; - NetworkManager.NetworkMetrics.TrackOwnershipChangeReceived(clientId, networkObject.NetworkObjectId, networkObject.name, stream.Length); + if (ownerClientId == NetworkManager.LocalClientId) + { + //We are new owner. + networkObject.InvokeBehaviourOnGainedOwnership(); } + + NetworkManager.NetworkMetrics.TrackOwnershipChangeReceived(clientId, networkObject.NetworkObjectId, networkObject.name, stream.Length); } public void HandleTimeSync(ulong clientId, Stream stream) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs new file mode 100644 index 0000000000..2f5015aff5 --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs @@ -0,0 +1,153 @@ +using System.Collections; +using NUnit.Framework; +using UnityEngine; +using UnityEngine.TestTools; + +namespace Unity.Netcode.RuntimeTests +{ + public class NetworkObjectOwnershipComponent : NetworkBehaviour + { + public bool OnLostOwnershipFired = false; + public ulong CachedOwnerIdOnLostOwnership = 0; + + public override void OnLostOwnership() + { + OnLostOwnershipFired = true; + CachedOwnerIdOnLostOwnership = OwnerClientId; + } + + public bool OnGainedOwnershipFired = false; + public ulong CachedOwnerIdOnGainedOwnership = 0; + + public override void OnGainedOwnership() + { + OnGainedOwnershipFired = true; + CachedOwnerIdOnGainedOwnership = OwnerClientId; + } + } + + [TestFixture(true)] + [TestFixture(false)] + public class NetworkObjectOwnershipTests + { + private const int k_ClientInstanceCount = 1; + + private NetworkManager m_ServerNetworkManager; + private NetworkManager[] m_ClientNetworkManagers; + + private GameObject m_DummyPrefab; + private GameObject m_DummyGameObject; + + private readonly bool m_IsHost; + + public NetworkObjectOwnershipTests(bool isHost) + { + m_IsHost = isHost; + } + + [UnitySetUp] + public IEnumerator Setup() + { + // we need at least 1 client for tests + Assert.That(k_ClientInstanceCount, Is.GreaterThan(0)); + + // create NetworkManager instances + Assert.That(MultiInstanceHelpers.Create(k_ClientInstanceCount, out m_ServerNetworkManager, out m_ClientNetworkManagers)); + Assert.That(m_ServerNetworkManager, Is.Not.Null); + Assert.That(m_ClientNetworkManagers, Is.Not.Null); + Assert.That(m_ClientNetworkManagers.Length, Is.EqualTo(k_ClientInstanceCount)); + + // create and register our ad-hoc DummyPrefab (we'll spawn it later during tests) + m_DummyPrefab = new GameObject("DummyPrefabPrototype"); + m_DummyPrefab.AddComponent(); + m_DummyPrefab.AddComponent(); + MultiInstanceHelpers.MakeNetworkedObjectTestPrefab(m_DummyPrefab.GetComponent()); + m_ServerNetworkManager.NetworkConfig.NetworkPrefabs.Add(new NetworkPrefab { Prefab = m_DummyPrefab }); + foreach (var clientNetworkManager in m_ClientNetworkManagers) + { + clientNetworkManager.NetworkConfig.NetworkPrefabs.Add(new NetworkPrefab { Prefab = m_DummyPrefab }); + } + + // start server and client NetworkManager instances + Assert.That(MultiInstanceHelpers.Start(m_IsHost, m_ServerNetworkManager, m_ClientNetworkManagers)); + + // wait for connection on client side + yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnected(m_ClientNetworkManagers)); + + // wait for connection on server side + yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientConnectedToServer(m_ServerNetworkManager)); + } + + [TearDown] + public void Teardown() + { + MultiInstanceHelpers.Destroy(); + + if (m_DummyGameObject != null) + { + Object.DestroyImmediate(m_DummyGameObject); + } + + if (m_DummyPrefab != null) + { + Object.DestroyImmediate(m_DummyPrefab); + } + } + + [UnityTest] + public IEnumerator TestOwnershipCallbacks() + { + m_DummyGameObject = Object.Instantiate(m_DummyPrefab); + var dummyNetworkObject = m_DummyGameObject.GetComponent(); + Assert.That(dummyNetworkObject, Is.Not.Null); + + dummyNetworkObject.NetworkManagerOwner = m_ServerNetworkManager; + dummyNetworkObject.Spawn(); + var dummyNetworkObjectId = dummyNetworkObject.NetworkObjectId; + Assert.That(dummyNetworkObjectId, Is.GreaterThan(0)); + + int nextFrameNumber = Time.frameCount + 2; + yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber); + + Assert.That(m_ServerNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(dummyNetworkObjectId)); + foreach (var clientNetworkManager in m_ClientNetworkManagers) + { + Assert.That(clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(dummyNetworkObjectId)); + } + + + var serverObject = m_ServerNetworkManager.SpawnManager.SpawnedObjects[dummyNetworkObjectId]; + var clientObject = m_ClientNetworkManagers[0].SpawnManager.SpawnedObjects[dummyNetworkObjectId]; + Assert.That(serverObject, Is.Not.Null); + Assert.That(clientObject, Is.Not.Null); + + var serverComponent = serverObject.GetComponent(); + var clientComponent = clientObject.GetComponent(); + Assert.That(serverComponent, Is.Not.Null); + Assert.That(clientComponent, Is.Not.Null); + + + Assert.That(serverObject.OwnerClientId, Is.EqualTo(m_ServerNetworkManager.ServerClientId)); + Assert.That(clientObject.OwnerClientId, Is.EqualTo(m_ClientNetworkManagers[0].ServerClientId)); + + Assert.That(m_ServerNetworkManager.ConnectedClients.ContainsKey(m_ClientNetworkManagers[0].LocalClientId)); + serverObject.ChangeOwnership(m_ClientNetworkManagers[0].LocalClientId); + + nextFrameNumber = Time.frameCount + 2; + yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber); + + + Assert.That(clientComponent.OnGainedOwnershipFired); + Assert.That(clientComponent.CachedOwnerIdOnGainedOwnership, Is.EqualTo(m_ClientNetworkManagers[0].LocalClientId)); + serverObject.ChangeOwnership(m_ServerNetworkManager.ServerClientId); + + nextFrameNumber = Time.frameCount + 2; + yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber); + + + Assert.That(serverObject.OwnerClientId, Is.EqualTo(m_ServerNetworkManager.LocalClientId)); + Assert.That(clientComponent.OnLostOwnershipFired); + Assert.That(clientComponent.CachedOwnerIdOnLostOwnership, Is.EqualTo(m_ClientNetworkManagers[0].LocalClientId)); + } + } +} diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs.meta new file mode 100644 index 0000000000..d8dea9e61b --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 08f9390096acb44698be8b3eacf242ea +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: