refactor: Remove SIPTransport and replace it with UnityTransport #1870

NoelStephensUnity · 2022-04-12T19:06:59Z

This removes the SIPTransport from our integration tests and replaces it with UnityTransport.
This includes several integration test refactors, and some minor fixes to bugs that were not detected under SIPTransport.

MTT-3016

Changelog

Changed: NetcodeIntegrationTestHelpers to use UnityTransport.
Removed SIPTransport from integration tests.

Testing and Documentation

Includes adjustments to integration tests.

This fixes the timing issues with the MessageOrdering.SpawnRpcDespawn integration test. This also includes a minor update to NetcodeIntegrationTestHelpers (destroy as opposed to destroy immediate).

This fixes the issue with MultiClientConnectionApproval.ClientConnectedCallbackTest failing when using UTP as opposed to SIP.

Fixed issue with ConnectionApprovalHandler's prefab getting destroyed when a client was disconnected. Applied same fix to the NetcodeIntegrationTest when creating a prefab. Also fixing issue with NetworkManager spewing spam messages if the NetworkConfig.ConnectionApproval is set but there is no ConnectionApprovalCallback assigned. Adjusted it to make sure it is either the Server or Host that this applies to, but not a client.

This fixes the ConnectionApprovalMismatchTest issue that caused it to fail with UTP. Switching over to client notifications (i.e. the server detects the mismatch and disconnects the clients, we want to make sure the clients received the disconnection notification).

Global fix for teardown and NetworkObject destroying....make sure the server is the NetworkManagerOwner before destroying it.

Some NetcodeIntegrationTest adjustments to handle the switch over to UTP.

Fixes issues with NetworkObjectParentingTests (increased wait time between changes to 1 full Network Tick). Also the recent NetcodeIntegrationTest updates helped fix some of the issues with NetworkObjectParentingTests.

Fixes the issue with NetworkSpawnManagerTests.TestConnectAndDisconnect.

Switching to UnityTransport exposed a potential bug where the server disconnects a client while the client is still processing a SceneEventType.Synchronize message which by the time the client processes this it has already sent the SceneEventType.SynchronizeComplete message back to the server that *could* attempt to send a resynchronization message where the MessageSystem will crash because the m_SendQueues entry for the now disconnected client no longer exists.

Added some additional UnityTransport settings for payload size, send queue size, and reducing the connection attempts and time out to connect per attempt. Added the ability to specify whether you want the server or clients to be instantiated first (see comments under NetcodeIntegrationTest.m_CreateServerFirst for further explanation).

Removing UNet and just using UTP. Reducing the timeout period so it doesn't take over 60 seconds to timeout.

Fixes timing issues with this test that is causing it to not passing when using UTP.

Fixed timing issues exposed when using UTP.

This fixes an issue when I originally switched it to be the server as the default NetworkManager to be instantiated when using NetcodeIntegrationTestHelper.Create. This sets the, recently updated NetcodeIntegrationTest m_CreateServerFirst, clients to instantiate before the server (needed for in-scene objects)

This test requires the clients to be instantiated before the server.

MTT-3016 Changelog entries.

com.unity.netcode.gameobjects/CHANGELOG.md

0xFA11 · 2022-04-12T19:14:56Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

+                        // Check to see if the client needs to resynchronize and before sending the message make sure the client is still connected to avoid
+                        // a potential crash within the MessageSystem (i.e. sending to a client that no longer exists)
+                        if (sceneEventData.ClientNeedsReSynchronization() && !DisableReSynchronization && m_NetworkManager.ConnectedClients.ContainsKey(clientId))


@ShadauxCat ?

0xFA11 · 2022-04-12T19:15:58Z

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

+        /// <summary>
+        /// Set this to false to create the clients first.
+        /// Note: If you are using scene placed NetworkObjects or doing any form of scene testing and
+        /// get prefab hash id "soft synchronization" errors, then set this to false and run your test
+        /// again.  This is a work-around until we can resolve some issues with NetworkManagerOwner and
+        /// NetworkManager.Singleton.
+        /// </summary>


