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

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

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

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

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

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.