From 3307aab64f181270548969491f2ce33dfd0330de Mon Sep 17 00:00:00 2001 From: Emma Date: Wed, 9 Jul 2025 15:48:15 -0400 Subject: [PATCH 1/4] fix: collection authority OnValueChanged --- .../NetworkVariable/NetworkVariable.cs | 5 +- ...NetworkVariableCollectionsChangingTests.cs | 679 ++++++++++++++++++ ...rkVariableCollectionsChangingTests.cs.meta | 3 + .../NetworkVariableCollectionsTests.cs | 487 ------------- 4 files changed, 685 insertions(+), 489 deletions(-) create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs.meta diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs index 068a6bfb68..1405406c75 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs @@ -173,12 +173,13 @@ public bool CheckDirtyState(bool forceCheck = false) } // Compare the previous with the current if not dirty or forcing a check. - if ((!isDirty || forceCheck) && !NetworkVariableSerialization.AreEqual(ref m_PreviousValue, ref m_InternalValue)) + if ((!isDirty || forceCheck) && !NetworkVariableSerialization.AreEqual(ref m_InternalOriginalValue, ref m_InternalValue)) { SetDirty(true); - OnValueChanged?.Invoke(m_PreviousValue, m_InternalValue); + OnValueChanged?.Invoke(m_InternalOriginalValue, m_InternalValue); m_IsDisposed = false; isDirty = true; + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); } return isDirty; } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs new file mode 100644 index 0000000000..b54e874c38 --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs @@ -0,0 +1,679 @@ +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using NUnit.Framework; +using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine; +using UnityEngine.TestTools; + +namespace Unity.Netcode.RuntimeTests +{ + + [TestFixture(HostOrServer.DAHost, CollectionTypes.List)] + [TestFixture(HostOrServer.DAHost, CollectionTypes.Dictionary)] + [TestFixture(HostOrServer.Host, CollectionTypes.List)] + [TestFixture(HostOrServer.Host, CollectionTypes.Dictionary)] + [TestFixture(HostOrServer.Server, CollectionTypes.List)] + [TestFixture(HostOrServer.Server, CollectionTypes.Dictionary)] + internal class NetworkVariableCollectionsChangingTests : NetcodeIntegrationTest + { + protected override int NumberOfClients => 2; + public enum CollectionTypes + { + Dictionary, + List, + } + private StringBuilder m_ErrorLog = new StringBuilder(); + private CollectionTypes m_CollectionType; + private GameObject m_TestPrefab; + private NetworkObject m_Instance; + + public NetworkVariableCollectionsChangingTests(HostOrServer hostOrServer, CollectionTypes collectionType) : base(hostOrServer) + { + m_CollectionType = collectionType; + } + + protected override void OnServerAndClientsCreated() + { + m_TestPrefab = CreateNetworkObjectPrefab("TestObject"); + if (m_CollectionType == CollectionTypes.Dictionary) + { + m_TestPrefab.AddComponent(); + } + else + { + m_TestPrefab.AddComponent(); + } + if (m_DistributedAuthority) + { + var networkObject = m_TestPrefab.GetComponent(); + networkObject.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable); + } + base.OnServerAndClientsCreated(); + } + + private bool AllInstancesSpawned() + { + foreach (var client in m_NetworkManagers) + { + if (!client.SpawnManager.SpawnedObjects.ContainsKey(m_Instance.NetworkObjectId)) + { + return false; + } + } + return true; + } + + private Dictionary m_Managers = new Dictionary(); + + private bool ValidateAllInstances() + { + if (!m_Managers.ContainsKey(m_Instance.OwnerClientId)) + { + return false; + } + + if (!m_Managers[m_Instance.OwnerClientId].SpawnManager.SpawnedObjects.ContainsKey(m_Instance.NetworkObjectId)) + { + return false; + } + + var ownerNetworkManager = m_Managers[m_Instance.OwnerClientId]; + + var ownerClientInstance = m_Managers[m_Instance.OwnerClientId].SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); + + foreach (var client in m_Managers) + { + if (client.Value == ownerNetworkManager) + { + continue; + } + + var otherInstance = client.Value.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); + if (!ownerClientInstance.ValidateAgainst(otherInstance)) + { + return false; + } + } + return true; + } + + private bool ValidateAllValueChangedEqual() + { + if (!m_Managers.TryGetValue(m_Instance.OwnerClientId, out var ownerNetworkManager)) + { + return false; + } + + if (!ownerNetworkManager.SpawnManager.SpawnedObjects.TryGetValue(m_Instance.NetworkObjectId, out var ownerObject)) + { + return false; + } + + + var ownerClientInstance = ownerObject.GetComponent(); + + foreach (var client in m_Managers) + { + if (client.Value == ownerNetworkManager) + { + continue; + } + + var otherInstance = client.Value.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); + if (ownerClientInstance.OnValueChangedCount() != otherInstance.OnValueChangedCount()) + { + return false; + } + } + return true; + } + + /// + /// With CheckDirtyState(true), a change back to the original value should trigger an extra OnValueChanged call on the authority instance. + /// + private bool ValidateOwnerHasExtraValueChangedCall() + { + if (!m_Managers.TryGetValue(m_Instance.OwnerClientId, out var ownerNetworkManager)) + { + return false; + } + + if (!ownerNetworkManager.SpawnManager.SpawnedObjects.TryGetValue(m_Instance.NetworkObjectId, out var ownerObject)) + { + return false; + } + + + var ownerClientInstance = ownerObject.GetComponent(); + + // With forced check, owner should have an extra OnValueChanged callback + var expectedClientCallbackCount = ownerClientInstance.OnValueChangedCount() - 1; + + foreach (var client in m_Managers) + { + if (client.Value == ownerNetworkManager) + { + continue; + } + + var otherInstance = client.Value.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); + if (otherInstance.OnValueChangedCount() != expectedClientCallbackCount) + { + return false; + } + } + return true; + } + + + private bool OwnershipChangedOnAllClients(ulong expectedOwner) + { + m_ErrorLog.Clear(); + foreach (var client in m_Managers) + { + var otherInstance = client.Value.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); + if (otherInstance.OwnerClientId != expectedOwner) + { + m_ErrorLog.AppendLine($"Client-{client.Value.LocalClientId} instance of {m_Instance.name} still shows the owner is Client-{otherInstance.OwnerClientId} when it should be Client-{expectedOwner}!"); + return false; + } + } + return true; + } + + private BaseCollectionUpdateHelper GetOwnerInstance() + { + var ownerNetworkManager = m_Managers[m_Instance.OwnerClientId]; + return m_Managers[m_Instance.OwnerClientId].SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); + } + + /// + /// Gets the authority instance. + /// Client-Server: will always return the server-side instance + /// Distributed Authority: will always return the owner + /// + /// authority instance + private BaseCollectionUpdateHelper GetAuthorityInstance() + { + if (m_DistributedAuthority) + { + return GetOwnerInstance(); + } + else + { + return m_ServerNetworkManager.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); + } + } + + [UnityTest] + public IEnumerator CollectionAndOwnershipChangingTest() + { + BaseCollectionUpdateHelper.VerboseMode = m_EnableVerboseDebug; + var runWaitPeriod = new WaitForSeconds(0.5f); + m_Managers.Clear(); + foreach (var manager in m_NetworkManagers) + { + m_Managers.Add(manager.LocalClientId, manager); + } + + var authorityNetworkManager = GetAuthorityNetworkManager(); + + var instance = SpawnObject(m_TestPrefab, authorityNetworkManager); + m_Instance = instance.GetComponent(); + var helper = instance.GetComponent(); + var currentOwner = helper.OwnerClientId; + yield return WaitForConditionOrTimeOut(AllInstancesSpawned); + AssertOnTimeout($"[Pre][1st Phase] Timed out waiting for all clients to spawn {m_Instance.name}!"); + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Start); + yield return runWaitPeriod; + + // Update values, validate values, change owner, updates values, and repeat until all clients have been the owner at least once + for (int i = 0; i < 4; i++) + { + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Pause); + yield return WaitForConditionOrTimeOut(ValidateAllInstances); + AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to validdate their values!"); + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Start); + yield return s_DefaultWaitForTick; + + currentOwner = GetAuthorityInstance().ChangeOwner(); + Assert.IsFalse(currentOwner == ulong.MaxValue, "A non-authority instance attempted to change ownership!"); + + yield return WaitForConditionOrTimeOut(() => OwnershipChangedOnAllClients(currentOwner)); + AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to change ownership!\n {m_ErrorLog.ToString()}"); + helper = GetOwnerInstance(); + yield return runWaitPeriod; + } + + // Now reset the values + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Pause); + helper.Clear(); + + // Validate all instances are reset + yield return WaitForConditionOrTimeOut(ValidateAllInstances); + AssertOnTimeout($"[Pre][2nd Phase]Timed out waiting for all clients to validdate their values!"); + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Start); + + // Update, change ownership, and repeat until all clients have been the owner at least once + for (int i = 0; i < 4; i++) + { + yield return runWaitPeriod; + currentOwner = GetAuthorityInstance().ChangeOwner(); + Assert.IsFalse(currentOwner == ulong.MaxValue, "A non-authority instance attempted to change ownership!"); + yield return WaitForConditionOrTimeOut(() => OwnershipChangedOnAllClients(currentOwner)); + AssertOnTimeout($"[2nd Phase] Timed out waiting for all clients to change ownership!"); + helper = GetOwnerInstance(); + } + + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Pause); + yield return WaitForConditionOrTimeOut(ValidateAllInstances); + AssertOnTimeout($"[Last Validate] Timed out waiting for all clients to validdate their values!"); + } + + [UnityTest] + public IEnumerator CollectionFastChangingTest() + { + BaseCollectionUpdateHelper.VerboseMode = m_EnableVerboseDebug; + var runWaitPeriod = new WaitForSeconds(0.2f); + m_Managers.Clear(); + foreach (var manager in m_NetworkManagers) + { + m_Managers.Add(manager.LocalClientId, manager); + } + + var authorityNetworkManager = GetAuthorityNetworkManager(); + + var instance = SpawnObject(m_TestPrefab, authorityNetworkManager); + m_Instance = instance.GetComponent(); + var helper = instance.GetComponent(); + + yield return WaitForConditionOrTimeOut(AllInstancesSpawned); + AssertOnTimeout($"[Pre][1st Phase] Timed out waiting for all clients to spawn {m_Instance.name}!"); + + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Start); + yield return runWaitPeriod; + + helper.SetState(BaseCollectionUpdateHelper.HelperStates.Pause); + yield return WaitForConditionOrTimeOut(ValidateAllValueChangedEqual); + AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to have OnValueChanged an equal number of times!"); + + // Clear the collection + helper.Clear(); + + yield return WaitForConditionOrTimeOut(ValidateAllInstances); + AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to validate their values!"); + + // Change the collection and then change back in the same frame without forcing a dirty check + VerboseDebug("Doing fast change test without a forced dirty check"); + helper.AddItem(); + helper.Clear(); + + yield return WaitForConditionOrTimeOut(ValidateAllValueChangedEqual); + AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to have OnValueChanged an equal number of times!"); + + // Change the collection and then change back in the same frame with a forced dirty check + VerboseDebug("Doing fast change test with a forced dirty check"); + helper.AddItem(); + helper.Clear(true); + + yield return WaitForConditionOrTimeOut(ValidateOwnerHasExtraValueChangedCall); + AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to have OnValueChanged an equal number of times!"); + + yield return WaitForConditionOrTimeOut(ValidateAllInstances); + AssertOnTimeout($"[Last Validate] Timed out waiting for all clients to validate their values!"); + } + } + + #region COLLECTION CHANGING COMPONENTS + /// + /// Helper class to test adding dictionary entries rapidly with frequent ownership changes. + /// This includes a companion integer that is continually incremented and used as the key value for each entry. + /// + internal class DictionaryCollectionUpdateHelper : BaseCollectionUpdateHelper + { + private NetworkVariable> m_DictionaryCollection = new NetworkVariable>(new Dictionary(), NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); + private NetworkVariable m_CurrentKeyValue = new NetworkVariable(0, NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); + + protected override bool OnValidateAgainst(BaseCollectionUpdateHelper otherHelper) + { + var otherListHelper = otherHelper as DictionaryCollectionUpdateHelper; + var localValues = m_DictionaryCollection.Value; + var otherValues = otherListHelper.m_DictionaryCollection.Value; + + if (localValues.Count != otherValues.Count) + { + return false; + } + + foreach (var entry in m_DictionaryCollection.Value) + { + if (!otherValues.ContainsKey(entry.Key)) + { + return false; + } + + if (entry.Value != otherValues[entry.Key]) + { + return false; + } + } + return true; + } + protected override void OnClear(bool force) + { + m_DictionaryCollection.Value.Clear(); + m_DictionaryCollection.CheckDirtyState(force); + base.OnClear(force); + } + + public override void AddItem() + { + m_DictionaryCollection.Value.Add(m_CurrentKeyValue.Value, m_CurrentKeyValue.Value); + m_DictionaryCollection.CheckDirtyState(); + m_CurrentKeyValue.Value++; + } + + protected override void OnNetworkPostSpawn() + { + m_DictionaryCollection.OnValueChanged += OnValueChanged; + base.OnNetworkPostSpawn(); + } + public override void OnNetworkDespawn() + { + m_DictionaryCollection.OnValueChanged -= OnValueChanged; + base.OnNetworkDespawn(); + } + + private int m_ValueChangeCount; + private void OnValueChanged(Dictionary previous, Dictionary newValue) + { + Log($"[Client-{NetworkManager.LocalClientId}] OnValueChanged: previousKey {previous.Count}, newKey {newValue.Count}"); + m_ValueChangeCount++; + } + + public override int OnValueChangedCount() + { + return m_ValueChangeCount; + } + } + + /// + /// Helper class to test adding list entries rapidly with frequent ownership changes + /// + internal class ListCollectionUpdateHelper : BaseCollectionUpdateHelper + { + private NetworkVariable> m_ListCollection = new NetworkVariable>(new List(), NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); + + + protected override bool OnValidateAgainst(BaseCollectionUpdateHelper otherHelper) + { + var otherListHelper = otherHelper as ListCollectionUpdateHelper; + var localValues = m_ListCollection.Value; + var otherValues = otherListHelper.m_ListCollection.Value; + + if (localValues.Count != otherValues.Count) + { + return false; + } + + for (int i = 0; i < localValues.Count - 1; i++) + { + if (localValues[i] != i) + { + return false; + } + + if (localValues[i] != otherValues[i]) + { + return false; + } + } + return true; + } + + protected override void OnClear(bool force) + { + m_ListCollection.Value.Clear(); + m_ListCollection.CheckDirtyState(force); + base.OnClear(force); + } + + public override void AddItem() + { + m_ListCollection.Value.Add(m_ListCollection.Value.Count); + m_ListCollection.CheckDirtyState(); + } + + protected override void OnNetworkPostSpawn() + { + m_ListCollection.OnValueChanged += OnValueChanged; + base.OnNetworkPostSpawn(); + } + public override void OnNetworkDespawn() + { + m_ListCollection.OnValueChanged -= OnValueChanged; + base.OnNetworkDespawn(); + } + + private int m_ValueChangeCount; + private void OnValueChanged(List previous, List newValue) + { + Log($"[Client-{NetworkManager.LocalClientId}] OnValueChanged: previousKey {previous.Count}, newKey {newValue.Count}"); + m_ValueChangeCount++; + } + + public override int OnValueChangedCount() + { + return m_ValueChangeCount; + } + } + + /// + /// The base class to test rapidly adding items to a collection type + /// + internal class BaseCollectionUpdateHelper : NetworkBehaviour + { + public static bool VerboseMode; + private const int k_OwnershipTickDelay = 1; + + public enum HelperStates + { + Stop, + Start, + Pause, + ClearToChangeOwner, + ChangingOwner + } + + private HelperStates HelperState { get; set; } + + private int m_SendClearForOwnershipOnTick; + private ulong m_NextClient = 0; + private ulong m_ClientToSendClear = 0; + + public void SetState(HelperStates helperState) + { + HelperState = helperState; + } + + protected virtual bool OnValidateAgainst(BaseCollectionUpdateHelper otherHelper) + { + return true; + } + + public bool ValidateAgainst(BaseCollectionUpdateHelper otherHelper) + { + return OnValidateAgainst(otherHelper); + } + + public override void OnNetworkSpawn() + { + // Register for tick updates + NetworkManager.NetworkTickSystem.Tick += OnNetworkTick; + + base.OnNetworkSpawn(); + } + public override void OnNetworkDespawn() + { + NetworkManager.NetworkTickSystem.Tick -= OnNetworkTick; + base.OnNetworkDespawn(); + } + + protected virtual void OnClear(bool force) + { + } + + /// + /// Clears the underlying list state. + /// Use force to force the isDirty check + /// + /// Value passed into the call after clearing the list. + public void Clear(bool force = false) + { + OnClear(force); + } + + public virtual void AddItem() + { + } + + private bool CanUpdate() + { + return HelperState == HelperStates.Start; + } + + public virtual int OnValueChangedCount() + { + return 0; + } + + private void Update() + { + // Exit early if not spawn, updating is not enabled, or is not the owner + if (!IsSpawned || !CanUpdate() || !IsOwner) + { + return; + } + + AddItem(); + } + + protected override void OnOwnershipChanged(ulong previous, ulong current) + { + // When the ownership changes and the client is the owner, then immediately add an item to the collection + if (NetworkManager.LocalClientId == current) + { + AddItem(); + } + base.OnOwnershipChanged(previous, current); + } + + + /// + /// Sets the tick delay period of time to provide all in-flight deltas to be processed. + /// + private void SetTickDelay() + { + m_SendClearForOwnershipOnTick = NetworkManager.ServerTime.Tick + k_OwnershipTickDelay; + } + + /// + /// Changes the ownership + /// + /// next owner or ulong.MaxValue that means the authority did not invoke this method + public ulong ChangeOwner() + { + if (HasAuthority && !IsOwnershipChanging()) + { + var index = NetworkManager.ConnectedClientsIds.ToList().IndexOf(OwnerClientId); + index++; + index = index % NetworkManager.ConnectedClientsIds.Count; + m_NextClient = NetworkManager.ConnectedClientsIds[index]; + + // If we are in distributed authority and the authority or we are in client-server and the server, then make the change ourselves. + if (OwnerClientId == NetworkManager.LocalClientId && (NetworkManager.DistributedAuthorityMode || (!NetworkManager.DistributedAuthorityMode && NetworkManager.IsServer))) + { + HelperState = HelperStates.ChangingOwner; + SetTickDelay(); + Log($"Locally changing ownership to Client-{m_NextClient}"); + } + + if (!NetworkManager.DistributedAuthorityMode && NetworkManager.IsServer && OwnerClientId != NetworkManager.LocalClientId) + { + // If we are transitioning between a client to the host or client to client, + // send a "heads-up" Rpc to the client prior to changing ownership. The client + // will stop updating for the tick delay period and then send a confirmation + // to the host that it is clear to change ownership. + ChangingOwnershipRpc(RpcTarget.Single(OwnerClientId, RpcTargetUse.Temp)); + Log($"Remotely changing ownership to Client-{m_NextClient}"); + } + + return m_NextClient; + } + + return ulong.MaxValue; + } + + /// + /// Sent by the host to a client when ownership is transitioning from a client to + /// the host or to another client. + /// + [Rpc(SendTo.SpecifiedInParams)] + private void ChangingOwnershipRpc(RpcParams rpcParams = default) + { + // The sender is who we respond to that it is clear to change ownership + m_ClientToSendClear = rpcParams.Receive.SenderClientId; + HelperState = HelperStates.ClearToChangeOwner; + SetTickDelay(); + } + + /// + /// Notification that the current owner has stopped updating and ownership + /// updates can occur without missed updates. + /// + /// + [Rpc(SendTo.SpecifiedInParams)] + private void ChangingOwnershipClearRpc(RpcParams rpcParams = default) + { + HelperState = HelperStates.ChangingOwner; + SetTickDelay(); + Log($"Changing ownership to Client-{m_NextClient} based on ready request."); + } + + private bool IsOwnershipChanging() + { + return HelperState == HelperStates.ClearToChangeOwner || HelperState == HelperStates.ChangingOwner; + } + + private void OnNetworkTick() + { + if (!IsSpawned || !IsOwnershipChanging() || m_SendClearForOwnershipOnTick > NetworkManager.ServerTime.Tick) + { + return; + } + + if (HelperState == HelperStates.ChangingOwner) + { + NetworkObject.ChangeOwnership(m_NextClient); + Log($"Local Change ownership to Client-{m_NextClient} complete! New Owner is {NetworkObject.OwnerClientId} | Expected {m_NextClient}"); + } + else + { + ChangingOwnershipClearRpc(RpcTarget.Single(m_ClientToSendClear, RpcTargetUse.Temp)); + } + HelperState = HelperStates.Stop; + } + + protected void Log(string msg) + { + if (VerboseMode) + { + Debug.Log($"[Client-{NetworkManager.LocalClientId}] {msg}"); + } + } + } + #endregion + +} diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs.meta new file mode 100644 index 0000000000..9350ed2277 --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsChangingTests.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: adde18d8af294e4786577a51e48ef109 +timeCreated: 1752086183 \ No newline at end of file diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs index 388295c9e0..f8c0090522 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs @@ -1212,493 +1212,6 @@ public IEnumerator TestHashSetBuiltInTypeCollections() } } - [TestFixture(HostOrServer.DAHost, CollectionTypes.List)] - [TestFixture(HostOrServer.DAHost, CollectionTypes.Dictionary)] - [TestFixture(HostOrServer.Host, CollectionTypes.List)] - [TestFixture(HostOrServer.Host, CollectionTypes.Dictionary)] - [TestFixture(HostOrServer.Server, CollectionTypes.List)] - [TestFixture(HostOrServer.Server, CollectionTypes.Dictionary)] - internal class NetworkVariableCollectionsChangingTests : NetcodeIntegrationTest - { - protected override int NumberOfClients => 2; - public enum CollectionTypes - { - Dictionary, - List, - } - private StringBuilder m_ErrorLog = new StringBuilder(); - private CollectionTypes m_CollectionType; - private GameObject m_TestPrefab; - private NetworkObject m_Instance; - - public NetworkVariableCollectionsChangingTests(HostOrServer hostOrServer, CollectionTypes collectionType) : base(hostOrServer) - { - m_CollectionType = collectionType; - } - - protected override void OnServerAndClientsCreated() - { - m_TestPrefab = CreateNetworkObjectPrefab("TestObject"); - if (m_CollectionType == CollectionTypes.Dictionary) - { - m_TestPrefab.AddComponent(); - } - else - { - m_TestPrefab.AddComponent(); - } - if (m_DistributedAuthority) - { - var networkObject = m_TestPrefab.GetComponent(); - networkObject.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable); - } - base.OnServerAndClientsCreated(); - } - - private bool AllInstancesSpawned() - { - foreach (var client in m_NetworkManagers) - { - if (!client.SpawnManager.SpawnedObjects.ContainsKey(m_Instance.NetworkObjectId)) - { - return false; - } - } - return true; - } - - private Dictionary m_Managers = new Dictionary(); - - private bool ValidateAllInstances() - { - if (!m_Managers.ContainsKey(m_Instance.OwnerClientId)) - { - return false; - } - - if (!m_Managers[m_Instance.OwnerClientId].SpawnManager.SpawnedObjects.ContainsKey(m_Instance.NetworkObjectId)) - { - return false; - } - - var ownerNetworkManager = m_Managers[m_Instance.OwnerClientId]; - - var ownerClientInstance = m_Managers[m_Instance.OwnerClientId].SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); - - foreach (var client in m_Managers) - { - if (client.Value == ownerNetworkManager) - { - continue; - } - - var otherInstance = client.Value.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); - if (!ownerClientInstance.ValidateAgainst(otherInstance)) - { - return false; - } - } - return true; - } - - private bool OwnershipChangedOnAllClients(ulong expectedOwner) - { - m_ErrorLog.Clear(); - foreach (var client in m_Managers) - { - var otherInstance = client.Value.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); - if (otherInstance.OwnerClientId != expectedOwner) - { - m_ErrorLog.AppendLine($"Client-{client.Value.LocalClientId} instance of {m_Instance.name} still shows the owner is Client-{otherInstance.OwnerClientId} when it should be Client-{expectedOwner}!"); - return false; - } - } - return true; - } - - private BaseCollectionUpdateHelper GetOwnerInstance() - { - var ownerNetworkManager = m_Managers[m_Instance.OwnerClientId]; - return m_Managers[m_Instance.OwnerClientId].SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); - } - - /// - /// Gets the authority instance. - /// Client-Server: will always return the server-side instance - /// Distributed Authority: will always return the owner - /// - /// authority instance - private BaseCollectionUpdateHelper GetAuthorityInstance() - { - if (m_DistributedAuthority) - { - return GetOwnerInstance(); - } - else - { - return m_ServerNetworkManager.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].GetComponent(); - } - } - - [UnityTest] - public IEnumerator CollectionAndOwnershipChangingTest() - { - BaseCollectionUpdateHelper.VerboseMode = m_EnableVerboseDebug; - var runWaitPeriod = new WaitForSeconds(0.5f); - m_Managers.Clear(); - foreach (var manager in m_NetworkManagers) - { - m_Managers.Add(manager.LocalClientId, manager); - } - - var authorityNetworkManager = GetAuthorityNetworkManager(); - - var instance = SpawnObject(m_TestPrefab, authorityNetworkManager); - m_Instance = instance.GetComponent(); - var helper = instance.GetComponent(); - var currentOwner = helper.OwnerClientId; - yield return WaitForConditionOrTimeOut(AllInstancesSpawned); - AssertOnTimeout($"[Pre][1st Phase] Timed out waiting for all clients to spawn {m_Instance.name}!"); - helper.SetState(BaseCollectionUpdateHelper.HelperStates.Start); - yield return runWaitPeriod; - - // Update values, validate values, change owner, updates values, and repeat until all clients have been the owner at least once - for (int i = 0; i < 4; i++) - { - helper.SetState(BaseCollectionUpdateHelper.HelperStates.Pause); - yield return WaitForConditionOrTimeOut(ValidateAllInstances); - AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to validdate their values!"); - helper.SetState(BaseCollectionUpdateHelper.HelperStates.Start); - yield return s_DefaultWaitForTick; - - currentOwner = GetAuthorityInstance().ChangeOwner(); - Assert.IsFalse(currentOwner == ulong.MaxValue, "A non-authority instance attempted to change ownership!"); - - yield return WaitForConditionOrTimeOut(() => OwnershipChangedOnAllClients(currentOwner)); - AssertOnTimeout($"[1st Phase] Timed out waiting for all clients to change ownership!\n {m_ErrorLog.ToString()}"); - helper = GetOwnerInstance(); - yield return runWaitPeriod; - } - - // Now reset the values - helper.SetState(BaseCollectionUpdateHelper.HelperStates.Pause); - helper.Clear(); - - // Validate all instances are reset - yield return WaitForConditionOrTimeOut(ValidateAllInstances); - AssertOnTimeout($"[Pre][2nd Phase]Timed out waiting for all clients to validdate their values!"); - helper.SetState(BaseCollectionUpdateHelper.HelperStates.Start); - - // Update, change ownership, and repeat until all clients have been the owner at least once - for (int i = 0; i < 4; i++) - { - yield return runWaitPeriod; - currentOwner = GetAuthorityInstance().ChangeOwner(); - Assert.IsFalse(currentOwner == ulong.MaxValue, "A non-authority instance attempted to change ownership!"); - yield return WaitForConditionOrTimeOut(() => OwnershipChangedOnAllClients(currentOwner)); - AssertOnTimeout($"[2nd Phase] Timed out waiting for all clients to change ownership!"); - helper = GetOwnerInstance(); - } - - helper.SetState(BaseCollectionUpdateHelper.HelperStates.Pause); - yield return WaitForConditionOrTimeOut(ValidateAllInstances); - AssertOnTimeout($"[Last Validate] Timed out waiting for all clients to validdate their values!"); - } - } - - #region COLLECTION CHANGING COMPONENTS - /// - /// Helper class to test adding dictionary entries rapidly with frequent ownership changes. - /// This includes a companion integer that is continually incremented and used as the key value for each entry. - /// - internal class DictionaryCollectionUpdateHelper : BaseCollectionUpdateHelper - { - private NetworkVariable> m_DictionaryCollection = new NetworkVariable>(new Dictionary(), NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); - private NetworkVariable m_CurrentKeyValue = new NetworkVariable(0, NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); - - protected override bool OnValidateAgainst(BaseCollectionUpdateHelper otherHelper) - { - var otherListHelper = otherHelper as DictionaryCollectionUpdateHelper; - var localValues = m_DictionaryCollection.Value; - var otherValues = otherListHelper.m_DictionaryCollection.Value; - - if (localValues.Count != otherValues.Count) - { - return false; - } - - foreach (var entry in m_DictionaryCollection.Value) - { - if (!otherValues.ContainsKey(entry.Key)) - { - return false; - } - - if (entry.Value != otherValues[entry.Key]) - { - return false; - } - } - return true; - } - protected override void OnClear() - { - m_DictionaryCollection.Value.Clear(); - m_DictionaryCollection.CheckDirtyState(); - base.OnClear(); - } - - protected override void AddItem() - { - m_DictionaryCollection.Value.Add(m_CurrentKeyValue.Value, m_CurrentKeyValue.Value); - m_DictionaryCollection.CheckDirtyState(); - m_CurrentKeyValue.Value++; - } - } - - /// - /// Helper class to test adding list entries rapidly with frequent ownership changes - /// - internal class ListCollectionUpdateHelper : BaseCollectionUpdateHelper - { - private NetworkVariable> m_ListCollection = new NetworkVariable>(new List(), NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); - - - protected override bool OnValidateAgainst(BaseCollectionUpdateHelper otherHelper) - { - var otherListHelper = otherHelper as ListCollectionUpdateHelper; - var localValues = m_ListCollection.Value; - var otherValues = otherListHelper.m_ListCollection.Value; - - if (localValues.Count != otherValues.Count) - { - return false; - } - - for (int i = 0; i < localValues.Count - 1; i++) - { - if (localValues[i] != i) - { - return false; - } - - if (localValues[i] != otherValues[i]) - { - return false; - } - } - return true; - } - - protected override void OnClear() - { - m_ListCollection.Value.Clear(); - m_ListCollection.CheckDirtyState(); - base.OnClear(); - } - - protected override void AddItem() - { - m_ListCollection.Value.Add(m_ListCollection.Value.Count); - m_ListCollection.CheckDirtyState(); - } - } - - /// - /// The base class to test rapidly adding items to a collection type - /// - internal class BaseCollectionUpdateHelper : NetworkBehaviour - { - public static bool VerboseMode; - private const int k_OwnershipTickDelay = 1; - - public enum HelperStates - { - Stop, - Start, - Pause, - ClearToChangeOwner, - ChangingOwner - } - public HelperStates HelperState { get; private set; } - - private int m_SendClearForOwnershipOnTick; - private ulong m_NextClient = 0; - private ulong m_ClientToSendClear = 0; - - public void SetState(HelperStates helperState) - { - HelperState = helperState; - } - - protected virtual bool OnValidateAgainst(BaseCollectionUpdateHelper otherHelper) - { - return true; - } - - public bool ValidateAgainst(BaseCollectionUpdateHelper otherHelper) - { - return OnValidateAgainst(otherHelper); - } - - public override void OnNetworkSpawn() - { - // Register for tick updates - NetworkManager.NetworkTickSystem.Tick += OnNetworkTick; - - base.OnNetworkSpawn(); - } - public override void OnNetworkDespawn() - { - NetworkManager.NetworkTickSystem.Tick -= OnNetworkTick; - base.OnNetworkDespawn(); - } - - protected virtual void OnClear() - { - } - - public void Clear() - { - OnClear(); - } - - protected virtual void AddItem() - { - } - - private bool CanUpdate() - { - return HelperState == HelperStates.Start; - } - - private void Update() - { - // Exit early if not spawn, updating is not enabled, or is not the owner - if (!IsSpawned || !CanUpdate() || !IsOwner) - { - return; - } - - AddItem(); - } - - protected override void OnOwnershipChanged(ulong previous, ulong current) - { - // When the ownership changes and the client is the owner, then immediately add an item to the collection - if (NetworkManager.LocalClientId == current) - { - AddItem(); - } - base.OnOwnershipChanged(previous, current); - } - - - /// - /// Sets the tick delay period of time to provide all in-flight deltas to be processed. - /// - private void SetTickDelay() - { - m_SendClearForOwnershipOnTick = NetworkManager.ServerTime.Tick + k_OwnershipTickDelay; - } - - /// - /// Changes the ownership - /// - /// next owner or ulong.MaxValue that means the authority did not invoke this method - public ulong ChangeOwner() - { - if (HasAuthority && !IsOwnershipChanging()) - { - var index = NetworkManager.ConnectedClientsIds.ToList().IndexOf(OwnerClientId); - index++; - index = index % NetworkManager.ConnectedClientsIds.Count; - m_NextClient = NetworkManager.ConnectedClientsIds[index]; - - // If we are in distributed authority and the authority or we are in client-server and the server, then make the change ourselves. - if (OwnerClientId == NetworkManager.LocalClientId && (NetworkManager.DistributedAuthorityMode || (!NetworkManager.DistributedAuthorityMode && NetworkManager.IsServer))) - { - HelperState = HelperStates.ChangingOwner; - SetTickDelay(); - Log($"Locally changing ownership to Client-{m_NextClient}"); - } - - if (!NetworkManager.DistributedAuthorityMode && NetworkManager.IsServer && OwnerClientId != NetworkManager.LocalClientId) - { - // If we are transitioning between a client to the host or client to client, - // send a "heads-up" Rpc to the client prior to changing ownership. The client - // will stop updating for the tick delay period and then send a confirmation - // to the host that it is clear to change ownership. - ChangingOwnershipRpc(RpcTarget.Single(OwnerClientId, RpcTargetUse.Temp)); - Log($"Remotely changing ownership to Client-{m_NextClient}"); - } - - return m_NextClient; - } - - return ulong.MaxValue; - } - - /// - /// Sent by the host to a client when ownership is transitioning from a client to - /// the host or to another client. - /// - [Rpc(SendTo.SpecifiedInParams)] - private void ChangingOwnershipRpc(RpcParams rpcParams = default) - { - // The sender is who we respond to that it is clear to change ownership - m_ClientToSendClear = rpcParams.Receive.SenderClientId; - HelperState = HelperStates.ClearToChangeOwner; - SetTickDelay(); - } - - /// - /// Notification that the current owner has stopped updating and ownership - /// updates can occur without missed updates. - /// - /// - [Rpc(SendTo.SpecifiedInParams)] - private void ChangingOwnershipClearRpc(RpcParams rpcParams = default) - { - HelperState = HelperStates.ChangingOwner; - SetTickDelay(); - Log($"Changing ownership to Client-{m_NextClient} based on ready request."); - } - - private bool IsOwnershipChanging() - { - return HelperState == HelperStates.ClearToChangeOwner || HelperState == HelperStates.ChangingOwner; - } - - private void OnNetworkTick() - { - if (!IsSpawned || !IsOwnershipChanging() || m_SendClearForOwnershipOnTick > NetworkManager.ServerTime.Tick) - { - return; - } - - if (HelperState == HelperStates.ChangingOwner) - { - NetworkObject.ChangeOwnership(m_NextClient); - Log($"Local Change ownership to Client-{m_NextClient} complete! New Owner is {NetworkObject.OwnerClientId} | Expected {m_NextClient}"); - } - else - { - ChangingOwnershipClearRpc(RpcTarget.Single(m_ClientToSendClear, RpcTargetUse.Temp)); - } - HelperState = HelperStates.Stop; - } - - protected void Log(string msg) - { - if (VerboseMode) - { - Debug.Log($"[Client-{NetworkManager.LocalClientId}] {msg}"); - } - } - } - #endregion - #region HASHSET COMPONENT HELPERS internal class HashSetBaseTypeTestHelper : ListTestHelperBase, IHashSetTestHelperBase { From 578754bf37bf39513a3bb849e53dad5159d55e0b Mon Sep 17 00:00:00 2001 From: Emma Date: Wed, 9 Jul 2025 16:17:28 -0400 Subject: [PATCH 2/4] Add non-collections test --- .../NetworkVariableCollectionsTests.cs | 1 - .../NetworkVariable/NetworkVariableTests.cs | 49 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs index f8c0090522..85d283df3c 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs @@ -5,7 +5,6 @@ using System.Text; using NUnit.Framework; using Unity.Netcode.TestHelpers.Runtime; -using UnityEngine; using UnityEngine.TestTools; using Random = UnityEngine.Random; diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs index 48ccccf319..9ed5854b4e 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs @@ -1173,6 +1173,55 @@ public void WhenCreatingAnArrayOfNetVars_InitializingVariablesDoesNotThrowAnExce Assert.AreSame(testComp.AllInts[4], testComp.Int4); } + [Test] + public void TestNetworkVariableChangeAndReturnInSameFrame([Values] HostOrServer useHost) + { + InitializeServerAndClients(useHost); + + var serverOnValueChangedCount = 0; + var clientOnValueChangedCount = 0; + + void ServerOnValueChanged(NetworkVariableTest.SomeEnum previous, NetworkVariableTest.SomeEnum next) + { + serverOnValueChangedCount++; + } + + void ClientOnValueChanged(NetworkVariableTest.SomeEnum previous, NetworkVariableTest.SomeEnum next) + { + clientOnValueChangedCount++; + } + + bool VerifyStructure() + { + return m_Player1OnClient1.TheEnum.Value == m_Player1OnServer.TheEnum.Value; + } + + // Wait for the client-side to notify it is finished initializing and spawning. + Assert.True(WaitForConditionOrTimeOutWithTimeTravel(VerifyStructure)); + + m_Player1OnServer.TheEnum.OnValueChanged += ServerOnValueChanged; + m_Player1OnClient1.TheEnum.OnValueChanged += ClientOnValueChanged; + + // Change the value once in a frame + m_Player1OnServer.TheEnum.Value = NetworkVariableTest.SomeEnum.B; + + // Wait for the value to sync and assert that both server and client had OnValueChanged called an equal number of times + Assert.True(WaitForConditionOrTimeOutWithTimeTravel(VerifyStructure), "Timed out waiting for client and server values to match"); + Assert.AreEqual(serverOnValueChangedCount, clientOnValueChangedCount, $"Expected OnValueChanged call count to be equal. Server OnValueChanged was called {serverOnValueChangedCount} times but client was called {clientOnValueChangedCount} times!"); + + // Change the value twice in a frame + m_Player1OnServer.TheEnum.Value = NetworkVariableTest.SomeEnum.A; + m_Player1OnServer.TheEnum.Value = NetworkVariableTest.SomeEnum.B; + + // Wait for the value to sync and assert that the server had OnValueChanged called once more than the client + TimeTravelAdvanceTick(); + Assert.True(WaitForConditionOrTimeOutWithTimeTravel(VerifyStructure), "Timed out waiting for client and server values to match"); + Assert.AreEqual(serverOnValueChangedCount - 1, clientOnValueChangedCount, $"Unexpected OnValueChanged call count. Server OnValueChanged was called {serverOnValueChangedCount} times but client was called {clientOnValueChangedCount} times!"); + + m_Player1OnServer.TheEnum.OnValueChanged -= ServerOnValueChanged; + m_Player1OnClient1.TheEnum.OnValueChanged -= ClientOnValueChanged; + } + private void TestValueType(T testValue, T changedValue) where T : unmanaged { var serverVariable = new NetworkVariable(testValue); From 6b544373c7b9ebcda8cc820479a05ecaf0fe58fd Mon Sep 17 00:00:00 2001 From: Emma Date: Wed, 9 Jul 2025 16:23:21 -0400 Subject: [PATCH 3/4] Update Changelog --- com.unity.netcode.gameobjects/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 7bce305b1b..705e23cde5 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed ensuring OnValueChanged callback is still triggered on the authority when a collection changes and then reverts to the previous value in the same frame. (#3539) - Fixed distributed authority related issue where enabling the `NetworkObject.DestroyWithScene` would cause errors when a destroying non-authority instances due to loading (single mode) or unloading scene events. (#3500) ### Changed From d454cb6c90346ee170dd6ae7eeb090a81b59dc3c Mon Sep 17 00:00:00 2001 From: Emma Date: Fri, 11 Jul 2025 11:44:16 -0400 Subject: [PATCH 4/4] rename m_InternalOriginalValue and add code doc --- .../NetworkVariable/NetworkVariable.cs | 74 ++++++++++++------- 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs index 1405406c75..b34b3cfeb8 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs @@ -57,7 +57,7 @@ public override void OnInitialize() base.OnInitialize(); m_HasPreviousValue = true; - NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_LastInternalValue); NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_PreviousValue); } @@ -73,7 +73,7 @@ public NetworkVariable(T value = default, : base(readPerm, writePerm) { m_InternalValue = value; - m_InternalOriginalValue = default; + m_LastInternalValue = default; // Since we start with IsDirty = true, this doesn't need to be duplicated // right away. It won't get read until after ResetDirty() is called, and // the duplicate will be made there. Avoiding calling @@ -92,25 +92,45 @@ public void Reset(T value = default) if (m_NetworkBehaviour == null || m_NetworkBehaviour != null && !m_NetworkBehaviour.NetworkObject.IsSpawned) { m_InternalValue = value; - NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_LastInternalValue); m_PreviousValue = default; } } /// - /// The internal value of the NetworkVariable + /// The current internal value of the NetworkVariable. /// + /// + /// When using collections, this InternalValue can be updated directly without going through the setter. + /// [SerializeField] private protected T m_InternalValue; - // The introduction of standard .NET collections caused an issue with permissions since there is no way to detect changes in the - // collection without doing a full comparison. While this approach does consume more memory per collection instance, it is the - // lowest risk approach to resolving the issue where a client with no write permissions could make changes to a collection locally - // which can cause a myriad of issues. - private protected T m_InternalOriginalValue; + /// + /// The last valid/authorized value of the network variable. + /// + /// + /// The introduction of standard .NET collections caused an issue with permissions since there is no way to detect changes in the + /// collection without doing a full comparison. While this approach does consume more memory per collection instance, it is the + /// lowest risk approach to resolving the issue where a client with no write permissions could make changes to a collection locally + /// which can cause a myriad of issues. + /// + private protected T m_LastInternalValue; + /// + /// The most recent value that was synchronized over the network. + /// Synchronized over the network at the end of the frame in which the was marked dirty. + /// + /// + /// Only contains the value synchronized over the network at the end of the last frame. + /// All in-between changes on the authority are tracked by . + /// private protected T m_PreviousValue; + /// + /// Whether this network variable has had changes synchronized over the network. + /// Indicates whether is populated and valid. + /// private bool m_HasPreviousValue; private bool m_IsDisposed; @@ -139,7 +159,7 @@ public virtual T Value { T previousValue = m_InternalValue; m_InternalValue = value; - NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_LastInternalValue); SetDirty(true); m_IsDisposed = false; OnValueChanged?.Invoke(previousValue, m_InternalValue); @@ -165,21 +185,21 @@ public bool CheckDirtyState(bool forceCheck = false) if (CannotWrite) { // If modifications are detected, then revert back to the last known current value - if (!NetworkVariableSerialization.AreEqual(ref m_InternalValue, ref m_InternalOriginalValue)) + if (!NetworkVariableSerialization.AreEqual(ref m_InternalValue, ref m_LastInternalValue)) { - NetworkVariableSerialization.Duplicate(m_InternalOriginalValue, ref m_InternalValue); + NetworkVariableSerialization.Duplicate(m_LastInternalValue, ref m_InternalValue); } return false; } - // Compare the previous with the current if not dirty or forcing a check. - if ((!isDirty || forceCheck) && !NetworkVariableSerialization.AreEqual(ref m_InternalOriginalValue, ref m_InternalValue)) + // Compare the last internal value with the current value if not dirty or forcing a check. + if ((!isDirty || forceCheck) && !NetworkVariableSerialization.AreEqual(ref m_LastInternalValue, ref m_InternalValue)) { SetDirty(true); - OnValueChanged?.Invoke(m_InternalOriginalValue, m_InternalValue); + OnValueChanged?.Invoke(m_LastInternalValue, m_InternalValue); m_IsDisposed = false; isDirty = true; - NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_LastInternalValue); } return isDirty; } @@ -213,11 +233,11 @@ public override void Dispose() m_InternalValue = default; // Dispose the internal original value - if (m_InternalOriginalValue is IDisposable internalOriginalValueDisposable) + if (m_LastInternalValue is IDisposable internalOriginalValueDisposable) { internalOriginalValueDisposable.Dispose(); } - m_InternalOriginalValue = default; + m_LastInternalValue = default; // Dispose the previous value if there is one if (m_HasPreviousValue && m_PreviousValue is IDisposable previousValueDisposable) @@ -246,9 +266,9 @@ public override bool IsDirty() { // If the client does not have write permissions but the internal value is determined to be locally modified and we are applying updates, then we should revert // to the original collection value prior to applying updates (primarily for collections). - if (!NetworkUpdaterCheck && CannotWrite && !NetworkVariableSerialization.AreEqual(ref m_InternalValue, ref m_InternalOriginalValue)) + if (!NetworkUpdaterCheck && CannotWrite && !NetworkVariableSerialization.AreEqual(ref m_InternalValue, ref m_LastInternalValue)) { - NetworkVariableSerialization.Duplicate(m_InternalOriginalValue, ref m_InternalValue); + NetworkVariableSerialization.Duplicate(m_LastInternalValue, ref m_InternalValue); return true; } // For most cases we can use the dirty flag. @@ -285,7 +305,7 @@ public override void ResetDirty() m_HasPreviousValue = true; NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_PreviousValue); // Once updated, assure the original current value is updated for future comparison purposes - NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_LastInternalValue); } base.ResetDirty(); } @@ -308,9 +328,9 @@ public override void ReadDelta(FastBufferReader reader, bool keepDirtyDelta) { // If the client does not have write permissions but the internal value is determined to be locally modified and we are applying updates, then we should revert // to the original collection value prior to applying updates (primarily for collections). - if (CannotWrite && !NetworkVariableSerialization.AreEqual(ref m_InternalOriginalValue, ref m_InternalValue)) + if (CannotWrite && !NetworkVariableSerialization.AreEqual(ref m_LastInternalValue, ref m_InternalValue)) { - NetworkVariableSerialization.Duplicate(m_InternalOriginalValue, ref m_InternalValue); + NetworkVariableSerialization.Duplicate(m_LastInternalValue, ref m_InternalValue); } NetworkVariableSerialization.ReadDelta(reader, ref m_InternalValue); @@ -342,7 +362,7 @@ internal override void PostDeltaRead() m_HasPreviousValue = true; NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_PreviousValue); // Once updated, assure the original current value is updated for future comparison purposes - NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_LastInternalValue); } /// @@ -350,9 +370,9 @@ public override void ReadField(FastBufferReader reader) { // If the client does not have write permissions but the internal value is determined to be locally modified and we are applying updates, then we should revert // to the original collection value prior to applying updates (primarily for collections). - if (CannotWrite && !NetworkVariableSerialization.AreEqual(ref m_InternalOriginalValue, ref m_InternalValue)) + if (CannotWrite && !NetworkVariableSerialization.AreEqual(ref m_LastInternalValue, ref m_InternalValue)) { - NetworkVariableSerialization.Duplicate(m_InternalOriginalValue, ref m_InternalValue); + NetworkVariableSerialization.Duplicate(m_LastInternalValue, ref m_InternalValue); } NetworkVariableSerialization.Read(reader, ref m_InternalValue); @@ -364,7 +384,7 @@ public override void ReadField(FastBufferReader reader) NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_PreviousValue); // Once updated, assure the original current value is updated for future comparison purposes - NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_InternalOriginalValue); + NetworkVariableSerialization.Duplicate(m_InternalValue, ref m_LastInternalValue); } ///