Merge branch 'develop-2.0.0' into fix/bitwriter-grow-keeps-bitwise-bytes

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -23,6 +23,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ### Fixed
 
 - Issue where bits written with `BitWriter` were lost when a later `BitWriter.TryBeginWriteBits` call in the same bitwise context grew the `FastBufferWriter`. The bytes written earlier in that context were replaced with zeros or uninitialized memory. (#4225)
+- Issue where a `FastBufferReader` created with an offset and the default length of -1 read past the end of its source. The `ArraySegment<byte>` overload that takes an offset now applies it from the start of the segment. (#4221)
+- Issue where a client synchronized while a `NetworkList<T>` still had unsent changes applied those changes twice, resulting in duplicate entries and index-based events landing on the wrong elements on that client. (#4215)
+- Issue where showing a `NetworkObject` with `NetworkShow` while its `NetworkVariable` updates were held back by `NetworkVariableUpdateTraits` stopped those pending changes from being sent until the variable was modified again. (#4215)
 - Issue where moving a `NetworkObject` into another scene made the clients that did not observe it log "Trying to synchronize NetworkObjectId but it was not spawned". The scene migration is now only sent to the clients that observe the `NetworkObject`. (#4185)
 - Issue where a `NetworkObject` that was moved into another scene while hidden from a client spawned in that client's active scene when it was shown with `NetworkShow`, instead of the scene it is in on the server. (#4185)
 - Issue where mixed authority nested `NetworkTransform` instances could stop child/nested instances from updating because an authoritative `NetworkTransform` (root or otherwise) would remove the `NetworkObject` from the update group, preventing non-authoritative instances from updating their state on the authority side. (#4169)

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

@@ -138,6 +138,19 @@ internal void ForceSendIfDirtyOnNetworkShow(NetworkObject networkObject)
             // Remove it from the pending and queued dirty objects lists
             m_PendingDirtyNetworkObjects.Remove(networkObject);
             m_DirtyNetworkObjects.Remove(networkObject);
+
+            // Anything still dirty was held back by its update traits, keep it queued so a later tick sends it
+            foreach (var behaviour in networkObject.ChildNetworkBehaviours.Values)
+            {
+                foreach (var networkVariable in behaviour.NetworkVariableFields)
+                {
+                    if (networkVariable.IsDirty())
+                    {
+                        AddForUpdate(networkObject);
+                        return;
+                    }
+                }
+            }
         }
 
         /// <summary>

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

@@ -16,6 +16,10 @@ public class NetworkList<T> : NetworkVariableBase where T : unmanaged, IEquatabl
         private NativeList<T> m_List = new NativeList<T>(64, Allocator.Persistent);
         private NativeList<NetworkListEvent<T>> m_DirtyEvents = new NativeList<NetworkListEvent<T>>(64, Allocator.Persistent);
 
+        // The list as it was before the first local change that is still pending in m_DirtyEvents
+        private NativeList<T> m_LastSentList = new NativeList<T>(64, Allocator.Persistent);
+        private bool m_HasLastSentList;
+
         /// <summary>
         /// Delegate type for list changed event
         /// </summary>
@@ -67,6 +71,7 @@ public override void ResetDirty()
             {
                 m_DirtyEvents.Clear();
             }
+            m_HasLastSentList = false;
         }
 
         /// <inheritdoc cref="NetworkVariable{T}.IsDirty"/>
@@ -149,10 +154,42 @@ private T ReadValue(FastBufferReader reader)
         /// <inheritdoc cref="NetworkVariable{T}.WriteField"/>
         public override void WriteField(FastBufferWriter writer)
         {
-            writer.WriteValueSafe((ushort)m_List.Length);
-            for (int i = 0; i < m_List.Length; i++)
+            WriteList(writer, m_List);
+        }
+
+        internal override void WriteFieldSynchronization(FastBufferWriter writer)
+        {
+            // The pending events are sent to the synchronizing client with the next delta, so it must
+            // receive the list they apply to. Otherwise it would apply them twice.
+            if (m_HasLastSentList && m_DirtyEvents.Length > 0)
+            {
+                WriteList(writer, m_LastSentList);
+            }
+            else
+            {
+                base.WriteFieldSynchronization(writer);
+            }
+        }
+
+        private static void WriteList(FastBufferWriter writer, NativeList<T> list)
+        {
+            writer.WriteValueSafe((ushort)list.Length);
+            for (int i = 0; i < list.Length; i++)
             {
-                NetworkVariableSerialization<T>.Serializer.Write(writer, ref m_List.ElementAt(i));
+                NetworkVariableSerialization<T>.Serializer.Write(writer, ref list.ElementAt(i));
+            }
+        }
+
+        /// <summary>
+        /// Invoked before a local change. Keeps a copy of the list as of the last sent delta
+        /// so <see cref="WriteFieldSynchronization"/> can send it while changes are pending.
+        /// </summary>
+        private void CaptureLastSentList()
+        {
+            if (m_DirtyEvents.Length == 0)
+            {
+                m_LastSentList.CopyFrom(m_List);
+                m_HasLastSentList = true;
             }
         }
 
@@ -439,6 +476,7 @@ public void Add(T item)
                 return;
             }
 
+            CaptureLastSentList();
             m_List.Add(item);
 
             var listEvent = new NetworkListEvent<T>()
@@ -466,6 +504,7 @@ public void Clear()
                 return;
             }
 
+            CaptureLastSentList();
             m_List.Clear();
 
             var listEvent = new NetworkListEvent<T>()
@@ -510,6 +549,7 @@ public bool Remove(T item)
                 return false;
             }
 
+            CaptureLastSentList();
             m_List.RemoveAt(index);
             var listEvent = new NetworkListEvent<T>()
             {
@@ -553,6 +593,7 @@ public void Insert(int index, T item)
                 return;
             }
 
+            CaptureLastSentList();
             if (index < m_List.Length)
             {
                 m_List.InsertRangeWithBeginEnd(index, index + 1);
@@ -589,6 +630,7 @@ public void RemoveAt(int index)
             }
 
             var value = m_List[index];
+            CaptureLastSentList();
             m_List.RemoveAt(index);
 
             var listEvent = new NetworkListEvent<T>()
@@ -631,6 +673,7 @@ public void Set(int index, T value, bool forceUpdate = false)
                 return;
             }
 
