Merge pull request #2986 from LogicParrot/entityDestroy_approach4

[Queue approach] Fix entity destruction in unticking chunks

Server/Plugins/APIDump/APIDesc.lua

@@ -902,7 +902,8 @@ local Hash = cCryptoHash.sha1HexString("DataToHash")
 				IsA = { Params = "ClassName", Return = "bool", Notes = "Returns true if the entity class is a descendant of the specified class name, or the specified class itself" },
 				IsBoat = { Params = "", Return = "bool", Notes = "Returns true if the entity is a {{cBoat|boat}}." },
 				IsCrouched = { Params = "", Return = "bool", Notes = "Returns true if the entity is crouched. Always false for entities that don't support crouching." },
-				IsDestroyed = { Params = "", Return = "bool", Notes = "Returns true if the entity has been destroyed and is awaiting removal from the internal structures." },
+				IsDestroyed = { Params = "", Return = "bool", Notes = "(<b>DEPRECATED</b>) Please use cEntity:IsTicking()." },
+				IsTicking = { Params = "", Return = "bool", Notes = "Returns true if the entity is valid and ticking. Returns false if the entity is not ticking and is about to leave its current world either via teleportation or destruction. If this returns false, you must stop using the cEntity pointer you have." },
 				IsEnderCrystal = { Params = "", Return = "bool", Notes = "Returns true if the entity is an ender crystal." },
 				IsExpOrb = { Params = "", Return = "bool", Notes = "Returns true if the entity represents an experience orb" },
 				IsFallingBlock = { Params = "", Return = "bool", Notes = "Returns true if the entity represents a {{cFallingBlock}} entity." },
@@ -2958,7 +2959,7 @@ end
 				StringToDamageType = {Params = "string", Return = "{{Globals#DamageType|DamageType}}", Notes = "Converts a string representation to a {{Globals#DamageType|DamageType}} enumerated value."},
 				StringToDimension = {Params = "string", Return = "{{Globals#WorldDimension|Dimension}}", Notes = "Converts a string representation to a {{Globals#WorldDimension|Dimension}} enumerated value"},
 				StringToItem = {Params = "string, {{cItem|cItem}}", Return = "bool", Notes = "Parses the given string and sets the item; returns true if successful"},
-				StringToMobType = {Params = "string", Return = "{{Globals#MobType|MobType}}", Notes = "<b>DEPRECATED!</b> Please use cMonster:StringToMobType(). Converts a string representation to a {{Globals#MobType|MobType}} enumerated value"},
+				StringToMobType = {Params = "string", Return = "{{Globals#MobType|MobType}}", Notes = "(<b>DEPRECATED!</b>) Please use cMonster:StringToMobType(). Converts a string representation to a {{Globals#MobType|MobType}} enumerated value"},
 				StripColorCodes = {Params = "string", Return = "string", Notes = "Removes all control codes used by MC for colors and styles"},
 				TrimString = {Params = "string", Return = "string", Notes = "Trims whitespace at both ends of the string"},
 				md5 = {Params = "string", Return = "string", Notes = "<b>OBSOLETE</b>, use the {{cCryptoHash}} functions instead.<br>Converts a string to a raw binary md5 hash."},

src/BlockEntities/HopperEntity.cpp

