fix: networkvariable collections can be modified without write permissions [backport] (#3126)

* Update

Backporting #3081 to NGO v1

* test

Back porting #3081 test updates (where applicable).

* update

adding changelog entries

* update

Disposing the internal original.

* fix

Making sure the force send is passed along to the NetworkBehaviour

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -17,6 +17,10 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where a newly synchronizing client would be synchronized with the current `NetworkVariable` values always which could cause issues with collections if there were any pending state updates. Now, when initially synchronizing a client, if a `NetworkVariable` has a pending state update it will serialize the previously known value(s) to the synchronizing client so when the pending updates are sent they aren't duplicate values on the newly connected client side. (#3126)
+- Fixed issue where changing ownership would mark every `NetworkVariable` dirty. Now, it will only mark any `NetworkVariable` with owner read permissions as dirty and will send/flush any pending updates to all clients prior to sending the change in ownership message.  (#3126)
+- Fixed issue with `NetworkVariable` collections where transferring ownership to another client would not update the new owner's previous value to the most current value which could cause the last/previous added value to be detected as a change when adding or removing an entry (as long as the entry removed was not the last/previously added value).  (#3126)
+- Fixed issue where a client (or server) with no write permissions for a `NetworkVariable` using a standard .NET collection type could still modify the collection which could cause various issues depending upon the modification and collection type.  (#3126)
 - Fixed issue where `NetworkAnimator` would statically allocate write buffer space for `Animator` parameters that could cause a write error if the number of parameters exceeded the space allocated. (#3124)
 - Fixed issue with the in-scene network prefab instance update menu tool where it was not properly updating scenes when invoked on the root prefab instance. (#3084)
 - Fixed issue where `NetworkAnimator` would send updates to non-observer clients. (#3058)
@@ -28,6 +32,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
+- Changed `NetworkVariableDeltaMessage` so the server now forwards delta state updates (owner write permission based from a client) to other clients immediately as opposed to keeping a `NetworkVariable` or `NetworkList` dirty and processing them at the end of the frame or potentially on the next network tick. (#3126)
 - The Debug Simulator section of the Unity Transport component will now be hidden if Unity Transport 2.0 or later is installed. It was already non-functional in that situation and users should instead use the more featureful [Network Simulator](https://docs-multiplayer.unity3d.com/tools/current/tools-network-simulator/) tool from the Multiplayer Tools package. (#3120)
 
 

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

@@ -765,6 +765,13 @@ public virtual void OnGainedOwnership() { }
         internal void InternalOnGainedOwnership()
         {
             UpdateNetworkProperties();
+            // New owners need to assure any NetworkVariables they have write permissions 
+            // to are updated so the previous and original values are aligned with the
+            // current value (primarily for collections).
+            if (OwnerClientId == NetworkManager.LocalClientId)
+            {
+                UpdateNetworkVariableOnOwnershipChanged();
+            }
             OnGainedOwnership();
         }
 
@@ -946,20 +953,20 @@ internal void PreVariableUpdate()
             PreNetworkVariableWrite();
         }
 
-        internal void VariableUpdate(ulong targetClientId)
-        {
-            NetworkVariableUpdate(targetClientId, NetworkBehaviourId);
-        }
-
         internal readonly List<int> NetworkVariableIndexesToReset = new List<int>();
         internal readonly HashSet<int> NetworkVariableIndexesToResetSet = new HashSet<int>();
 
-        private void NetworkVariableUpdate(ulong targetClientId, int behaviourIndex)
+        internal void NetworkVariableUpdate(ulong targetClientId, int behaviourIndex, bool forceSend = false)
         {
-            if (!CouldHaveDirtyNetworkVariables())
+            if (!forceSend && !CouldHaveDirtyNetworkVariables())
             {
                 return;
             }
+            // Getting these ahead of time actually improves performance
+            var networkManager = NetworkManager;
+            var networkObject = NetworkObject;
+            var messageManager = networkManager.MessageManager;
+            var connectionManager = networkManager.ConnectionManager;
 
             for (int j = 0; j < m_DeliveryMappedNetworkVariableIndices.Count; j++)
             {
@@ -982,10 +989,14 @@ private void NetworkVariableUpdate(ulong targetClientId, int behaviourIndex)
                     var message = new NetworkVariableDeltaMessage
                     {
                         NetworkObjectId = NetworkObjectId,
-                        NetworkBehaviourIndex = NetworkObject.GetNetworkBehaviourOrderIndex(this),
+                        NetworkBehaviourIndex = networkObject.GetNetworkBehaviourOrderIndex(this),
                         NetworkBehaviour = this,
                         TargetClientId = targetClientId,
-                        DeliveryMappedNetworkVariableIndex = m_DeliveryMappedNetworkVariableIndices[j]
+                        DeliveryMappedNetworkVariableIndex = m_DeliveryMappedNetworkVariableIndices[j],
+                        // By sending the network delivery we can forward messages immediately as opposed to processing them
+                        // at the end. While this will send updates to clients that cannot read, the handler will ignore anything
+                        // sent to a client that does not have read permissions.
+                        NetworkDelivery = m_DeliveryTypesForNetworkVariableGroups[j]
                     };
                     // TODO: Serialization is where the IsDirty flag gets changed.
                     // Messages don't get sent from the server to itself, so if we're host and sending to ourselves,
@@ -994,15 +1005,15 @@ private void NetworkVariableUpdate(ulong targetClientId, int behaviourIndex)
                     // so we don't have to do this serialization work if we're not going to use the result.
                     if (IsServer && targetClientId == NetworkManager.ServerClientId)
                     {
-                        var tmpWriter = new FastBufferWriter(NetworkManager.MessageManager.NonFragmentedMessageMaxSize, Allocator.Temp, NetworkManager.MessageManager.FragmentedMessageMaxSize);
+                        var tmpWriter = new FastBufferWriter(messageManager.NonFragmentedMessageMaxSize, Allocator.Temp, messageManager.FragmentedMessageMaxSize);
                         using (tmpWriter)
                         {
                             message.Serialize(tmpWriter, message.Version);
                         }
                     }
                     else
                     {
-                        NetworkManager.ConnectionManager.SendMessage(ref message, m_DeliveryTypesForNetworkVariableGroups[j], targetClientId);
+                        connectionManager.SendMessage(ref message, m_DeliveryTypesForNetworkVariableGroups[j], targetClientId);
                     }
                 }
             }
@@ -1029,6 +1040,26 @@ private bool CouldHaveDirtyNetworkVariables()
             return false;
         }
 
+        /// <summary>
+        /// Invoked on a new client to assure the previous and original values
+        /// are synchronized with the current known value.
+        /// </summary>
+        /// <remarks>
+        /// Primarily for collections to assure the previous value(s) is/are the
+        /// same as the current value(s) in order to not re-send already known entries.
+        /// </remarks>
+        internal void UpdateNetworkVariableOnOwnershipChanged()
+        {
+            for (int j = 0; j < NetworkVariableFields.Count; j++)
+            {
+                // Only invoke OnInitialize on NetworkVariables the owner can write to
+                if (NetworkVariableFields[j].CanClientWrite(OwnerClientId))
+                {
+                    NetworkVariableFields[j].OnInitialize();
+                }
+            }
+        }
+
         internal void MarkVariablesDirty(bool dirty)
         {
             for (int j = 0; j < NetworkVariableFields.Count; j++)
@@ -1037,6 +1068,17 @@ internal void MarkVariablesDirty(bool dirty)
             }
         }
 
+        internal void MarkOwnerReadVariablesDirty()
+        {
+            for (int j = 0; j < NetworkVariableFields.Count; j++)
+            {
+                if (NetworkVariableFields[j].ReadPerm == NetworkVariableReadPermission.Owner)
+                {
+                    NetworkVariableFields[j].SetDirty(true);
+                }
+            }
+        }
+
         /// <summary>
         /// Synchronizes by setting only the NetworkVariable field values that the client has permission to read.
         /// Note: This is only invoked when first synchronizing a NetworkBehaviour (i.e. late join or spawned NetworkObject)
@@ -1067,15 +1109,23 @@ internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClie
                         // The way we do packing, any value > 63 in a ushort will use the full 2 bytes to represent.
                         writer.WriteValueSafe((ushort)0);
                         var startPos = writer.Position;
-                        NetworkVariableFields[j].WriteField(writer);
+                        // Write the NetworkVariable field value
+                        // WriteFieldSynchronization will write the current value only if there are no pending changes.
+                        // Otherwise, it will write the previous value if there are pending changes since the pending
+                        // changes will be sent shortly after the client's synchronization.
+                        NetworkVariableFields[j].WriteFieldSynchronization(writer);
                         var size = writer.Position - startPos;
                         writer.Seek(writePos);
                         writer.WriteValueSafe((ushort)size);
                         writer.Seek(startPos + size);
                     }
                     else
                     {
-                        NetworkVariableFields[j].WriteField(writer);
+                        // Write the NetworkVariable field value
+                        // WriteFieldSynchronization will write the current value only if there are no pending changes.
+                        // Otherwise, it will write the previous value if there are pending changes since the pending
+                        // changes will be sent shortly after the client's synchronization.
+                        NetworkVariableFields[j].WriteFieldSynchronization(writer);
                     }
                 }
                 else // Only if EnsureNetworkVariableLengthSafety, otherwise just skip

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

@@ -19,10 +19,15 @@ public class NetworkBehaviourUpdater
 
         internal void AddForUpdate(NetworkObject networkObject)
         {
+            // Since this is a HashSet, we don't need to worry about duplicate entries
             m_PendingDirtyNetworkObjects.Add(networkObject);
         }
 
-        internal void NetworkBehaviourUpdate()
+        /// <summary>
+        /// Sends NetworkVariable deltas
+        /// </summary>
+        /// <param name="forceSend">internal only, when changing ownership we want to send this before the change in ownership message</param>
+        internal void NetworkBehaviourUpdate(bool forceSend = false)
         {
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
             m_NetworkBehaviourUpdate.Begin();
@@ -54,7 +59,7 @@ internal void NetworkBehaviourUpdate()
                                 // Sync just the variables for just the objects this client sees
                                 for (int k = 0; k < dirtyObj.ChildNetworkBehaviours.Count; k++)
                                 {
-                                    dirtyObj.ChildNetworkBehaviours[k].VariableUpdate(client.ClientId);
+                                    dirtyObj.ChildNetworkBehaviours[k].NetworkVariableUpdate(client.ClientId, k, forceSend);
                                 }
                             }
                         }
@@ -73,7 +78,7 @@ internal void NetworkBehaviourUpdate()
                             }
                             for (int k = 0; k < sobj.ChildNetworkBehaviours.Count; k++)
                             {
-                                sobj.ChildNetworkBehaviours[k].VariableUpdate(NetworkManager.ServerClientId);
+                                sobj.ChildNetworkBehaviours[k].NetworkVariableUpdate(NetworkManager.ServerClientId, k, forceSend);
                             }
                         }
                     }
