fix: networkvariable collections can be modified without write permissions and more (#3081)

* fix

Provide additional copy of last known current NetworkVariable.Value to be able to compare against the current local value in order to detect if a client without write permissions has modified a collection.

* test

Updated collections validation tests to spot check the restore known current state when client without write permissions modifies a collection.

* update

added changelog entry

* fix

Changing order of operations within CheckDirtyState for clients without write permissions

* test

Add some spot checks to one of the dictionary tests

* fix

Fixing issue where upon a client gaining ownership of a NetworkObject that has one or more owner write permission NetworkVariables using a collection type with keyed indices (like a dictionary) can resend already synchronized entries when making a change to the collection causing non-owner clients to throw a key already exists exception.

* update

Adding changelog entrry

* update

Adjusted when the NetworkVariable update for ownership change is invoked to account for updates to owner write NetworkVariables within the OnGainedOwnership callback.

When changing ownership:
- Marking any owner read permissions NetworkVariables as dirty when sending to the new owner (both within NetworkSpawnManager for server-side update and within the NetworkVariableDeltaMessage).
- Sending any pending updates to all clients prior to sending the change in ownership message to assure pending updates are synchronized as the owner.

When initially synchronizing a client, if a NetworkVariable has a pending state update then send serialize the previously known value(s) to the synchronizing client so when the pending updates are sent they don't duplicate values.

* test

Adjusting two deferred message tests to not account for a NetworkVariable delta state update message when changing ownership and there are no pending updates.

* update

updating changelog entries

* style

* fix

This includes additional fixes for NetworkVariable collections that ended up requiring a different approach to how a server forwards NetworkVariable deltas. Now, NetworkVariableDeltaMessage forwards NetworkVariable field deltas immediately after a server has finished processing the deltas (i.e. the keeping a NetworkVariable dirty concept is not used from this point forward). I went ahead and kept the compatibility of this functionality.

NetworkVariableDeltaMessage has had its Version incremented as we now send the NetworkDelivery type in order to be able to handle this (it seemed less risky to include this than to try and bubble up this property to all message types).

This also separates the duplication of the "original internal value" and the "previous value" until after processing all deltas. If the server has to foward, it forwards first then invokes the PostDeltaRead method that performs the duplication.
This includes some minor adjustments to NetworkList in order to adjust to this new order of operations while also preserving the legacy approach.

This also includes some adjustments to NetworkBehaviourUpdater where it can force a send (flush) any pending dirty fields when changing ownership. This includes some minor modifications to NetworkObject.PostNetworkVariableWrite.

* test

Just some updates to two integration tests. These are all primarily for debugging purposes.

* update

Adding last change log entry for this PR.

* style

Updated a changelog entry to make it clearer.

* fix

DAHost fixes:
Fixed issue where it was possible to ignore forwarding a change in ownership message to the destination owner if the original owner changed ownership.
Fixed issue where it was possible to re-send a change in ownership message back to the original owner if the original owner sent the message.

* test

Adding and additional test that validates several of the fixes in this PR.

* test fix

Removing the DAHost testfixture that wasn't supposed to be added.
Fixing an issue with the NetworkObjectOwnershipTests when running in DAHost mode. It was passing because the DAHost was sending the ChangeOwnershipMessage back to the owner that changed ownership to another client (without the fix for that in this PR it would send the message to the authority/owner which is why the test was passing).

* Update com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs

Co-authored-by: Dominick <[email protected]>

* style

Updating all instances of k_ServerDeltaForwadingAndNetworkDelivery and replacing with k_ServerDeltaForwardingAndNetworkDelivery

---------

Co-authored-by: Dominick <[email protected]>

Author: NoelStephensUnity

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);