fix: NCCBUG-175: BitReader/BitWriter Reading/writing more than 8 bits was producing the wrong result

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -1,3 +1,4 @@
+
 # Changelog
 
 All notable changes to this project will be documented in this file.
@@ -26,6 +27,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed NetworkLists not populating on client. NetworkList now uses the most recent list as opposed to the list at the end of previous frame, when sending full updates to dynamically spawned NetworkObject. The difference in behaviour is required as scene management spawns those objects at a different time in the frame, relative to updates. (#2062)
 - Fixed NetworkList Value event on the server. PreviousValue is now set correctly when a new value is set through property setter. (#2067)
 - Fixed NetworkList issue that showed when inserting at the very end of a NetworkList (#2099)
+- Fixed an issue where reading/writing more than 8 bits at a time with BitReader/BitWriter would write/read from the wrong place, returning and incorrect result. (#2130)
 
 ## [1.0.0] - 2022-06-27
 

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

@@ -21,6 +21,8 @@ public ref struct BitReader
 
         private const int k_BitsPerByte = 8;
 
+        private int BytePosition => m_BitPosition >> 3;
+
         /// <summary>
         /// Whether or not the current BitPosition is evenly divisible by 8. I.e. whether or not the BitPosition is at a byte boundary.
         /// </summary>
@@ -98,11 +100,6 @@ public unsafe void ReadBits(out ulong value, uint bitCount)
                 throw new ArgumentOutOfRangeException(nameof(bitCount), "Cannot read more than 64 bits from a 64-bit value!");
             }
 
-            if (bitCount < 0)
-            {
-                throw new ArgumentOutOfRangeException(nameof(bitCount), "Cannot read fewer than 0 bits!");
-            }
-
             int checkPos = (int)(m_BitPosition + bitCount);
             if (checkPos > m_AllowedBitwiseReadMark)
             {
@@ -165,7 +162,7 @@ public unsafe void ReadBit(out bool bit)
 #endif
 
             int offset = m_BitPosition & 7;
-            int pos = m_BitPosition >> 3;
+            int pos = BytePosition;
             bit = (m_BufferPointer[pos] & (1 << offset)) != 0;
             ++m_BitPosition;
         }
@@ -175,7 +172,7 @@ private unsafe void ReadPartialValue<T>(out T value, int bytesToRead, int offset
         {
             var val = new T();
             byte* ptr = ((byte*)&val) + offsetBytes;
-            byte* bufferPointer = m_BufferPointer + m_Position;
+            byte* bufferPointer = m_BufferPointer + BytePosition;
             UnsafeUtility.MemCpy(ptr, bufferPointer, bytesToRead);
 
             m_BitPosition += bytesToRead * k_BitsPerByte;

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

@@ -29,6 +29,8 @@ public bool BitAligned
             get => (m_BitPosition & 7) == 0;
         }
 
+        private int BytePosition => m_BitPosition >> 3;
+
         internal unsafe BitWriter(FastBufferWriter writer)
         {
             m_Writer = writer;
@@ -181,7 +183,7 @@ public unsafe void WriteBit(bool bit)
 #endif
 
             int offset = m_BitPosition & 7;
-            int pos = m_BitPosition >> 3;
+            int pos = BytePosition;
             ++m_BitPosition;
             m_BufferPointer[pos] = (byte)(bit ? (m_BufferPointer[pos] & ~(1 << offset)) | (1 << offset) : (m_BufferPointer[pos] & ~(1 << offset)));
         }
@@ -190,7 +192,7 @@ public unsafe void WriteBit(bool bit)
         private unsafe void WritePartialValue<T>(T value, int bytesToWrite, int offsetBytes = 0) where T : unmanaged
         {
             byte* ptr = ((byte*)&value) + offsetBytes;
-            byte* bufferPointer = m_BufferPointer + m_Position;
+            byte* bufferPointer = m_BufferPointer + BytePosition;
             UnsafeUtility.MemCpy(bufferPointer, ptr, bytesToWrite);
 
             m_BitPosition += bytesToWrite * k_BitsPerByte;

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

@@ -0,0 +1,95 @@
+using NUnit.Framework;
+using Unity.Collections;
+
+namespace Unity.Netcode.EditorTests
+{
+    public class UserBitReaderAndBitWriterTests_NCCBUG175
+    {
+
+        [Test]
+        public void WhenBitwiseWritingMoreThan8Bits_ValuesAreCorrect()
+        {
+            using var writer = new FastBufferWriter(1024, Allocator.Temp);
+            ulong inVal = 123456789;
+
+            for (int i = 0; i < 100; ++i)
+            {
+                writer.WriteValueSafe(i);
+            }
+
+            using (var bitWriter = writer.EnterBitwiseContext())
+            {
+                for (int i = 0; i < 16; ++i)
+                {
+                    Assert.IsTrue((bitWriter.TryBeginWriteBits(32)));
+                    bitWriter.WriteBits(inVal, 31);
+                    bitWriter.WriteBit(true);
+                }
+            }
+
+            using var reader = new FastBufferReader(writer, Allocator.Temp);
+
+            for (int i = 0; i < 100; ++i)
+            {
+                reader.ReadValueSafe(out int outVal);
+                Assert.AreEqual(i, outVal);
+            }
+
+            using var bitReader = reader.EnterBitwiseContext();
+            for (int i = 0; i < 16; ++i)
+            {
+                Assert.IsTrue(bitReader.TryBeginReadBits(32));
+                bitReader.ReadBits(out ulong outVal, 31);
+                bitReader.ReadBit(out bool bit);
+                Assert.AreEqual(inVal, outVal);
+                Assert.AreEqual(true, bit);
+            }
+        }
+
+        [Test]
+        public void WhenBitwiseReadingMoreThan8Bits_ValuesAreCorrect()
+        {
+            using var writer = new FastBufferWriter(1024, Allocator.Temp);
+            ulong inVal = 123456789;
+
+            for (int i = 0; i < 100; ++i)
+            {
+                writer.WriteValueSafe(i);
+            }
+
+            uint combined = (uint)inVal | (1u << 31);
+            writer.WriteValueSafe(combined);
+            writer.WriteValueSafe(combined);
+            writer.WriteValueSafe(combined);
+
+            using var reader = new FastBufferReader(writer, Allocator.Temp);
+
+            for (int i = 0; i < 100; ++i)
+            {
+                reader.ReadValueSafe(out int outVal);
+                Assert.AreEqual(i, outVal);
+            }
+
+            using (var bitReader = reader.EnterBitwiseContext())
+            {
+                Assert.IsTrue(bitReader.TryBeginReadBits(32));
+                bitReader.ReadBits(out ulong outVal, 31);
+                bitReader.ReadBit(out bool bit);
+                Assert.AreEqual(inVal, outVal);
+                Assert.AreEqual(true, bit);
+
+                Assert.IsTrue(bitReader.TryBeginReadBits(32));
+                bitReader.ReadBits(out outVal, 31);
+                bitReader.ReadBit(out bit);
+                Assert.AreEqual(inVal, outVal);
+                Assert.AreEqual(true, bit);
+
+                Assert.IsTrue(bitReader.TryBeginReadBits(32));
+                bitReader.ReadBits(out outVal, 31);
+                bitReader.ReadBit(out bit);
+                Assert.AreEqual(inVal, outVal);
+                Assert.AreEqual(true, bit);
+            }
+        }
+    }
+}

dotnet-tools/netcode.standards/Program.cs

@@ -35,8 +35,8 @@ private static int Main(
             var procInfo = new ProcessStartInfo("dotnet");
 
             procInfo.Arguments = check
-                ? $"format {file} whitespace --no-restore --verify-no-changes --verbosity {verbosity}"
-                : $"format {file} whitespace --no-restore --verbosity {verbosity}";
+                ? $"format whitespace {file} --no-restore --verify-no-changes --verbosity {verbosity}"
+                : $"format whitespace {file} --no-restore --verbosity {verbosity}";
             Console.WriteLine($"######## START -> {(check ? "check" : "fix")} whitespace issues");
             var whitespace = Process.Start(procInfo);
             whitespace.WaitForExit();
@@ -48,8 +48,8 @@ private static int Main(
             Console.WriteLine("######## SUCCEEDED -> no whitespace issues");
 
             procInfo.Arguments = check
-                ? $"format {file} style --severity error --no-restore --verify-no-changes --verbosity {verbosity}"
-                : $"format {file} style --severity error --no-restore --verbosity {verbosity}";
+                ? $"format style {file} --severity error --no-restore --verify-no-changes --verbosity {verbosity}"
+                : $"format style {file} --severity error --no-restore --verbosity {verbosity}";
             Console.WriteLine($"######## START -> {(check ? "check" : "fix")} style/naming issues");
             var style = Process.Start(procInfo);
             style.WaitForExit();