do you mind elaborating a little more? what's going on here?

Its a little complicated, but it has to do with our integration tests running in the same thread, how we populate newly loaded (not yet "spawned") in-scene placed NetworkObjects, and NetworkObject.NetworkManager referencing NetworkManager.Singleton if NetworkObject.NetworkManagerOwner is null.

Same Thread: In several places we check to make sure the NetworkObject.NetworkManager is the "right one" for the NetworkManager context (i.e. NetworkObject.NetworkManager == NetworkManager)

Populating In-Scene Objects: This checks to make sure the NetworkSceneManager.m_NetworkManager is the same as the NetworkObject.NetworkManager before adding it to the ScenePlacedObjects list.

Default NetworkManager: This happens to be NetworkManager.Singleton that is set upon the first NetworkManager being instantiated.

So, under some conditions you might want NetworkManager.Singleton to be the server and under other conditions you might want it to be a client. If you are loading a scene with in-scene placed NetworkObjects, then you will want the NetworkManager.Singleton to be the first client instantiated in order to properly synchronize/find the in-scene placed NetworkObjects (which are loaded by the server and not loaded by the client since they share the same scene space).

If you want more details feel free to DM me in slack... really we need to handle in-scene placed NetworkObjects better... like that "pre-rfc" document outlines.

0xFA11 · 2022-04-12T19:17:29Z

testproject/Assets/Tests/Runtime/MessageOrdering.cs

+            var allHandlersDestroyed = false;
+            var waitForTick = new WaitForSeconds(1.0f / m_ServerNetworkManager.NetworkConfig.TickRate);
+
+            while (!(allHandlersSpawned && clientCountReached && allHandlersDestroyed))


I'd add @ShadauxCat as a reviewer to review changes around these tests.

(just in case)

This resolves the issues with tools integration tests and switching over to UnityTransport.

com.unity.netcode.gameobjects/CHANGELOG.md

0xFA11

lgtm 🚀

Co-authored-by: Fatih Mar <mfatihmar@gmail.com>

The following fixes some edge-case timing related issues that are being exposed in Yamato where it might take longer than 1 network tick depending upon the virtual machine's current load. Primarily, the majority of the fixes are handled by just waiting for the condition or timing out.

the time out was never yielding and causing issues on PS4/

Rosme

I'm fine with the changes in the tests integration stuff as well as whatever may touch the tools.

Removing some code that didn't need to make it into this PR.

0xFA11 and others added 22 commits April 4, 2022 16:46

delete SIPTransport & SIPTransportTests

744ea2b

fix compile errors

be93240

fix

0e27cb9

This fixes the timing issues with the MessageOrdering.SpawnRpcDespawn integration test. This also includes a minor update to NetcodeIntegrationTestHelpers (destroy as opposed to destroy immediate).

fix

87d9615

This fixes the issue with MultiClientConnectionApproval.ClientConnectedCallbackTest failing when using UTP as opposed to SIP.

fix

72ee0b3

This fixes the ConnectionApprovalMismatchTest issue that caused it to fail with UTP. Switching over to client notifications (i.e. the server detects the mismatch and disconnects the clients, we want to make sure the clients received the disconnection notification).

fix

8981be6

Global fix for teardown and NetworkObject destroying....make sure the server is the NetworkManagerOwner before destroying it.

Merge branch 'develop' into refactor/kill-sip-use-utp

c59eaf6

fix

388d3b0

Some NetcodeIntegrationTest adjustments to handle the switch over to UTP.

fix

7609c35

Fixes issues with NetworkObjectParentingTests (increased wait time between changes to 1 full Network Tick). Also the recent NetcodeIntegrationTest updates helped fix some of the issues with NetworkObjectParentingTests.

fix

5cd33f3

Fixes the issue with NetworkSpawnManagerTests.TestConnectAndDisconnect.

Fix - ClientOnlyConnectionTests

87f8154

Removing UNet and just using UTP. Reducing the timeout period so it doesn't take over 60 seconds to timeout.

Fix - DisconnectTests

cd8c589

