Stabilise MoveToWorld (cuberite#4004)

* Stabilise MoveToWorld

* Fix comments and deprecate ScheduleMoveToWorld

* Enhanced thread safety for m_WorldChangeInfo

* Return unique_ptr from cAtomicUniquePtr::exchange

* cWorld now calls entity cEntity::OnAddToWorld and cEntity::OnRemoveFromWorld.

Allows broadcasting entities added to the world from the world's tick thread.
This also factors out some common code from cEntity::DoMoveToWorld and cEntity::Initialize.

As a consequence, cEntity::Destroy(false) (i.e. Destroying the entity without broadcasting) is impossible.
This isn't used anywhere in Cuberite so it's now deprecated.

* Update entity position after removing it from the world.
Fixes broadcasts being sent to the wrong chunk.

* Fix style

* cEntity: Update LastSentPosition when sending spawn packet

* Add Wno-deprecated-declarations to the lua bindings

* Kill uses of ScheduleMoveToWorld

Server/Plugins/APIDump/APIDesc.lua

@@ -3996,7 +3996,7 @@ local Hash = cCryptoHash.sha1HexString("DataToHash")
 								Type = "boolean",
 							},
 						},
-						Notes = "Removes the entity from this world and starts moving it to the specified world's spawn point. Note that to avoid deadlocks, the move is asynchronous - the entity is moved into a queue and will be moved from that queue into the destination world at some (unpredictable) time in the future. ShouldSendRespawn is used only for players, it specifies whether the player should be sent a Respawn packet upon leaving the world (The client handles respawns only between different dimensions). <b>OBSOLETE</b>, use ScheduleMoveToWorld() instead.",
+						Notes = "Removes the entity from this world and starts moving it to the specified world's spawn point. Note that to avoid deadlocks, the move is asynchronous - the entity is moved into a queue and will be moved from that queue into the destination world at some (unpredictable) time in the future. ShouldSendRespawn is used only for players, it specifies whether the player should be sent a Respawn packet upon leaving the world (The client handles respawns only between different dimensions).",
 					},
 					{
 						Params =
@@ -4017,7 +4017,7 @@ local Hash = cCryptoHash.sha1HexString("DataToHash")
 								Type = "boolean",
 							},
 						},
-						Notes = "Removes the entity from this world and starts moving it to the specified world's spawn point. Note that to avoid deadlocks, the move is asynchronous - the entity is moved into a queue and will be moved from that queue into the destination world at some (unpredictable) time in the future. ShouldSendRespawn is used only for players, it specifies whether the player should be sent a Respawn packet upon leaving the world (The client handles respawns only between different dimensions). <b>OBSOLETE</b>, use ScheduleMoveToWorld() instead.",
+						Notes = "Removes the entity from this world and starts moving it to the specified world's spawn point. Note that to avoid deadlocks, the move is asynchronous - the entity is moved into a queue and will be moved from that queue into the destination world at some (unpredictable) time in the future. ShouldSendRespawn is used only for players, it specifies whether the player should be sent a Respawn packet upon leaving the world (The client handles respawns only between different dimensions).",
 					},
 					{
 						Params =
@@ -4041,7 +4041,37 @@ local Hash = cCryptoHash.sha1HexString("DataToHash")
 								Type = "boolean",
 							},
 						},
-						Notes = "Removes the entity from this world and starts moving it to the specified world. Note that to avoid deadlocks, the move is asynchronous - the entity is moved into a queue and will be moved from that queue into the destination world at some (unpredictable) time in the future. ShouldSendRespawn is used only for players, it specifies whether the player should be sent a Respawn packet upon leaving the world (The client handles respawns only between different dimensions). The Position parameter specifies the location that the entity should be placed in, in the new world. <b>OBSOLETE</b>, use ScheduleMoveToWorld() instead.",
+						Notes = "Removes the entity from this world and starts moving it to the specified world. Note that to avoid deadlocks, the move is asynchronous - the entity is moved into a queue and will be moved from that queue into the destination world at some (unpredictable) time in the future. ShouldSendRespawn is used only for players, it specifies whether the player should be sent a Respawn packet upon leaving the world (The client handles respawns only between different dimensions). The Position parameter specifies the location that the entity should be placed in, in the new world.",
+					},
+					{
+						Params =
+						{
+							{
+								Name = "World",
+								Type = "cWorld",
+							},
+							{
+								Name = "Position",
+								Type = "Vector3d",
+							},
+							{
+								Name = "ShouldSetPortalCooldown",
+								Type = "boolean",
+								IsOptional = true,
+							},
+							{
+								Name = "ShouldSendRespawn",
+								Type = "boolean",
+								IsOptional = true,
+							},
+						},
+						Returns =
+						{
+							{
+								Type = "boolean",
+							},
+						},
+						Notes = "Removes the entity from this world and starts moving it to the specified world. Note that to avoid deadlocks, the move is asynchronous - the entity is moved into a queue and will be moved from that queue into the destination world at some (unpredictable) time in the future. If ShouldSetPortalCooldown is false (default), doesn't set any portal cooldown, if it is true, the default portal cooldown is applied to the entity. ShouldSendRespawn is used only for players, it specifies whether the player should be sent a Respawn packet upon leaving the world (The client handles respawns only between different dimensions). The Position parameter specifies the location that the entity should be placed in, in the new world.",
 					},
 				},
 				ScheduleMoveToWorld =