@@ -86,19 +91,28 @@ internal void NetworkBehaviourUpdate()
                         var behaviour = dirtyObj.ChildNetworkBehaviours[k];
                         for (int i = 0; i < behaviour.NetworkVariableFields.Count; i++)
                         {
+                            // Set to true for NetworkVariable to ignore duplication of the
+                            // "internal original value" for collections support.
+                            behaviour.NetworkVariableFields[i].NetworkUpdaterCheck = true;
                             if (behaviour.NetworkVariableFields[i].IsDirty() &&
                                 !behaviour.NetworkVariableIndexesToResetSet.Contains(i))
                             {
                                 behaviour.NetworkVariableIndexesToResetSet.Add(i);
                                 behaviour.NetworkVariableIndexesToReset.Add(i);
                             }
+                            // Set to true for NetworkVariable to ignore duplication of the
+                            // "internal original value" for collections support.
+                            behaviour.NetworkVariableFields[i].NetworkUpdaterCheck = false;
                         }
                     }
                 }
+
                 // Now, reset all the no-longer-dirty variables
                 foreach (var dirtyobj in m_DirtyNetworkObjects)
                 {
-                    dirtyobj.PostNetworkVariableWrite();
+                    dirtyobj.PostNetworkVariableWrite(forceSend);
+                    // Once done processing, we set the previous owner id to the current owner id
+                    dirtyobj.PreviousOwnerId = dirtyobj.OwnerClientId;
                 }
                 m_DirtyNetworkObjects.Clear();
             }

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

