fix: NetworkBehaviour Property Initialization Updates (#1785)

* fix

Updating to more stable and performant NetworkBehaviour properties through changing the how and when the properties are set.

* test fix

Really MessageOrdering and the SpawnRpcDespawn classes need to be reviewed and updated.  This fixes a few timing related issues that were exposed with this update.

* fix

Removing the update to HasNetworkObject

* update

really only ever need to wait for almost 1 tic interval to make sure the GameObject and associated components are destroyed and the handler destroyed.

* style

clarity in a comment

* update

removing the NetworkManager.IsListening since that is part of how the property values are determined (host, client, server).

* update

handling an unlikely condition where the ownership or property update is invoked and NetworkObject is null.

* update

Includes all of Fatih's suggested adjustments
Also, went ahead and just moved the invocation of OnNetworkSpawn and OnNetworkDespawn into the internal methods.

* update

Coming to alternate solution of IsLocalPlayer to reduce the processing cost of the other implementation.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -235,42 +235,42 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
         public NetworkManager NetworkManager => NetworkObject.NetworkManager;
 
         /// <summary>
-        /// Gets if the object is the the personal clients player object
+        /// If a NetworkObject is assigned, it will return whether or not this NetworkObject
+        /// is the local player object.  If no NetworkObject is assigned it will always return false.
         /// </summary>
-        public bool IsLocalPlayer => NetworkObject.IsLocalPlayer;
+        public bool IsLocalPlayer { get; private set; }
 
         /// <summary>
         /// Gets if the object is owned by the local player or if the object is the local player object
         /// </summary>
-        public bool IsOwner => NetworkObject.IsOwner;
+        public bool IsOwner { get; internal set; }
 
         /// <summary>
         /// Gets if we are executing as server
         /// </summary>
-        protected bool IsServer => IsRunning && NetworkManager.IsServer;
+        protected bool IsServer { get; private set; }
 
         /// <summary>
         /// Gets if we are executing as client
         /// </summary>
-        protected bool IsClient => IsRunning && NetworkManager.IsClient;
+        protected bool IsClient { get; private set; }
+
 
         /// <summary>
         /// Gets if we are executing as Host, I.E Server and Client
         /// </summary>
-        protected bool IsHost => IsRunning && NetworkManager.IsHost;
-
-        private bool IsRunning => NetworkManager && NetworkManager.IsListening;
+        protected bool IsHost { get; private set; }
 
         /// <summary>
         /// Gets Whether or not the object has a owner
         /// </summary>
-        public bool IsOwnedByServer => NetworkObject.IsOwnedByServer;
+        public bool IsOwnedByServer { get; internal set; }
 
         /// <summary>
         /// Used to determine if it is safe to access NetworkObject and NetworkManager from within a NetworkBehaviour component
         /// Primarily useful when checking NetworkObject/NetworkManager properties within FixedUpate
         /// </summary>
-        public bool IsSpawned => HasNetworkObject ? NetworkObject.IsSpawned : false;
+        public bool IsSpawned { get; internal set; }
 
         internal bool IsBehaviourEditable()
         {
@@ -327,12 +327,12 @@ public NetworkObject NetworkObject
         /// <summary>
         /// Gets the NetworkId of the NetworkObject that owns this NetworkBehaviour
         /// </summary>
-        public ulong NetworkObjectId => NetworkObject.NetworkObjectId;
+        public ulong NetworkObjectId { get; internal set; }
 
         /// <summary>
         /// Gets NetworkId for this NetworkBehaviour from the owner NetworkObject
         /// </summary>
-        public ushort NetworkBehaviourId => NetworkObject.GetNetworkBehaviourOrderIndex(this);
+        public ushort NetworkBehaviourId { get; internal set; }
 
         /// <summary>
         /// Internally caches the Id of this behaviour in a NetworkObject. Makes look-up faster
@@ -352,7 +352,47 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId)
         /// <summary>
         /// Gets the ClientId that owns the NetworkObject
         /// </summary>
-        public ulong OwnerClientId => NetworkObject.OwnerClientId;
+        public ulong OwnerClientId { get; internal set; }
+
+        /// <summary>
+        /// Updates properties with network session related
+        /// dependencies such as a NetworkObject's spawned
+        /// state or NetworkManager's session state.
+        /// </summary>
+        internal void UpdateNetworkProperties()
+        {
+            // Set NetworkObject dependent properties
+            if (NetworkObject != null)
+            {
+                // Set identification related properties
+                NetworkObjectId = NetworkObject.NetworkObjectId;
+                IsLocalPlayer = NetworkObject.IsLocalPlayer;
+
+                // This is "OK" because GetNetworkBehaviourOrderIndex uses the order of
+                // NetworkObject.ChildNetworkBehaviours which is set once when first
+                // accessed.
+                NetworkBehaviourId = NetworkObject.GetNetworkBehaviourOrderIndex(this);
+
+                // Set ownership related properties
+                IsOwnedByServer = NetworkObject.IsOwnedByServer;
+                IsOwner = NetworkObject.IsOwner;
+                OwnerClientId = NetworkObject.OwnerClientId;
+
+                // Set NetworkManager dependent properties
+                if (NetworkManager != null)
+                {
+                    IsHost = NetworkManager.IsListening && NetworkManager.IsHost;
+                    IsClient = NetworkManager.IsListening && NetworkManager.IsClient;
+                    IsServer = NetworkManager.IsListening && NetworkManager.IsServer;
+                }
+            }
+            else // Shouldn't happen, but if so then set the properties to their default value;
+            {
+                OwnerClientId = NetworkObjectId = default;
+                IsOwnedByServer = IsOwner = IsHost = IsClient = IsServer = default;
+                NetworkBehaviourId = default;
+            }
+        }
 
         /// <summary>
         /// Gets called when the <see cref="NetworkObject"/> gets spawned, message handlers are ready to be registered and the network is setup.
@@ -366,21 +406,41 @@ public virtual void OnNetworkDespawn() { }
 
         internal void InternalOnNetworkSpawn()
         {
+            IsSpawned = true;
             InitializeVariables();
+            UpdateNetworkProperties();
+            OnNetworkSpawn();
         }
 
-        internal void InternalOnNetworkDespawn() { }
+        internal void InternalOnNetworkDespawn()
+        {
+            IsSpawned = false;
+            UpdateNetworkProperties();
+            OnNetworkDespawn();
+        }
 
         /// <summary>
         /// Gets called when the local client gains ownership of this object
         /// </summary>
         public virtual void OnGainedOwnership() { }
 
+        internal void InternalOnGainedOwnership()
+        {
+            UpdateNetworkProperties();
+            OnGainedOwnership();
+        }
+
         /// <summary>
         /// Gets called when we loose ownership of this object
         /// </summary>
         public virtual void OnLostOwnership() { }
 
+        internal void InternalOnLostOwnership()
+        {
+            UpdateNetworkProperties();
+            OnLostOwnership();
+        }
+
         /// <summary>
         /// Gets called when the parent NetworkObject of this NetworkBehaviour's NetworkObject has changed
         /// </summary>

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -549,15 +549,15 @@ internal void InvokeBehaviourOnLostOwnership()
         {
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
-                ChildNetworkBehaviours[i].OnLostOwnership();
+                ChildNetworkBehaviours[i].InternalOnLostOwnership();
             }
         }
 
         internal void InvokeBehaviourOnGainedOwnership()
         {
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
-                ChildNetworkBehaviours[i].OnGainedOwnership();
+                ChildNetworkBehaviours[i].InternalOnGainedOwnership();
             }
         }
 
