Only store IDs across ticks

src/Entities/Entity.cpp

@@ -73,6 +73,7 @@ cEntity::cEntity(eEntityType a_EntityType, Vector3d a_Pos, double a_Width, doubl
 	m_Height(a_Height),
 	m_InvulnerableTicks(0)
 {
+	m_WorldChangeInfo.m_NewWorld = nullptr;
 }
 
 
@@ -1406,7 +1407,7 @@ bool cEntity::DetectPortal()
 					cWorld * TargetWorld = cRoot::Get()->GetWorld(GetWorld()->GetLinkedOverworldName());
 					ASSERT(TargetWorld != nullptr);  // The linkage checker should have prevented this at startup. See cWorld::start()
 					LOGD("Jumping %s -> %s", DimensionToString(dimNether).c_str(), DimensionToString(DestionationDim).c_str());
-					new cNetherPortalScanner(this, TargetWorld, TargetPos, cChunkDef::Height);
+					new cNetherPortalScanner(*this, TargetWorld, TargetPos, cChunkDef::Height);
 					return true;
 				}
 				// Nether portal in the overworld
@@ -1438,7 +1439,7 @@ bool cEntity::DetectPortal()
 					cWorld * TargetWorld = cRoot::Get()->GetWorld(GetWorld()->GetLinkedNetherWorldName());
 					ASSERT(TargetWorld != nullptr);  // The linkage checker should have prevented this at startup. See cWorld::start()
 					LOGD("Jumping %s -> %s", DimensionToString(dimOverworld).c_str(), DimensionToString(DestionationDim).c_str());
-					new cNetherPortalScanner(this, TargetWorld, TargetPos, (cChunkDef::Height / 2));
+					new cNetherPortalScanner(*this, TargetWorld, TargetPos, (cChunkDef::Height / 2));
 					return true;
 				}
 			}
@@ -1587,25 +1588,46 @@ bool cEntity::MoveToWorld(cWorld * a_World, Vector3d a_NewPosition, bool a_SetPo
 		return false;
 	}
 
+	if (m_WorldChangeInfo.m_NewWorld != nullptr)
+	{
+		// Avoid scheduling multiple warp tasks
+		return true;
+	}
+
 	// Create new world change info
-	auto NewWCI = cpp14::make_unique<sWorldChangeInfo>();
-	*NewWCI = { a_World,  a_NewPosition, a_SetPortalCooldown, a_ShouldSendRespawn };
+	m_WorldChangeInfo = { a_World,  a_NewPosition, a_SetPortalCooldown, a_ShouldSendRespawn };
 
-	// Publish atomically
-	auto OldWCI = m_WorldChangeInfo.exchange(std::move(NewWCI));
+	// TODO: move to capture when C++14
+	const auto EntityID = GetUniqueID();
 
-	if (OldWCI == nullptr)
-	{
-		// Schedule a new world change.
+	/* Requirements:
+	Only one world change in-flight at any time
+	No ticking during world changes
+	The last invocation takes effect
+
+	As of writing, cWorld ticks entities, clients, and then processes tasks
+	We may call MoveToWorld (any number of times - consider multiple /portal commands within a tick) in the first and second stages
+	Queue a task onto the third stage to invoke DoMoveToWorld ONCE with the last given destination world
+	Store entity IDs in case client tick found the player disconnected and immediately destroys the object
+
+	After the move begins, no further calls to MoveToWorld is possible since neither the client nor entity is ticked
+	This remains until the warp is complete and the destination world resumes ticking.
+	*/
 	GetWorld()->QueueTask(
-			[this](cWorld & a_CurWorld)
+		[EntityID](cWorld & a_CurWorld)
 		{
-				auto WCI = m_WorldChangeInfo.exchange(nullptr);
-				cWorld::cLock Lock(a_CurWorld);
-				DoMoveToWorld(*WCI);
+			a_CurWorld.DoWithEntityByID(
+				EntityID,
+				[](cEntity & a_Entity)
+				{
+					auto & WCI = a_Entity.m_WorldChangeInfo;
+					a_Entity.DoMoveToWorld(WCI);
+					WCI.m_NewWorld = nullptr;
+					return true;
 				}
 			);
 		}
+	);
 
 	return true;
 }