@@ -272,6 +272,8 @@ private GlobalObjectId GetGlobalId()
         /// </summary>
         public ulong OwnerClientId { get; internal set; }
 
+        internal ulong PreviousOwnerId;
+
         /// <summary>
         /// If true, the object will always be replicated as root on clients and the parent will be ignored.
         /// </summary>
@@ -1484,6 +1486,14 @@ internal void MarkVariablesDirty(bool dirty)
             }
         }
 
+        internal void MarkOwnerReadVariablesDirty()
+        {
+            for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
+            {
+                ChildNetworkBehaviours[i].MarkOwnerReadVariablesDirty();
+            }
+        }
+
         // NGO currently guarantees that the client will receive spawn data for all objects in one network tick.
         //  Children may arrive before their parents; when they do they are stored in OrphanedChildren and then
         //  resolved when their parents arrived.  Because we don't send a partial list of spawns (yet), something
@@ -1725,11 +1735,11 @@ public void Deserialize(FastBufferReader reader)
             }
         }
 
-        internal void PostNetworkVariableWrite()
+        internal void PostNetworkVariableWrite(bool forceSend)
         {
             for (int k = 0; k < ChildNetworkBehaviours.Count; k++)
             {
-                ChildNetworkBehaviours[k].PostNetworkVariableWrite();
+                ChildNetworkBehaviours[k].PostNetworkVariableWrite(forceSend);
             }
         }
 

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs

