fix: NetworkBehaviour Property Initialization Updates #1785

NoelStephensUnity · 2022-03-08T21:13:02Z

Accessing specific properties of NetworkBehaviour can too easily cause exceptions to occur. Depending upon frequency of usage, they can also impact performance unnecessarily.

MTT-2785

Changelog

com.unity.netcode.gameobjects

Fixed: If you update multiple packages, create a new section with a new header for the other package.

Testing and Documentation

One test was updated.
The rest validate the change

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

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.

Removing the update to HasNetworkObject

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.

clarity in a comment

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

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

…itialization

mattwalsh-unity · 2022-03-09T02:25:20Z

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

@@ -237,40 +237,41 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
        /// <summary>
        /// Gets if the object is the the personal clients player object
        /// </summary>
-        public bool IsLocalPlayer => NetworkObject.IsLocalPlayer;
+        public bool IsLocalPlayer => m_NetworkObject != null ? m_NetworkObject.IsLocalPlayer : false;


Is this actually an improvement though? If the NO is null, I think IsLocalPlayer should be undefined / access should crash as we actually don't really know. If there's code later that wants to tolerate not having the NO set up yet, fine, then let that code do this check. Also, since we call this frequently...

I went ahead and made this a private set and just initialize it during the spawning/despawning stages.
It removes the processing overhead and is cleaner.

0xFA11 · 2022-03-09T19:58:56Z

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

-        public ulong OwnerClientId => NetworkObject.OwnerClientId;
+        public ulong OwnerClientId { get; internal set; }
+
+        internal void UpdatePropertyStates()


Suggested change

internal void UpdatePropertyStates()

internal void UpdateNetworkProperties()

0xFA11 · 2022-03-09T19:59:31Z

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

+        internal void UpdateOwnership()
+        {
+            if (NetworkObject != null)
+            {
+                IsOwnedByServer = NetworkObject.IsOwnedByServer;
+                IsOwner = NetworkObject.IsOwner;
+                OwnerClientId = NetworkObject.OwnerClientId;
+            }
+            else // Shouldn't happen, but if it does then just clear out everything
+            {
+                IsOwnedByServer = IsOwner = false;
+                OwnerClientId = 0;
+            }
+        }


move this logic into UpdateNetworkProperties() and maintain updating all net props in one place. easier to maintain.

0xFA11 · 2022-03-09T20:00:22Z

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


        /// <summary>
        /// Gets called when the local client gains ownership of this object
        /// </summary>
        public virtual void OnGainedOwnership() { }

+        internal void GainedOwnership()


Suggested change

internal void GainedOwnership()

internal void InternalOnGainedOwnership()

0xFA11 · 2022-03-09T20:00:32Z

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

        /// <summary>
        /// Gets called when we loose ownership of this object
        /// </summary>
        public virtual void OnLostOwnership() { }

+        internal void LostOwnership()


Suggested change

internal void LostOwnership()

internal void InternalOnLostOwnership()

0xFA11 · 2022-03-09T20:03:11Z

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

+        private bool m_IsServer;
        /// <summary>
        /// Gets if we are executing as server
        /// </summary>
-        protected bool IsServer => IsRunning && NetworkManager.IsServer;
+        protected bool IsServer { get { return m_IsServer; } }


I'd suggest a simpler syntax like:

protected bool IsServer { get; private set; }

and for other properties too :)

and you don't need to have another field to back it up.

0xFA11 · 2022-03-09T20:04:20Z

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

@@ -366,21 +392,56 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId)

        internal void InternalOnNetworkSpawn()


I like the fact that now we are relying on OnNetworkSpawn and OnNetworkDespawn now <3

…itialization

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

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

…itialization

mattwalsh-unity · 2022-03-09T21:56:57Z

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

+            }
+            else // Shouldn't happen, but if so then set the properties to their default value;
+            {
+                OwnerClientId = NetworkObjectId = default;


Is this an error condition we should log? Would be a way to spot mis-use / regressions

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

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

…itialization

NoelStephensUnity added 7 commits March 8, 2022 14:58

fix

30387b6

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

test fix

74c071f

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

5babbe4

Removing the update to HasNetworkObject

update

069feac

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

e1f0a30

clarity in a comment

update

8a582fd

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

update

ea8e30c

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

NoelStephensUnity requested a review from 0xFA11 March 8, 2022 21:46

Merge branch 'develop' into fix/mtt-2785-networkbehaviour-property-in…

fd8ece8

…itialization

mattwalsh-unity reviewed Mar 9, 2022

View reviewed changes

0xFA11 reviewed Mar 9, 2022

View reviewed changes

0xFA11 changed the title ~~fix: mtt-2785 NetworkBehaviour Property Initialization Updates~~ fix: NetworkBehaviour Property Initialization Updates Mar 9, 2022

NoelStephensUnity added 4 commits March 9, 2022 14:20

Merge branch 'develop' into fix/mtt-2785-networkbehaviour-property-in…

3bf6c48

…itialization

update

f7d2fb1

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

update

8bf9aa0

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

Merge branch 'develop' into fix/mtt-2785-networkbehaviour-property-in…

fde6871

…itialization

NoelStephensUnity marked this pull request as ready for review March 9, 2022 21:51

NoelStephensUnity requested review from 0xFA11 and mattwalsh-unity March 9, 2022 21:51

mattwalsh-unity reviewed Mar 9, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs Show resolved Hide resolved

mattwalsh-unity reviewed Mar 9, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs Show resolved Hide resolved

0xFA11 approved these changes Mar 9, 2022

View reviewed changes

Merge branch 'develop' into fix/mtt-2785-networkbehaviour-property-in…

83904a5

…itialization

NoelStephensUnity merged commit c7dc103 into develop Mar 10, 2022

NoelStephensUnity deleted the fix/mtt-2785-networkbehaviour-property-initialization branch March 10, 2022 21:45

NoelStephensUnity mentioned this pull request Mar 28, 2022

fix: client writing to netvar error Unity-Technologies/com.unity.multiplayer.samples.coop#468

Merged

4 tasks

This was referenced Jul 26, 2022

fix: add exception for IsOwner/IsLocalPlayer at inappropriate times #2079

Closed

Owner checks during Awake/Start #982

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: NetworkBehaviour Property Initialization Updates #1785

fix: NetworkBehaviour Property Initialization Updates #1785

Uh oh!

NoelStephensUnity commented Mar 8, 2022

Uh oh!

mattwalsh-unity Mar 9, 2022

Uh oh!

NoelStephensUnity Mar 9, 2022

Uh oh!

0xFA11 Mar 9, 2022

Uh oh!

0xFA11 Mar 9, 2022

Uh oh!

0xFA11 Mar 9, 2022

Uh oh!

0xFA11 Mar 9, 2022

Uh oh!

0xFA11 Mar 9, 2022

Uh oh!

0xFA11 Mar 9, 2022

Uh oh!

0xFA11 Mar 9, 2022

Uh oh!

mattwalsh-unity Mar 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	internal void UpdatePropertyStates()
	internal void UpdateNetworkProperties()

	internal void GainedOwnership()
	internal void InternalOnGainedOwnership()

	internal void LostOwnership()
	internal void InternalOnLostOwnership()

		@@ -366,21 +392,56 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId)

		internal void InternalOnNetworkSpawn()

fix: NetworkBehaviour Property Initialization Updates #1785

fix: NetworkBehaviour Property Initialization Updates #1785

Uh oh!

Conversation

NoelStephensUnity commented Mar 8, 2022

Changelog

com.unity.netcode.gameobjects

Testing and Documentation

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!

Uh oh!

Uh oh!

Uh oh!