fix: soft synchronization issues can be avoided if object creation and destruction messages are held until player has completely loaded scene by NoelStephensUnity · Pull Request #531 · Unity-Technologies/com.unity.netcode.gameobjects

NoelStephensUnity · 2021-03-04T18:23:00Z

This is a fix for soft synchronization issues when a client is transitioning to a new or existing in game session.
What this fix does:

All internal MLAPI create object or destroy object commands are held in the queue for a specified client while the client is loading a scene. Upon the client notifying the server (or host) that it has finished loading the scene, all create or destroy commands held in the queue are sent to the client.
Any queue items remaining in the internal mlapi queue (within the RPC queue framework) for longer than 60 seconds will be dropped (assuming a client disconnected or dropped during the scene loading process)
Network Manager automated spawning now sends a create object command via the queue as opposed to being added to the existing "scene state" binary stream sent to the player when a scene transition occurs (or when they are newly joining an existing game). This helps to prevent issues with objects being created on the client side while it is loading.
Removed the PostProcessing hack-fix to return if the application was running as this is no longer needed.

Other than some additional comments and clean up, that is the summary of this PR.

This fixes two issues: It prevents the postprocessing method ProcessScene from being run during scene transitions while in PlayMode within the editor. This fixes the issue where the transport is no longer being polled for events upon shutting down the server,host, or client.

This prevents this post processing code from being executed when in PlayMode (editor and stand alone build)

This ended up not being needed in the end. Reverting back to the original file.

Removing the commented out OnDisable function (the containing code was moved into the Shutdown method) and removed the Awake method (the containing code was moved into the Init method).

This hadn't fully registered the merge.

Just getting the footprint ready and made some adjustments to the ground and wall materials.

WIP files for the scene transition samples.

Refactoring some comments

0xFA11

I think we should have 2 separate PRs for MLAPI changes and testproject changes.
There are so many things going under this PR and make it much harder to review.

