fix: Regression between comb-server and NGO network variables (#2974)

* fix: Regression between comb-server and NGO network variables

* Ensure read logic is mirrored

* Ensure read logic is fully mirrored

* fix and update

Removing the read variable type from the typed serializers.
Did a little legacy code (POC DA work) clean up in the WriteNetworkVariableData and SetNetworkVariableData methods. Also added additional information in the error logging to more easily track down where any issues might be occurring.

* fix

length safety needed to be moved into a nested if.

---------

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

Author: EmandM

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

@@ -1112,26 +1112,38 @@ internal void MarkVariablesDirty(bool dirty)
         /// </remarks>
         internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
         {
+            // Create any values that require accessing the NetworkManager locally (it is expensive to access it in NetworkBehaviour)
             var networkManager = NetworkManager;
-            if (networkManager.DistributedAuthorityMode)
+            var distributedAuthority = networkManager.DistributedAuthorityMode;
+            var ensureLengthSafety = networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety;
+
+            // Always write the NetworkVariable count even if zero for distributed authority (used by comb server)
+            if (distributedAuthority)
             {
                 writer.WriteValueSafe((ushort)NetworkVariableFields.Count);
             }
 
+            // Exit early if there are no NetworkVariables
             if (NetworkVariableFields.Count == 0)
             {
                 return;
             }
 
-            // DANGO-TODO: Made some modifications here that overlap/won't play nice with EnsureNetworkVariableLenghtSafety.
-            // Worth either merging or more cleanly separating these codepaths.
             for (int j = 0; j < NetworkVariableFields.Count; j++)
             {
-                // Note: In distributed authority mode, all clients can read
+                // Client-Server: Try to write values only for clients that have read permissions.
+                // Distributed Authority: All clients have read permissions, always try to write the value.
                 if (NetworkVariableFields[j].CanClientRead(targetClientId))
                 {
-                    if (networkManager.DistributedAuthorityMode || networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
+                    // Write additional NetworkVariable information when length safety is enabled or when in distributed authority mode 
+                    if (ensureLengthSafety || distributedAuthority)
                     {
+                        // Write the type being serialized for distributed authority (only for comb-server)
+                        if (distributedAuthority)
+                        {
+                            writer.WriteValueSafe(NetworkVariableFields[j].Type);
+                        }
+
                         var writePos = writer.Position;
                         // Note: This value can't be packed because we don't know how large it will be in advance
                         // we reserve space for it, then write the data, then come back and fill in the space
@@ -1143,18 +1155,19 @@ internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClie
                         NetworkVariableFields[j].WriteField(writer);
                         var size = writer.Position - startPos;
                         writer.Seek(writePos);
+                        // Write the NetworkVariable value
                         writer.WriteValueSafe((ushort)size);
                         writer.Seek(startPos + size);
                     }
-                    else
+                    else // Client-Server Only: Should only ever be invoked when using a client-server NetworkTopology
                     {
+                        // Write the NetworkVariable value
                         NetworkVariableFields[j].WriteField(writer);
                     }
                 }
-                else
+                else if (ensureLengthSafety)
                 {
-                    // Only if EnsureNetworkVariableLengthSafety, otherwise just skip
-                    if (networkManager.DistributedAuthorityMode || networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
+                    // Client-Server Only: If the client cannot read this field, then skip it but write a 0 for this NetworkVariable's position
                     {
                         writer.WriteValueSafe((ushort)0);
                     }
@@ -1172,73 +1185,78 @@ internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClie
         /// </remarks>
         internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
         {
+            // Stack cache any values that requires accessing the NetworkManager (it is expensive to access it in NetworkBehaviour)
             var networkManager = NetworkManager;
-            if (networkManager.DistributedAuthorityMode)
+            var distributedAuthority = networkManager.DistributedAuthorityMode;
+            var ensureLengthSafety = networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety;
+
+            // Always read the NetworkVariable count when in distributed authority (sanity check if comb-server matches what client has locally)
+            if (distributedAuthority)
             {
                 reader.ReadValueSafe(out ushort variableCount);
                 if (variableCount != NetworkVariableFields.Count)
                 {
-                    Debug.LogError("NetworkVariable count mismatch.");
+                    Debug.LogError($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}] NetworkVariable count mismatch! (Read: {variableCount} vs. Expected: {NetworkVariableFields.Count})");
                     return;
                 }
             }
 
+            // Exit early if nothing else to read
             if (NetworkVariableFields.Count == 0)
             {
                 return;
             }
 
-            // DANGO-TODO: Made some modifications here that overlap/won't play nice with EnsureNetworkVariableLenghtSafety.
-            // Worth either merging or more cleanly separating these codepaths.
             for (int j = 0; j < NetworkVariableFields.Count; j++)
             {
                 var varSize = (ushort)0;
                 var readStartPos = 0;
-                if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
+                // Client-Server: Clients that only have read permissions will try to read the value
+                // Distributed Authority: All clients have read permissions, always try to read the value
+                if (NetworkVariableFields[j].CanClientRead(clientId))
                 {
-                    reader.ReadValueSafe(out varSize);
-                    if (varSize == 0)
+                    if (ensureLengthSafety || distributedAuthority)
                     {
-                        continue;
+                        // Read the type being serialized and discard it (for now) when in a distributed authority network topology (only used by comb-server)
+                        if (distributedAuthority)
+                        {
+                            reader.ReadValueSafe(out NetworkVariableType _);
+                        }
+
+                        reader.ReadValueSafe(out varSize);
+                        if (varSize == 0)
+                        {
+                            Debug.LogError($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] Expected non-zero size readable NetworkVariable! (Skipping)");
+                            continue;
+                        }
+                        readStartPos = reader.Position;
                     }
-                    readStartPos = reader.Position;
                 }
-                else // If the client cannot read this field, then skip it
-                if (!NetworkVariableFields[j].CanClientRead(clientId))
+                else // Client-Server Only: If the client cannot read this field, then skip it
                 {
-                    if (networkManager.DistributedAuthorityMode)
+                    // If skipping and length safety, then fill in a 0 size for this one spot
+                    if (ensureLengthSafety)
                     {
                         reader.ReadValueSafe(out ushort size);
                         if (size != 0)
                         {
-                            Debug.LogError("Expected zero size");
+                            Debug.LogError($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] Expected zero size for non-readable NetworkVariable when EnsureNetworkVariableLengthSafety is enabled! (Skipping)");
                         }
                     }
                     continue;
                 }
 
-                if (networkManager.DistributedAuthorityMode)
-                {
-                    reader.ReadValueSafe(out ushort size);
-                    var start_marker = reader.Position;
-                    NetworkVariableFields[j].ReadField(reader);
-                    if (reader.Position - start_marker != size)
-                    {
-                        Debug.LogError("Mismatched network variable size");
-                    }
-                }
-                else
-                {
-                    NetworkVariableFields[j].ReadField(reader);
-                }
+                // Read the NetworkVarible value
+                NetworkVariableFields[j].ReadField(reader);
 
-                if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
+                // When EnsureNetworkVariableLengthSafety or DistributedAuthorityMode always do a bounds check
+                if (ensureLengthSafety || distributedAuthority)
                 {
                     if (reader.Position > (readStartPos + varSize))
                     {
                         if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                         {
-                            NetworkLog.LogWarning($"Var data read too far. {reader.Position - (readStartPos + varSize)} bytes.");
+                            NetworkLog.LogWarning($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] NetworkVariable data read too big. {reader.Position - (readStartPos + varSize)} bytes.");
                         }
 
                         reader.Seek(readStartPos + varSize);
@@ -1247,7 +1265,7 @@ internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
                     {
                         if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                         {
-                            NetworkLog.LogWarning($"Var data read too little. {(readStartPos + varSize) - reader.Position} bytes.");
+                            NetworkLog.LogWarning($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] NetworkVariable data read too small. {(readStartPos + varSize) - reader.Position} bytes.");
                         }
 
                         reader.Seek(readStartPos + varSize);

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs

@@ -24,6 +24,7 @@ public class NetworkList<T> : NetworkVariableBase where T : unmanaged, IEquatabl
         /// The callback to be invoked when the list gets changed
         /// </summary>
         public event OnListChangedDelegate OnListChanged;
+        internal override NetworkVariableType Type => NetworkVariableType.NetworkList;
 
         /// <summary>
         /// Constructor method for <see cref="NetworkList"/>
@@ -132,7 +133,6 @@ public override void WriteField(FastBufferWriter writer)
         {
             if (m_NetworkManager.DistributedAuthorityMode)
             {
-                SerializationTools.WriteType(writer, NetworkVariableType.NetworkList);
                 writer.WriteValueSafe(NetworkVariableSerialization<T>.Serializer.Type);
                 if (NetworkVariableSerialization<T>.Serializer.Type == NetworkVariableType.Unmanaged)
                 {
@@ -158,7 +158,6 @@ public override void ReadField(FastBufferReader reader)
             m_List.Clear();
             if (m_NetworkManager.DistributedAuthorityMode)
             {
-                reader.ReadValueSafe(out NetworkVariableType _);
                 SerializationTools.ReadType(reader, NetworkVariableSerialization<T>.Serializer);
                 // Collection item type is used by the DA server, drop value here.
                 if (NetworkVariableSerialization<T>.Serializer.Type == NetworkVariableType.Unmanaged)

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs

@@ -45,6 +45,8 @@ public override void OnInitialize()
             NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_PreviousValue);
         }
 
+        internal override NetworkVariableType Type => NetworkVariableType.Value;
+
         /// <summary>
         /// Constructor for <see cref="NetworkVariable{T}"/>
         /// </summary>

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs

@@ -34,6 +34,9 @@ public abstract class NetworkVariableBase : IDisposable
         private protected NetworkBehaviour m_NetworkBehaviour;
 
         private NetworkManager m_InternalNetworkManager;
+
+        internal virtual NetworkVariableType Type => NetworkVariableType.Unknown;
+
         private protected NetworkManager m_NetworkManager
         {
             get
@@ -308,7 +311,6 @@ internal ulong OwnerClientId()
         /// </summary>
         /// <param name="reader">The stream to read the state from</param>
         public abstract void ReadField(FastBufferReader reader);
-
         /// <summary>
         /// Reads delta from the reader and applies them to the internal value
         /// </summary>

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableTypes.cs

@@ -14,28 +14,27 @@ namespace Unity.Netcode
     /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
     internal enum NetworkVariableType : byte
     {
-        /// <summary>
-        ///     For any type that is not known at runtime
-        /// </summary>
-        Unknown = 0,
         /// <summary>
         ///     Value
         ///     Used for all of the basic NetworkVariables that contain a single value
         /// </summary>
-        Value = 1,
-
+        Value = 0,
+        /// <summary>
+        ///     For any type that is not known at runtime
+        /// </summary>
+        Unknown = 1,
         /// <summary>
         ///     NetworkList
         /// </summary>
         NetworkList = 2,
 
         // The following types are valid types inside of NetworkVariable collections
-        Short = 3,
-        UShort = 4,
-        Int = 5,
-        UInt = 6,
-        Long = 7,
-        ULong = 8,
-        Unmanaged = 9,
+        Short = 11,
+        UShort = 12,
+        Int = 13,
+        UInt = 14,
+        Long = 15,
+        ULong = 16,
+        Unmanaged = 17,
     }
 }