From 2687f2df30210ada485c28c98e52db08d460d5a3 Mon Sep 17 00:00:00 2001 From: Tiger Wang Date: Sun, 28 Mar 2021 14:44:20 +0100 Subject: [PATCH] Fix chunk block changes being sent out of order (#5169) * Flush out all pending, buffered changes at the end of each tick, after every chunk is ticked. This makes every block update client-side in unison, instead of unlucky ones only being sent 1 tick later. * Re-add buffer for outgoing network data; IOCP async WSASend has higher overhead than expected... Fixes regression introduced in 054a89dd9 --- src/Chunk.cpp | 88 ++++++++++++++++++++++++-------------------- src/Chunk.h | 6 +-- src/ChunkMap.cpp | 8 ++++ src/ClientHandle.cpp | 31 ++++++++++++---- src/ClientHandle.h | 9 ++++- 5 files changed, 91 insertions(+), 51 deletions(-) diff --git a/src/Chunk.cpp b/src/Chunk.cpp index 4aa28d257..5d54f3c16 100644 --- a/src/Chunk.cpp +++ b/src/Chunk.cpp @@ -116,6 +116,55 @@ cChunk::~cChunk() +void cChunk::BroadcastPendingChanges(void) +{ + if (const auto PendingBlocksCount = m_PendingSendBlocks.size(); PendingBlocksCount >= 10240) + { + // Resend the full chunk: + for (const auto ClientHandle : m_LoadedByClient) + { + m_World->ForceSendChunkTo(m_PosX, m_PosZ, cChunkSender::Priority::Medium, ClientHandle); + } + } + else if (PendingBlocksCount == 0) + { + // Only send block entity changes: + for (const auto ClientHandle : m_LoadedByClient) + { + for (const auto BlockEntity : m_PendingSendBlockEntities) + { + BlockEntity->SendTo(*ClientHandle); + } + } + } + else + { + // Send block and block entity changes: + for (const auto ClientHandle : m_LoadedByClient) + { + ClientHandle->SendBlockChanges(m_PosX, m_PosZ, m_PendingSendBlocks); + + for (const auto BlockEntity : m_PendingSendBlockEntities) + { + BlockEntity->SendTo(*ClientHandle); + } + } + } + + // Flush out all buffered data: + for (const auto ClientHandle : m_LoadedByClient) + { + ClientHandle->ProcessProtocolOut(); + } + + m_PendingSendBlocks.clear(); + m_PendingSendBlockEntities.clear(); +} + + + + + void cChunk::SetPresence(cChunk::ePresence a_Presence) { m_Presence = a_Presence; @@ -707,9 +756,6 @@ void cChunk::Tick(std::chrono::milliseconds a_Dt) // Check blocks after everything else to apply at least one round of queued ticks (i.e. cBlockHandler::Check) this tick: CheckBlocks(); - - // Finally, tell the client about all block changes: - BroadcastPendingBlockChanges(); } @@ -771,42 +817,6 @@ void cChunk::MoveEntityToNewChunk(OwnedEntity a_Entity) -void cChunk::BroadcastPendingBlockChanges(void) -{ - if (const auto PendingBlocksCount = m_PendingSendBlocks.size(); PendingBlocksCount >= 10240) - { - // Resend the full chunk: - for (const auto ClientHandle : m_LoadedByClient) - { - m_World->ForceSendChunkTo(m_PosX, m_PosZ, cChunkSender::Priority::Medium, ClientHandle); - } - } - else if (PendingBlocksCount != 0) - { - // Send block changes: - for (const auto ClientHandle : m_LoadedByClient) - { - ClientHandle->SendBlockChanges(m_PosX, m_PosZ, m_PendingSendBlocks); - } - } - - // Send block entity changes: - for (const auto Entity : m_PendingSendBlockEntities) - { - for (const auto ClientHandle : m_LoadedByClient) - { - Entity->SendTo(*ClientHandle); - } - } - - m_PendingSendBlocks.clear(); - m_PendingSendBlockEntities.clear(); -} - - - - - void cChunk::CheckBlocks() { cChunkInterface ChunkInterface(m_World->GetChunkMap()); diff --git a/src/Chunk.h b/src/Chunk.h index c51794d24..405f5253e 100644 --- a/src/Chunk.h +++ b/src/Chunk.h @@ -53,6 +53,9 @@ public: cChunk(const cChunk & Other) = delete; ~cChunk(); + /** Flushes the pending block (entity) queue, and clients' outgoing data buffers. */ + void BroadcastPendingChanges(void); + /** Returns true iff the chunk block data is valid (loaded / generated) */ bool IsValid(void) const {return (m_Presence == cpPresent); } @@ -546,9 +549,6 @@ private: /** Wakes up each simulator for its specific blocks; through all the blocks in the chunk */ void WakeUpSimulators(void); - /** Sends m_PendingSendBlocks to all clients */ - void BroadcastPendingBlockChanges(void); - /** Checks the block scheduled for checking in m_ToTickBlocks[] */ void CheckBlocks(); diff --git a/src/ChunkMap.cpp b/src/ChunkMap.cpp index 7bdd1c649..669a4f564 100644 --- a/src/ChunkMap.cpp +++ b/src/ChunkMap.cpp @@ -1339,10 +1339,18 @@ void cChunkMap::SpawnMobs(cMobSpawner & a_MobSpawner) void cChunkMap::Tick(std::chrono::milliseconds a_Dt) { cCSLock Lock(m_CSChunks); + + // Do the magic of updating the world: for (auto & Chunk : m_Chunks) { Chunk.second.Tick(a_Dt); } + + // Finally, only after all chunks are ticked, tell the client about all aggregated changes: + for (auto & Chunk : m_Chunks) + { + Chunk.second.BroadcastPendingChanges(); + } } diff --git a/src/ClientHandle.cpp b/src/ClientHandle.cpp index 26db7c2df..20b03e190 100644 --- a/src/ClientHandle.cpp +++ b/src/ClientHandle.cpp @@ -231,6 +231,27 @@ bool cClientHandle::IsUUIDOnline(const cUUID & a_UUID) +void cClientHandle::ProcessProtocolOut() +{ + decltype(m_OutgoingData) OutgoingData; + { + cCSLock Lock(m_CSOutgoingData); + std::swap(OutgoingData, m_OutgoingData); + } + + // Due to cTCPLink's design of holding a strong pointer to ourself, we need to explicitly reset m_Link. + // This means we need to check it's not nullptr before trying to send, but also capture the link, + // to prevent it being reset between the null check and the Send: + if (auto Link = m_Link; Link != nullptr) + { + Link->Send(OutgoingData.data(), OutgoingData.size()); + } +} + + + + + void cClientHandle::Kick(const AString & a_Reason) { if (m_State >= csAuthenticating) // Don't log pings @@ -1899,14 +1920,8 @@ void cClientHandle::SendData(const ContiguousByteBufferView a_Data) return; } - // Due to cTCPLink's design of holding a strong pointer to ourself, we need to explicitly reset m_Link. - // This means we need to check it's not nullptr before trying to send, but also capture the link, - // to prevent it being reset between the null check and the Send: - if (auto Link = m_Link; Link != nullptr) - { - cCSLock Lock(m_CSOutgoingData); - Link->Send(a_Data.data(), a_Data.size()); - } + cCSLock Lock(m_CSOutgoingData); + m_OutgoingData += a_Data; } diff --git a/src/ClientHandle.h b/src/ClientHandle.h index 983cb039d..d18b66280 100644 --- a/src/ClientHandle.h +++ b/src/ClientHandle.h @@ -105,6 +105,9 @@ public: // tolua_export We use Version-3 UUIDs for offline UUIDs, online UUIDs are Version-4, thus we can tell them apart. */ static bool IsUUIDOnline(const cUUID & a_UUID); // Exported in ManualBindings.cpp + /** Flushes all buffered outgoing data to the network. */ + void ProcessProtocolOut(); + /** Formats the type of message with the proper color and prefix for sending to the client. */ static AString FormatMessageType(bool ShouldAppendChatPrefixes, eMessageType a_ChatPrefix, const AString & a_AdditionalData); @@ -443,13 +446,17 @@ private: /** Protects m_IncomingData against multithreaded access. */ cCriticalSection m_CSIncomingData; - /** Queue for the incoming data received on the link until it is processed in Tick(). + /** Queue for the incoming data received on the link until it is processed in ProcessProtocolIn(). Protected by m_CSIncomingData. */ ContiguousByteBuffer m_IncomingData; /** Protects m_OutgoingData against multithreaded access. */ cCriticalSection m_CSOutgoingData; + /** Buffer for storing outgoing data from any thread; will get sent in ProcessProtocolOut() at the end of each tick. + Protected by m_CSOutgoingData. */ + ContiguousByteBuffer m_OutgoingData; + /** A pointer to a World-owned player object, created in FinishAuthenticate when authentication succeeds. The player should only be accessed from the tick thread of the World that owns him. After the player object is handed off to the World, lifetime is managed automatically, guaranteed to outlast this client handle.