From c032b55a127ef5f5a21766f9be9c973f55ac3105 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Fri, 24 Jan 2025 15:38:12 -0600 Subject: [PATCH] fix: networkvariablebase not being reinitialized if networkobject persists between sessions (#3217) * fix Back-port of #3181 final fix. * test The test to validate this PR. * update Only log something if we have something to log. * test remove verbose mode. * update adding changelog entry --- com.unity.netcode.gameobjects/CHANGELOG.md | 1 + .../Runtime/Core/NetworkBehaviour.cs | 23 + .../NetworkVariable/NetworkVariableBase.cs | 87 +++- .../Runtime/NetcodeIntegrationTest.cs | 8 +- ...orkVariableBaseInitializesWhenPersisted.cs | 408 ++++++++++++++++++ ...riableBaseInitializesWhenPersisted.cs.meta | 11 + 6 files changed, 518 insertions(+), 20 deletions(-) create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs.meta diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index dcd384061b..57246fb5aa 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed issue where `NetworkVariableBase` derived classes were not being re-initialized if the associated `NetworkObject` instance was not destroyed and re-spawned. (#3217) - Fixed issue where a spawned `NetworkObject` that was registered with a prefab handler and owned by a client would invoke destroy more than once on the host-server side if the client disconnected while the `NetworkObject` was still spawned. (#3202) - Fixed issue where `NetworkRigidBody2D` was still using the deprecated `isKinematic` property in Unity versions 2022.3 and newer. (#3199) - Fixed issue where an exception was thrown when calling `NetworkManager.Shutdown` after calling `UnityTransport.Shutdown`. (#3118) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index 25f134959a..2754fdf8c0 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -755,6 +755,13 @@ internal void InternalOnNetworkDespawn() { Debug.LogException(e); } + + // Deinitialize all NetworkVariables in the event the associated + // NetworkObject is recylced (in-scene placed or pooled). + for (int i = 0; i < NetworkVariableFields.Count; i++) + { + NetworkVariableFields[i].Deinitialize(); + } } /// @@ -858,6 +865,22 @@ internal void InitializeVariables() { if (m_VarInit) { + // If the primary initialization has already been done, then go ahead + // and re-initialize each NetworkVariable in the event it is an in-scene + // placed NetworkObject in an already loaded scene that has already been + // used within a network session =or= if this is a pooled NetworkObject + // that is being repurposed. + for (int i = 0; i < NetworkVariableFields.Count; i++) + { + // If already initialized, then skip + if (NetworkVariableFields[i].HasBeenInitialized) + { + continue; + } + NetworkVariableFields[i].Initialize(this); + } + // Exit early as we don't need to run through the rest of this initialization + // process return; } diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs index f35a6c25f7..4d5a3b51a0 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs @@ -34,6 +34,12 @@ public abstract class NetworkVariableBase : IDisposable private protected NetworkBehaviour m_NetworkBehaviour; private NetworkManager m_InternalNetworkManager; + // Determines if this NetworkVariable has been "initialized" to prevent initializing more than once which can happen when first + // instantiated and spawned. If this NetworkVariable instance is on an in-scene placed NetworkObject =or= a pooled NetworkObject + // that can persist between sessions and/or be recycled we need to reset the LastUpdateSent value prior to spawning otherwise + // this NetworkVariableBase property instance will not update until the last session time used. + internal bool HasBeenInitialized { get; private set; } + public NetworkBehaviour GetBehaviour() { return m_NetworkBehaviour; @@ -49,17 +55,7 @@ internal void LogWritePermissionError() Debug.LogError(GetWritePermissionError()); } - private protected NetworkManager m_NetworkManager - { - get - { - if (m_InternalNetworkManager == null && m_NetworkBehaviour && m_NetworkBehaviour.NetworkObject?.NetworkManager) - { - m_InternalNetworkManager = m_NetworkBehaviour.NetworkObject?.NetworkManager; - } - return m_InternalNetworkManager; - } - } + private protected NetworkManager m_NetworkManager => m_InternalNetworkManager; /// /// Initializes the NetworkVariable @@ -67,19 +63,74 @@ private protected NetworkManager m_NetworkManager /// The NetworkBehaviour the NetworkVariable belongs to public void Initialize(NetworkBehaviour networkBehaviour) { - m_InternalNetworkManager = null; + // If we have already been initialized, then exit early. + // This can happen on the very first instantiation and spawning of the associated NetworkObject + if (HasBeenInitialized) + { + return; + } + + // Throw an exception if there is an invalid NetworkBehaviour parameter + if (!networkBehaviour) + { + throw new Exception($"[{GetType().Name}][Initialize] {nameof(NetworkBehaviour)} parameter passed in is null!"); + } m_NetworkBehaviour = networkBehaviour; - if (m_NetworkBehaviour && m_NetworkBehaviour.NetworkObject?.NetworkManager) + + // Throw an exception if there is no NetworkManager available + if (!m_NetworkBehaviour.NetworkManager) { - m_InternalNetworkManager = m_NetworkBehaviour.NetworkObject?.NetworkManager; + // Exit early if there has yet to be a NetworkManager assigned. + // This is ok because Initialize is invoked multiple times until + // it is considered "initialized". + return; + } - if (m_NetworkBehaviour.NetworkManager.NetworkTimeSystem != null) - { - UpdateLastSentTime(); - } + if (!m_NetworkBehaviour.NetworkObject) + { + // Exit early if there has yet to be a NetworkObject assigned. + // This is ok because Initialize is invoked multiple times until + // it is considered "initialized". + return; } + if (!m_NetworkBehaviour.NetworkObject.NetworkManagerOwner) + { + // Exit early if there has yet to be a NetworkManagerOwner assigned + // to the NetworkObject. This is ok because Initialize is invoked + // multiple times until it is considered "initialized". + return; + } + m_InternalNetworkManager = m_NetworkBehaviour.NetworkObject.NetworkManagerOwner; + OnInitialize(); + + // Some unit tests don't operate with a running NetworkManager. + // Only update the last time if there is a NetworkTimeSystem. + if (m_InternalNetworkManager.NetworkTimeSystem != null) + { + // Update our last sent time relative to when this was initialized + UpdateLastSentTime(); + + // At this point, this instance is considered initialized + HasBeenInitialized = true; + } + else if (m_InternalNetworkManager.LogLevel == LogLevel.Developer) + { + Debug.LogWarning($"[{m_NetworkBehaviour.name}][{m_NetworkBehaviour.GetType().Name}][{GetType().Name}][Initialize] {nameof(NetworkManager)} has no {nameof(NetworkTimeSystem)} assigned!"); + } + } + + /// + /// Deinitialize is invoked when a NetworkObject is despawned. + /// This allows for a recyled NetworkObject (in-scene or pooled) + /// to be properly initialized upon the next use/spawn. + /// + internal void Deinitialize() + { + // When despawned, reset the HasBeenInitialized so if the associated NetworkObject instance + // is recylced (i.e. in-scene placed or pooled) it will re-initialize the LastUpdateSent time. + HasBeenInitialized = false; } /// diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs index e5a9aa649d..db88d77d5e 100644 --- a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs +++ b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs @@ -1550,8 +1550,12 @@ private void UnloadRemainingScenes() private void LogWaitForMessages() { - VerboseDebug(m_WaitForLog.ToString()); - m_WaitForLog.Clear(); + // If there is nothing to log, then don't log anything + if (m_WaitForLog.Length > 0) + { + VerboseDebug(m_WaitForLog.ToString()); + m_WaitForLog.Clear(); + } } private IEnumerator WaitForTickAndFrames(NetworkManager networkManager, int tickCount, float targetFrames) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs new file mode 100644 index 0000000000..1d17fcccdb --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs @@ -0,0 +1,408 @@ +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using NUnit.Framework; +using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine; +using UnityEngine.SceneManagement; +using UnityEngine.TestTools; + +namespace Unity.Netcode.RuntimeTests +{ + [TestFixture(HostOrServer.Server)] + [TestFixture(HostOrServer.Host)] + public class NetworkVariableBaseInitializesWhenPersisted : NetcodeIntegrationTest + { + protected override int NumberOfClients => 1; + + private static GameObject s_NetworkPrefab; + + public NetworkVariableBaseInitializesWhenPersisted(HostOrServer hostOrServer) : base(hostOrServer) { } + + protected override void OnOneTimeSetup() + { + // Create a prefab to persist for all tests + s_NetworkPrefab = new GameObject("PresistPrefab"); + var networkObject = s_NetworkPrefab.AddComponent(); + networkObject.GlobalObjectIdHash = 8888888; + networkObject.IsSceneObject = false; + s_NetworkPrefab.AddComponent(); + s_NetworkPrefab.AddComponent(); + // Create enough prefab instance handlers to be re-used for all tests. + PrefabInstanceHandler.OneTimeSetup(NumberOfClients + 1, s_NetworkPrefab); + Object.DontDestroyOnLoad(s_NetworkPrefab); + base.OnOneTimeSetup(); + } + + protected override void OnServerAndClientsCreated() + { + // Assign one of the precreated prefab instance handlers to the server + PrefabInstanceHandler.AssignHandler(m_ServerNetworkManager); + // Add the network prefab to the server networkmanager + m_ServerNetworkManager.AddNetworkPrefab(s_NetworkPrefab); + // Repeat these steps for clients + foreach (var client in m_ClientNetworkManagers) + { + PrefabInstanceHandler.AssignHandler(client); + client.AddNetworkPrefab(s_NetworkPrefab); + } + // !!! IMPORTANT !!! + // Disable the persisted network prefab so it isn't spawned nor destroyed + s_NetworkPrefab.SetActive(false); + base.OnServerAndClientsCreated(); + } + + private List m_NetworkManagers = new List(); + private List m_SpawnedObjects = new List(); + + [UnityTest] + public IEnumerator PrefabSessionIstantiationPass([Values(4, 3, 2, 1)] int iterationsLeft) + { + // Start out waiting for a long duration before updating the NetworkVariable so each + // next iteration we change it earlier than the previous. This validates that the + // NetworkVariable's last update time is being reset each time a persisted NetworkObject + // is being spawned. + var baseWaitTime = 0.35f; + var waitPeriod = baseWaitTime * iterationsLeft; + m_NetworkManagers.Add(m_ServerNetworkManager); + m_NetworkManagers.AddRange(m_ClientNetworkManagers); + + foreach (var networkManager in m_NetworkManagers) + { + // Validate Pooled Objects Persisted Between Tests + // We start with having 4 iterations (including the first of the 4), which means that when we have 4 + // iterations remaining we haven't spawned any instances so it will test that there are no instances. + // When we there are 3 iterations left, every persisted instance of the network prefab should have been + // spawned at least once...when 2 then all should have been spawned twice...when 1 then all should been + // spawned three times. + Assert.True(PrefabInstanceHandler.ValidatePersistedInstances(networkManager, 4 - iterationsLeft), "Failed validation of persisted pooled objects!"); + + // Spawn 1 NetworkObject per NetworkManager with the NetworkManager's client being the owner + var networkManagerToSpawn = m_ServerNetworkManager; + var objectToSpawn = PrefabInstanceHandler.GetInstanceToSpawn(networkManagerToSpawn); + objectToSpawn.NetworkManagerOwner = networkManagerToSpawn; + objectToSpawn.SpawnWithOwnership(networkManager.LocalClientId); + m_SpawnedObjects.Add(objectToSpawn); + } + + // Conditional wait for all clients to spawn all objects + bool AllInstancesSpawnedOnAllCLients() + { + foreach (var spawnedObject in m_SpawnedObjects) + { + foreach (var networkManager in m_NetworkManagers) + { + if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(spawnedObject.NetworkObjectId)) + { + return false; + } + } + } + return true; + } + + // Wait for all clients to locally clone and spawn all spawned NetworkObjects + yield return WaitForConditionOrTimeOut(AllInstancesSpawnedOnAllCLients); + AssertOnTimeout($"Failed to spawn all instances on all clients!"); + + // Wait for the continually decreasing waitPeriod to validate that + // NetworkVariableBase has reset the last update time. + yield return new WaitForSeconds(waitPeriod); + + // Have the owner of each spawned NetworkObject assign a new NetworkVariable + foreach (var spawnedObject in m_SpawnedObjects) + { + var testBehaviour = spawnedObject.GetComponent(); + var client = m_NetworkManagers.Where((c) => c.LocalClientId == testBehaviour.OwnerClientId).First(); + testBehaviour = client.SpawnManager.SpawnedObjects[testBehaviour.NetworkObjectId].GetComponent(); + testBehaviour.TestNetworkVariable.Value = Random.Range(0, 1000); + } + + // Wait for half of the base time before checking to see if the time delta is within the acceptable range. + // The time delta is the time from spawn to when the value changes. Each iteration of this test decreases + // the total wait period, and so the delta should always be less than the wait period plus the base wait time. + yield return new WaitForSeconds(baseWaitTime * 0.5f); + + // Conditional to verify the time delta between spawn and the NetworkVariable being updated is within the + // expected range + bool AllSpawnedObjectsUpdatedWithinTimeSpan() + { + foreach (var spawnedObject in m_SpawnedObjects) + { + foreach (var networkManager in m_NetworkManagers) + { + if (spawnedObject.OwnerClientId == networkManager.LocalClientId) + { + continue; + } + if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(spawnedObject.NetworkObjectId)) + { + return false; + } + var instance = networkManager.SpawnManager.SpawnedObjects[spawnedObject.NetworkObjectId].GetComponent(); + // Check to make sure all clients' delta between updates is not greater than the wait period plus the baseWaitTime. + // Ignore the first iteration as that becomes our baseline. + if (iterationsLeft < 4 && instance.LastUpdateDelta >= (waitPeriod + baseWaitTime)) + { + VerboseDebug($"Last Spawn Delta = {instance.LastUpdateDelta} is greater or equal to {waitPeriod + baseWaitTime}"); + return false; + } + } + } + return true; + } + + yield return WaitForConditionOrTimeOut(AllSpawnedObjectsUpdatedWithinTimeSpan); + AssertOnTimeout($"Failed to reset NetworkVariableBase instances!"); + } + + + protected override IEnumerator OnTearDown() + { + m_NetworkManagers.Clear(); + m_SpawnedObjects.Clear(); + PrefabInstanceHandler.ReleaseAll(); + yield return base.OnTearDown(); + } + + protected override void OnOneTimeTearDown() + { + Object.DestroyImmediate(s_NetworkPrefab); + s_NetworkPrefab = null; + PrefabInstanceHandler.ReleaseAll(true); + base.OnOneTimeTearDown(); + } + + /// + /// Test NetworkBehaviour that updates a NetworkVariable and tracks the time between + /// spawn and when the NetworkVariable is updated. + /// + public class TestBehaviour : NetworkBehaviour + { + public NetworkVariable TestNetworkVariable = new NetworkVariable(default, NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); + + public float LastUpdateDelta { get; private set; } + private float m_TimeSinceLastUpdate = 0.0f; + + // How many times has this instance been spawned. + public int SpawnedCount { get; private set; } + + public override void OnNetworkSpawn() + { + SpawnedCount++; + if (IsOwner) + { + TestNetworkVariable.Value = 0; + } + base.OnNetworkSpawn(); + } + + protected override void OnNetworkPostSpawn() + { + if (!IsOwner) + { + TestNetworkVariable.OnValueChanged += OnTestValueChanged; + } + m_TimeSinceLastUpdate = Time.realtimeSinceStartup; + base.OnNetworkPostSpawn(); + } + + public override void OnDestroy() + { + base.OnDestroy(); + } + + public override void OnNetworkDespawn() + { + TestNetworkVariable.OnValueChanged -= OnTestValueChanged; + LastUpdateDelta = 0.0f; + base.OnNetworkDespawn(); + } + + private void OnTestValueChanged(int previous, int current) + { + LastUpdateDelta = Time.realtimeSinceStartup - m_TimeSinceLastUpdate; + } + } + + /// + /// Creates a specified number of instances that persist throughout the entire test session + /// and will only be destroyed/released during the OneTimeTeardown + /// + public class PrefabInstanceHandler : INetworkPrefabInstanceHandler + { + private static Dictionary s_AssignedInstances = new Dictionary(); + private static Queue s_PrefabInstanceHandlers = new Queue(); + public Queue PrefabInstances = new Queue(); + private GameObject m_NetworkPrefab; + private NetworkManager m_NetworkManager; + private ulong m_AssignedClientId; + + public static void OneTimeSetup(int numberOfInstances, GameObject prefabInstance) + { + for (int i = 0; i < numberOfInstances; i++) + { + s_PrefabInstanceHandlers.Enqueue(new PrefabInstanceHandler(prefabInstance)); + } + } + + /// + /// Invoke when s are created but not started. + /// + /// + public static void AssignHandler(NetworkManager networkManager) + { + if (s_PrefabInstanceHandlers.Count > 0) + { + var instance = s_PrefabInstanceHandlers.Dequeue(); + instance.Initialize(networkManager); + s_AssignedInstances.Add(networkManager, instance); + } + else + { + Debug.LogError($"[{nameof(PrefabInstanceHandler)}] Exhausted total number of instances available!"); + } + } + + public static NetworkObject GetInstanceToSpawn(NetworkManager networkManager) + { + if (s_AssignedInstances.ContainsKey(networkManager)) + { + return s_AssignedInstances[networkManager].GetInstance(); + } + return null; + } + + + public static bool ValidatePersistedInstances(NetworkManager networkManager, int minCount) + { + if (s_AssignedInstances.ContainsKey(networkManager)) + { + var prefabInstanceHandler = s_AssignedInstances[networkManager]; + return prefabInstanceHandler.ValidateInstanceSpawnCount(minCount); + } + return false; + } + + /// + /// Releases back to the queue and if destroy is true it will completely + /// remove all references so they are cleaned up when + /// + /// + public static void ReleaseAll(bool destroy = false) + { + foreach (var entry in s_AssignedInstances) + { + entry.Value.DeregisterHandler(); + if (!destroy) + { + s_PrefabInstanceHandlers.Enqueue(entry.Value); + } + else if (entry.Value.PrefabInstances.Count > 0) + { + entry.Value.CleanInstances(); + } + } + s_AssignedInstances.Clear(); + + if (destroy) + { + while (s_PrefabInstanceHandlers.Count > 0) + { + s_PrefabInstanceHandlers.Dequeue(); + } + } + } + + public PrefabInstanceHandler(GameObject gameObject) + { + m_NetworkPrefab = gameObject; + } + + + public void Initialize(NetworkManager networkManager) + { + m_NetworkManager = networkManager; + networkManager.PrefabHandler.AddHandler(m_NetworkPrefab, this); + } + + /// + /// This validates that the instances persisted to the next test set and persisted + /// between network sessions + public bool ValidateInstanceSpawnCount(int minCount) + { + // First pass we should have no instances + if (minCount == 0) + { + return PrefabInstances.Count == 0; + } + else + { + foreach (var instance in PrefabInstances) + { + var testBehaviour = instance.GetComponent(); + if (testBehaviour.SpawnedCount < minCount) + { + return false; + } + } + } + return true; + } + + /// + /// When we are done with all tests, we finally destroy the persisted objects + /// + public void CleanInstances() + { + while (PrefabInstances.Count > 0) + { + var instance = PrefabInstances.Dequeue(); + Object.DestroyImmediate(instance); + } + } + + public void DeregisterHandler() + { + if (m_NetworkManager != null && m_NetworkManager.PrefabHandler != null) + { + m_NetworkManager.PrefabHandler.RemoveHandler(m_NetworkPrefab); + } + } + + public NetworkObject GetInstance() + { + var instanceToReturn = (NetworkObject)null; + if (PrefabInstances.Count == 0) + { + instanceToReturn = Object.Instantiate(m_NetworkPrefab).GetComponent(); + instanceToReturn.IsSceneObject = false; + instanceToReturn.gameObject.SetActive(true); + } + else + { + instanceToReturn = PrefabInstances.Dequeue().GetComponent(); + instanceToReturn.gameObject.SetActive(true); + SceneManager.MoveGameObjectToScene(instanceToReturn.gameObject, SceneManager.GetActiveScene()); + } + return instanceToReturn; + } + + public NetworkObject Instantiate(ulong ownerClientId, Vector3 position, Quaternion rotation) + { + return GetInstance(); + } + + public void Destroy(NetworkObject networkObject) + { + if (m_NetworkPrefab != null && m_NetworkPrefab.gameObject == networkObject.gameObject) + { + return; + } + Object.DontDestroyOnLoad(networkObject.gameObject); + networkObject.gameObject.SetActive(false); + PrefabInstances.Enqueue(networkObject.gameObject); + } + } + } +} diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs.meta new file mode 100644 index 0000000000..2c070d6b67 --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableBaseInitializesWhenPersisted.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: ed69e6c738b3c47418a3d374ca779bab +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: