Move travelling to two-step system

Mark Actors meant to travel early so their flag can be written into the snapshot, followed by moving them over all at once at the same time as previously. On finish, destroy any marked Actors to prevent duplication in hubs. Switch to a unique function that does the verification instead of ChangeStatNum and lock out STAT_TRAVELLING entirely since it shouldn't be used for regular gameplay.

src/common/objects/dobject.cpp

@@ -647,6 +647,7 @@ void DObject::Serialize(FSerializer &arc)
 	SerializeFlag("spawned", OF_Spawned);
 	SerializeFlag("networked", OF_Networked);
 	SerializeFlag("clientside", OF_ClientSide);
+	SerializeFlag("travelling", OF_Travelling);
 		
 	ObjectFlags |= OF_SerialSuccess;
 

src/g_level.cpp

@@ -1292,8 +1292,9 @@ bool FLevelLocals::DoCompleted (FString nextlevel, wbstartstruct_t &wminfo)
 			G_PlayerFinishLevel (i, mode, changeflags);
 		}
 	}
+	StartTravel();
 	soundEngine->BlockNewSounds(false);
-
+	
 	if (mode == FINISH_SameHub)
 	{ // Remember the level's state for re-entry.
 		if (!(flags2 & LEVEL2_FORGETSTATE))
@@ -1613,7 +1614,7 @@ void G_DoWorldDone (void)
 		Printf ("No next map specified.\n");
 		nextlevel = primaryLevel->MapName;
 	}
-	primaryLevel->StartTravel ();
+	primaryLevel->MoveTravellers();
 	G_DoLoadLevel (nextlevel, startpos, true, false);
 	gameaction = ga_nothing;
 	viewactive = true; 
@@ -1640,74 +1641,72 @@ void FLevelLocals::UnlinkActorFromLevel(AActor& mo)
 	mo.DeleteAttachedLights();
 }
 
+void FLevelLocals::AddToTravellingList(DThinker* th)
+{
+	if (th == nullptr || (th->ObjectFlags & (OF_EuthanizeMe | OF_Travelling)))
+		return;
+
+	auto mo = dyn_cast<AActor>(th);
+	// No voodoo dolls allowed.
+	if (mo != nullptr && mo->player != nullptr && mo->player->mo != mo)
+		return;
+
+	th->ObjectFlags |= OF_Travelling;
+	TravellingThinkers.Push(th);
+	if (mo != nullptr)
+	{
+		if (mo->player != nullptr)
+			AddToTravellingList(mo->player->Bot);
+
+		if (!(mo->flags & MF_UNMORPHED))
+			AddToTravellingList(mo->alternative);
+
+		if (!mo->IsKindOf(NAME_Inventory) || mo->PointerVar<AActor>(NAME_Owner) == nullptr)
+		{
+			for (AActor* inv = mo->Inventory; inv != nullptr; inv = inv->Inventory)
+				AddToTravellingList(inv);
+		}
+	}
+}
+
 void FLevelLocals::StartTravel()
 {
-	bTravelling = true;
-
 	if (!deathmatch)
 	{
 		for (size_t i = 0u; i < MAXPLAYERS; ++i)
 		{
-			if (PlayerInGame(i) && Players[i]->health > 0 && !(Players[i]->mo->ObjectFlags & OF_EuthanizeMe))
-				Players[i]->mo->ChangeStatNum(STAT_TRAVELLING);
+			if (PlayerInGame(i) && Players[i]->health > 0)
+				AddToTravellingList(Players[i]->mo);
 		}
 	}
 
-	// Start building the list of everything that needs to travel. Since things can only be added
-	// to STAT_TRAVELLING at this point, it's safe to assume nothing will be removed unless destroyed
-	// (which will clean up anything it owns as well).
-	auto it = GetThinkerIterator<DThinker>(NAME_None, STAT_TRAVELLING);
-	DThinker* th = nullptr;
-	unsigned count = 0u;
-	do
+	// Start building the list of everything that needs to travel. This will keep growing
+	// as we add more things so don't use the C++ iterators.
+	for (size_t i = 0u; i < TravellingThinkers.Size(); ++i)
 	{
-		count = 0u;
-		it.Reinit();
-
-		while ((th = it.Next()) != nullptr)
+		auto th = TravellingThinkers[i];
+		if (!(th->ObjectFlags & OF_EuthanizeMe))
 		{
-			if (th->ObjectFlags & OF_Travelling)
-				continue;
-
-			++count;
-			auto mo = dyn_cast<AActor>(th);
-			if (mo != nullptr)
-			{
-				if (mo->player != nullptr)
-				{
-					// No voodoo dolls allowed.
-					if (mo->player->mo != mo)
-					{
-						mo->ChangeStatNum(mo->GetStatNum());
-						continue;
-					}
-
-					if (mo->player->Bot != nullptr)
-						mo->player->Bot->ChangeStatNum(STAT_TRAVELLING);
-				}
-
-				if (!mo->IsKindOf(NAME_Inventory) || mo->PointerVar<AActor>(NAME_Owner) == nullptr)
-				{
-					for (AActor* inv = mo->Inventory; inv != nullptr; inv = inv->Inventory)
-						inv->ChangeStatNum(STAT_TRAVELLING);
-				}
-
-				if (!(mo->flags & MF_UNMORPHED) && mo->alternative != nullptr)
-					mo->alternative->ChangeStatNum(STAT_TRAVELLING);
-			}
-
-			th->ObjectFlags |= OF_Travelling;
 			IFOVERRIDENVIRTUALPTRNAME(th, NAME_Thinker, PreTravelled)
 				VMCallVoid<DThinker*>(func, th);
 		}
-	} while (count);
+	}
+}
 
-	// Now that the list is complete, unlink everything. We want to do this after the
-	// callback so someone can't attempt to relink things.
-	auto unlinker = GetThinkerIterator<AActor>(NAME_Actor, STAT_TRAVELLING);
-	AActor* mo = nullptr;
-	while ((mo = unlinker.Next()) != nullptr)
+// We do this after snapshotting so the flag can be saved out but it won't be saved into
+// the actual STAT_TRAVELLING list when snapshotting.
+void FLevelLocals::MoveTravellers()
+{
+	for (auto th : TravellingThinkers)
 	{
+		if (th->ObjectFlags & OF_EuthanizeMe)
+			continue;
+
+		th->ChangeStatNum(STAT_TRAVELLING);
+		auto mo = dyn_cast<AActor>(th);
+		if (mo == nullptr)
+			continue;
+
 		UnlinkActorFromLevel(*mo);
 		mo->BlockingMobj = nullptr;
 		mo->BlockingLine = mo->MovementBlockingLine = nullptr;
@@ -1718,7 +1717,7 @@ void FLevelLocals::StartTravel()
 		mo->floorsector = mo->ceilingsector = nullptr;
 	}
 
-	bTravelling = false;
+	TravellingThinkers.Clear();
 }
 
 //==========================================================================
@@ -1744,8 +1743,6 @@ void FLevelLocals::LinkActorToLevel(AActor& mo)
 
 int FLevelLocals::FinishTravel()
 {
-	// From this point onward things can't be added back into the list, only removed. Callbacks
-	// are done after setting everything up so that the Thinkers have initialized objects to work with.
 	TArray<DThinker*> toCallBack = {};
 	int failNum = 0;
 	auto it = GetThinkerIterator<DThinker>(NAME_None, STAT_TRAVELLING);
@@ -1850,13 +1847,16 @@ int FLevelLocals::FinishTravel()
 			mo->Speed = mo->GetDefault()->Speed;
 	}
 
-	Thinkers.DestroyThinkersInList(STAT_TRAVELLING);
-	ClientsideThinkers.DestroyThinkersInList(STAT_TRAVELLING);
+	// Clean up anything that wasn't linked back to avoid memory leaks. We also need to wipe any
+	// remaining thinkers that were set to travel when they left if recovering from a snapshot,
+	// otherwise they'll pile up infinitely.
+	Thinkers.CleanUpTravellers(savegamerestore);
+	ClientsideThinkers.CleanUpTravellers(savegamerestore);
 
 	// Some ZScript will be called here so we have to do this last.
 	for (size_t i = 0u; i < MAXPLAYERS; ++i)
 	{
-		if (PlayerInGame(i))
+		if (PlayerInGame(i) && !(Players[i]->mo->ObjectFlags & OF_EuthanizeMe) && toCallBack.Find(Players[i]->mo) < toCallBack.Size())
 			Players[i]->mo->SetState(Players[i]->mo->SpawnState);
 	}
 

src/g_levellocals.h

@@ -270,6 +270,8 @@ public:
 	FPlayerStart *PickPlayerStart(int playernum, int flags = 0);
 	bool DoCompleted(FString nextlevel, wbstartstruct_t &wminfo);
 	void StartTravel();