+            CaptureLastSentList();
             m_List[index] = value;
 
             var listEvent = new NetworkListEvent<T>()
@@ -706,6 +749,11 @@ public override void Dispose()
                 m_DirtyEvents.Dispose();
             }
 
+            if (m_LastSentList.IsCreated)
+            {
+                m_LastSentList.Dispose();
+            }
+
             base.Dispose();
         }
     }

com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs

@@ -90,6 +90,16 @@ internal unsafe void CommitBitwiseReads(int amount)
             return readerHandle;
         }
 
+        // A copy starts at offset, so -1 copies what follows it. Allocator.None keeps the whole source and starts reading at offset.
+        private static int ResolveLength(int length, int sourceLength, int offset, Allocator copyAllocator)
+        {
+            if (length != -1)
+            {
+                return length;
+            }
+            return copyAllocator == Allocator.None ? sourceLength : sourceLength - offset;
+        }
+
         /// <summary>
         /// Create a FastBufferReader from a NativeArray.
         ///
@@ -107,12 +117,12 @@ internal unsafe void CommitBitwiseReads(int amount)
         /// </summary>
         /// <param name="buffer">The NativeArray to create the reader from</param>
         /// <param name="copyAllocator">The allocator type used for internal data when copying an existing buffer if other than Allocator.None is specified, that memory will be owned by this FastBufferReader instance</param>