@@ -4067,7 +4097,7 @@ local Hash = cCryptoHash.sha1HexString("DataToHash")
 							IsOptional  = true,
 						},
 					},
-					Notes = "Schedules a MoveToWorld call to occur on the next Tick of the entity. If ShouldSetPortalCooldown is false (default), doesn't set any portal cooldown, if it is true, the default portal cooldown is applied to the entity. If ShouldSendRespawn is false (default), no respawn packet is sent, if it is true then a respawn packet is sent to the client.",
+					Notes = "Schedules a MoveToWorld call to occur on the next Tick of the entity. If ShouldSetPortalCooldown is false (default), doesn't set any portal cooldown, if it is true, the default portal cooldown is applied to the entity. If ShouldSendRespawn is false (default), no respawn packet is sent, if it is true then a respawn packet is sent to the client. <b>OBSOLETE</b>, use MoveToWorld instead.",
 				},
 				SetGravity =
 				{

src/Bindings/CMakeLists.txt

@@ -164,7 +164,8 @@ set_source_files_properties(${BINDING_OUTPUTS} PROPERTIES GENERATED TRUE)
 set_source_files_properties(${CMAKE_SOURCE_DIR}/src/Bindings/Bindings.cpp PROPERTIES COMPILE_FLAGS -Wno-error)
 
 if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
-	set_source_files_properties(Bindings.cpp PROPERTIES COMPILE_FLAGS "-Wno-old-style-cast -Wno-missing-prototypes")
+	set_source_files_properties(Bindings.cpp PROPERTIES COMPILE_FLAGS
+		"-Wno-old-style-cast -Wno-missing-prototypes -Wno-deprecated-declarations")
 endif()
 
 if(NOT MSVC)

src/ClientHandle.cpp

@@ -189,7 +189,7 @@ void cClientHandle::Destroy(void)
 				// If ownership was transferred, our own smart pointer should be unset
 				ASSERT(!m_PlayerPtr);
 
-				m_PlayerPtr = world->RemovePlayer(*player, true);
+				m_PlayerPtr = world->RemovePlayer(*player);
 
 				// And RemovePlayer should have returned a valid smart pointer
 				ASSERT(m_PlayerPtr);

src/Entities/Boat.cpp

@@ -60,7 +60,7 @@ bool cBoat::DoTakeDamage(TakeDamageInfo & TDI)
 				m_World->SpawnItemPickups(Pickups, GetPosX(), GetPosY(), GetPosZ(), 0, 0, 0, true);
 			}
 		}
-		Destroy(true);
+		Destroy();
 	}
 	return true;
 }

src/Entities/Entity.cpp

@@ -47,7 +47,6 @@ cEntity::cEntity(eEntityType a_EntityType, Vector3d a_Pos, double a_Width, doubl
 	m_LastPosition(a_Pos),
 	m_EntityType(a_EntityType),
 	m_World(nullptr),
-	m_IsWorldChangeScheduled(false),
 	m_IsFireproof(false),
 	m_TicksSinceLastBurnDamage(0),
 	m_TicksSinceLastLavaDamage(0),
@@ -158,19 +157,29 @@ bool cEntity::Initialize(OwnedEntity a_Self, cWorld & a_EntityWorld)
 
 	cPluginManager::Get()->CallHookSpawnedEntity(a_EntityWorld, *this);
 
+	return true;
+}
+
+
+
+
+
+void cEntity::OnAddToWorld(cWorld & a_World)
+{
 	// Spawn the entity on the clients:
-	a_EntityWorld.BroadcastSpawnEntity(*this);
+	m_LastSentPosition = GetPosition();
+	a_World.BroadcastSpawnEntity(*this);
+	BroadcastLeashedMobs();
+}
 
-	// If has any mob leashed broadcast every leashed entity to this
-	if (HasAnyMobLeashed())
-	{
-		for (auto LeashedMob : m_LeashedMobs)
-		{
-			m_World->BroadcastLeashEntity(*LeashedMob, *this);
-		}
-	}
 
