diff --git a/Assets/Mirror/Core/Batching/Batcher.cs b/Assets/Mirror/Core/Batching/Batcher.cs index 41f467dbf..6f7cad960 100644 --- a/Assets/Mirror/Core/Batching/Batcher.cs +++ b/Assets/Mirror/Core/Batching/Batcher.cs @@ -49,8 +49,16 @@ public static int MaxMessageOverhead(int messageSize) => // => best to build batches on the fly. readonly Queue batches = new Queue(); - // current batch in progress + // current batch in progress. + // we also store the timestamp to ensure we don't add a message from another frame, + // as this would introduce subtle jitter! + // + // for example: + // - a batch is started at t=1, another message is added at t=2 and then it's flushed + // - NetworkTransform uses remoteTimestamp which is t=1 + // - snapshot interpolation would off by one (or multiple) frames! NetworkWriterPooled batch; + double batchTimestamp; public Batcher(int threshold) { @@ -62,6 +70,32 @@ public Batcher(int threshold) // caller needs to make sure they are within max packet size. public void AddMessage(ArraySegment message, double timeStamp) { + // safety: message timestamp is only written once. + // make sure all messages in this batch are from the same timestamp. + // otherwise it could silently introduce jitter. + // + // this happened before: + // - NetworkEarlyUpdate @ t=1 processes transport messages + // - a handler replies by sending a message + // - a new batch is started @ t=1, timestamp is encoded + // - NetworkLateUpdate @ t=2 decides it's time to broadcast + // - NetworkTransform sends @ t=2 + // - we add to the above batch which already encoded t=1 + // - Client receives the batch which timestamp t=1 + // - NetworkTransform uses remoteTime for interpolation + // remoteTime is the batch timestamp which is t=1 + // - the NetworkTransform message is actually t=2 + // => smooth interpolation would be impossible! + // NT thinks the position was @ t=1 but actually it was @ t=2 ! + // + // the solution: if timestamp changed, enqueue the existing batch + if (batch != null && batchTimestamp != timeStamp) + { + batches.Enqueue(batch); + batch = null; + batchTimestamp = 0; + } + // predict the needed size, which is varint(size) + content int headerSize = Compression.VarUIntSize((ulong)message.Count); int neededSize = headerSize + message.Count; @@ -76,6 +110,7 @@ public void AddMessage(ArraySegment message, double timeStamp) { batches.Enqueue(batch); batch = null; + batchTimestamp = 0; } // initialize a new batch if necessary @@ -89,6 +124,9 @@ public void AddMessage(ArraySegment message, double timeStamp) // -> batches are per-frame, it doesn't matter which message's // timestamp we use. batch.WriteDouble(timeStamp); + + // remember the encoded timestamp, see safety check below. + batchTimestamp = timeStamp; } // add serialization to current batch. even if > threshold. @@ -155,6 +193,7 @@ public void Clear() { NetworkWriterPool.Return(batch); batch = null; + batchTimestamp = 0; } // return all queued batches diff --git a/Assets/Mirror/Tests/Editor/Batching/BatcherTests.cs b/Assets/Mirror/Tests/Editor/Batching/BatcherTests.cs index f609bdc98..59f6c23fe 100644 --- a/Assets/Mirror/Tests/Editor/Batching/BatcherTests.cs +++ b/Assets/Mirror/Tests/Editor/Batching/BatcherTests.cs @@ -56,6 +56,45 @@ public void AddMessage() batcher.AddMessage(new ArraySegment(message), TimeStamp); } + // test to prevent the following issue from ever happening again: + // + // - NetworkEarlyUpdate @ t=1 processes transport messages + // - a handler replies by sending a message + // - a new batch is started @ t=1, timestamp is encoded + // - NetworkLateUpdate @ t=2 decides it's time to broadcast + // - NetworkTransform sends @ t=2 + // - we add to the above batch which already encoded t=1 + // - Client receives the batch which timestamp t=1 + // - NetworkTransform uses remoteTime for interpolation + // remoteTime is the batch timestamp which is t=1 + // - the NetworkTransform message is actually t=2 + // => smooth interpolation would be impossible! + // NT thinks the position was @ t=1 but actually it was @ t=2 ! + [Test] + public void AddMessage_TimestampMismatch() + { + // add message @ t=1 + byte[] message1 = {0x01, 0x01}; + batcher.AddMessage(new ArraySegment(message1), 1); + + // add message @ t=2 + byte[] message2 = {0x02, 0x02}; + batcher.AddMessage(new ArraySegment(message2), 2); + + // call getbatch: this should only contain the message @ t=1 ! + // <> + bool result = batcher.GetBatch(writer); + Assert.That(result, Is.EqualTo(true)); + Assert.That(writer.ToArray().SequenceEqual(MakeBatch(1, message1))); + + // call getbatch: this should now contain the message @ t=2 ! + // <> + writer.Position = 0; + result = batcher.GetBatch(writer); + Assert.That(result, Is.EqualTo(true)); + Assert.That(writer.ToArray().SequenceEqual(MakeBatch(2, message2))); + } + [Test] public void MakeNextBatch_OnlyAcceptsFreshWriter() {