Fixes timing issues with this test that is causing it to not passing when using UTP.

fix - NetworkObjectOwnershipTests

3d3421f

Fixed timing issues exposed when using UTP.

fix - NetworkSceneManagerSeneVerification

74e75b2

This test requires the clients to be instantiated before the server.

style

ea87ed3

style

0325622

update MTT-3016

2503736

MTT-3016 Changelog entries.

Update CHANGELOG.md

8f2ac41

NoelStephensUnity requested a review from 0xFA11 April 12, 2022 19:06

NoelStephensUnity requested a review from a team as a code owner April 12, 2022 19:07

Update CHANGELOG.md

e0f7fa7

0xFA11 reviewed Apr 12, 2022

View reviewed changes

com.unity.netcode.gameobjects/CHANGELOG.md Outdated Show resolved Hide resolved

0xFA11 reviewed Apr 12, 2022

View reviewed changes

NoelStephensUnity requested a review from ShadauxCat April 12, 2022 19:18

NoelStephensUnity marked this pull request as draft April 12, 2022 19:41

simon-lemay-unity approved these changes Apr 12, 2022

View reviewed changes

fix - tools

4a23b77

This resolves the issues with tools integration tests and switching over to UnityTransport.

NoelStephensUnity marked this pull request as ready for review April 12, 2022 20:00

NoelStephensUnity requested a review from a team as a code owner April 12, 2022 20:00

NoelStephensUnity requested a review from becksebenius-unity April 12, 2022 20:02

0xFA11 reviewed Apr 12, 2022

View reviewed changes

com.unity.netcode.gameobjects/CHANGELOG.md Outdated Show resolved Hide resolved

0xFA11 reviewed Apr 12, 2022

View reviewed changes

com.unity.netcode.gameobjects/CHANGELOG.md Outdated Show resolved Hide resolved

0xFA11 approved these changes Apr 12, 2022

View reviewed changes

NoelStephensUnity and others added 4 commits April 12, 2022 15:15

Update com.unity.netcode.gameobjects/CHANGELOG.md

2d816c1

Co-authored-by: Fatih Mar <mfatihmar@gmail.com>

Update com.unity.netcode.gameobjects/CHANGELOG.md

9c80c35

Co-authored-by: Fatih Mar <mfatihmar@gmail.com>

fix - console - ps4

dcca79c

the time out was never yielding and causing issues on PS4/

Rosme approved these changes Apr 13, 2022

View reviewed changes

NoelStephensUnity and others added 2 commits April 13, 2022 09:33

Merge branch 'develop' into refactor/kill-sip-use-utp

d850635

revert

1549c2f

Removing some code that didn't need to make it into this PR.

NoelStephensUnity merged commit 6555a1e into develop Apr 14, 2022

NoelStephensUnity deleted the refactor/kill-sip-use-utp branch April 14, 2022 02:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Remove SIPTransport and replace it with UnityTransport #1870

refactor: Remove SIPTransport and replace it with UnityTransport #1870

Uh oh!

NoelStephensUnity commented Apr 12, 2022

Uh oh!

Uh oh!

0xFA11 Apr 12, 2022

Uh oh!

0xFA11 Apr 12, 2022

Uh oh!

NoelStephensUnity Apr 12, 2022 •

edited

Loading

Uh oh!

0xFA11 Apr 12, 2022

Uh oh!

0xFA11 Apr 12, 2022

Uh oh!

Uh oh!

Uh oh!

0xFA11 left a comment

Uh oh!

Rosme left a comment

Uh oh!

Uh oh!

refactor: Remove SIPTransport and replace it with UnityTransport #1870

refactor: Remove SIPTransport and replace it with UnityTransport #1870

Uh oh!

Conversation

NoelStephensUnity commented Apr 12, 2022

Changelog

Testing and Documentation

Uh oh!

Uh oh!

0xFA11 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

0xFA11 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xFA11 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

0xFA11 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

0xFA11 left a comment

Choose a reason for hiding this comment

Uh oh!

Rosme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NoelStephensUnity Apr 12, 2022 •

edited

Loading