diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 127203e7d3..8730fc4cb2 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -24,6 +24,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed issue where `NetworkVariable` was not properly synchronizing to changes made by the spawn and write authority during `OnNetworkSpawn` and `OnNetworkPostSpawn`. (#3878) - Fixed issue where `NetworkManager` was not cleaning itself up if an exception was thrown while starting. (#3864) - Prevented a `NullReferenceException` in `UnityTransport` when using a custom `INetworkStreamDriverConstructor` that doesn't use all the default pipelines and the multiplayer tools package is installed. (#3853) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index 81a2aa03a1..3d3c919986 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -838,7 +838,7 @@ internal void NetworkPostSpawn() // all spawn related methods have been invoked. for (int i = 0; i < NetworkVariableFields.Count; i++) { - NetworkVariableFields[i].OnSpawned(); + NetworkVariableFields[i].InternalOnSpawned(); } } @@ -891,7 +891,7 @@ internal void InternalOnNetworkPreDespawn() // all spawn related methods have been invoked. for (int i = 0; i < NetworkVariableFields.Count; i++) { - NetworkVariableFields[i].OnPreDespawn(); + NetworkVariableFields[i].InternalOnPreDespawn(); } } diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs index 3b2117bf11..18322ee332 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs @@ -58,28 +58,6 @@ public NetworkList(IEnumerable values = default, Dispose(); } - internal override void OnSpawned() - { - // If the NetworkList is: - // - Dirty - // - State updates can be sent: - // -- The instance has write permissions. - // -- The last sent time plus the max send time period is less than the current time. - // - User script has modified the list during spawn. - // - This instance is on the spawn authority side. - // When the NetworkObject is finished spawning (on the same frame), go ahead and reset - // the dirty related properties and last sent time to prevent duplicate entries from - // being sent (i.e. CreateObjectMessage will contain the changes so we don't need to - // send a proceeding NetworkVariableDeltaMessage). - if (IsDirty() && CanSend() && m_NetworkObject.IsSpawnAuthority) - { - UpdateLastSentTime(); - ResetDirty(); - SetDirty(false); - } - base.OnSpawned(); - } - /// public override void ResetDirty() { diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs index 5186c7a13f..0c3532208a 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs @@ -140,27 +140,37 @@ public void Initialize(NetworkBehaviour networkBehaviour) } } - /// TODO-API: After further vetting and alignment on these, we might make them part of the public API. - /// Could actually be like an interface that gets automatically registered for these kinds of notifications - /// without having to be a NetworkBehaviour. - #region OnSpawn and OnPreDespawn (ETC) - /// /// Invoked after the associated has been invoked. /// - internal virtual void OnSpawned() + internal void InternalOnSpawned() { - + // If the NetworkVariableBase derived class is: + // - On the spawn authority side. + // - Dirty. + // - State updates can be sent: + // -- The instance has write permissions. + // -- The last sent time plus the max send time period is less than the current time. + // - User script has modified the list during spawn. + // When the NetworkObject is finished spawning (on the same frame), go ahead and reset + // the dirty related properties and last sent time to prevent duplicate updates from + // being sent (i.e. CreateObjectMessage will contain the changes so we don't need to + // send a proceeding NetworkVariableDeltaMessage). + if (m_NetworkObject.IsSpawnAuthority && IsDirty() && CanWrite() && CanSend()) + { + UpdateLastSentTime(); + ResetDirty(); + SetDirty(false); + } } /// /// Invoked after the associated has been invoked. /// - internal virtual void OnPreDespawn() + internal void InternalOnPreDespawn() { } - #endregion /// /// Deinitialize is invoked when a NetworkObject is despawned. diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs index 4bad45e088..7e882d0085 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs @@ -831,7 +831,7 @@ public IEnumerator TestDictionaryCollections() { // Server-side add same key and SerializableObject prior to being added to the owner side compDictionaryServer.ListCollectionOwner.Value.Add(newEntry.Item1, newEntry.Item2); - // Checking if dirty on server side should revert back to origina known current dictionary state + // Checking if dirty on server side should revert back to original known current dictionary state compDictionaryServer.ListCollectionOwner.IsDirty(); yield return WaitForConditionOrTimeOut(() => compDictionaryServer.CompareTrackedChanges(ListTestHelperBase.Targets.Owner)); AssertOnTimeout($"Server add to owner write collection property failed to restore on {className} {compDictionaryServer.name}! {compDictionaryServer.GetLog()}"); @@ -840,7 +840,6 @@ public IEnumerator TestDictionaryCollections() // Server-side add a completely new key and SerializableObject to to owner write permission property compDictionaryServer.ListCollectionOwner.Value.Add(GetNextKey(), SerializableObject.GetRandomObject()); // Both should be overridden by the owner-side update - } VerboseDebug($"[{compDictionary.name}][Owner] Adding Key: {newEntry.Item1}"); // Add key and SerializableObject to owner side @@ -852,15 +851,17 @@ public IEnumerator TestDictionaryCollections() ////////////////////////////////// // Server Add SerializableObject Entry newEntry = (GetNextKey(), SerializableObject.GetRandomObject()); + // Only test restore on non-host clients (otherwise a host is both server and client/owner) if (!client.IsServer) { // Client-side add same key and SerializableObject to server write permission property compDictionary.ListCollectionServer.Value.Add(newEntry.Item1, newEntry.Item2); - // Checking if dirty on client side should revert back to origina known current dictionary state + // Checking if dirty on client side should revert back to original known current dictionary state compDictionary.ListCollectionServer.IsDirty(); yield return WaitForConditionOrTimeOut(() => compDictionary.CompareTrackedChanges(ListTestHelperBase.Targets.Server)); AssertOnTimeout($"Client-{client.LocalClientId} add to server write collection property failed to restore on {className} {compDictionary.name}! {compDictionary.GetLog()}"); + // Client-side add the same key and SerializableObject to server write permission property (would throw key exists exception too if previous failed) compDictionary.ListCollectionServer.Value.Add(newEntry.Item1, newEntry.Item2); // Client-side add a completely new key and SerializableObject to to server write permission property @@ -892,7 +893,6 @@ public IEnumerator TestDictionaryCollections() yield return WaitForConditionOrTimeOut(() => compDictionaryServer.ValidateInstances()); AssertOnTimeout($"[Server] Not all instances of client-{compDictionaryServer.OwnerClientId}'s {className} {compDictionaryServer.name} component match! {compDictionaryServer.GetLog()}"); - ValidateClientsFlat(client); //////////////////////////////////// // Owner Change SerializableObject Entry @@ -915,7 +915,7 @@ public IEnumerator TestDictionaryCollections() { // Server-side update same key value prior to being updated to the owner side compDictionaryServer.ListCollectionOwner.Value[valueInt] = randomObject; - // Checking if dirty on server side should revert back to origina known current dictionary state + // Checking if dirty on server side should revert back to original known current dictionary state compDictionaryServer.ListCollectionOwner.IsDirty(); yield return WaitForConditionOrTimeOut(() => compDictionaryServer.CompareTrackedChanges(ListTestHelperBase.Targets.Owner)); AssertOnTimeout($"Server update collection entry value to local owner write collection property failed to restore on {className} {compDictionaryServer.name}! {compDictionaryServer.GetLog()}"); @@ -956,7 +956,7 @@ public IEnumerator TestDictionaryCollections() { // Owner-side update same key value prior to being updated to the server side compDictionary.ListCollectionServer.Value[valueInt] = randomObject; - // Checking if dirty on owner side should revert back to origina known current dictionary state + // Checking if dirty on owner side should revert back to original known current dictionary state compDictionary.ListCollectionServer.IsDirty(); yield return WaitForConditionOrTimeOut(() => compDictionary.CompareTrackedChanges(ListTestHelperBase.Targets.Server)); AssertOnTimeout($"Client-{client.LocalClientId} update collection entry value to local server write collection property failed to restore on {className} {compDictionary.name}! {compDictionary.GetLog()}"); @@ -1841,10 +1841,10 @@ private bool ChangesMatch(ulong clientId, Dictionary().ToList(); foreach (var deltaType in deltaTypes) { - LogMessage($"Comparing {deltaType}:"); + LogMessage($"[Comparing {deltaType}] Local: {local[deltaType].Count} | Other: {other[deltaType].Count}"); if (local[deltaType].Count != other[deltaType].Count) { - LogMessage($"{deltaType}s count did not match!"); + LogMessage($"[Client-{clientId}] Local {deltaType}s count of {local[deltaType].Count} did not match the other's count of {other[deltaType].Count}!"); return false; } if (!CompareDictionaries(clientId, local[deltaType], other[deltaType])) @@ -1970,6 +1970,18 @@ public void TrackChanges(Targets target, Dictionary pre contextTable[DeltaTypes.Removed] = whatWasRemoved; contextTable[DeltaTypes.Changed] = whatChanged; contextTable[DeltaTypes.UnChanged] = whatRemainedTheSame; + + // Log all incoming changes when debug mode is enabled + if (!IsOwner && IsDebugMode) + { + LogMessage($"[{NetworkManager.name}][TrackChanges-> Client-{OwnerClientId}] Collection was updated!"); + LogMessage($"Added: {whatWasAdded.Count} "); + LogMessage($"Removed: {whatWasRemoved.Count} "); + LogMessage($"Changed: {whatChanged.Count} "); + LogMessage($"UnChanged: {whatRemainedTheSame.Count} "); + UnityEngine.Debug.Log($"{GetLog()}"); + LogStart(); + } } public void OnServerListValuesChanged(Dictionary previous, Dictionary current) @@ -2016,8 +2028,8 @@ public void ResetTrackedChanges() protected override void OnNetworkPostSpawn() { - TrackRelativeInstances(); + TrackRelativeInstances(); ListCollectionServer.OnValueChanged += OnServerListValuesChanged; ListCollectionOwner.OnValueChanged += OnOwnerListValuesChanged; @@ -2025,6 +2037,8 @@ protected override void OnNetworkPostSpawn() { InitValues(); } + + base.OnNetworkPostSpawn(); } public void InitValues() @@ -2032,7 +2046,7 @@ public void InitValues() if (IsServer) { ListCollectionServer.Value = OnSetServerValues(); - ListCollectionOwner.CheckDirtyState(); + ListCollectionServer.CheckDirtyState(); } if (IsOwner) @@ -2040,6 +2054,17 @@ public void InitValues() ListCollectionOwner.Value = OnSetOwnerValues(); ListCollectionOwner.CheckDirtyState(); } + + // When running a host, the changes being tracked will not match because clients will be synchronized with changes + // already applied. This fixing this issue by injecting "added" server targeted changes during initialization on + // the connected clients' side. + if (!IsServer) + { + if (ListCollectionServer.Value.Count > 0 && NetworkVariableChanges[Targets.Server][DeltaTypes.Added].Count == 0) + { + TrackChanges(Targets.Server, new Dictionary(), ListCollectionServer.Value); + } + } } public override void OnNetworkDespawn() diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableGeneralTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableGeneralTests.cs new file mode 100644 index 0000000000..b0f3e8be56 --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableGeneralTests.cs @@ -0,0 +1,168 @@ +using System.Collections; +using System.Text; +using NUnit.Framework; +using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine; +using UnityEngine.TestTools; + +namespace Unity.Netcode.RuntimeTests +{ + /// + /// General integration tests. + /// + [TestFixture(HostOrServer.Host)] + [TestFixture(HostOrServer.DAHost)] + [TestFixture(HostOrServer.Server)] + internal class NetworkVariableGeneralTests : NetcodeIntegrationTest + { + protected override int NumberOfClients => 2; + + private GameObject m_PrefabToSpawn; + private GeneralNetVarTest m_AuthorityNetVarTest; + + internal class GeneralNetVarTest : NetworkBehaviour + { + /// + /// NetworkVariable that is set by the authority during spawn. + /// + public NetworkVariable TestValueOnSpawn = new NetworkVariable(default); + /// + /// NetworkVariable that is set by the authority during post spawn. + /// + public NetworkVariable TestValueOnPostSpawn = new NetworkVariable(default); + + /// + /// Field value set to during . + /// + public float OnNetworkSpawnValue; + + /// + /// Field value set to during . + /// + public float OnNetworkPostSpawnValue; + + public override void OnNetworkSpawn() + { + if (HasAuthority) + { + TestValueOnSpawn.Value = Random.Range(0.01f, 100.0f); + } + else + { + // Only set these values during OnNetworkSpawn to + // verify this value is valid for non-authority instances. + OnNetworkSpawnValue = TestValueOnSpawn.Value; + OnNetworkPostSpawnValue = TestValueOnPostSpawn.Value; + } + base.OnNetworkSpawn(); + } + + protected override void OnNetworkPostSpawn() + { + if (HasAuthority) + { + TestValueOnPostSpawn.Value = Random.Range(0.01f, 100.0f); + } + base.OnNetworkPostSpawn(); + } + } + + public NetworkVariableGeneralTests(HostOrServer host) : base(host) { } + + protected override void OnServerAndClientsCreated() + { + m_PrefabToSpawn = CreateNetworkObjectPrefab("TestNetVar"); + m_PrefabToSpawn.AddComponent(); + base.OnServerAndClientsCreated(); + } + + /// + /// Verifies that upon spawn or post spawn the value is set within OnNetworkSpawn on the + /// non-authority instances. + /// + private bool SpawnValuesMatch(StringBuilder errorLog) + { + var authority = GetAuthorityNetworkManager(); + var authorityOnSpawnValue = m_AuthorityNetVarTest.TestValueOnSpawn.Value; + var authorityOnPostSpawnValue = m_AuthorityNetVarTest.TestValueOnPostSpawn.Value; + foreach (var networkManager in m_NetworkManagers) + { + if (authority) + { + continue; + } + if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(m_AuthorityNetVarTest.NetworkObjectId)) + { + errorLog.AppendLine($"[{networkManager.name}] Does not have a spawned instance of {m_AuthorityNetVarTest.name}!"); + continue; + } + var netVarTest = networkManager.SpawnManager.SpawnedObjects[m_AuthorityNetVarTest.NetworkObjectId].GetComponent(); + if (netVarTest.OnNetworkSpawnValue != authorityOnSpawnValue) + { + errorLog.AppendLine($"[{networkManager.name}][OnNetworkSpawn Value] Non-authority value: {netVarTest.OnNetworkSpawnValue} does not match " + + $"the authority value: {authorityOnSpawnValue}!"); + } + if (netVarTest.OnNetworkPostSpawnValue != authorityOnPostSpawnValue) + { + errorLog.AppendLine($"[{networkManager.name}][OnNetworkPostSpawn Value] Non-authority value: {netVarTest.OnNetworkPostSpawnValue} does not match " + + $"the authority value: {authorityOnPostSpawnValue}!"); + } + } + return errorLog.Length == 0; + } + + /// + /// Verifies that changing the value synchronizes properly. + /// + private bool ChangedValueMatches(StringBuilder errorLog) + { + var authority = GetAuthorityNetworkManager(); + var authorityValue = m_AuthorityNetVarTest.TestValueOnSpawn.Value; + foreach (var networkManager in m_NetworkManagers) + { + if (authority) + { + continue; + } + var netVarTest = networkManager.SpawnManager.SpawnedObjects[m_AuthorityNetVarTest.NetworkObjectId].GetComponent(); + if (netVarTest.TestValueOnSpawn.Value != authorityValue) + { + errorLog.AppendLine($"[{networkManager.name}][Changed] Non-auhoroty value: {netVarTest.TestValueOnSpawn.Value} does not match " + + $"the authority value: {authorityValue}!"); + } + } + return errorLog.Length == 0; + } + + /// + /// Validates when the authority applies a value during spawn or + /// post spawn of a newly instantiated and spawned object the value is set by the time non-authority + /// instances invoke . + /// + [UnityTest] + public IEnumerator ApplyValueDuringSpawnSequence() + { + var authority = GetAuthorityNetworkManager(); + m_AuthorityNetVarTest = SpawnObject(m_PrefabToSpawn, authority).GetComponent(); + yield return WaitForSpawnedOnAllOrTimeOut(m_AuthorityNetVarTest.gameObject); + AssertOnTimeout($"Not all clients spawned {m_AuthorityNetVarTest.name}!"); + + yield return WaitForConditionOrTimeOut(SpawnValuesMatch); + AssertOnTimeout($"Values did not match for {m_AuthorityNetVarTest.name}!"); + + // Verify late joined clients synchronize correctly + yield return CreateAndStartNewClient(); + + yield return WaitForSpawnedOnAllOrTimeOut(m_AuthorityNetVarTest.gameObject); + AssertOnTimeout($"Not all clients spawned {m_AuthorityNetVarTest.name}!"); + + yield return WaitForConditionOrTimeOut(SpawnValuesMatch); + AssertOnTimeout($"Values did not match for {m_AuthorityNetVarTest.name}!"); + + // Verify changing the value synchronizes properly + m_AuthorityNetVarTest.TestValueOnSpawn.Value += Random.Range(0.01f, 100.0f); + yield return WaitForConditionOrTimeOut(ChangedValueMatches); + AssertOnTimeout($"Values did not match for {m_AuthorityNetVarTest.name}!"); + } + } +} diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableGeneralTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableGeneralTests.cs.meta new file mode 100644 index 0000000000..e744b6506b --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableGeneralTests.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: e5a5c7d93dfa81642a2138c3cc5e3300 \ No newline at end of file diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerModifiedTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerModifiedTests.cs index ecd4d58fa7..1526684aee 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerModifiedTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerModifiedTests.cs @@ -154,20 +154,26 @@ public IEnumerator VerifyDoesNotRepeatOnSomeClients() Assert.True(OwnerModifiedObject.Updates > 20); } - - private ChangeValueOnAuthority m_SessionAuthorityInstance; private ChangeValueOnAuthority m_InstanceAuthority; private bool NetworkVariablesMatch(StringBuilder errorLog) { foreach (var networkManager in m_NetworkManagers) { + var changeValue = networkManager.SpawnManager.SpawnedObjects[m_InstanceAuthority.NetworkObjectId].GetComponent(); if (networkManager == m_InstanceAuthority.NetworkManager) { + if (m_InstanceAuthority.SomeIntValue.Value != 2) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] {changeValue.name} value is {changeValue.SomeIntValue.Value} but was expecting 2!"); + } continue; } + if (changeValue.SomeIntValue.Value != 2) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] {changeValue.name} value is {changeValue.SomeIntValue.Value} but was expecting 2!"); + } - var changeValue = networkManager.SpawnManager.SpawnedObjects[m_InstanceAuthority.NetworkObjectId].GetComponent(); if (changeValue.SomeIntValue.Value != m_InstanceAuthority.SomeIntValue.Value) { errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] {changeValue.name} value is {changeValue.SomeIntValue.Value} but was expecting {m_InstanceAuthority.SomeIntValue.Value}!"); @@ -185,19 +191,15 @@ public IEnumerator OwnershipSpawnedAndUpdatedDuringSpawn() { var authority = GetAuthorityNetworkManager(); var nonAuthority = GetNonAuthorityNetworkManager(); - m_SessionAuthorityInstance = Object.Instantiate(m_SpawnObject).GetComponent(); - - SpawnInstanceWithOwnership(m_SessionAuthorityInstance.GetComponent(), authority, nonAuthority.LocalClientId); - yield return WaitForSpawnedOnAllOrTimeOut(m_SessionAuthorityInstance.NetworkObjectId); - AssertOnTimeout($"Failed to spawn {m_SessionAuthorityInstance.name} on all clients!"); + // If running in distributed authority mode, we use the nonauthority (i.e. not SessionOwner) instance to spawn. + var spawnAuthority = m_DistributedAuthority ? nonAuthority : authority; + m_InstanceAuthority = SpawnObject(m_SpawnObject, spawnAuthority).GetComponent(); - m_InstanceAuthority = nonAuthority.SpawnManager.SpawnedObjects[m_SessionAuthorityInstance.NetworkObjectId].GetComponent(); + yield return WaitForSpawnedOnAllOrTimeOut(m_InstanceAuthority.NetworkObjectId); + AssertOnTimeout($"Failed to spawn {m_InstanceAuthority.name} on all clients!"); yield return WaitForConditionOrTimeOut(NetworkVariablesMatch); AssertOnTimeout($"The {nameof(ChangeValueOnAuthority.SomeIntValue)} failed to synchronize on all clients!"); - - Assert.IsTrue(m_SessionAuthorityInstance.SomeIntValue.Value == 2, "No values were updated on the spawn authority instance!"); - Assert.IsTrue(m_InstanceAuthority.SomeIntValue.Value == 2, "No values were updated on the owner's instance!"); } } }