diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 30247ba839..dd1c9347e2 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -24,8 +24,10 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed issue when using a client-server topology where a `NetworkList` with owner write permissions was resetting sent time and dirty flags after having been spawned on owning clients that were not the spawn authority. (#3850) - Fixed an integer overflow that occurred when configuring a large disconnect timeout with Unity Transport. (#3810) + ### Security diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index cbcd40ed4d..45d4b3311e 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -76,6 +76,16 @@ public uint PrefabIdHash /// public List NetworkTransforms { get; private set; } + /// + /// Set to true if this instance is the original/first instance created and spawned which + /// means the was generated from this instance and sent + /// to all other clients. + /// + /// Client-Server: This will be true for all instances on the server or host. + /// Distributed Authority: This will be true on the client that created the first instance + /// and spawned it (even if spawning with ownership being assigned to a different client). + /// + internal bool IsSpawnAuthority; #if COM_UNITY_MODULES_PHYSICS || COM_UNITY_MODULES_PHYSICS2D /// @@ -2008,6 +2018,7 @@ internal void ResetOnDespawn() { // Always clear out the observers list when despawned Observers.Clear(); + IsSpawnAuthority = false; IsSpawned = false; DeferredDespawnTick = 0; m_LatestParent = null; diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs index 42bc1b22d0..3b2117bf11 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs @@ -60,11 +60,18 @@ public NetworkList(IEnumerable values = default, internal override void OnSpawned() { - // If we are dirty and have write permissions by the time the NetworkObject - // is finished spawning (same frame), then go ahead and reset the dirty related - // properties for NetworkList in the event user script has made changes when - // spawning to prevent duplicate entries. - if (IsDirty() && CanSend()) + // 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(); diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index c157c68cea..3efd90779d 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs @@ -1054,6 +1054,9 @@ internal void AuthorityLocalSpawn([NotNull] NetworkObject networkObject, ulong n Debug.LogError("Spawning NetworkObjects with nested NetworkObjects is only supported for scene objects. Child NetworkObjects will not be spawned over the network!"); } } + + networkObject.IsSpawnAuthority = true; + // Invoke NetworkBehaviour.OnPreSpawn methods networkObject.NetworkManagerOwner = NetworkManager; networkObject.InvokeBehaviourNetworkPreSpawn(); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkListTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkListTests.cs index 15c737fbc3..5f3be93f57 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkListTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkListTests.cs @@ -27,6 +27,12 @@ public NetworkListTests(HostOrServer host) : base(host) { } private ulong m_TestObjectId; + protected override IEnumerator OnSetup() + { + IsOwnerWriteTest = false; + return base.OnSetup(); + } + protected override void OnServerAndClientsCreated() { m_ListObjectPrefab = CreateNetworkObjectPrefab("ListObject"); @@ -285,6 +291,161 @@ private int[] Shuffle(List list) // This will do a shuffle of the list return list.OrderBy(_ => rng.Next()).ToArray(); } + + private List m_SpawnedObjects = new List(); + internal const int ValueCount = 10; + internal static bool IsOwnerWriteTest; + internal NetworkManager LateJoinedClient; + internal static List OwnerWriteExpectedValues = new List(); + + protected override void OnNewClientCreated(NetworkManager networkManager) + { + if (IsOwnerWriteTest) + { + LateJoinedClient = networkManager; + } + else + { + LateJoinedClient = null; + } + base.OnNewClientCreated(networkManager); + } + + [UnityTest] + public IEnumerator OwnerWriteTests() + { + IsOwnerWriteTest = true; + var authorityNetworkManager = GetAuthorityNetworkManager(); + m_SpawnedObjects.Clear(); + OwnerWriteExpectedValues.Clear(); + // Set our initial expected values as 0 - 9 + for (int i = 0; i < ValueCount; i++) + { + OwnerWriteExpectedValues.Add(i); + } + + // Each spawned instance will be owned by each NetworkManager instance in order + // to validate owner write NetworkLists. + foreach (var networkManager in m_NetworkManagers) + { + m_SpawnedObjects.Add(SpawnObject(m_ListObjectPrefab, networkManager).GetComponent()); + } + + // Verify all NetworkManager instances spawned the objects + yield return WaitForSpawnedOnAllOrTimeOut(m_SpawnedObjects); + AssertOnTimeout("Not all instances were spawned on all clients!"); + + // Verify all spawned object instances have the expected owner write NetworkList values + yield return WaitForConditionOrTimeOut(OnVerifyOwnerWriteData); + AssertOnTimeout("Detected invalid count or value on one of the spawned instances!"); + + // Late join a client + yield return CreateAndStartNewClient(); + + // Spawn an instance with the new client being the owner + m_SpawnedObjects.Add(SpawnObject(m_ListObjectPrefab, LateJoinedClient).GetComponent()); + + // Verify all NetworkManager instances spawned the objects + yield return WaitForSpawnedOnAllOrTimeOut(m_SpawnedObjects); + AssertOnTimeout("Not all instances were spawned on all clients!"); + + // Verify all spawned object instances have the expected owner write NetworkList values + yield return WaitForConditionOrTimeOut(OnVerifyOwnerWriteData); + AssertOnTimeout("Detected invalid count or value on one of the spawned instances!"); + + // Now have all of the clients update their list values to randomly assigned values + // in order to verify changes to owner write NetworkLists are synchronized properly. + OwnerWriteExpectedValues.Clear(); + for (int i = 0; i < ValueCount; i++) + { + OwnerWriteExpectedValues.Add(Random.Range(10, 100)); + } + UpdateOwnerWriteValues(); + + // Verify all spawned object instances have the expected owner write NetworkList values + yield return WaitForConditionOrTimeOut(OnVerifyOwnerWriteData); + AssertOnTimeout("Detected invalid count or value on one of the spawned instances!"); + + // Verifies that spawning with ownership in distributed authority mode work properly. + // Where: + // Client-A spawns with ownership assigned to Client-B + // Client-B applies values at spawn. + // All clients then should be updated with those new values applied. + if (m_DistributedAuthority) + { + var prefabNetworkObject = m_ListObjectPrefab.GetComponent(); + foreach (var networkManager in m_NetworkManagers) + { + var instance = Object.Instantiate(m_ListObjectPrefab).GetComponent(); + SpawnInstanceWithOwnership(instance, authorityNetworkManager, networkManager.LocalClientId); + m_SpawnedObjects.Add(instance); + } + + // Verify all NetworkManager instances spawned the objects + yield return WaitForSpawnedOnAllOrTimeOut(m_SpawnedObjects); + AssertOnTimeout("Not all instances were spawned on all clients!"); + + // Verify all spawned object instances have the expected owner write NetworkList values + yield return WaitForConditionOrTimeOut(OnVerifyOwnerWriteData); + AssertOnTimeout("Detected invalid count or value on one of the spawned instances!"); + } + } + + private void UpdateOwnerWriteValues() + { + foreach (var spawnedObject in m_SpawnedObjects) + { + var owningNetworkManager = m_NetworkManagers.Where((c) => c.LocalClientId == spawnedObject.OwnerClientId).First(); + var networkObjectId = spawnedObject.NetworkObjectId; + var listComponent = owningNetworkManager.SpawnManager.SpawnedObjects[networkObjectId].GetComponent(); + for (int i = 0; i < ValueCount; i++) + { + listComponent.OwnerWriteList[i] = OwnerWriteExpectedValues[i]; + } + } + } + + private bool OnVerifyOwnerWriteData(StringBuilder errorLog) + { + foreach (var spawnedObject in m_SpawnedObjects) + { + var networkObjectId = spawnedObject.NetworkObjectId; + foreach (var networkManager in m_NetworkManagers) + { + if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObjectId)) + { + errorLog.Append($"[Client-{networkManager.LocalClientId}] Does not have an instance of spawned object NetworkObjectId: {networkObjectId}"); + return false; + } + var listComponent = networkManager.SpawnManager.SpawnedObjects[networkObjectId].GetComponent(); + + if (listComponent == null) + { + errorLog.Append($"[Client-{networkManager.LocalClientId}] List component was not found"); + return false; + } + + if (listComponent.OwnerWriteList.Count != ValueCount) + { + errorLog.Append($"[Client-{networkManager.LocalClientId}] List component has the incorrect number of items. Expected: {ValueCount}, Have: {listComponent.TheList.Count}"); + return false; + } + + for (int i = 0; i < ValueCount; i++) + { + var actual = listComponent.OwnerWriteList[i]; + var expected = OwnerWriteExpectedValues[i]; + if (expected != actual) + { + errorLog.Append($"[Client-{networkManager.LocalClientId}] Incorrect value at index {i}, expected: {expected}, actual: {actual}"); + return false; + } + } + } + } + + return true; + } } internal class NetworkListTest : NetworkBehaviour @@ -292,6 +453,7 @@ internal class NetworkListTest : NetworkBehaviour public readonly NetworkList TheList = new(); public readonly NetworkList TheStructList = new(); public readonly NetworkList TheLargeList = new(); + public readonly NetworkList OwnerWriteList = new NetworkList(default, NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner); private void ListChanged(NetworkListEvent e) { @@ -309,6 +471,18 @@ public override void OnDestroy() base.OnDestroy(); } + public override void OnNetworkSpawn() + { + if (NetworkListTests.IsOwnerWriteTest && IsOwner) + { + for (int i = 0; i < NetworkListTests.ValueCount; i++) + { + OwnerWriteList.Add(NetworkListTests.OwnerWriteExpectedValues[i]); + } + } + base.OnNetworkSpawn(); + } + public bool ListDelegateTriggered; } @@ -325,8 +499,6 @@ internal class NetworkListTestPredicate : ConditionalPredicateBase private readonly NetworkListTest m_NonAuthorityInstance; - private string m_TestStageFailedMessage; - /// /// Determines if the condition has been reached for the current NetworkListTestState ///