From 8759d97e7123d19c8525b2fde1a2882e957fae31 Mon Sep 17 00:00:00 2001 From: Jaedyn Draper Date: Mon, 2 Aug 2021 17:35:03 -0500 Subject: [PATCH 1/4] fix: corrected an off-by-one copying the netvar buffer into NetworkVariableDelta message Other tests missed this because they were sending small numbers and the extra space in the receive buffer was initialized with 0s (i.e., in little endian, a 32 bit number representing 1 would be 01 00 00 00, if you lose the last byte you've lost 00, when the buffer is initialized with 0s, you happen to get the correct byte back). Added a test to specifically exercise this. --- .../Runtime/Core/NetworkBehaviour.cs | 2 +- .../Tests/Runtime/NetworkVarBufferCopyTest.cs | 149 ++++++++++++++++++ .../Runtime/NetworkVarBufferCopyTest.cs.meta | 3 + 3 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs create mode 100644 com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs.meta diff --git a/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs b/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs index 24168da409..2f4436ff4c 100644 --- a/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs @@ -580,7 +580,7 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex) { using (var nonNullContext = (InternalCommandContext)context) { - nonNullContext.NetworkWriter.WriteBytes(buffer.GetBuffer(), buffer.Position); + nonNullContext.NetworkWriter.WriteBytes(buffer.GetBuffer(), buffer.Position+1); } } } diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs new file mode 100644 index 0000000000..93f169b27d --- /dev/null +++ b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs @@ -0,0 +1,149 @@ +using System.Collections; +using System.IO; +using MLAPI.NetworkVariable; +using MLAPI.Serialization.Pooled; +using MLAPI.Transports; +using NUnit.Framework; +using UnityEngine.TestTools; + +namespace MLAPI.RuntimeTests +{ + public class NetworkVarBufferCopyTest : BaseMultiInstanceTest + { + class DummyNetVar : INetworkVariable + { + private const int k_DummyValue = 0x13579BDF; + public bool DeltaWritten; + public bool FieldWritten; + public bool DeltaRead; + public bool FieldRead; + public bool Dirty = true; + + public string Name { get; internal set; } + + public NetworkChannel GetChannel() + { + return NetworkChannel.NetworkVariable; + } + + public void ResetDirty() + { + Dirty = false; + } + + public bool IsDirty() + { + return Dirty; + } + + public bool CanClientWrite(ulong clientId) + { + return true; + } + + public bool CanClientRead(ulong clientId) + { + return true; + } + + public void WriteDelta(Stream stream) + { + using (var writer = PooledNetworkWriter.Get(stream)) + { + writer.WriteInt32(k_DummyValue); + } + + DeltaWritten = true; + } + + public void WriteField(Stream stream) + { + using (var writer = PooledNetworkWriter.Get(stream)) + { + writer.WriteInt32(k_DummyValue); + } + + FieldWritten = true; + } + + public void ReadField(Stream stream) + { + using (var reader = PooledNetworkReader.Get(stream)) + { + Assert.AreEqual(k_DummyValue, reader.ReadInt32()); + } + + FieldRead = true; + } + + public void ReadDelta(Stream stream, bool keepDirtyDelta) + { + using (var reader = PooledNetworkReader.Get(stream)) + { + Assert.AreEqual(k_DummyValue, reader.ReadInt32()); + } + + DeltaRead = true; + } + + public void SetNetworkBehaviour(NetworkBehaviour behaviour) + { + // nop + } + } + + class DummyNetBehaviour : NetworkBehaviour + { + public DummyNetVar NetVar; + } + protected override int NbClients => 1; + + [UnitySetUp] + public override IEnumerator Setup() + { + yield return StartSomeClientsAndServerWithPlayers(useHost: true, nbClients: NbClients, + updatePlayerPrefab: playerPrefab => + { + var dummyNetBehaviour = playerPrefab.AddComponent(); + }); + } + + [UnityTest] + public IEnumerator TestEntireBufferIsCopiedOnNetworkVariableDelta() + { + // This is the *SERVER VERSION* of the *CLIENT PLAYER* + var serverClientPlayerResult = new MultiInstanceHelpers.CoroutineResultWrapper(); + yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.GetNetworkObjectByRepresentation( + x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, + m_ServerNetworkManager, serverClientPlayerResult)); + + // This is the *CLIENT VERSION* of the *CLIENT PLAYER* + var clientClientPlayerResult = new MultiInstanceHelpers.CoroutineResultWrapper(); + yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.GetNetworkObjectByRepresentation( + x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, + m_ClientNetworkManagers[0], clientClientPlayerResult)); + + var serverSideClientPlayer = serverClientPlayerResult.Result; + var clientSideClientPlayer = clientClientPlayerResult.Result; + + var serverComponent = (serverSideClientPlayer).GetComponent(); + var clientComponent = (clientSideClientPlayer).GetComponent(); + + var waitResult = new MultiInstanceHelpers.CoroutineResultWrapper(); + + yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForCondition( + () => clientComponent.NetVar.DeltaRead == true, + waitResult, + maxFrames: 120)); + + if (!waitResult.Result) + { + Assert.Fail("Failed to send a delta within 120 frames"); + } + Assert.True(serverComponent.NetVar.FieldWritten); + Assert.True(serverComponent.NetVar.DeltaWritten); + Assert.True(clientComponent.NetVar.FieldRead); + Assert.True(clientComponent.NetVar.DeltaRead); + } + } +} \ No newline at end of file diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs.meta b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs.meta new file mode 100644 index 0000000000..88e49e3e87 --- /dev/null +++ b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: a7d44caa76a64b02978f5aca0e7b576a +timeCreated: 1627926008 \ No newline at end of file From da03f55600bbb9641689dccdc5e642f07f79cdd7 Mon Sep 17 00:00:00 2001 From: Jaedyn Draper Date: Mon, 2 Aug 2021 17:56:30 -0500 Subject: [PATCH 2/4] Fixed standards issues --- .../Tests/Runtime/NetworkVarBufferCopyTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs index 93f169b27d..781864ebd9 100644 --- a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs +++ b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs @@ -10,7 +10,7 @@ namespace MLAPI.RuntimeTests { public class NetworkVarBufferCopyTest : BaseMultiInstanceTest { - class DummyNetVar : INetworkVariable + public class DummyNetVar : INetworkVariable { private const int k_DummyValue = 0x13579BDF; public bool DeltaWritten; @@ -92,7 +92,7 @@ public void SetNetworkBehaviour(NetworkBehaviour behaviour) } } - class DummyNetBehaviour : NetworkBehaviour + public class DummyNetBehaviour : NetworkBehaviour { public DummyNetVar NetVar; } From ac2e4f4df0869c823d3415b4d74fd931a7bb11d9 Mon Sep 17 00:00:00 2001 From: Jaedyn Draper Date: Mon, 2 Aug 2021 18:53:00 -0500 Subject: [PATCH 3/4] Fixed it correctly this time. --- .../Runtime/Core/NetworkBehaviour.cs | 8 +++++--- .../Tests/Runtime/NetworkVarBufferCopyTest.cs | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs b/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs index 2f4436ff4c..5a434ca275 100644 --- a/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs @@ -561,6 +561,7 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex) else { NetworkVariableFields[k].WriteDelta(buffer); + buffer.PadBuffer(); } if (!m_NetworkVariableIndexesToResetSet.Contains(k)) @@ -580,7 +581,7 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex) { using (var nonNullContext = (InternalCommandContext)context) { - nonNullContext.NetworkWriter.WriteBytes(buffer.GetBuffer(), buffer.Position+1); + nonNullContext.NetworkWriter.WriteBytes(buffer.GetBuffer(), buffer.Position); } } } @@ -666,11 +667,10 @@ internal static void HandleNetworkVariableDeltas(List networkV PerformanceDataManager.Increment(ProfilerConstants.NetworkVarDeltas); ProfilerStatManager.NetworkVarsRcvd.Record(); + (stream as NetworkBuffer).SkipPadBits(); if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety) { - (stream as NetworkBuffer).SkipPadBits(); - if (stream.Position > (readStartPos + varSize)) { if (NetworkLog.CurrentLogLevel <= LogLevel.Normal) @@ -826,6 +826,7 @@ internal static void WriteNetworkVariableData(List networkVari else { networkVariableList[j].WriteField(stream); + writer.WritePadBits(); } } } @@ -865,6 +866,7 @@ internal static void SetNetworkVariableData(List networkVariab long readStartPos = stream.Position; networkVariableList[j].ReadField(stream); + reader.SkipPadBits(); if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety) { diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs index 781864ebd9..387d780940 100644 --- a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs +++ b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs @@ -50,6 +50,7 @@ public void WriteDelta(Stream stream) { using (var writer = PooledNetworkWriter.Get(stream)) { + writer.WriteBits((byte)1, 1); writer.WriteInt32(k_DummyValue); } @@ -60,6 +61,7 @@ public void WriteField(Stream stream) { using (var writer = PooledNetworkWriter.Get(stream)) { + writer.WriteBits((byte)1, 1); writer.WriteInt32(k_DummyValue); } @@ -70,6 +72,7 @@ public void ReadField(Stream stream) { using (var reader = PooledNetworkReader.Get(stream)) { + reader.ReadBits(1); Assert.AreEqual(k_DummyValue, reader.ReadInt32()); } @@ -80,6 +83,7 @@ public void ReadDelta(Stream stream, bool keepDirtyDelta) { using (var reader = PooledNetworkReader.Get(stream)) { + reader.ReadBits(1); Assert.AreEqual(k_DummyValue, reader.ReadInt32()); } From a8df0d1cc0d511369ade0e395c8865b6030ea446 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Mon, 9 Aug 2021 14:51:44 -0500 Subject: [PATCH 4/4] fix Just fixing the namespace issues. --- .../Tests/Runtime/NetworkVarBufferCopyTest.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs index 387d780940..42ddc12774 100644 --- a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs +++ b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVarBufferCopyTest.cs @@ -1,12 +1,9 @@ -using System.Collections; +using System.Collections; using System.IO; -using MLAPI.NetworkVariable; -using MLAPI.Serialization.Pooled; -using MLAPI.Transports; using NUnit.Framework; using UnityEngine.TestTools; -namespace MLAPI.RuntimeTests +namespace Unity.Netcode.RuntimeTests { public class NetworkVarBufferCopyTest : BaseMultiInstanceTest { @@ -20,7 +17,7 @@ public class DummyNetVar : INetworkVariable public bool Dirty = true; public string Name { get; internal set; } - + public NetworkChannel GetChannel() { return NetworkChannel.NetworkVariable; @@ -132,7 +129,7 @@ public IEnumerator TestEntireBufferIsCopiedOnNetworkVariableDelta() var serverComponent = (serverSideClientPlayer).GetComponent(); var clientComponent = (clientSideClientPlayer).GetComponent(); - + var waitResult = new MultiInstanceHelpers.CoroutineResultWrapper(); yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForCondition( @@ -150,4 +147,4 @@ public IEnumerator TestEntireBufferIsCopiedOnNetworkVariableDelta() Assert.True(clientComponent.NetVar.DeltaRead); } } -} \ No newline at end of file +}