-        /// <param name="length">The number of bytes to read from the buffer. If set to -1, the entire buffer length will be used</param>
+        /// <param name="length">The number of bytes to read from the buffer. If set to -1, every byte from offset to the end of the buffer will be used</param>
         /// <param name="offset">The offset into the buffer to start reading from</param>
         /// <param name="internalAllocator">The allocator type used for internal data when this reader points directly at a buffer owned by someone else</param>
         public unsafe FastBufferReader(NativeArray<byte> buffer, Allocator copyAllocator, int length = -1, int offset = 0, Allocator internalAllocator = Allocator.Temp)
         {
-            Handle = CreateHandle((byte*)buffer.GetUnsafePtr(), length == -1 ? buffer.Length : length, offset, copyAllocator, internalAllocator);
+            Handle = CreateHandle((byte*)buffer.GetUnsafePtr(), ResolveLength(length, buffer.Length, offset, copyAllocator), offset, copyAllocator, internalAllocator);
         }
 
         /// <summary>
@@ -127,7 +137,7 @@ public unsafe FastBufferReader(NativeArray<byte> buffer, Allocator copyAllocator
         /// <param name="buffer">The buffer to copy from</param>
         /// <param name="copyAllocator">The allocator type used for internal data when copying an existing buffer if other than Allocator.None is specified, that memory will be owned by this FastBufferReader instance</param>
         /// <param name="length">The number of bytes to copy (all if this is -1)</param>
-        /// <param name="offset">The offset of the buffer to start copying from</param>
+        /// <param name="offset">The offset into the segment to start copying from</param>
         public unsafe FastBufferReader(ArraySegment<byte> buffer, Allocator copyAllocator, int length = -1, int offset = 0)
         {
             if (copyAllocator == Allocator.None)
@@ -136,7 +146,7 @@ public unsafe FastBufferReader(ArraySegment<byte> buffer, Allocator copyAllocato
             }
             fixed (byte* data = buffer.Array)
             {
-                Handle = CreateHandle(data, length == -1 ? buffer.Count : length, offset, copyAllocator, Allocator.Temp);
+                Handle = CreateHandle(data + buffer.Offset, ResolveLength(length, buffer.Count, offset, copyAllocator), offset, copyAllocator, Allocator.Temp);
             }
         }
 
@@ -209,7 +219,7 @@ public unsafe FastBufferReader(byte[] buffer, Allocator copyAllocator, int lengt
             }
             fixed (byte* data = buffer)
             {
-                Handle = CreateHandle(data, length == -1 ? buffer.Length : length, offset, copyAllocator, Allocator.Temp);
+                Handle = CreateHandle(data, ResolveLength(length, buffer.Length, offset, copyAllocator), offset, copyAllocator, Allocator.Temp);
             }
         }
 
