fix: networkvariable collections can be modified without write permissions and more

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -22,13 +22,19 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed issue with nested `NetworkTransform` components clearing their initial prefab settings when in owner authoritative mode on the server side while using a client-server network topology which resulted in improper synchronization of the nested `NetworkTransform` components. (#3099)
 - Fixed issue with service not getting synchronized with in-scene placed `NetworkObject` instances when a session owner starts a `SceneEventType.Load` event. (#3096)
 - 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. (#3092)
+- 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. (#3081)
+- 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. (#3081)
+- 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). (#3081)
+- 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. (#3081)
 - Fixed issue where applying the position and/or rotation to the `NetworkManager.ConnectionApprovalResponse` when connection approval and auto-spawn player prefab were enabled would not apply the position and/or rotation when the player prefab was instantiated. (#3078)
 - Fixed issue where `NetworkObject.SpawnWithObservers` was not being honored when spawning the player prefab. (#3077)
 - Fixed issue with the client count not being correct on the host or server side when a client disconnects itself from a session. (#3075)
 
 ### Changed
 
 - Changed `NetworkConfig.AutoSpawnPlayerPrefabClientSide` is no longer automatically set when starting `NetworkManager`. (#3097)
+- 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. (#3081)
+
 
 ## [2.0.0] - 2024-09-12
 

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

@@ -826,6 +826,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();
         }
 
@@ -1016,9 +1023,14 @@ internal void PreVariableUpdate()
         internal readonly List<int> NetworkVariableIndexesToReset = new List<int>();
         internal readonly HashSet<int> NetworkVariableIndexesToResetSet = new HashSet<int>();
 
-        internal void NetworkVariableUpdate(ulong targetClientId)
+        /// <summary>
+        /// Determines if a NetworkVariable should have any changes to state sent out
+        /// </summary>
+        /// <param name="targetClientId">target to send the updates to</param>
+        /// <param name="forceSend">specific to change in ownership</param>
+        internal void NetworkVariableUpdate(ulong targetClientId, bool forceSend = false)
         {
-            if (!CouldHaveDirtyNetworkVariables())
+            if (!forceSend && !CouldHaveDirtyNetworkVariables())
             {
                 return;
             }
@@ -1069,7 +1081,11 @@ internal void NetworkVariableUpdate(ulong targetClientId)
                     NetworkBehaviourIndex = behaviourIndex,
                     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,
@@ -1114,6 +1130,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++)
@@ -1122,6 +1158,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)
@@ -1172,17 +1219,24 @@ 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);
-                        // Write the NetworkVariable value
+                        // Write the NetworkVariable field value size
                         writer.WriteValueSafe((ushort)size);
                         writer.Seek(startPos + size);
                     }
                     else // Client-Server Only: Should only ever be invoked when using a client-server NetworkTopology
                     {
-                        // Write the NetworkVariable value
-                        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 if (ensureLengthSafety)

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();
@@ -53,7 +58,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].NetworkVariableUpdate(client.ClientId);
+                                    dirtyObj.ChildNetworkBehaviours[k].NetworkVariableUpdate(client.ClientId, forceSend);
                                 }
                             }
                         }
@@ -72,7 +77,7 @@ internal void NetworkBehaviourUpdate()
                             }
                             for (int k = 0; k < sobj.ChildNetworkBehaviours.Count; k++)
                             {
-                                sobj.ChildNetworkBehaviours[k].NetworkVariableUpdate(NetworkManager.ServerClientId);
+                                sobj.ChildNetworkBehaviours[k].NetworkVariableUpdate(NetworkManager.ServerClientId, forceSend);
                             }
                         }
                     }
@@ -85,19 +90,24 @@ 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);
                             }
+                            // Reset back to false when done
+                            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;
                 }

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

@@ -2445,6 +2445,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
@@ -2771,11 +2779,11 @@ public void Deserialize(FastBufferReader reader)
             }
         }
 
-        internal void PostNetworkVariableWrite()
+        internal void PostNetworkVariableWrite(bool forced = false)
         {
             for (int k = 0; k < ChildNetworkBehaviours.Count; k++)
             {
-                ChildNetworkBehaviours[k].PostNetworkVariableWrite();
+                ChildNetworkBehaviours[k].PostNetworkVariableWrite(forced);
             }
         }
 

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

@@ -9,7 +9,6 @@ internal struct ChangeOwnershipMessage : INetworkMessage, INetworkSerializeByMem
 
         public ulong NetworkObjectId;
         public ulong OwnerClientId;
-        // DANGOEXP TODO: Remove these notes or change their format
         // SERVICE NOTES:
         // When forwarding the message to clients on the CMB Service side,
         // you can set the ClientIdCount to 0 and skip writing the ClientIds.
@@ -258,15 +257,18 @@ public void Handle(ref NetworkContext context)
                             continue;
                         }
 
-                        // If ownership is changing and this is not an ownership request approval then ignore the OnwerClientId
+                        // If ownership is changing and this is not an ownership request approval then ignore the SenderId
+                        if (OwnershipIsChanging && !RequestApproved && context.SenderId == clientId)
+                        {
+                            continue;
+                        }
+
                         // If it is just updating flags then ignore sending to the owner
                         // If it is a request or approving request, then ignore the RequestClientId
-                        if ((OwnershipIsChanging && !RequestApproved && OwnerClientId == clientId) || (OwnershipFlagsUpdate && clientId == OwnerClientId)
-                            || ((RequestOwnership || RequestApproved) && clientId == RequestClientId))
+                        if ((OwnershipFlagsUpdate && clientId == OwnerClientId) || ((RequestOwnership || RequestApproved) && clientId == RequestClientId))
                         {
                             continue;
                         }
-
                         networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, clientId);
                     }
                 }
@@ -327,10 +329,12 @@ private void HandleOwnershipChange(ref NetworkContext context)
             var networkManager = (NetworkManager)context.SystemOwner;
             var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId];
 
-            // DANGO-TODO: This probably shouldn't be allowed to happen.
+            // Sanity check that we are not sending duplicated change ownership messages
             if (networkObject.OwnerClientId == OwnerClientId)
             {
-                UnityEngine.Debug.LogWarning($"Unnecessary ownership changed message for {NetworkObjectId}");
+                UnityEngine.Debug.LogError($"Unnecessary ownership changed message for {NetworkObjectId}.");
+                // Ignore the message
+                return;
             }
 
             var originalOwner = networkObject.OwnerClientId;
@@ -347,12 +351,6 @@ private void HandleOwnershipChange(ref NetworkContext context)
                 networkObject.InvokeBehaviourOnLostOwnership();
             }
 
-            // We are new owner or (client-server) or running in distributed authority mode
-            if (OwnerClientId == networkManager.LocalClientId || networkManager.DistributedAuthorityMode)
-            {
-                networkObject.InvokeBehaviourOnGainedOwnership();
-            }
-
             // If in distributed authority mode 
             if (networkManager.DistributedAuthorityMode)
             {
@@ -374,6 +372,22 @@ private void HandleOwnershipChange(ref NetworkContext context)
                 }
             }
 
+            // We are new owner or (client-server) or running in distributed authority mode
+            if (OwnerClientId == networkManager.LocalClientId || networkManager.DistributedAuthorityMode)
+            {
+                networkObject.InvokeBehaviourOnGainedOwnership();
+            }
+
+
+            if (originalOwner == networkManager.LocalClientId && !networkManager.DistributedAuthorityMode)
+            {
+                // 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);
+            }
+
             // Always invoke ownership change notifications
             networkObject.InvokeOwnershipChanged(originalOwner, OwnerClientId);