From 10492fe6cf1b9e51fcff32357b0cd1e9445e43ed Mon Sep 17 00:00:00 2001 From: Jeffrey Rainy Date: Wed, 25 Aug 2021 15:19:30 -0400 Subject: [PATCH 1/3] fix: Addressing latest review comments, making it safer when receiving OOO messages or old messages --- .../Runtime/Core/SnapshotSystem.cs | 49 +++++++++++++------ 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs b/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs index 3c435752d2..6ed636fc09 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs @@ -959,26 +959,45 @@ internal void ReadSnapshot(ulong clientId, Stream snapshotStream) snapshotTick = reader.ReadInt32Packed(); var sequence = reader.ReadUInt16(); - // todo: check we didn't miss any and deal with gaps - - if (m_ClientData[clientId].ReceivedSequenceMask != 0) + if (sequence >= m_ClientData[clientId].LastReceivedSequence) { - // since each bit in ReceivedSequenceMask is relative to the last received sequence - // we need to shift all the bits by the difference in sequence - m_ClientData[clientId].ReceivedSequenceMask <<= - (sequence - m_ClientData[clientId].LastReceivedSequence); - } + if (m_ClientData[clientId].ReceivedSequenceMask != 0) + { + // since each bit in ReceivedSequenceMask is relative to the last received sequence + // we need to shift all the bits by the difference in sequence + var shift = sequence - m_ClientData[clientId].LastReceivedSequence; + if (shift < sizeof(ushort) * 8) + { + m_ClientData[clientId].ReceivedSequenceMask <<= shift; + } + else + { + m_ClientData[clientId].ReceivedSequenceMask = 0; + } + } - if (m_ClientData[clientId].LastReceivedSequence != 0) + if (m_ClientData[clientId].LastReceivedSequence != 0) + { + // because the bit we're adding for the previous ReceivedSequenceMask + // was implicit, it needs to be shift by one less + var shift = (ushort)(sequence - 1 - m_ClientData[clientId].LastReceivedSequence); + if (shift < sizeof(ushort) * 8) + { + m_ClientData[clientId].ReceivedSequenceMask |= (ushort) (1 << shift); + } + } + + m_ClientData[clientId].LastReceivedSequence = sequence; + } + else { - // because the bit we're adding for the previous ReceivedSequenceMask - // was implicit, it needs to be shift by one less - m_ClientData[clientId].ReceivedSequenceMask += - (ushort)(1 << (ushort)((sequence - 1) - m_ClientData[clientId].LastReceivedSequence)); + // todo: Missing: dealing with out-of-order message acknowledgments + // we should set m_ClientData[clientId].ReceivedSequenceMask accordingly + // testing this will require a way to reorder SnapshotMessages, which we lack at the moment + // + // without this, we incur extra retransmit, not a catastrophic failure } - m_ClientData[clientId].LastReceivedSequence = sequence; - var sentinel = reader.ReadUInt16(); if (sentinel != SentinelBefore) { From 9c7d4253e9f5f1bdad658ddeed8da9ee403b969c Mon Sep 17 00:00:00 2001 From: Jeffrey Rainy Date: Wed, 25 Aug 2021 15:22:18 -0400 Subject: [PATCH 2/3] fix: Addressing latest review comments, whitespace --- com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs b/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs index 6ed636fc09..a89fbacf2f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs @@ -983,7 +983,7 @@ internal void ReadSnapshot(ulong clientId, Stream snapshotStream) var shift = (ushort)(sequence - 1 - m_ClientData[clientId].LastReceivedSequence); if (shift < sizeof(ushort) * 8) { - m_ClientData[clientId].ReceivedSequenceMask |= (ushort) (1 << shift); + m_ClientData[clientId].ReceivedSequenceMask |= (ushort)(1 << shift); } } From 2fff30affe5fb85c3521c40381123566af4dffd6 Mon Sep 17 00:00:00 2001 From: Jeffrey Rainy Date: Thu, 26 Aug 2021 13:18:54 -0400 Subject: [PATCH 3/3] fix: removing extraneous cast to ushort --- com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs b/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs index a89fbacf2f..11ea94a576 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs @@ -980,7 +980,7 @@ internal void ReadSnapshot(ulong clientId, Stream snapshotStream) { // because the bit we're adding for the previous ReceivedSequenceMask // was implicit, it needs to be shift by one less - var shift = (ushort)(sequence - 1 - m_ClientData[clientId].LastReceivedSequence); + var shift = sequence - 1 - m_ClientData[clientId].LastReceivedSequence; if (shift < sizeof(ushort) * 8) { m_ClientData[clientId].ReceivedSequenceMask |= (ushort)(1 << shift);