Merge pull request #2941 from LogicParrot/chunkBug2

[Attempt 2] Fixed cChunk::m_Entities corruption upon world travel

src/Chunk.cpp

@@ -603,6 +603,7 @@ void cChunk::SpawnMobs(cMobSpawner & a_MobSpawner)
 
 void cChunk::Tick(std::chrono::milliseconds a_Dt)
 {
+	m_IsInTick = true;
 	BroadcastPendingBlockChanges();
 
 	CheckBlocks();
@@ -637,7 +638,7 @@ void cChunk::Tick(std::chrono::milliseconds a_Dt)
 		else if ((*itr)->IsWorldTravellingFrom(m_World))
 		{
 			// Remove all entities that are travelling to another world
-			LOGD("Removing entity from [%d, %d] that's travelling between worlds.", m_PosX, m_PosZ);
+			LOGD("Removing entity from [%d, %d] that's travelling between worlds. (Scheduled)", m_PosX, m_PosZ);
 			MarkDirty();
 			(*itr)->SetWorldTravellingFrom(nullptr);
 			itr = m_Entities.erase(itr);
@@ -659,6 +660,7 @@ void cChunk::Tick(std::chrono::milliseconds a_Dt)
 	}  // for itr - m_Entitites[]
 
 	ApplyWeatherToTop();
+	m_IsInTick = false;
 }
 
 
@@ -1910,6 +1912,31 @@ void cChunk::RemoveEntity(cEntity * a_Entity)
 
 
 
+void cChunk::SafeRemoveEntity(cEntity * a_Entity)
+{
+	if (!m_IsInTick)
+	{
+		LOGD("Removing entity from [%d, %d] that's travelling between worlds. (immediate)", m_PosX, m_PosZ);
+		// If we're not in a tick, just remove it.
+		m_Entities.remove(a_Entity);
+	}
+	else
+	{
+		// If we are in a tick, we don't want to invalidate the iterator, so we schedule the removal. Removal will be done in cChunk::tick()
+		a_Entity->SetWorldTravellingFrom(GetWorld());
+	}
+
+	// Mark as dirty if it was a server-generated entity:
+	if (!a_Entity->IsPlayer())
+	{
+		MarkDirty();
+	}
+}
+
+
+
+
+
 bool cChunk::HasEntity(UInt32 a_EntityID)
 {
 	for (cEntityList::const_iterator itr = m_Entities.begin(), end = m_Entities.end(); itr != end; ++itr)

src/Chunk.h

@@ -260,6 +260,9 @@ public:
 
 	void AddEntity(cEntity * a_Entity);
 	void RemoveEntity(cEntity * a_Entity);
+	/** RemoveEntity is dangerous if the chunk is inside the tick() method because it invalidates the iterator.
+	This will safely remove an entity. */
+	void SafeRemoveEntity(cEntity * a_Entity);
 	bool HasEntity(UInt32 a_EntityID);
 
 	/** Calls the callback for each entity; returns true if all entities processed, false if the callback aborted by returning true */
@@ -502,7 +505,7 @@ private:
 
 	/** If the chunk fails to load, should it be queued in the generator or reset back to invalid? */
 	bool m_ShouldGenerateIfLoadFailed;
-
+	bool m_IsInTick;       // True if the chunk is executing the tick() method.
 	bool m_IsLightValid;   // True if the blocklight and skylight are calculated
 	bool m_IsDirty;        // True if the chunk has changed since it was last saved
 	bool m_IsSaving;       // True if the chunk is being saved

src/Entities/Entity.cpp

@@ -1499,8 +1499,17 @@ bool cEntity::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 		return false;
 	}
 
-	// Remove all links to the old world
-	SetWorldTravellingFrom(GetWorld());  // cChunk::Tick() handles entity removal
+	// Remove entity from chunk
+	if (!GetWorld()->DoWithChunk(GetChunkX(), GetChunkZ(), [this](cChunk & a_Chunk) -> bool
+	{
+		a_Chunk.SafeRemoveEntity(this);
+		return true;
+	}))
+	{
+		LOGD("Entity Teleportation failed! Didn't find the source chunk!\n");
+		return false;
+	}
+
 	GetWorld()->BroadcastDestroyEntity(*this);
 
 	SetPosition(a_NewPosition);

src/Entities/Player.cpp

@@ -1695,6 +1695,20 @@ bool cPlayer::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 		return false;
 	}
 
+	// Remove player from chunk
+	if (!GetWorld()->DoWithChunk(GetChunkX(), GetChunkZ(), [this](cChunk & a_Chunk) -> bool
+	{
+		a_Chunk.SafeRemoveEntity(this);
+		return true;
+	}))
+	{
+		LOGD("Entity Teleportation failed! Didn't find the source chunk!\n");
+		return false;
+	}
+
+	// Remove player from world
+	GetWorld()->RemovePlayer(this, false);
+
 	// Send the respawn packet:
 	if (a_ShouldSendRespawn && (m_ClientHandle != nullptr))
 	{
@@ -1704,10 +1718,6 @@ bool cPlayer::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 	// Broadcast for other people that the player is gone.
 	GetWorld()->BroadcastDestroyEntity(*this);
 
-	// Remove player from the old world
-	SetWorldTravellingFrom(GetWorld());  // cChunk handles entity removal
-	GetWorld()->RemovePlayer(this, false);
-
 	SetPosition(a_NewPosition);
 
 	// Queue adding player to the new world, including all the necessary adjustments to the object