@@ -260,7 +270,7 @@ public unsafe FastBufferReader(byte* buffer, Allocator copyAllocator, int length
         /// <param name="internalAllocator">The allocator type used for internal data when this reader points directly at a buffer owned by someone else</param>
         public unsafe FastBufferReader(FastBufferWriter writer, Allocator copyAllocator, int length = -1, int offset = 0, Allocator internalAllocator = Allocator.Temp)
         {
-            Handle = CreateHandle(writer.GetUnsafePtr(), length == -1 ? writer.Length : length, offset, copyAllocator, internalAllocator);
+            Handle = CreateHandle(writer.GetUnsafePtr(), ResolveLength(length, writer.Length, offset, copyAllocator), offset, copyAllocator, internalAllocator);
         }
 
         /// <summary>
@@ -285,7 +295,7 @@ public unsafe FastBufferReader(FastBufferWriter writer, Allocator copyAllocator,
         /// <param name="internalAllocator">The allocator type used for internal data when this reader points directly at a buffer owned by someone else</param>
         public unsafe FastBufferReader(FastBufferReader reader, Allocator copyAllocator, int length = -1, int offset = 0, Allocator internalAllocator = Allocator.Temp)
         {
-            Handle = CreateHandle(reader.GetUnsafePtr(), length == -1 ? reader.Length : length, offset, copyAllocator, internalAllocator);
+            Handle = CreateHandle(reader.GetUnsafePtr(), ResolveLength(length, reader.Length, offset, copyAllocator), offset, copyAllocator, internalAllocator);
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs

@@ -1118,6 +1118,78 @@ public void WhenCreatingAReaderFromAnEmptyBuffer_LengthIsZero()
             }
         }
 
+        private const int k_SourceLength = 10;
+        private const int k_ReaderOffset = 2;
+
+        private static byte[] CreateSequentialBytes()
+        {
+            var bytes = new byte[k_SourceLength];
+            for (var i = 0; i < bytes.Length; ++i)
+            {
+                bytes[i] = (byte)i;
+            }
+            return bytes;
+        }
+
+        private static void AssertReadsToEndFrom(FastBufferReader reader, int firstValue)
+        {
+            var expectedCount = k_SourceLength - firstValue;
+            Assert.AreEqual(expectedCount, reader.Length - reader.Position);
+            for (var i = 0; i < expectedCount; ++i)
+            {
+                reader.ReadByteSafe(out byte value);
+                Assert.AreEqual((byte)(firstValue + i), value);
+            }
+        }
+
+        [Test]
+        public void WhenCreatingAReaderFromAnArrayWithAnOffsetAndNoLength_TheRestOfTheArrayIsCopied()
+        {
+            using var reader = new FastBufferReader(CreateSequentialBytes(), Allocator.Temp, -1, k_ReaderOffset);
+            AssertReadsToEndFrom(reader, k_ReaderOffset);
+        }
+
+        [Test]
+        public void WhenCreatingAReaderFromAnArraySegmentWithAnOffsetAndNoLength_TheRestOfTheSegmentIsCopied()
+        {
+            using var reader = new FastBufferReader(new ArraySegment<byte>(CreateSequentialBytes()), Allocator.Temp, -1, k_ReaderOffset);
+            AssertReadsToEndFrom(reader, k_ReaderOffset);
+        }
+
+        [Test]
+        public void WhenCreatingAReaderFromAnOffsetArraySegmentWithAnOffset_TheOffsetIsRelativeToTheSegment()
+        {
+            var segmentOffset = 3;
+            var segment = new ArraySegment<byte>(CreateSequentialBytes(), segmentOffset, k_SourceLength - segmentOffset);
+            using var reader = new FastBufferReader(segment, Allocator.Temp, -1, k_ReaderOffset);
+            AssertReadsToEndFrom(reader, segmentOffset + k_ReaderOffset);
+        }
+
+        [Test]
+        public void WhenCreatingAReaderFromANativeArrayWithAnOffsetAndNoLength_TheRestOfTheArrayIsRead([Values(Allocator.Temp, Allocator.None)] Allocator copyAllocator)
+        {
+            using var source = new NativeArray<byte>(CreateSequentialBytes(), Allocator.Temp);
+            using var reader = new FastBufferReader(source, copyAllocator, -1, k_ReaderOffset);
+            AssertReadsToEndFrom(reader, k_ReaderOffset);
+        }
+
+        [Test]
+        public void WhenCreatingAReaderFromAWriterWithAnOffsetAndNoLength_TheRestOfTheWriterIsRead([Values(Allocator.Temp, Allocator.None)] Allocator copyAllocator)
+        {
+            using var writer = new FastBufferWriter(k_SourceLength, Allocator.Temp);
+            writer.WriteBytesSafe(CreateSequentialBytes());
+            using var reader = new FastBufferReader(writer, copyAllocator, -1, k_ReaderOffset);
+            AssertReadsToEndFrom(reader, k_ReaderOffset);
+        }
+
+        [Test]
+        public void WhenCreatingAReaderFromAReaderWithAnOffsetAndNoLength_TheRestOfTheReaderIsRead([Values(Allocator.Temp, Allocator.None)] Allocator copyAllocator)
+        {
+            using var source = new FastBufferReader(CreateSequentialBytes(), Allocator.Temp);
+            using var reader = new FastBufferReader(source, copyAllocator, -1, k_ReaderOffset);
+            AssertReadsToEndFrom(reader, k_ReaderOffset);
+        }
+
         [Test]
         public void WhenCreatingNewFastBufferReader_IsInitializedIsTrue()
         {