@@ -195,7 +195,7 @@ bool cHopperEntity::MovePickupsIn(cChunk & a_Chunk, Int64 a_CurrentTick)
 		{
 			ASSERT(a_Entity != nullptr);
 
-			if (!a_Entity->IsPickup() || a_Entity->IsDestroyed())
+			if (!a_Entity->IsPickup())
 			{
 				return false;
 			}

src/Chunk.cpp

@@ -139,7 +139,8 @@ cChunk::~cChunk()
 	{
 		if (!(*itr)->IsPlayer())
 		{
-			(*itr)->Destroy(false);
+			// Scheduling a normal destruction is neither possible (Since this chunk will be gone till the schedule occurs) nor necessary.
+			(*itr)->DestroyNoScheduling(false);  // No point in broadcasting in an unloading chunk. Chunks unload when no one is nearby.
 			delete *itr;
 		}
 	}
@@ -621,37 +622,44 @@ void cChunk::Tick(std::chrono::milliseconds a_Dt)
 
 	for (cEntityList::iterator itr = m_Entities.begin(); itr != m_Entities.end();)
 	{
+		// Do not tick mobs that are detached from the world. They're either scheduled for teleportation or for removal.
+		if (!(*itr)->IsTicking())
+		{
+			++itr;
+			continue;
+		}
+
 		if (!((*itr)->IsMob()))  // Mobs are ticked inside cWorld::TickMobs() (as we don't have to tick them if they are far away from players)
 		{
 			// Tick all entities in this chunk (except mobs):
+			ASSERT((*itr)->GetParentChunk() == this);
 			(*itr)->Tick(a_Dt, *this);
+			ASSERT((*itr)->GetParentChunk() == this);
 		}
 
-		if ((*itr)->IsDestroyed())  // Remove all entities that were scheduled for removal:
+		// Do not move mobs that are detached from the world to neighbors. They're either scheduled for teleportation or for removal.
+		if (!(*itr)->IsTicking())
 		{
-			LOGD("Destroying entity #%i (%s)", (*itr)->GetUniqueID(), (*itr)->GetClass());
+			++itr;
-			MarkDirty();
+			continue;
-			cEntity * ToDelete = *itr;
-			itr = m_Entities.erase(itr);
-			delete ToDelete;
 		}
-		else if ((*itr)->IsWorldTravellingFrom(m_World))
+
-		{
+		// Because the schedulded destruction is going to look for them in this chunk. See cEntity::destroy.
-			// Remove all entities that are travelling to another world
+		if ((((*itr)->GetChunkX() != m_PosX) ||
-			LOGD("Removing entity from [%d, %d] that's travelling between worlds. (Scheduled)", m_PosX, m_PosZ);
+			((*itr)->GetChunkZ() != m_PosZ))
-			MarkDirty();
-			(*itr)->SetWorldTravellingFrom(nullptr);
-			itr = m_Entities.erase(itr);
-		}
-		else if (
-			((*itr)->GetChunkX() != m_PosX) ||
-			((*itr)->GetChunkZ() != m_PosZ)
 		)
 		{
+			// This block is very similar to RemoveEntity, except it uses an iterator to avoid scanning the whole m_Entities
 			// The entity moved out of the chunk, move it to the neighbor
-			MarkDirty();
+
+			(*itr)->SetParentChunk(nullptr);
 			MoveEntityToNewChunk(*itr);
 			itr = m_Entities.erase(itr);
+			// Mark as dirty if it was a server-generated entity:
+			if (!(*itr)->IsPlayer())
+			{
+				MarkDirty();
+			}
 		}
 		else
 		{
@@ -1891,6 +1899,8 @@ void cChunk::AddEntity(cEntity * a_Entity)
 	ASSERT(std::find(m_Entities.begin(), m_Entities.end(), a_Entity) == m_Entities.end());  // Not there already
 
 	m_Entities.push_back(a_Entity);
+	ASSERT(a_Entity->GetParentChunk() == nullptr);
+	a_Entity->SetParentChunk(this);
 }
 
 
@@ -1899,33 +1909,9 @@ void cChunk::AddEntity(cEntity * a_Entity)
 
 void cChunk::RemoveEntity(cEntity * a_Entity)
 {
+	ASSERT(a_Entity->GetParentChunk() == this);
+	a_Entity->SetParentChunk(nullptr);
 	m_Entities.remove(a_Entity);
-
-	// Mark as dirty if it was a server-generated entity:
-	if (!a_Entity->IsPlayer())
-	{
-		MarkDirty();
-	}
-}
-
-
-
-
-
-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())
 	{
@@ -1959,7 +1945,7 @@ bool cChunk::ForEachEntity(cEntityCallback & a_Callback)
 	for (cEntityList::iterator itr = m_Entities.begin(), itr2 = itr; itr != m_Entities.end(); itr = itr2)
 	{
 		++itr2;
-		if ((*itr)->IsDestroyed())
+		if (!(*itr)->IsTicking())
 		{
 			continue;
 		}
@@ -1981,7 +1967,7 @@ bool cChunk::ForEachEntityInBox(const cBoundingBox & a_Box, cEntityCallback & a_
 	for (cEntityList::iterator itr = m_Entities.begin(), itr2 = itr; itr != m_Entities.end(); itr = itr2)
 	{
 		++itr2;
-		if ((*itr)->IsDestroyed())
+		if (!(*itr)->IsTicking())
 		{
 			continue;
 		}
@@ -2008,7 +1994,7 @@ bool cChunk::DoWithEntityByID(UInt32 a_EntityID, cEntityCallback & a_Callback, b
 	// The entity list is locked by the parent chunkmap's CS
 	for (cEntityList::iterator itr = m_Entities.begin(), end = m_Entities.end(); itr != end; ++itr)
 	{
-		if (((*itr)->GetUniqueID() == a_EntityID) && (!(*itr)->IsDestroyed()))
+		if (((*itr)->GetUniqueID() == a_EntityID) && ((*itr)->IsTicking()))
 		{
 			a_CallbackResult = a_Callback.Item(*itr);
 			return true;

src/Chunk.h

@@ -260,9 +260,6 @@ 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 */

src/ClientHandle.cpp

@@ -120,14 +120,15 @@ cClientHandle::~cClientHandle()
 		cWorld * World = m_Player->GetWorld();
 		if (World != nullptr)
 		{
+			RemoveFromAllChunks();
+			m_Player->GetWorld()->RemoveClientFromChunkSender(this);
+			m_Player->SetIsTicking(false);
 			if (!m_Username.empty())
 			{
 				// Send the Offline PlayerList packet:
 				World->BroadcastPlayerListRemovePlayer(*m_Player, this);
 			}
-
+			m_Player->DestroyNoScheduling(true);
-			World->RemovePlayer(m_Player, true);  // Must be called before cPlayer::Destroy() as otherwise cChunk tries to delete the player, and then we do it again
-			m_Player->Destroy();
 		}
 		delete m_Player;
 		m_Player = nullptr;
@@ -164,18 +165,18 @@ void cClientHandle::Destroy(void)
 		m_State = csDestroying;
 	}
 
-	// DEBUG:
 	LOGD("%s: client %p, \"%s\"", __FUNCTION__, static_cast<void *>(this), m_Username.c_str());
 
-	if ((m_Player != nullptr) && (m_Player->GetWorld() != nullptr))
-	{
-		RemoveFromAllChunks();
-		m_Player->GetWorld()->RemoveClientFromChunkSender(this);
-	}
 	if (m_Player != nullptr)
 	{
+		cWorld * World = m_Player->GetWorld();
+		if (World != nullptr)
+		{
+			World->RemovePlayer(m_Player, true);  // TODO this is NOT thread safe.
+		}
 		m_Player->RemoveClientHandle();
 	}
+
 	m_State = csDestroyed;
 }
 
@@ -384,7 +385,7 @@ void cClientHandle::Authenticate(const AString & a_Name, const AString & a_UUID,
 	cRoot::Get()->BroadcastPlayerListsAddPlayer(*m_Player);
 	cRoot::Get()->SendPlayerLists(m_Player);
 
-	m_Player->Initialize(*World);
+	m_Player->SetWorld(World);
 	m_State = csAuthenticated;
 
 	// Query player team
@@ -2020,7 +2021,7 @@ void cClientHandle::ServerTick(float a_Dt)
 
 		// Add the player to the world (start ticking from there):
 		m_State = csDownloadingWorld;
-		m_Player->GetWorld()->AddPlayer(m_Player);
+		m_Player->Initialize(*(m_Player->GetWorld()));
 		return;
 	}
 
@@ -3032,14 +3033,6 @@ void cClientHandle::SocketClosed(void)
 		LOGD("Client %s @ %s disconnected", m_Username.c_str(), m_IPString.c_str());
 		cRoot::Get()->GetPluginManager()->CallHookDisconnect(*this, "Player disconnected");
 	}
-	if ((m_State < csDestroying) && (m_Player != nullptr))
-	{
-		cWorld * World = m_Player->GetWorld();
-		if (World != nullptr)
-		{
-			World->RemovePlayer(m_Player, true);  // Must be called before cPlayer::Destroy() as otherwise cChunk tries to delete the player, and then we do it again
-		}
-	}
 	Destroy();
 }
 

src/Entities/Entity.cpp

@@ -39,8 +39,6 @@ cEntity::cEntity(eEntityType a_EntityType, double a_X, double a_Y, double a_Z, d
 	m_Gravity(-9.81f),
 	m_AirDrag(0.02f),
 	m_LastPosition(a_X, a_Y, a_Z),
-	m_IsInitialized(false),
-	m_WorldTravellingFrom(nullptr),
 	m_EntityType(a_EntityType),
 	m_World(nullptr),
 	m_IsWorldChangeScheduled(false),
@@ -55,6 +53,8 @@ cEntity::cEntity(eEntityType a_EntityType, double a_X, double a_Y, double a_Z, d
 	m_AirLevel(0),
 	m_AirTickTimer(0),
 	m_TicksAlive(0),
+	m_IsTicking(false),
+	m_ParentChunk(nullptr),
 	m_HeadYaw(0.0),
 	m_Rot(0.0, 0.0, 0.0),
 	m_Position(a_X, a_Y, a_Z),
@@ -77,8 +77,10 @@ cEntity::cEntity(eEntityType a_EntityType, double a_X, double a_Y, double a_Z, d
 
 cEntity::~cEntity()
 {
+
 	// Before deleting, the entity needs to have been removed from the world, if ever added
 	ASSERT((m_World == nullptr) || !m_World->HasEntity(m_UniqueID));
+	ASSERT(!IsTicking());
 
 	/*
 	// DEBUG:
@@ -98,12 +100,6 @@ cEntity::~cEntity()
 	{
 		m_Attachee->Detach();
 	}
-
-	if (m_IsInitialized)
-	{
-		LOGWARNING("ERROR: Entity deallocated without being destroyed");
-		ASSERT(!"Entity deallocated without being destroyed or unlinked");
-	}
 }
 
 
@@ -139,7 +135,7 @@ const char * cEntity::GetParentClass(void) const
 
 bool cEntity::Initialize(cWorld & a_World)
 {
-	if (cPluginManager::Get()->CallHookSpawningEntity(a_World, *this) && !IsPlayer())
+	if (cPluginManager::Get()->CallHookSpawningEntity(a_World, *this))
 	{
 		return false;
 	}
@@ -151,9 +147,10 @@ bool cEntity::Initialize(cWorld & a_World)
 	);
 	*/
 
-	m_IsInitialized = true;
+	ASSERT(m_World == nullptr);
-	m_World = &a_World;
+	ASSERT(GetParentChunk() == nullptr);
-	m_World->AddEntity(this);
+	a_World.AddEntity(this);
+	ASSERT(m_World != nullptr);
 
 	cPluginManager::Get()->CallHookSpawnedEntity(a_World, *this);
 
@@ -175,7 +172,6 @@ void cEntity::WrapHeadYaw(void)
 
 
 
-
 void cEntity::WrapRotation(void)
 {
 	m_Rot.x = NormalizeAngleDegrees(m_Rot.x);
@@ -196,19 +192,59 @@ void cEntity::WrapSpeed(void)
 
 
 
+void cEntity::SetParentChunk(cChunk * a_Chunk)
+{
+	m_ParentChunk = a_Chunk;
+}
+
+
+
+
+
+cChunk * cEntity::GetParentChunk()
+{
+	return m_ParentChunk;
+}
+
+
+
+
+
 void cEntity::Destroy(bool a_ShouldBroadcast)
 {
-	if (!m_IsInitialized)
+	ASSERT(IsTicking());
-	{
+	ASSERT(GetParentChunk() != nullptr);
-		return;
+	SetIsTicking(false);
-	}
 
 	if (a_ShouldBroadcast)
 	{
 		m_World->BroadcastDestroyEntity(*this);
 	}
 
-	m_IsInitialized = false;
+	cChunk * ParentChunk = GetParentChunk();
+	m_World->QueueTask([this, ParentChunk](cWorld & a_World)
+	{
+		LOGD("Destroying entity #%i (%s) from chunk (%d, %d)",
+			this->GetUniqueID(), this->GetClass(),
+			ParentChunk->GetPosX(), ParentChunk->GetPosZ()
+		);
+		ParentChunk->RemoveEntity(this);
+		delete this;
+	});
+	Destroyed();
+}
+
+
+
+
+
+void cEntity::DestroyNoScheduling(bool a_ShouldBroadcast)
+{
+	SetIsTicking(false);
+	if (a_ShouldBroadcast)
+	{
+		m_World->BroadcastDestroyEntity(*this);
+	}
 
 	Destroyed();
 }
@@ -217,10 +253,10 @@ void cEntity::Destroy(bool a_ShouldBroadcast)
 
 
 
+
 void cEntity::TakeDamage(cEntity & a_Attacker)
 {
 	int RawDamage = a_Attacker.GetRawDamageAgainst(*this);
-
 	TakeDamage(dtAttack, &a_Attacker, RawDamage, a_Attacker.GetKnockbackAmountAgainst(*this));
 }
 
@@ -833,6 +869,8 @@ void cEntity::SetHealth(int a_Health)
 
 void cEntity::Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
 {
+	ASSERT(IsTicking());
+	ASSERT(GetWorld() != nullptr);
 	m_TicksAlive++;
 
 	if (m_InvulnerableTicks > 0)
@@ -1491,6 +1529,7 @@ bool cEntity::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 {
 	UNUSED(a_ShouldSendRespawn);
 	ASSERT(a_World != nullptr);
+	ASSERT(IsTicking());
 
 	if (GetWorld() == a_World)
 	{
@@ -1505,21 +1544,17 @@ bool cEntity::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 		return false;
 	}
 
-	// Remove entity from chunk
+	// Stop ticking, in preperation for detaching from this world.
-	if (!GetWorld()->DoWithChunk(GetChunkX(), GetChunkZ(), [this](cChunk & a_Chunk) -> bool
+	SetIsTicking(false);
-	{
-		a_Chunk.SafeRemoveEntity(this);
-		return true;
-	}))
-	{
-		LOGD("Entity Teleportation failed! Didn't find the source chunk!\n");
-		return false;
-	}
 
+	// Tell others we are gone
 	GetWorld()->BroadcastDestroyEntity(*this);
 
+	// Set position to the new position
 	SetPosition(a_NewPosition);
 
+	// Stop all mobs from targeting this entity
+	// Stop this entity from targeting other mobs
 	if (this->IsMob())
 	{
 		cMonster * Monster = static_cast<cMonster*>(this);
@@ -1527,14 +1562,21 @@ bool cEntity::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 		Monster->StopEveryoneFromTargetingMe();
 	}
 
-	// Queue add to new world
+	// Queue add to new world and removal from the old one
-	a_World->AddEntity(this);
+	cWorld * OldWorld = GetWorld();
-	cWorld * OldWorld = cRoot::Get()->GetWorld(GetWorld()->GetName());  // Required for the hook HOOK_ENTITY_CHANGED_WORLD
+	cChunk * ParentChunk = GetParentChunk();
-	SetWorld(a_World);
+	SetWorld(a_World);  // Chunks may be streamed before cWorld::AddPlayer() sets the world to the new value
-
+	OldWorld->QueueTask([this, ParentChunk, a_World](cWorld & a_OldWorld)
-	// Entity changed the world, call the hook
+	{
-	cRoot::Get()->GetPluginManager()->CallHookEntityChangedWorld(*this, *OldWorld);
+		LOGD("Warping entity #%i (%s) from world \"%s\" to \"%s\". Source chunk: (%d, %d) ",
-
+			this->GetUniqueID(), this->GetClass(),
+			a_OldWorld.GetName().c_str(), a_World->GetName().c_str(),
+			ParentChunk->GetPosX(), ParentChunk->GetPosZ()
+		);
+		ParentChunk->RemoveEntity(this);
+		a_World->AddEntity(this);
+		cRoot::Get()->GetPluginManager()->CallHookEntityChangedWorld(*this, a_OldWorld);
+	});
 	return true;
 }
 
@@ -1595,6 +1637,16 @@ void cEntity::SetSwimState(cChunk & a_Chunk)
 
 
 
+void cEntity::SetIsTicking(bool a_IsTicking)
+{
+	m_IsTicking = a_IsTicking;
+	ASSERT(!(m_IsTicking && (m_ParentChunk == nullptr)));  // We shouldn't be ticking if we have no parent chunk
+}
+
+
+
+
+
 void cEntity::DoSetSpeed(double a_SpeedX, double a_SpeedY, double a_SpeedZ)
 {
 	m_Speed.Set(a_SpeedX, a_SpeedY, a_SpeedZ);
@@ -2069,6 +2121,12 @@ void cEntity::SteerVehicle(float a_Forward, float a_Sideways)
 
 
 
+bool cEntity::IsTicking(void) const
+{
+	ASSERT(!(m_IsTicking && (m_ParentChunk == nullptr)));  // We shouldn't be ticking if we have no parent chunk
+	return m_IsTicking;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Get look vector (this is NOT a rotation!)
 Vector3d cEntity::GetLookVector(void) const

src/Entities/Entity.h

@@ -252,9 +252,16 @@ public:
 	void SteerVehicle(float a_Forward, float a_Sideways);
 
 	inline UInt32 GetUniqueID(void) const { return m_UniqueID; }
-	inline bool IsDestroyed(void) const { return !m_IsInitialized; }
 
-	/** Schedules the entity for destroying; if a_ShouldBroadcast is set to true, broadcasts the DestroyEntity packet */
+	/** Deprecated. Use IsTicking instead. */
+	inline bool IsDestroyed() const {return !IsTicking();}
+
+	/** Returns true if the entity is valid and ticking. Returns false if the entity is not ticking and is about to leave
+	its current world either via teleportation or destruction.
+	If this returns false, you must stop using the cEntity pointer you have. */
+	bool IsTicking(void) const;
+
+	/** Destroys the entity and schedules it for memory freeing; if a_ShouldBroadcast is set to true, broadcasts the DestroyEntity packet */
 	void Destroy(bool a_ShouldBroadcast = true);
 
 	/** Makes this pawn take damage from an attack by a_Attacker. Damage values are calculated automatically and DoTakeDamage() called */
@@ -285,6 +292,9 @@ public:
 
 	// tolua_end
 
+	/** Destroy the entity without scheduling memory freeing. This should only be used by cChunk or cClientHandle for internal memory management. */
+	void DestroyNoScheduling(bool a_ShouldBroadcast);
+
 	/** Makes this entity take damage specified in the a_TDI.
 	The TDI is sent through plugins first, then applied.
 	If it returns false, the entity hasn't receive any damage. */
@@ -408,12 +418,6 @@ public:
 
 	virtual bool DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d a_NewPosition);
 
-	/** Returns if the entity is travelling away from a specified world */
-	bool IsWorldTravellingFrom(cWorld * a_World) const { return (m_WorldTravellingFrom == a_World); }
-
-	/** Sets the world the entity will be leaving */
-	void SetWorldTravellingFrom(cWorld * a_World) { m_WorldTravellingFrom = a_World; }
-
 	/** Updates clients of changes in the entity. */
 	virtual void BroadcastMovementUpdate(const cClientHandle * a_Exclude = nullptr);
 
@@ -481,6 +485,16 @@ public:
 	/** Sets the internal world pointer to a new cWorld, doesn't update anything else. */
 	void SetWorld(cWorld * a_World) { m_World = a_World; }
 
+	/** Sets the parent chunk, which is the chunk responsible for ticking this entity.
+	Only cChunk::AddEntity and cChunk::RemoveEntity cChunk::~cChunk should ever call this. */
+	void SetParentChunk(cChunk * a_Chunk);
+
+	/** Returns the chunk responsible for ticking this entity. */
+	cChunk * GetParentChunk();
+
+	/** Set the entity's status to either ticking or not ticking. */
+	void SetIsTicking(bool a_IsTicking);
+
 protected:
 	/** Structure storing the portal delay timer and cooldown boolean */
 	struct sPortalCooldownData
@@ -538,14 +552,6 @@ protected:
 
 	Vector3d m_LastPosition;
 
-	/** True when entity is initialised (Initialize()) and false when destroyed pending deletion (Destroy()) */
-	bool m_IsInitialized;
-
-	/** World entity is travelling from (such as when using portals).
-	Set to a valid world pointer by MoveToWorld; reset to nullptr when the entity is removed from the old world.
-	Can't be a simple boolean as context switches between worlds may leave the new chunk processing (and therefore immediately removing) the entity before the old chunk could remove it. */
-	cWorld * m_WorldTravellingFrom;
-
 	eEntityType m_EntityType;
 
 	cWorld * m_World;
@@ -606,6 +612,13 @@ protected:
 	virtual void SetSwimState(cChunk & a_Chunk);
 
 private:
+
+	/** Whether the entity is ticking or not. If not, it is scheduled for removal or world-teleportation. */
+	bool m_IsTicking;
+
+	/** The chunk which is responsible for ticking this entity. */
+	cChunk * m_ParentChunk;
+
 	/** Measured in degrees, [-180, +180) */
 	double   m_HeadYaw;
 

src/Entities/Minecart.cpp

@@ -116,10 +116,7 @@ void cMinecart::SpawnOn(cClientHandle & a_ClientHandle)
 
 void cMinecart::HandlePhysics(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
 {
-	if (IsDestroyed())  // Mainly to stop detector rails triggering again after minecart is dead
+	ASSERT(IsTicking());
-	{
-		return;
-	}
 
 	int PosY = POSY_TOINT;
 	if ((PosY <= 0) || (PosY >= cChunkDef::Height))

src/Entities/Pawn.cpp

@@ -221,7 +221,7 @@ void cPawn::ClearEntityEffects()
 
 void cPawn::NoLongerTargetingMe(cMonster * a_Monster)
 {
-	ASSERT(!IsDestroyed());  // Our destroy override is supposed to clear all targets before we're destroyed.
+	ASSERT(IsTicking());  // Our destroy override is supposed to clear all targets before we're destroyed.
 	for (auto i = m_TargetingMe.begin(); i != m_TargetingMe.end(); ++i)
 	{
 		cMonster * Monster = *i;
@@ -241,7 +241,7 @@ void cPawn::NoLongerTargetingMe(cMonster * a_Monster)
 
 void cPawn::TargetingMe(cMonster * a_Monster)
 {
-	ASSERT(!IsDestroyed());
+	ASSERT(IsTicking());
 	ASSERT(m_TargetingMe.size() < 10000);
 	ASSERT(a_Monster->GetTarget() == this);
 	m_TargetingMe.push_back(a_Monster);

src/Entities/Pickup.cpp

@@ -30,11 +30,13 @@ public:
 
 	virtual bool Item(cEntity * a_Entity) override
 	{
-		if (!a_Entity->IsPickup() || (a_Entity->GetUniqueID() <= m_Pickup->GetUniqueID()) || a_Entity->IsDestroyed())
+		ASSERT(a_Entity->IsTicking());
+		if (!a_Entity->IsPickup() || (a_Entity->GetUniqueID() <= m_Pickup->GetUniqueID()))
 		{
 			return false;
 		}
 
+
 		Vector3d EntityPos = a_Entity->GetPosition();
 		double Distance = (EntityPos - m_Position).Length();
 
@@ -152,7 +154,7 @@ void cPickup::Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
 			}
 
 			// Try to combine the pickup with adjacent same-item pickups:
-			if (!IsDestroyed() && (m_Item.m_ItemCount < m_Item.GetMaxStackSize()))  // Don't combine if already full
+			if ((m_Item.m_ItemCount < m_Item.GetMaxStackSize()))  // Don't combine if already full
 			{
 				// By using a_Chunk's ForEachEntity() instead of cWorld's, pickups don't combine across chunk boundaries.
 				// That is a small price to pay for not having to traverse the entire world for each entity.

src/Entities/Player.cpp

@@ -146,6 +146,25 @@ cPlayer::cPlayer(cClientHandlePtr a_Client, const AString & a_PlayerName) :
 
 
 
+bool cPlayer::Initialize(cWorld & a_World)
+{
+	UNUSED(a_World);
+	ASSERT(GetWorld() != nullptr);
+	ASSERT(GetParentChunk() == nullptr);
+	GetWorld()->AddPlayer(this);
+
+	cPluginManager::Get()->CallHookSpawnedEntity(*GetWorld(), *this);
+
+	// Spawn the entity on the clients:
+	GetWorld()->BroadcastSpawnEntity(*this);
+
+	return true;
+}
+
+
+
+
+
 cPlayer::~cPlayer(void)
 {
 	if (!cRoot::Get()->GetPluginManager()->CallHookPlayerDestroyed(*this))
@@ -1682,6 +1701,8 @@ void cPlayer::FreezeInternal(const Vector3d & a_Location, bool a_ManuallyFrozen)
 bool cPlayer::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d a_NewPosition)
 {
 	ASSERT(a_World != nullptr);
+	ASSERT(IsTicking());
+
 	if (GetWorld() == a_World)
 	{
 		// Don't move to same world
@@ -1695,21 +1716,19 @@ bool cPlayer::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 		return false;
 	}
 
-	// Remove player from chunk
+	// Prevent further ticking in this world
-	if (!GetWorld()->DoWithChunk(GetChunkX(), GetChunkZ(), [this](cChunk & a_Chunk) -> bool
+	SetIsTicking(false);
-	{
+
-		a_Chunk.SafeRemoveEntity(this);
+	// Tell others we are gone
-		return true;
+	GetWorld()->BroadcastDestroyEntity(*this);
-	}))
-	{
-		LOGD("Entity Teleportation failed! Didn't find the source chunk!\n");
-		return false;
-	}
 
 	// Remove player from world
 	GetWorld()->RemovePlayer(this, false);
-	// Stop all mobs from targeting this player
 
+	// Set position to the new position
+	SetPosition(a_NewPosition);
+
+	// Stop all mobs from targeting this player
 	StopEveryoneFromTargetingMe();
 
 	// Send the respawn packet:
@@ -1718,19 +1737,6 @@ bool cPlayer::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 		m_ClientHandle->SendRespawn(a_World->GetDimension());
 	}
 
-	// Broadcast for other people that the player is gone.
-	GetWorld()->BroadcastDestroyEntity(*this);
-
-	SetPosition(a_NewPosition);
-
-	// Stop all mobs from targeting this player
-	StopEveryoneFromTargetingMe();
-
-	// Queue adding player to the new world, including all the necessary adjustments to the object
-	a_World->AddPlayer(this);
-	cWorld * OldWorld = cRoot::Get()->GetWorld(GetWorld()->GetName());  // Required for the hook HOOK_ENTITY_CHANGED_WORLD
-	SetWorld(a_World);  // Chunks may be streamed before cWorld::AddPlayer() sets the world to the new value
-
 	// Update the view distance.
 	m_ClientHandle->SetViewDistance(m_ClientHandle->GetRequestedViewDistance());
 
@@ -1743,9 +1749,21 @@ bool cPlayer::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d
 	// Broadcast the player into the new world.
 	a_World->BroadcastSpawnEntity(*this);
 
-	// Player changed the world, call the hook
+	// Queue add to new world and removal from the old one
-	cRoot::Get()->GetPluginManager()->CallHookEntityChangedWorld(*this, *OldWorld);
+	cChunk * ParentChunk = GetParentChunk();
-
+	cWorld * OldWorld = GetWorld();
+	SetWorld(a_World);  // Chunks may be streamed before cWorld::AddPlayer() sets the world to the new value
+	OldWorld->QueueTask([this, ParentChunk, a_World](cWorld & a_OldWorld)
+	{
+		LOGD("Warping player \"%s\" from world \"%s\" to \"%s\". Source chunk: (%d, %d) ",
+			this->GetName().c_str(),
+			a_OldWorld.GetName().c_str(), a_World->GetName().c_str(),
+			ParentChunk->GetPosX(), ParentChunk->GetPosZ()
+		);
+		ParentChunk->RemoveEntity(this);
+		a_World->AddPlayer(this);
+		cRoot::Get()->GetPluginManager()->CallHookEntityChangedWorld(*this, a_OldWorld);
+	});
 	return true;
 }
 

src/Entities/Player.h

@@ -42,6 +42,8 @@ public:
 
 	cPlayer(cClientHandlePtr a_Client, const AString & a_PlayerName);
 
+	virtual bool Initialize(cWorld & a_World) override;
+
 	virtual ~cPlayer();
 
 	virtual void SpawnOn(cClientHandle & a_Client) override;

src/Inventory.cpp

@@ -713,7 +713,7 @@ void cInventory::OnSlotChanged(cItemGrid * a_ItemGrid, int a_SlotNum)
 {
 	// Send the neccessary updates to whoever needs them
 
-	if (m_Owner.IsDestroyed())
+	if (!m_Owner.IsTicking())
 	{
 		// Owner is not (yet) valid, skip for now
 		return;

src/Mobs/Monster.cpp

@@ -256,7 +256,7 @@ void cMonster::Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
 	}
 	if ((GetTarget() != nullptr))
 	{
-		ASSERT(!GetTarget()->IsDestroyed());
+		ASSERT(GetTarget()->IsTicking());
 
 		if (GetTarget()->IsPlayer())
 		{
@@ -912,7 +912,7 @@ int cMonster::GetSpawnDelay(cMonster::eFamily a_MobFamily)
 /** Sets the target. */
 void cMonster::SetTarget (cPawn * a_NewTarget)
 {
-	ASSERT((a_NewTarget == nullptr) || (!IsDestroyed()));
+	ASSERT((a_NewTarget == nullptr) || (IsTicking()));
 	if (m_Target == a_NewTarget)
 	{
 		return;
@@ -928,7 +928,7 @@ void cMonster::SetTarget (cPawn * a_NewTarget)
 
 	if (a_NewTarget != nullptr)
 	{
-		ASSERT(!a_NewTarget->IsDestroyed());
+		ASSERT(a_NewTarget->IsTicking());
 		// Notify the new target that we are now targeting it.
 		m_Target->TargetingMe(this);
 	}

src/Mobs/PassiveMonster.cpp

@@ -65,6 +65,18 @@ void cPassiveMonster::ResetLoveMode()
 
 
 
+void cPassiveMonster::Destroyed()
+{
+	if (m_LovePartner != nullptr)
+	{
+		m_LovePartner->ResetLoveMode();
+	}
+}
+
+
+
+
+
 void cPassiveMonster::Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
 {
 	super::Tick(a_Dt, a_Chunk);
@@ -73,10 +85,6 @@ void cPassiveMonster::Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
 	{
 		CheckEventLostPlayer();
 	}
-	if ((m_LovePartner != nullptr) && m_LovePartner->IsDestroyed())
-	{
-		m_LovePartner = nullptr;
-	}
 
 	// if we have a partner, mate
 	if (m_LovePartner != nullptr)

src/Mobs/PassiveMonster.h

@@ -45,6 +45,8 @@ public:
 	/** Returns whether the monster is tired of breeding and is in the cooldown state. */
 	bool IsInLoveCooldown() const { return (m_LoveCooldown > 0); }
 
+	virtual void Destroyed(void) override;
+
 protected:
 	/** The monster's breeding partner. */
 	cPassiveMonster * m_LovePartner;

src/World.cpp

@@ -1018,6 +1018,8 @@ void cWorld::Tick(std::chrono::milliseconds a_Dt, std::chrono::milliseconds a_La
 		{
 			(*itr)->SetWorld(this);
 			m_ChunkMap->AddEntity(*itr);
+			ASSERT(!(*itr)->IsTicking());
+			(*itr)->SetIsTicking(true);
 		}
 		m_EntitiesToAdd.clear();
 	}
@@ -1026,6 +1028,7 @@ void cWorld::Tick(std::chrono::milliseconds a_Dt, std::chrono::milliseconds a_La
 	AddQueuedPlayers();
 
 	m_ChunkMap->Tick(a_Dt);
+	TickMobs(a_Dt);
 	m_MapManager.TickMaps();
 
 	TickClients(static_cast<float>(a_Dt.count()));
@@ -1046,7 +1049,6 @@ void cWorld::Tick(std::chrono::milliseconds a_Dt, std::chrono::milliseconds a_La
 		UnloadUnusedChunks();
 	}
 
-	TickMobs(a_Dt);
 }
 
 
@@ -1134,7 +1136,16 @@ void cWorld::TickMobs(std::chrono::milliseconds a_Dt)
 	cMobProximityCounter::sIterablePair allCloseEnoughToMoveMobs = MobCensus.GetProximityCounter().getMobWithinThosesDistances(-1, 64 * 16);// MG TODO : deal with this magic number (the 16 is the size of a block)
 	for (cMobProximityCounter::tDistanceToMonster::const_iterator itr = allCloseEnoughToMoveMobs.m_Begin; itr != allCloseEnoughToMoveMobs.m_End; ++itr)
 	{
-		itr->second.m_Monster.Tick(a_Dt, itr->second.m_Chunk);
+		cEntity & Entity = itr->second.m_Monster;
+		cChunk * Chunk = Entity.GetParentChunk();
+		if (!Entity.IsTicking())
+		{
+			++itr;
+			continue;
+		}
+		ASSERT(Chunk == &(itr->second.m_Chunk));
+		Entity.Tick(a_Dt, *Chunk);
+		ASSERT(Chunk == &(itr->second.m_Chunk));
 	}
 
 	// remove too far mobs
@@ -2919,7 +2930,7 @@ bool cWorld::ForEachPlayer(cPlayerListCallback & a_Callback)
 	for (cPlayerList::iterator itr = m_Players.begin(), itr2 = itr; itr != m_Players.end(); itr = itr2)
 	{
 		++itr2;
-		if ((*itr)->IsDestroyed())
+		if (!(*itr)->IsTicking())
 		{
 			continue;
 		}
@@ -2941,7 +2952,7 @@ bool cWorld::DoWithPlayer(const AString & a_PlayerName, cPlayerListCallback & a_
 	cCSLock Lock(m_CSPlayers);
 	for (cPlayerList::iterator itr = m_Players.begin(); itr != m_Players.end(); ++itr)
 	{
-		if ((*itr)->IsDestroyed())
+		if (!(*itr)->IsTicking())
 		{
 			continue;
 		}
@@ -2967,7 +2978,7 @@ bool cWorld::FindAndDoWithPlayer(const AString & a_PlayerNameHint, cPlayerListCa
 	cCSLock Lock(m_CSPlayers);
 	for (cPlayerList::iterator itr = m_Players.begin(); itr != m_Players.end(); ++itr)
 	{
-		if ((*itr)->IsDestroyed())
+		if (!(*itr)->IsTicking())
 		{
 			continue;
 		}
@@ -2999,7 +3010,7 @@ bool cWorld::DoWithPlayerByUUID(const AString & a_PlayerUUID, cPlayerListCallbac
 	cCSLock Lock(m_CSPlayers);
 	for (cPlayerList::iterator itr = m_Players.begin(); itr != m_Players.end(); ++itr)
 	{
-		if ((*itr)->IsDestroyed())
+		if (!(*itr)->IsTicking())
 		{
 			continue;
 		}
@@ -3026,7 +3037,7 @@ cPlayer * cWorld::FindClosestPlayer(const Vector3d & a_Pos, float a_SightLimit,
 	cCSLock Lock(m_CSPlayers);
 	for (cPlayerList::const_iterator itr = m_Players.begin(); itr != m_Players.end(); ++itr)
 	{
-		if ((*itr)->IsDestroyed())
+		if (!(*itr)->IsTicking())
 		{
 			continue;
 		}
@@ -3406,6 +3417,7 @@ void cWorld::ScheduleTask(int a_DelayTicks, std::function<void (cWorld &)> a_Tas
 
 void cWorld::AddEntity(cEntity * a_Entity)
 {
+	a_Entity->SetWorld(this);
 	cCSLock Lock(m_CSEntitiesToAdd);
 	m_EntitiesToAdd.push_back(a_Entity);
 }
@@ -3792,6 +3804,8 @@ void cWorld::AddQueuedPlayers(void)
 
 			// Add to chunkmap, if not already there (Spawn vs MoveToWorld):
 			m_ChunkMap->AddEntityIfNotPresent(*itr);
+			ASSERT(!(*itr)->IsTicking());
+			(*itr)->SetIsTicking(true);
 		}  // for itr - PlayersToAdd[]
 	}  // Lock(m_CSPlayers)
 
@@ -3909,6 +3923,3 @@ cBroadcaster cWorld::GetBroadcaster()
 {
 	return cBroadcaster(this);
 }
-
-
-

src/World.h

@@ -819,7 +819,6 @@ private:
 		virtual void Execute(void) override;
 	} ;
 
-
 	/** Implementation of the callbacks that the ChunkGenerator uses to store new chunks and interface to plugins */
 	class cChunkGeneratorCallbacks :
 		public cChunkGenerator::cChunkSink,