-	return true;
+
+
+
+void cEntity::OnRemoveFromWorld(cWorld & a_World)
+{
+	RemoveAllLeashedMobs();
+	a_World.BroadcastDestroyEntity(*this);
 }
 
 
@@ -216,7 +225,7 @@ void cEntity::SetParentChunk(cChunk * a_Chunk)
 
 
 
-void cEntity::Destroy(bool a_ShouldBroadcast)
+void cEntity::Destroy()
 {
 	SetIsTicking(false);
 
@@ -226,11 +235,6 @@ void cEntity::Destroy(bool a_ShouldBroadcast)
 		m_LeashedMobs.front()->Unleash(true, true);
 	}
 
-	if (a_ShouldBroadcast)
-	{
-		m_World->BroadcastDestroyEntity(*this);
-	}
-
 	auto ParentChunkCoords = cChunkDef::BlockToChunk(GetPosition());
 	m_World->QueueTask([this, ParentChunkCoords](cWorld & a_World)
 	{
@@ -1166,7 +1170,7 @@ void cEntity::ApplyFriction(Vector3d & a_Speed, double a_SlowdownMultiplier, flo
 void cEntity::TickBurning(cChunk & a_Chunk)
 {
 	// If we're about to change worlds, then we can't accurately determine whether we're in lava (#3939)
-	if (m_IsWorldChangeScheduled)
+	if (IsWorldChangeScheduled())
 	{
 		return;
 	}
@@ -1310,34 +1314,11 @@ void cEntity::DetectCacti(void)
 
 
 
-void cEntity::ScheduleMoveToWorld(cWorld * a_World, Vector3d a_NewPosition, bool a_SetPortalCooldown, bool a_ShouldSendRespawn)
-{
-	m_NewWorld = a_World;
-	m_NewWorldPosition = a_NewPosition;
-	m_IsWorldChangeScheduled = true;
-	m_WorldChangeSetPortalCooldown = a_SetPortalCooldown;
-	m_WorldChangeSendRespawn = a_ShouldSendRespawn;
-}
-
-
-
-
-
 bool cEntity::DetectPortal()
 {
-	// If somebody scheduled a world change with ScheduleMoveToWorld, change worlds now.
-	if (m_IsWorldChangeScheduled)
+	// If somebody scheduled a world change, do nothing.
+	if (IsWorldChangeScheduled())
 	{
-		m_IsWorldChangeScheduled = false;
-
-		if (m_WorldChangeSetPortalCooldown)
-		{
-			// Delay the portal check.
-			m_PortalCooldownData.m_TicksDelayed = 0;
-			m_PortalCooldownData.m_ShouldPreventTeleportation = true;
-		}
-
-		MoveToWorld(m_NewWorld, m_WorldChangeSendRespawn, m_NewWorldPosition);
 		return true;
 	}
 
@@ -1519,69 +1500,81 @@ bool cEntity::DetectPortal()
 
 
 
-bool cEntity::DoMoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d a_NewPosition)
+void cEntity::DoMoveToWorld(const sWorldChangeInfo & a_WorldChangeInfo)
 {
-	UNUSED(a_ShouldSendRespawn);
-	ASSERT(a_World != nullptr);
+	ASSERT(a_WorldChangeInfo.m_NewWorld != nullptr);
 
-	if (GetWorld() == a_World)
+	if (a_WorldChangeInfo.m_SetPortalCooldown)
 	{
-		// Don't move to same world
-		return false;
+		m_PortalCooldownData.m_TicksDelayed = 0;
+		m_PortalCooldownData.m_ShouldPreventTeleportation = true;
 	}
 
-	//  Ask the plugins if the entity is allowed to changing the world
-	if (cRoot::Get()->GetPluginManager()->CallHookEntityChangingWorld(*this, *a_World))
+	if (GetWorld() == a_WorldChangeInfo.m_NewWorld)
 	{
-		// A Plugin doesn't allow the entity to changing the world
-		return false;
+		// Moving to same world, don't need to remove from world
+		SetPosition(a_WorldChangeInfo.m_NewPosition);
+		return;
 	}
 
+	LOGD("Warping entity #%i (%s) from world \"%s\" to \"%s\". Source chunk: (%d, %d) ",
+		GetUniqueID(), GetClass(),
+		m_World->GetName(), a_WorldChangeInfo.m_NewWorld->GetName(),
+		GetChunkX(), GetChunkZ()
+	);
+
 	// Stop ticking, in preperation for detaching from this world.
 	SetIsTicking(false);
 
-	// Tell others we are gone
-	GetWorld()->BroadcastDestroyEntity(*this);
+	// Remove from the old world
+	auto Self = m_World->RemoveEntity(*this);
 
-	// Take note of old chunk coords
-	auto OldChunkCoords = cChunkDef::BlockToChunk(GetPosition());
+	// Update entity before calling hook
+	ResetPosition(a_WorldChangeInfo.m_NewPosition);
+	SetWorld(a_WorldChangeInfo.m_NewWorld);
 
-	// Set position to the new position
-	ResetPosition(a_NewPosition);
+	cRoot::Get()->GetPluginManager()->CallHookEntityChangedWorld(*this, *m_World);
 
-	// Stop all mobs from targeting this entity
-	// Stop this entity from targeting other mobs
-	if (this->IsMob())
-	{
-		cMonster * Monster = static_cast<cMonster*>(this);
-		Monster->SetTarget(nullptr);
-		Monster->StopEveryoneFromTargetingMe();
-	}
-
-	// Queue add to new world and removal from the old one
-	cWorld * OldWorld = GetWorld();
-	SetWorld(a_World);  // Chunks may be streamed before cWorld::AddPlayer() sets the world to the new value
-	OldWorld->QueueTask([this, OldChunkCoords, a_World](cWorld & a_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(),
-			OldChunkCoords.m_ChunkX, OldChunkCoords.m_ChunkZ
-		);
-		UNUSED(OldChunkCoords);  // Non Debug mode only
-		a_World->AddEntity(a_OldWorld.RemoveEntity(*this));
-		cRoot::Get()->GetPluginManager()->CallHookEntityChangedWorld(*this, a_OldWorld);
-	});
-	return true;
+	// Don't do anything after adding as the old world's CS no longer protects us
+	a_WorldChangeInfo.m_NewWorld->AddEntity(std::move(Self));
 }
 
 
 
 
 
-bool cEntity::MoveToWorld(cWorld * a_World, bool a_ShouldSendRespawn, Vector3d a_NewPosition)
+bool cEntity::MoveToWorld(cWorld * a_World, Vector3d a_NewPosition, bool a_SetPortalCooldown, bool a_ShouldSendRespawn)
 {
-	return DoMoveToWorld(a_World, a_ShouldSendRespawn, a_NewPosition);
+	ASSERT(a_World != nullptr);
+
+	// Ask the plugins if the entity is allowed to change world
+	if (cRoot::Get()->GetPluginManager()->CallHookEntityChangingWorld(*this, *a_World))
+	{
+		// A Plugin isn't allowing the entity to change world
+		return false;
+	}
+
+	// Create new world change info
+	auto NewWCI = cpp14::make_unique<sWorldChangeInfo>();
+	*NewWCI = { a_World,  a_NewPosition, a_SetPortalCooldown, a_ShouldSendRespawn };
+
+	// Publish atomically
+	auto OldWCI = m_WorldChangeInfo.exchange(std::move(NewWCI));
+
+	if (OldWCI == nullptr)
+	{
+		// Schedule a new world change.
+		GetWorld()->QueueTask(
+			[this](cWorld & a_CurWorld)
+			{
+				auto WCI = m_WorldChangeInfo.exchange(nullptr);
+				cWorld::cLock Lock(a_CurWorld);
+				DoMoveToWorld(*WCI);
+			}
+		);
+	}
+
+	return true;
 }
 
 
@@ -1606,7 +1599,7 @@ bool cEntity::MoveToWorld(const AString & a_WorldName, bool a_ShouldSendRespawn)
 		return false;
 	}
 
-	return DoMoveToWorld(World, a_ShouldSendRespawn, Vector3d(World->GetSpawnX(), World->GetSpawnY(), World->GetSpawnZ()));
+	return MoveToWorld(World, Vector3d(World->GetSpawnX(), World->GetSpawnY(), World->GetSpawnZ()), false, a_ShouldSendRespawn);
 }
 
 
@@ -2253,6 +2246,34 @@ void cEntity::RemoveLeashedMob(cMonster * a_Monster)
 
 
 
+void cEntity::RemoveAllLeashedMobs()
+{
+	while (!m_LeashedMobs.empty())
+	{
+		m_LeashedMobs.front()->Unleash(false, true);
+	}
+}
+
+
+
+
+
+void cEntity::BroadcastLeashedMobs()
+{
+	// If has any mob leashed broadcast every leashed entity to this
+	if (HasAnyMobLeashed())
+	{
+		for (auto LeashedMob : m_LeashedMobs)
+		{
+			m_World->BroadcastLeashEntity(*LeashedMob, *this);
+		}
+	}
+}
+
+
+
+
+
 float cEntity::GetExplosionExposureRate(Vector3d a_ExplosionPosition, float a_ExlosionPower)
 {
 	double EntitySize = m_Width * m_Width * m_Height;