src/Entities/Entity.h

@@ -489,7 +489,7 @@ public:
 	/** Returns true if a world change is scheduled to happen. */
 	bool IsWorldChangeScheduled() const
 	{
-		return (m_WorldChangeInfo.load() != nullptr);
+		return (m_WorldChangeInfo.m_NewWorld != nullptr);
 	}
 
 	/** Updates clients of changes in the entity. */
@@ -662,8 +662,8 @@ protected:
 
 	cWorld * m_World;
 
-	/** If not nullptr, a world change is scheduled and a task is queued in the current world. */
-	cAtomicUniquePtr<sWorldChangeInfo> m_WorldChangeInfo;
+	/** If field m_NewWorld not nullptr, a world change is scheduled and a task is queued in the current world. */
+	sWorldChangeInfo m_WorldChangeInfo;
 
 	/** Whether the entity is capable of taking fire or lava damage. */
 	bool m_IsFireproof;

src/NetherPortalScanner.cpp

@@ -17,8 +17,9 @@ const double cNetherPortalScanner::AcrossOffset = 0.5;
 
 
 
-cNetherPortalScanner::cNetherPortalScanner(cEntity * a_MovingEntity, cWorld * a_DestinationWorld, Vector3d a_DestPosition, int a_MaxY) :
-	m_Entity(a_MovingEntity),
+cNetherPortalScanner::cNetherPortalScanner(cEntity & a_MovingEntity, cWorld * a_DestinationWorld, Vector3d a_DestPosition, int a_MaxY) :
+	m_EntityID(a_MovingEntity.GetUniqueID()),
+	m_SourceWorld(*a_MovingEntity.GetWorld()),
 	m_World(a_DestinationWorld),
 	m_FoundPortal(false),
 	m_BuildPlatform(true),
@@ -296,8 +297,18 @@ void cNetherPortalScanner::OnDisabled(void)
 		Position.z += OutOffset;
 	}
 
+	// Lookup our warping entity by ID
+	// Necessary as they may have been destroyed in the meantime (#4582)
+	m_SourceWorld.DoWithEntityByID(
+		m_EntityID,
+		[this, &Position](cEntity & a_Entity)
+		{
 			FLOGD("Placing player at {0}", Position);
-	m_Entity->MoveToWorld(m_World, Position, true, false);
+			a_Entity.MoveToWorld(m_World, Position, true, false);
+			return true;
+		}
+	);
+
 	delete this;
 }
 

src/NetherPortalScanner.h

@@ -15,7 +15,7 @@ class cWorld;
 class cNetherPortalScanner : public cChunkStay
 {
 public:
-	cNetherPortalScanner(cEntity * a_MovingEntity, cWorld * a_DestinationWorld, Vector3d a_DestPosition, int a_MaxY);
+	cNetherPortalScanner(cEntity & a_MovingEntity, cWorld * a_DestinationWorld, Vector3d a_DestPosition, int a_MaxY);
 	virtual void OnChunkAvailable(int a_ChunkX, int a_ChunkY) override;
 	virtual bool OnAllChunksAvailable(void) override;
 	virtual void OnDisabled(void) override;
@@ -48,8 +48,11 @@ private:
 	/** Whether the given location is a valid location to build a portal. */
 	bool IsValidBuildLocation(Vector3i a_BlockPosition);
 
-	/** The entity that's being moved. */
-	cEntity * m_Entity;
+	/** The ID of the entity that's being moved. */
+	UInt32 m_EntityID;
+
+	/** The world we're moving the entity from, used to query the entity ID. */
+	cWorld & m_SourceWorld;
 
 	/** The world we're moving the entity to. */
 	cWorld * m_World;