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

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

* Standards + changelog

Co-authored-by: Noel Stephens <noel.stephens@unity3d.com>

Author: ShadauxCat

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -1,3 +1,4 @@
+
 # Changelog
 
 All notable changes to this project will be documented in this file.
@@ -16,6 +17,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- 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)
 - Fixed Owner-written NetworkVariable infinitely write themselves (#2109)
 - Fixed NetworkList issue that showed when inserting at the very end of a NetworkList (#2099)
 - Fixed issue where a client owner of a `NetworkVariable` with both owner read and write permissions would not update the server side when changed. (#2097)

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

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

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