@@ -45,12 +45,6 @@ public void Handle(ref NetworkContext context)
                 networkObject.InvokeBehaviourOnLostOwnership();
             }
 
-            // We are new owner.
-            if (OwnerClientId == networkManager.LocalClientId)
-            {
-                networkObject.InvokeBehaviourOnGainedOwnership();
-            }
-
             // For all other clients that are neither the former or current owner, update the behaviours' properties
             if (OwnerClientId != networkManager.LocalClientId && originalOwner != networkManager.LocalClientId)
             {
@@ -60,6 +54,21 @@ public void Handle(ref NetworkContext context)
                 }
             }
 
+            // We are new owner.
+            if (OwnerClientId == networkManager.LocalClientId)
+            {
+                networkObject.InvokeBehaviourOnGainedOwnership();
+            }
+
+            if (originalOwner == networkManager.LocalClientId)
+            {
+                // Mark any owner read variables as dirty
+                networkObject.MarkOwnerReadVariablesDirty();
+                // Immediately queue any pending deltas and order the message before the
+                // change in ownership message.
+                networkManager.BehaviourUpdater.NetworkBehaviourUpdate(true);
+            }
+
             networkObject.InvokeOwnershipChanged(originalOwner, OwnerClientId);
 
             networkManager.NetworkMetrics.TrackOwnershipChangeReceived(context.SenderId, networkObject, context.MessageSize);