@@ -796,7 +796,6 @@ internal void InvokeBehaviourNetworkSpawn()
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 ChildNetworkBehaviours[i].InternalOnNetworkSpawn();
-                ChildNetworkBehaviours[i].OnNetworkSpawn();
             }
         }
 
@@ -805,7 +804,6 @@ internal void InvokeBehaviourNetworkDespawn()
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 ChildNetworkBehaviours[i].InternalOnNetworkDespawn();
-                ChildNetworkBehaviours[i].OnNetworkDespawn();
             }
         }
 

testproject/Assets/Tests/Runtime/MessageOrdering.cs

@@ -146,12 +146,14 @@ public IEnumerator SpawnRpcDespawn()
             var serverObject = Object.Instantiate(m_Prefab, Vector3.zero, Quaternion.identity);
             NetworkObject serverNetworkObject = serverObject.GetComponent<NetworkObject>();
             serverNetworkObject.NetworkManagerOwner = server;
+            serverNetworkObject.Spawn();
+
             SpawnRpcDespawn srdComponent = serverObject.GetComponent<SpawnRpcDespawn>();
             srdComponent.Activate();
 
             // Wait until all objects have spawned.
             int expectedCount = Support.SpawnRpcDespawn.ClientUpdateCount + 1;
-            const int maxFrames = 240;
+            int maxFrames = 240 + Time.frameCount;
             var doubleCheckTime = Time.realtimeSinceStartup + 5.0f;
             while (Support.SpawnRpcDespawn.ClientUpdateCount < expectedCount && !handler.WasSpawned)
             {
@@ -171,8 +173,10 @@ public IEnumerator SpawnRpcDespawn()
 
             Assert.AreEqual(NetworkUpdateStage.EarlyUpdate, Support.SpawnRpcDespawn.StageExecutedByReceiver);
             Assert.AreEqual(Support.SpawnRpcDespawn.ServerUpdateCount, Support.SpawnRpcDespawn.ClientUpdateCount);
-            var lastFrameNumber = Time.frameCount + 1;
-            yield return new WaitUntil(() => Time.frameCount >= lastFrameNumber);
+
+            // Wait 1 tic for the GameObjet and associated components to be destroyed
+            yield return new WaitForSeconds(1.0f / server.NetworkConfig.TickRate);
+
             Assert.True(handler.WasDestroyed);
         }
 

testproject/Assets/Tests/Runtime/Support/SpawnRpcDespawn.cs

@@ -86,7 +86,6 @@ public override void OnDestroy()
         private void RunTest()
         {
             Debug.Log("Running test...");
-            GetComponent<NetworkObject>().Spawn();
             IncrementUpdateCount();
             Destroy(gameObject);
             m_Active = false;