From 6ce5b11c70ba4f7803b3dc3feeae6b6518337822 Mon Sep 17 00:00:00 2001 From: PitouGames Date: Mon, 1 May 2023 15:42:35 +0200 Subject: [PATCH 1/3] fix: missing value on NetworkListEvent for EventType.RemoveAt events server side (#2542) --- com.unity.netcode.gameobjects/CHANGELOG.md | 1 + .../Runtime/NetworkVariable/Collections/NetworkList.cs | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 80769a1d74..dcdd035415 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -13,6 +13,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed - Fixed the "Generate Default Network Prefabs List" setting not loading correctly and always reverting to being checked. (#2545) +- Fixed missing value on `NetworkListEvent` for `EventType.RemoveAt` events server side. (#2542) - Fixed the inspector throwing exceptions when attempting to render `NetworkVariable`s of enum types. (#2529) - Making a `NetworkVariable` with an `INetworkSerializable` type that doesn't meet the `new()` constraint will now create a compile-time error instead of an editor crash (#2528) - Fixed Multiplayer Tools package installation docs page link on the NetworkManager popup. (#2526) diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs index 3d03d44134..aee004f803 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs @@ -490,12 +490,14 @@ public void RemoveAt(int index) throw new InvalidOperationException("Client is not allowed to write to this NetworkList"); } + var value = m_List[index]; m_List.RemoveAt(index); var listEvent = new NetworkListEvent() { Type = NetworkListEvent.EventType.RemoveAt, - Index = index + Index = index, + Value = value }; HandleAddListEvent(listEvent); From 1de9713590eede3b374bd9e38a590e1dca265ccb Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Fri, 12 May 2023 10:53:05 -0500 Subject: [PATCH 2/3] Update CHANGELOG.md --- com.unity.netcode.gameobjects/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index dcdd035415..b7051e41a8 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -13,7 +13,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed - Fixed the "Generate Default Network Prefabs List" setting not loading correctly and always reverting to being checked. (#2545) -- Fixed missing value on `NetworkListEvent` for `EventType.RemoveAt` events server side. (#2542) +- Fixed missing value on `NetworkListEvent` for `EventType.RemoveAt` events. (#2542,#2543) - Fixed the inspector throwing exceptions when attempting to render `NetworkVariable`s of enum types. (#2529) - Making a `NetworkVariable` with an `INetworkSerializable` type that doesn't meet the `new()` constraint will now create a compile-time error instead of an editor crash (#2528) - Fixed Multiplayer Tools package installation docs page link on the NetworkManager popup. (#2526) From ca4bc47dd269dd840193534b41e7482cebf8af02 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Fri, 12 May 2023 11:42:05 -0500 Subject: [PATCH 3/3] test Updated the NetworkList remove tests to be combined into one test and to validate that the list changed event is returning the value of the element removed. Cleaned up the tests a bit and removed some legacy tests that were being ignored and no longer serve a purpose. --- .../Tests/Runtime/NetworkVariableTests.cs | 155 +++++++++--------- 1 file changed, 75 insertions(+), 80 deletions(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs index 4faaa73c84..616ac26420 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs @@ -557,7 +557,7 @@ protected override bool CanStartServerAndClients() /// This is an adjustment to how the server and clients are started in order /// to avoid timing issues when running in a stand alone test runner build. /// - private void InitializeServerAndClients(bool useHost) + private void InitializeServerAndClients(HostOrServer useHost) { s_ClientNetworkVariableTestInstances.Clear(); m_PlayerPrefab.AddComponent(); @@ -574,7 +574,7 @@ private void InitializeServerAndClients(bool useHost) client.NetworkConfig.PlayerPrefab = m_PlayerPrefab; } - Assert.True(NetcodeIntegrationTestHelpers.Start(useHost, m_ServerNetworkManager, m_ClientNetworkManagers), "Failed to start server and client instances"); + Assert.True(NetcodeIntegrationTestHelpers.Start(useHost == HostOrServer.Host, m_ServerNetworkManager, m_ClientNetworkManagers), "Failed to start server and client instances"); RegisterSceneManagerHandler(); @@ -611,7 +611,7 @@ private void InitializeServerAndClients(bool useHost) throw new Exception("at least one client network container not empty at start"); } - var instanceCount = useHost ? NumberOfClients * 3 : NumberOfClients * 2; + var instanceCount = useHost == HostOrServer.Host ? NumberOfClients * 3 : NumberOfClients * 2; // Wait for the client-side to notify it is finished initializing and spawning. success = WaitForConditionOrTimeOutWithTimeTravel(() => s_ClientNetworkVariableTestInstances.Count == instanceCount); @@ -624,15 +624,15 @@ private void InitializeServerAndClients(bool useHost) /// Runs generalized tests on all predefined NetworkVariable types /// [Test] - public void AllNetworkVariableTypes([Values(true, false)] bool useHost) + public void AllNetworkVariableTypes([Values] HostOrServer useHost) { // Create, instantiate, and host // This would normally go in Setup, but since every other test but this one // uses NetworkManagerHelper, and it does its own NetworkManager setup / teardown, // for now we put this within this one test until we migrate it to MIH - Assert.IsTrue(NetworkManagerHelper.StartNetworkManager(out NetworkManager server, useHost ? NetworkManagerHelper.NetworkManagerOperatingMode.Host : NetworkManagerHelper.NetworkManagerOperatingMode.Server)); + Assert.IsTrue(NetworkManagerHelper.StartNetworkManager(out NetworkManager server, useHost == HostOrServer.Host ? NetworkManagerHelper.NetworkManagerOperatingMode.Host : NetworkManagerHelper.NetworkManagerOperatingMode.Server)); - Assert.IsTrue(server.IsHost == useHost, $"{nameof(useHost)} does not match the server.IsHost value!"); + Assert.IsTrue(server.IsHost == (useHost == HostOrServer.Host), $"{nameof(useHost)} does not match the server.IsHost value!"); Guid gameObjectId = NetworkManagerHelper.AddGameNetworkObject("NetworkVariableTestComponent"); @@ -662,7 +662,7 @@ public void AllNetworkVariableTypes([Values(true, false)] bool useHost) } [Test] - public void ClientWritePermissionTest([Values(true, false)] bool useHost) + public void ClientWritePermissionTest([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -674,7 +674,7 @@ public void ClientWritePermissionTest([Values(true, false)] bool useHost) /// Runs tests that network variables sync on client whatever the local value of . /// [Test] - public void NetworkVariableSync_WithDifferentTimeScale([Values(true, false)] bool useHost, [Values(0.0f, 1.0f, 2.0f)] float timeScale) + public void NetworkVariableSync_WithDifferentTimeScale([Values] HostOrServer useHost, [Values(0.0f, 1.0f, 2.0f)] float timeScale) { Time.timeScale = timeScale; @@ -688,7 +688,7 @@ public void NetworkVariableSync_WithDifferentTimeScale([Values(true, false)] boo } [Test] - public void FixedString32Test([Values(true, false)] bool useHost) + public void FixedString32Test([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); m_Player1OnServer.FixedString32.Value = k_FixedStringTestValue; @@ -699,7 +699,7 @@ public void FixedString32Test([Values(true, false)] bool useHost) } [Test] - public void NetworkListAdd([Values(true, false)] bool useHost) + public void NetworkListAdd([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); m_NetworkListPredicateHandler = new NetworkListTestPredicate(m_Player1OnServer, m_Player1OnClient1, NetworkListTestPredicate.NetworkListTestStates.Add, 10); @@ -707,7 +707,7 @@ public void NetworkListAdd([Values(true, false)] bool useHost) } [Test] - public void WhenListContainsManyLargeValues_OverflowExceptionIsNotThrown([Values(true, false)] bool useHost) + public void WhenListContainsManyLargeValues_OverflowExceptionIsNotThrown([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); m_NetworkListPredicateHandler = new NetworkListTestPredicate(m_Player1OnServer, m_Player1OnClient1, NetworkListTestPredicate.NetworkListTestStates.ContainsLarge, 20); @@ -715,7 +715,7 @@ public void WhenListContainsManyLargeValues_OverflowExceptionIsNotThrown([Values } [Test] - public void NetworkListContains([Values(true, false)] bool useHost) + public void NetworkListContains([Values] HostOrServer useHost) { // Re-use the NetworkListAdd to initialize the server and client as well as make sure the list is populated NetworkListAdd(useHost); @@ -725,23 +725,9 @@ public void NetworkListContains([Values(true, false)] bool useHost) Assert.True(WaitForConditionOrTimeOutWithTimeTravel(m_NetworkListPredicateHandler)); } - [Test] - public void NetworkListRemove([Values(true, false)] bool useHost) - { - // Re-use the NetworkListAdd to initialize the server and client as well as make sure the list is populated - NetworkListAdd(useHost); - - // Remove two entries by index - m_Player1OnServer.TheList.Remove(3); - m_Player1OnServer.TheList.Remove(5); - - // Really just verifies the data at this point - m_NetworkListPredicateHandler.SetNetworkListTestState(NetworkListTestPredicate.NetworkListTestStates.VerifyData); - Assert.True(WaitForConditionOrTimeOutWithTimeTravel(m_NetworkListPredicateHandler)); - } [Test] - public void NetworkListInsert([Values(true, false)] bool useHost) + public void NetworkListInsert([Values] HostOrServer useHost) { // Re-use the NetworkListAdd to initialize the server and client as well as make sure the list is populated NetworkListAdd(useHost); @@ -755,7 +741,7 @@ public void NetworkListInsert([Values(true, false)] bool useHost) } [Test] - public void NetworkListIndexOf([Values(true, false)] bool useHost) + public void NetworkListIndexOf([Values] HostOrServer useHost) { // Re-use the NetworkListAdd to initialize the server and client as well as make sure the list is populated NetworkListAdd(useHost); @@ -765,7 +751,7 @@ public void NetworkListIndexOf([Values(true, false)] bool useHost) } [Test] - public void NetworkListValueUpdate([Values(true, false)] bool useHost) + public void NetworkListValueUpdate([Values] HostOrServer useHost) { var testSucceeded = false; InitializeServerAndClients(useHost); @@ -796,24 +782,68 @@ void TestValueUpdatedCallback(NetworkListEvent changedEvent) m_Player1OnClient1.TheList.OnListChanged -= TestValueUpdatedCallback; } + private List m_ExpectedValuesServer = new List(); + private List m_ExpectedValuesClient = new List(); + + public enum ListRemoveTypes + { + Remove, + RemoveAt + } + + [Test] - public void NetworkListRemoveAt([Values(true, false)] bool useHost) + public void NetworkListRemoveTests([Values] HostOrServer useHost, [Values] ListRemoveTypes listRemoveType) { + m_ExpectedValuesServer.Clear(); + m_ExpectedValuesClient.Clear(); // Re-use the NetworkListAdd to initialize the server and client as well as make sure the list is populated NetworkListAdd(useHost); // Randomly remove a few entries - m_Player1OnServer.TheList.RemoveAt(Random.Range(0, m_Player1OnServer.TheList.Count - 1)); - m_Player1OnServer.TheList.RemoveAt(Random.Range(0, m_Player1OnServer.TheList.Count - 1)); - m_Player1OnServer.TheList.RemoveAt(Random.Range(0, m_Player1OnServer.TheList.Count - 1)); + m_Player1OnServer.TheList.OnListChanged += Server_OnListChanged; + m_Player1OnClient1.TheList.OnListChanged += Client_OnListChanged; + + // Remove half of the elements + for (int i = 0; i < (int)(m_Player1OnServer.TheList.Count * 0.5f); i++) + { + var index = Random.Range(0, m_Player1OnServer.TheList.Count - 1); + var value = m_Player1OnServer.TheList[index]; + m_ExpectedValuesServer.Add(value); + m_ExpectedValuesClient.Add(value); + + if (listRemoveType == ListRemoveTypes.RemoveAt) + { + m_Player1OnServer.TheList.RemoveAt(index); + } + else + { + m_Player1OnServer.TheList.Remove(value); + } + } // Verify the element count and values on the client matches the server m_NetworkListPredicateHandler.SetNetworkListTestState(NetworkListTestPredicate.NetworkListTestStates.VerifyData); Assert.True(WaitForConditionOrTimeOutWithTimeTravel(m_NetworkListPredicateHandler)); + + Assert.True(m_ExpectedValuesServer.Count == 0, $"Server was not notified of all elements removed and still has {m_ExpectedValuesServer.Count} elements left!"); + Assert.True(m_ExpectedValuesClient.Count == 0, $"Client was not notified of all elements removed and still has {m_ExpectedValuesClient.Count} elements left!"); + } + + private void Server_OnListChanged(NetworkListEvent changeEvent) + { + Assert.True(m_ExpectedValuesServer.Contains(changeEvent.Value)); + m_ExpectedValuesServer.Remove(changeEvent.Value); + } + + private void Client_OnListChanged(NetworkListEvent changeEvent) + { + Assert.True(m_ExpectedValuesClient.Contains(changeEvent.Value)); + m_ExpectedValuesClient.Remove(changeEvent.Value); } [Test] - public void NetworkListClear([Values(true, false)] bool useHost) + public void NetworkListClear([Values] HostOrServer useHost) { // Re-use the NetworkListAdd to initialize the server and client as well as make sure the list is populated NetworkListAdd(useHost); @@ -824,7 +854,7 @@ public void NetworkListClear([Values(true, false)] bool useHost) } [Test] - public void TestNetworkVariableClass([Values(true, false)] bool useHost) + public void TestNetworkVariableClass([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -843,7 +873,7 @@ bool VerifyClass() } [Test] - public void TestNetworkVariableTemplateClass([Values(true, false)] bool useHost) + public void TestNetworkVariableTemplateClass([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -861,7 +891,7 @@ bool VerifyClass() } [Test] - public void TestNetworkListStruct([Values(true, false)] bool useHost) + public void TestNetworkListStruct([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -881,7 +911,7 @@ bool VerifyList() } [Test] - public void TestNetworkVariableStruct([Values(true, false)] bool useHost) + public void TestNetworkVariableStruct([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -899,7 +929,7 @@ bool VerifyStructure() } [Test] - public void TestNetworkVariableTemplateStruct([Values(true, false)] bool useHost) + public void TestNetworkVariableTemplateStruct([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -917,7 +947,7 @@ bool VerifyStructure() } [Test] - public void TestNetworkVariableTemplateBehaviourClass([Values(true, false)] bool useHost) + public void TestNetworkVariableTemplateBehaviourClass([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -939,7 +969,7 @@ bool VerifyClass() } [Test] - public void TestNetworkVariableTemplateBehaviourClassNotReferencedElsewhere([Values(true, false)] bool useHost) + public void TestNetworkVariableTemplateBehaviourClassNotReferencedElsewhere([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -957,7 +987,7 @@ bool VerifyClass() } [Test] - public void TestNetworkVariableTemplateBehaviourStruct([Values(true, false)] bool useHost) + public void TestNetworkVariableTemplateBehaviourStruct([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -975,7 +1005,7 @@ bool VerifyClass() } [Test] - public void TestNetworkVariableEnum([Values(true, false)] bool useHost) + public void TestNetworkVariableEnum([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); @@ -992,7 +1022,7 @@ bool VerifyStructure() } [Test] - public void TestINetworkSerializableClassCallsNetworkSerialize([Values(true, false)] bool useHost) + public void TestINetworkSerializableClassCallsNetworkSerialize([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); TestClass.NetworkSerializeCalledOnWrite = false; @@ -1010,7 +1040,7 @@ public void TestINetworkSerializableClassCallsNetworkSerialize([Values(true, fal } [Test] - public void TestINetworkSerializableStructCallsNetworkSerialize([Values(true, false)] bool useHost) + public void TestINetworkSerializableStructCallsNetworkSerialize([Values] HostOrServer useHost) { InitializeServerAndClients(useHost); TestStruct.NetworkSerializeCalledOnWrite = false; @@ -1023,41 +1053,6 @@ public void TestINetworkSerializableStructCallsNetworkSerialize([Values(true, fa Assert.True(WaitForConditionOrTimeOutWithTimeTravel(VerifyCallback)); } - [Test] - [Ignore("This is used several times already in the NetworkListPredicate")] - // TODO: If we end up using the new suggested pattern, then delete this - public void NetworkListArrayOperator([Values(true, false)] bool useHost) - { - NetworkListAdd(useHost); - } - - [Test] - [Ignore("This is used several times already in the NetworkListPredicate")] - // TODO: If we end up using the new suggested pattern, then delete this - public void NetworkListIEnumerator([Values(true, false)] bool useHost) - { - InitializeServerAndClients(useHost); - var correctVals = new int[3]; - correctVals[0] = k_TestVal1; - correctVals[1] = k_TestVal2; - correctVals[2] = k_TestVal3; - - m_Player1OnServer.TheList.Add(correctVals[0]); - m_Player1OnServer.TheList.Add(correctVals[1]); - m_Player1OnServer.TheList.Add(correctVals[2]); - - Assert.IsTrue(m_Player1OnServer.TheList.Count == 3); - - int index = 0; - foreach (var val in m_Player1OnServer.TheList) - { - if (val != correctVals[index++]) - { - Assert.Fail(); - } - } - } - [Test] public void TestUnsupportedManagedTypesThrowExceptions() {