+	void AddToTravellingList(DThinker* th);
+	void MoveTravellers();
 	int FinishTravel();
 	void UnlinkActorFromLevel(AActor& mo);
 	void LinkActorToLevel(AActor& mo);
@@ -730,6 +732,7 @@ public:
 	TArray<uint16_t>	ParticlesInSubsec;
 	FThinkerCollection Thinkers;
 	FThinkerCollection ClientsideThinkers;
+	TArray<DThinker*> TravellingThinkers;
 
 	TArray<DVector2>	Scrolls;		// NULL if no DScrollers in this level
 

src/playsim/dthinker.cpp

@@ -70,9 +70,6 @@ struct ProfileInfo
 static TMap<FName, ProfileInfo> Profiles;
 static unsigned int profilethinkers, profilelimit;
 DThinker *NextToThink;
-// Denote that non-travelling Thinkers are about to start doing so. While active, only
-// allow things to be added to the list, not removed.
-bool bTravelling = false;
 
 //==========================================================================
 //
@@ -365,6 +362,22 @@ void FThinkerCollection::DestroyAllThinkers(bool fullgc)
 //
 //==========================================================================
 
+void FThinkerCollection::CleanUpTravellers(bool saveGame)
+{
+	DestroyThinkersInList(STAT_TRAVELLING);
+	for (size_t i = 0u; i <= MAX_STATNUM; ++i)
+	{
+		FreshThinkers[i].RemoveTravellers(saveGame);
+		Thinkers[i].RemoveTravellers(saveGame);
+	}
+}
+
+//==========================================================================
+//
+//
+//
+//==========================================================================
+
 void FThinkerCollection::OnLoad()
 {
 	for (auto& list : FreshThinkers)
@@ -662,6 +675,32 @@ void FThinkerList::SaveList(FSerializer &arc)
 //
 //==========================================================================
 
+void FThinkerList::RemoveTravellers(bool saveGame)
+{
+	DThinker* node = GetHead();
+	if (node == nullptr)
+		return;
+
+	while (node != Sentinel)
+	{
+		NextToThink = node->NextThinker;
+		if ((node->ObjectFlags & OF_Travelling) && !(node->ObjectFlags & OF_EuthanizeMe))
+		{
+			if (saveGame)
+				node->ObjectFlags &= ~OF_Travelling;
+			else
+				node->Destroy();
+		}
+		node = NextToThink;
+	}
+}
+
+//==========================================================================
+//
+//
+//
+//==========================================================================
+
 void FThinkerList::OnLoad()
 {
 	DThinker* node = GetHead();
@@ -930,16 +969,6 @@ void DThinker::ChangeStatNum(int statnum)
 
 static void ChangeStatNum(DThinker *self, int statnum)
 {
-	// Wait until after these Thinkers are processed to allow modifying them,
-	// otherwise relinking and callbacks will be completely messed up. This can
-	// only happen from map objects spawning in intentionally trying to mess with
-	// the list.
-	if (self->ObjectFlags & OF_Travelling)
-	{
-		Printf(TEXTCOLOR_RED "Travelling Thinkers cannot have their statnum changed\n");
-		return;
-	}
-
 	// This will always break Actors, they should use STAT_TRAVELLING instead to
 	// transition between levels.
 	if (statnum == STAT_STATIC && self->IsKindOf(NAME_Actor))
@@ -947,44 +976,9 @@ static void ChangeStatNum(DThinker *self, int statnum)
 		Printf(TEXTCOLOR_RED "Actors cannot be added to STAT_STATIC\n");
 		return;
 	}
-
-	if (bTravelling)
-	{
-		// Don't let things be moved out of the list, only into it.
-		if (statnum != STAT_TRAVELLING)
-		{
-			Printf(TEXTCOLOR_RED "Thinkers can only be moved into STAT_TRAVELLING while changing levels\n");
-			return;
-		}
-		// These should be handled by the owning Actor, otherwise they'll lose them and become useless anyway.
-		if (self->IsKindOf(NAME_Inventory) && self->PointerVar<AActor>(NAME_Owner) != nullptr)
-		{
-			Printf(TEXTCOLOR_RED "Owned Inventory items must travel with their owner on level change\n");
-			return;
-		}
-		if (self->IsKindOf(NAME_Bot))
-		{
-			Printf(TEXTCOLOR_RED "Bot Thinkers must travel with their owner on level change\n");
-			return;
-		}
-		auto mo = dyn_cast<AActor>(self);
-		if (mo != nullptr && (mo->flags & MF_UNMORPHED))
-		{
-			Printf(TEXTCOLOR_RED "Unmorphed Actors must travel with their owner on level change\n");
-			return;
-		}
-		// These need to be locked down since they have native fields that won't be cleared
-		// properly at the moment.
-		auto cls = self->GetClass()->NativeClass();
-		if (cls->TypeName != NAME_Thinker && cls->TypeName != NAME_Actor)
-		{
-			Printf(TEXTCOLOR_RED "Native thinkers cannot travel\n");
-			return;
-		}
-	}
 	else if (statnum == STAT_TRAVELLING)
 	{
-		Printf(TEXTCOLOR_RED "Thinkers cannot be added to STAT_TRAVELLING while the game isn't changing levels\n");
+		Printf(TEXTCOLOR_RED "Thinkers cannot be added to STAT_TRAVELLING manually\n");
 		return;
 	}
 
@@ -1005,6 +999,50 @@ DEFINE_ACTION_FUNCTION_NATIVE(DThinker, ChangeStatNum, ChangeStatNum)
 //
 //==========================================================================
 
+static void AddToTravellingList(DThinker* self)
+{
+	// These should be handled by the owning Actor, otherwise they'll lose them and become useless anyway.
+	if (self->IsKindOf(NAME_Inventory) && self->PointerVar<AActor>(NAME_Owner) != nullptr)
+	{
+		Printf(TEXTCOLOR_RED "Owned Inventory items must travel with their owner on level change\n");
+		return;
+	}
+	if (self->IsKindOf(NAME_Bot))
+	{
+		Printf(TEXTCOLOR_RED "Bot Thinkers must travel with their owner on level change\n");
+		return;
+	}
+	auto mo = dyn_cast<AActor>(self);
+	if (mo != nullptr && (mo->flags & MF_UNMORPHED))
+	{
+		Printf(TEXTCOLOR_RED "Unmorphed Actors must travel with their owner on level change\n");
+		return;
+	}
+	// These need to be locked down since they have native fields that won't be cleared
+	// properly at the moment.
+	auto cls = self->GetClass()->NativeClass();
+	if (cls->TypeName != NAME_Thinker && cls->TypeName != NAME_Actor)
+	{
+		Printf(TEXTCOLOR_RED "Native thinkers cannot travel\n");
+		return;
+	}
+
+	self->Level->AddToTravellingList(self);
+}
+
+DEFINE_ACTION_FUNCTION_NATIVE(DThinker, AddToTravellingList, AddToTravellingList)
+{
+	PARAM_SELF_PROLOGUE(DThinker);
+	AddToTravellingList(self);
+	return 0;
+}
+
+//==========================================================================
+//
+//
+//
+//==========================================================================
+
 CCMD(profilethinkers)
 {
 	const int argc = argv.argc();

src/playsim/dthinker.h

@@ -50,8 +50,6 @@ class FThinkerIterator;
 
 enum { MAX_STATNUM = 127 };
 
-extern bool bTravelling;
-
 // Doubly linked ring list of thinkers
 struct FThinkerList
 {
@@ -62,6 +60,7 @@ struct FThinkerList
 	bool IsEmpty() const;
 	void DestroyThinkers();
 	bool DoDestroyThinkers();
+	void RemoveTravellers(bool saveGame);
 	void OnLoad();
 	int TickThinkers(FThinkerList *dest);	// Returns: # of thinkers ticked
 	int ProfileThinkers(FThinkerList *dest);
@@ -84,6 +83,7 @@ struct FThinkerCollection
 	void RunThinkers(FLevelLocals *Level);	// The level is needed to tick the lights
 	void RunClientsideThinkers(FLevelLocals* Level);
 	void DestroyAllThinkers(bool fullgc = true);
+	void CleanUpTravellers(bool saveGame);
 	void SerializeThinkers(FSerializer &arc, bool keepPlayers);
 	void MarkRoots();
 	void OnLoad();

wadsrc/static/zscript/doombase.zs

@@ -241,6 +241,7 @@ class Thinker : Object native play
 	virtual native void Tick();
 	virtual native void PostBeginPlay();
 	virtual void OnLoad() {}
+	native void AddToTravellingList();
 	native void ChangeStatNum(int stat);
 	native clearscope int GetStatNum() const;