mattwalsh-unity · 2021-03-05T21:47:55Z

        public static void ProcessScene()
        {
-            //If we are in playmode (editor or stand alone) we do not want this to execute
-            if (Application.isPlaying)


I wonder why the code before skipped out in this case. Any recollections @TwoTenPvP ?

This has been added as a fix by Noel but with the new better fix isn't necessary anymore I think.

mattwalsh-unity · 2021-03-05T21:50:07Z

    /// </summary>
    public class NetworkClient
    {
+        public bool IsClientDoneLoadingScene;


Please comment public's

+1 to Matt's point.
I'd also suggest naming it as IsLoadingScene too (and reverse the flag being false by default).

Fixing a merge artifact...

mattwalsh-unity · 2021-03-05T21:55:06Z

+                    if(createPlayerObject)
+                    {
+                        //Provide meaningful feedback to developers that this happened.
+                        NetworkLog.LogWarning("NetworkManager configured to spawn player prefab but no prefab is defined or there is a problem with the prefab hash.");


This is interesting. Is this different behavior than now, where if you don't have your default player spawn it crashes? Now you get a warning?

mattwalsh-unity · 2021-03-05T22:00:31Z

-
+                //Reset our observed objects list
+                //[NSS]: This assures we have an up to date list of all observed objects
                m_ObservedObjects.Clear();


I wonder why m_ObservedObjects is not a local to HandleApproval? It's only used in HA, and only in the approved state, and in that state, it is unconditionally cleared, then built up and read from.

Due to name changes, the default player prefab was removed from the NetworkManager.

0xFA11 · 2021-03-06T19:37:38Z

I also wonder how this relates to #493
Looks like we're unfixing a fix as a fix that went in very quickly in the first place :)

0xFA11 · 2021-03-07T04:50:00Z

-            //If we are in playmode (editor or stand alone) we do not want this to execute
-            if (Application.isPlaying)
-            {
-                return;
-            }


can this change go as an "unfix" for #493 on itself in isolation?

0xFA11 · 2021-03-07T04:54:14Z

+                if (PendingClients.ContainsKey(clientId))
+                    PendingClients.Remove(clientId);
+                NetworkClient client = new NetworkClient()
+                {
+                    IsClientDoneLoadingScene = false,
+                    ClientId = clientId,
+                };


Suggested change

if (PendingClients.ContainsKey(clientId))

PendingClients.Remove(clientId);

NetworkClient client = new NetworkClient()

{

IsClientDoneLoadingScene = false,

ClientId = clientId,

};

if (PendingClients.ContainsKey(clientId))

{

PendingClients.Remove(clientId);

}

var client = new NetworkClient

{

IsLoadingScene = true,

ClientId = clientId,

};

0xFA11 · 2021-03-07T04:55:53Z

+                }

+                //Create the new player from the assigned prefab
+                NetworkObject netObject = NetworkSpawnManager.CreateLocalNetworkObject(false, 0, (playerPrefabHash == null ? NetworkConfig.PlayerPrefabHash.Value : playerPrefabHash.Value), null, position, rotation);


Suggested change

NetworkObject netObject = NetworkSpawnManager.CreateLocalNetworkObject(false, 0, (playerPrefabHash == null ? NetworkConfig.PlayerPrefabHash.Value : playerPrefabHash.Value), null, position, rotation);

var networkObject = NetworkSpawnManager.CreateLocalNetworkObject(false, 0, (playerPrefabHash == null ? NetworkConfig.PlayerPrefabHash.Value : playerPrefabHash.Value), null, position, rotation);

0xFA11 · 2021-03-07T04:57:05Z

-        /// GetStreamBufferFrameCount
-        /// Returns how many frames have been processed (Inbound/Outbound)
-        /// </summary>
-        /// <param name="queueType"></param>
-        /// <returns>number of frames procssed</returns>
-        public uint GetStreamBufferFrameCount(RpcQueueHistoryFrame.QueueFrameType queueType)
-        {
-            return queueType == RpcQueueHistoryFrame.QueueFrameType.Inbound ? m_InboundFramesProcessed : m_OutboundFramesProcessed;
-        }
-
-        /// <summary>
-        /// AddToInternalMLAPISendQueue
-        /// NSS-TODO: This will need to be removed once we determine how we want to handle specific
-        /// internal MLAPI commands relative to RPCS.
-        /// Example: An network object is destroyed via server side (internal mlapi) command, but prior to this several RPCs are invoked for the to be destroyed object (Client RPC)
-        /// If both the DestroyObject internal mlapi command and the ClientRPCs are received in the same frame but the internal mlapi DestroyObject command is processed prior to the
-        /// RPCs being invoked then the object won't exist and additional warnings will be logged that the object no longer exists.
-        /// The vices versa scenario (create and then RPCs sent) is an unlikely/improbable scenario, but just in case added the CreateObject to this special case scenario.
-        ///
        /// To avoid the DestroyObject scenario, the internal MLAPI commands (DestroyObject and CreateObject) are always invoked after RPCs.
        /// </summary>
        /// <param name="queueItem">item to add to the internal MLAPI queue</param>
-        public void AddToInternalMLAPISendQueue(RpcFrameQueueItem queueItem)
+        internal void AddToInternalMLAPISendQueue(RpcFrameQueueItem queueItem)


changes in this file are looking more like refactorings, should probably go into another isolated PR (and maybe not even targeting release branch IMHO)

0xFA11 · 2021-03-07T04:58:38Z

+            //In case there is no Singleton, then just exit
+            if(!NetworkManager.Singleton)
+            {
+                return;
+            }


why? if there is no NetworkManager.Singleton, we are probably in trouble already, right?

0xFA11 · 2021-03-07T04:59:12Z

+                return;
+            }
+
+            bool AdvanceFrameHistory = false;


I believe local variables should be "camelCase" as it used to be...

Suggested change

bool AdvanceFrameHistory = false;

bool advanceFrameHistory = false;

0xFA11 · 2021-03-07T05:00:09Z

-        public void ProcessSendQueue()
+        internal void ProcessSendQueue()


why all these public to internal changes? the class itself is already internal, there is no extra protection coming from all these public to internal changes anyway?

0xFA11 · 2021-03-07T05:00:44Z

            {
-                var PoolStream = queueItem.NetworkBuffer;
-                if (NetworkManager.Singleton.IsListening)
+                List<RpcFrameQueueItem> CompletedRpcQueueItems = new List<RpcFrameQueueItem>();


Suggested change

List<RpcFrameQueueItem> CompletedRpcQueueItems = new List<RpcFrameQueueItem>();

var completedRpcQueueItems = new List<RpcFrameQueueItem>();

0xFA11 · 2021-03-07T05:01:00Z

                {
-                    switch (queueItem.QueueItemType)
+                    var clientIds = new List<ulong>(queueItem.ClientNetworkIds);
+                    var PoolStream = queueItem.NetworkBuffer;


Suggested change

var PoolStream = queueItem.NetworkBuffer;

var poolStream = queueItem.NetworkBuffer;

0xFA11 · 2021-03-07T05:02:31Z

+        internal void InternalMessagesSendAndFlush()
        {
-            foreach (RpcFrameQueueItem queueItem in m_InternalMLAPISendQueue)
+            if(NetworkManager.Singleton && NetworkManager.Singleton.IsListening)


again, similar to above, I'm not sure if this check is really needed. maybe IsListening but not sure if Singleton check is required here. also, why we're checking if we're IsListening here? shouldn't the call-site of InternalMessagesSendAndFlush method do that check first before calling this method in the first place? I have questions... :P

0xFA11 · 2021-03-07T05:03:14Z

-            var advanceFrameHistory = false;
+            //In case there is no Singleton, then just exit
+            if(!NetworkManager.Singleton)
+            {
+                return;
+            }
+
+            var AdvanceFrameHistory = false;


the same pattern followed here, see my comments above ^^^

0xFA11 · 2021-03-07T05:04:09Z

+                        //Right before the server sends the switch scene message, it marks the client it is sending the message to as having not loaded the scene being switched to
+                        NetworkManager.Singleton.ConnectedClientsList[j].IsClientDoneLoadingScene = false;


huh? why? what's going on here? :D

0xFA11 · 2021-03-07T05:06:19Z

-m_EditorVersion: 2019.4.17f1
-m_EditorVersionWithRevision: 2019.4.17f1 (667c8606c536)
+m_EditorVersion: 2019.4.20f1
+m_EditorVersionWithRevision: 2019.4.20f1 (6dd1c08eedfa)


this change is unnecessary!

0xFA11 · 2021-03-07T05:06:53Z

-    "dependencies": {
-      "com.unity.collab-proxy": "1.2.16",
-      "com.unity.ide.rider": "1.1.4",
-      "com.unity.ide.vscode": "1.2.3",
-      "com.unity.multiplayer.mlapi": "file:../../com.unity.multiplayer.mlapi",
-      "com.unity.package-validation-suite": "0.19.2-preview",
-      "com.unity.test-framework": "1.1.19",
-      "com.unity.textmeshpro": "2.1.1",
-      "com.unity.timeline": "1.2.17",
-      "com.unity.ugui": "1.0.0",
-      "com.unity.modules.ai": "1.0.0",
-      "com.unity.modules.androidjni": "1.0.0",
-      "com.unity.modules.animation": "1.0.0",
-      "com.unity.modules.assetbundle": "1.0.0",
-      "com.unity.modules.audio": "1.0.0",
-      "com.unity.modules.cloth": "1.0.0",
-      "com.unity.modules.director": "1.0.0",
-      "com.unity.modules.imageconversion": "1.0.0",
-      "com.unity.modules.imgui": "1.0.0",
-      "com.unity.modules.jsonserialize": "1.0.0",
-      "com.unity.modules.particlesystem": "1.0.0",
-      "com.unity.modules.physics": "1.0.0",
-      "com.unity.modules.physics2d": "1.0.0",
-      "com.unity.modules.screencapture": "1.0.0",
-      "com.unity.modules.terrain": "1.0.0",
-      "com.unity.modules.terrainphysics": "1.0.0",
-      "com.unity.modules.tilemap": "1.0.0",
-      "com.unity.modules.ui": "1.0.0",
-      "com.unity.modules.uielements": "1.0.0",
-      "com.unity.modules.umbra": "1.0.0",
-      "com.unity.modules.unityanalytics": "1.0.0",
-      "com.unity.modules.unitywebrequest": "1.0.0",
-      "com.unity.modules.unitywebrequestassetbundle": "1.0.0",
-      "com.unity.modules.unitywebrequestaudio": "1.0.0",
-      "com.unity.modules.unitywebrequesttexture": "1.0.0",
-      "com.unity.modules.unitywebrequestwww": "1.0.0",
-      "com.unity.modules.vehicles": "1.0.0",
-      "com.unity.modules.video": "1.0.0",
-      "com.unity.modules.vr": "1.0.0",
-      "com.unity.modules.wind": "1.0.0",
-      "com.unity.modules.xr": "1.0.0"
-    },
-    "testables": [
-      "com.unity.multiplayer.mlapi"
-    ]
-  }
-
+  "dependencies": {
+    "com.unity.collab-proxy": "1.2.16",
+    "com.unity.ide.rider": "1.1.4",
+    "com.unity.ide.vscode": "1.2.3",
+    "com.unity.multiplayer.mlapi": "file:../../com.unity.multiplayer.mlapi",
+    "com.unity.package-validation-suite": "0.19.2-preview",
+    "com.unity.test-framework": "1.1.22",
+    "com.unity.textmeshpro": "2.1.1",
+    "com.unity.timeline": "1.2.17",
+    "com.unity.ugui": "1.0.0",
+    "com.unity.modules.ai": "1.0.0",
+    "com.unity.modules.androidjni": "1.0.0",
+    "com.unity.modules.animation": "1.0.0",
+    "com.unity.modules.assetbundle": "1.0.0",
+    "com.unity.modules.audio": "1.0.0",
+    "com.unity.modules.cloth": "1.0.0",
+    "com.unity.modules.director": "1.0.0",
+    "com.unity.modules.imageconversion": "1.0.0",
+    "com.unity.modules.imgui": "1.0.0",
+    "com.unity.modules.jsonserialize": "1.0.0",
+    "com.unity.modules.particlesystem": "1.0.0",
+    "com.unity.modules.physics": "1.0.0",
+    "com.unity.modules.physics2d": "1.0.0",
+    "com.unity.modules.screencapture": "1.0.0",
+    "com.unity.modules.terrain": "1.0.0",
+    "com.unity.modules.terrainphysics": "1.0.0",
+    "com.unity.modules.tilemap": "1.0.0",
+    "com.unity.modules.ui": "1.0.0",
+    "com.unity.modules.uielements": "1.0.0",
+    "com.unity.modules.umbra": "1.0.0",
+    "com.unity.modules.unityanalytics": "1.0.0",
+    "com.unity.modules.unitywebrequest": "1.0.0",
+    "com.unity.modules.unitywebrequestassetbundle": "1.0.0",
+    "com.unity.modules.unitywebrequestaudio": "1.0.0",
+    "com.unity.modules.unitywebrequesttexture": "1.0.0",
+    "com.unity.modules.unitywebrequestwww": "1.0.0",
+    "com.unity.modules.vehicles": "1.0.0",
+    "com.unity.modules.video": "1.0.0",
+    "com.unity.modules.vr": "1.0.0",
+    "com.unity.modules.wind": "1.0.0",
+    "com.unity.modules.xr": "1.0.0"
+  },
+  "testables": [
+    "com.unity.multiplayer.mlapi"
+  ]
+}


this is probably due to the engine version upgrade, please leave this change out...

0xFA11 · 2021-03-07T05:07:47Z

+        public String GetSceneNameLinkedToState(GlobalGameState.GameStates gameState, int currentindex = 0)
+        {
+            var results = m_StateToSceneList.Where(entry => entry.stateToLoadScene == gameState);
+            if (results != null && ( (results.Count() - 1) >= currentindex) )
+            {
+                return results.ElementAt(currentindex).SceneToLoad;
+            }
+            return String.Empty;
+        }


Suggested change

public String GetSceneNameLinkedToState(GlobalGameState.GameStates gameState, int currentindex = 0)

{

var results = m_StateToSceneList.Where(entry => entry.stateToLoadScene == gameState);

if (results != null && ( (results.Count() - 1) >= currentindex) )

{

return results.ElementAt(currentindex).SceneToLoad;

}

return String.Empty;

}

public string GetSceneNameLinkedToState(GlobalGameState.GameStates gameState, int currentindex = 0)

{

var results = m_StateToSceneList.Where(entry => entry.stateToLoadScene == gameState);

if (results != null && ((results.Count() - 1) >= currentindex))

{

return results.ElementAt(currentindex).SceneToLoad;

}

return string.Empty;

}

NoelStephensUnity · 2021-03-08T00:15:58Z

Closing this fix because it needs to be refactored to only fix the soft synch issue when:

a host-server is already in a game session
the host-server is sending the MLAPI layer ADD_OBJECT command frequently enough to send when a newly joining client is still loading the scene.
the fix should only include the code base that will keep those commands in the queue for the client still loading a scene
upon the client signaling to the server that it is done loading the scene, the server will flush the pending commands to the client
The scene transitioning and global game state sample should not be part of this PR
The scene transitioning and global game state sample needs to be refactored to not use scene management or automated player spawning due to the more recent issues discovered.

0xFA11 and others added 30 commits February 5, 2021 04:20

implement NetworkUpdateLoop

f40a7d3

merge branch 'develop' into 'experimental/network-update-loop'

20ae513

merge branch 'develop' into 'experimental/network-update-loop'

c8f0a77

merge branch 'develop' into 'experimental/network-update-loop'

4568136

remove AdvanceFrame() and FrameCount

6da2c42

update player loop injection points and INetworkUpdateSystem API

7fe5c3b

add xmldoc and notes

379102b

add NetworkUpdateLoopTests

2c08e60

implement NetworkUpdateLoop plain and mixed tests

c891ddf

update xmldoc

558599f

small meta fix

7837599

replace old network update loop with new RFC-backed network update loop

db058d7

replace WaitForEndOfFrame with WaitUntil in tests

d4d6b3f

comment 'default' for NetworkUpdateStage.Update enum value

f82542c

optimization & refactoring pass

b50b31a

Merge branch 'develop' into feature/network-update-loop

104de45

dictionary lookups instead of index access to arrays

74e6a12

optimize array allocation

92d9732

Merge branch 'develop' into feature/network-update-loop

69a2c61

Merge branch 'develop' into feature/network-update-loop

d924684

fix: more robust fix for postprocessing bug

bd43e2d

This prevents this post processing code from being executed when in PlayMode (editor and stand alone build)

Merge branch 'develop' into experimental/scenemanagement

4738437

refactor: SceneSwitchProgress

146c062

This ended up not being needed in the end. Reverting back to the original file.

refactor: removing commented code and merge artifacts

b421f25

Removing the commented out OnDisable function (the containing code was moved into the Shutdown method) and removed the Awake method (the containing code was moved into the Init method).

Merge branch 'develop' into fix/mtt512

415cbe3

refactor: merge artifacts

f0e1248

This hadn't fully registered the merge.

First pass adjustments

3436287

Just getting the footprint ready and made some adjustments to the ground and wall materials.

test: initial setup for the scene transistion sample

cdc9144

WIP files for the scene transition samples.

syntax: comments

1d01968

Refactoring some comments

LukeStampfli approved these changes Mar 5, 2021

View reviewed changes

0xFA11 suggested changes Mar 5, 2021

View reviewed changes

Merge remote-tracking branch 'origin/release/0.1.0' into fix/softsynch

8406cec

mattwalsh-unity reviewed Mar 5, 2021

View reviewed changes

fix: Merging Artifact

c49f80c

Fixing a merge artifact...

mattwalsh-unity reviewed Mar 5, 2021

View reviewed changes

NoelStephensUnity added 2 commits March 5, 2021 16:13

fix: more merge artifacts

d4094ba

Due to name changes, the default player prefab was removed from the NetworkManager.

Merge remote-tracking branch 'origin/release/0.1.0' into fix/softsynch

a32d5f8

LukeStampfli mentioned this pull request Mar 6, 2021

ServerRPC request method not found #540

Closed

0xFA11 reviewed Mar 7, 2021

View reviewed changes

NoelStephensUnity closed this Mar 8, 2021

NoelStephensUnity deleted the fix/softsynch branch April 16, 2021 15:06

	NetworkObject netObject = NetworkSpawnManager.CreateLocalNetworkObject(false, 0, (playerPrefabHash == null ? NetworkConfig.PlayerPrefabHash.Value : playerPrefabHash.Value), null, position, rotation);
	var networkObject = NetworkSpawnManager.CreateLocalNetworkObject(false, 0, (playerPrefabHash == null ? NetworkConfig.PlayerPrefabHash.Value : playerPrefabHash.Value), null, position, rotation);

	bool AdvanceFrameHistory = false;
	bool advanceFrameHistory = false;

		public void ProcessSendQueue()
		internal void ProcessSendQueue()

	List<RpcFrameQueueItem> CompletedRpcQueueItems = new List<RpcFrameQueueItem>();
	var completedRpcQueueItems = new List<RpcFrameQueueItem>();

	var PoolStream = queueItem.NetworkBuffer;
	var poolStream = queueItem.NetworkBuffer;

		//Right before the server sends the switch scene message, it marks the client it is sending the message to as having not loaded the scene being switched to
		NetworkManager.Singleton.ConnectedClientsList[j].IsClientDoneLoadingScene = false;

Conversation

NoelStephensUnity commented Mar 4, 2021

Uh oh!

0xFA11 left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xFA11 commented Mar 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity commented Mar 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0xFA11 left a comment •

edited

Loading