- Fixed: When returning to a hub map, inventory belonging to the temporary

copies of the players stored in the archive would be destroyed and break
  apart the STAT_INVENTORY thinker list. Now they aren't destroyed while
  loading the archive but are unlinked from the inventory list instead, so
  the garbage collector will get them later.
- Changed pointer substitution to return the number of pointers changed.


SVN r803 (trunk)

docs/rh-log.txt

@@ -1,3 +1,11 @@
+March 13, 2008
+- Fixed: When returning to a hub map, inventory belonging to the temporary
+  copies of the players stored in the archive would be destroyed and break
+  apart the STAT_INVENTORY thinker list. Now they aren't destroyed while
+  loading the archive but are unlinked from the inventory list instead, so
+  the garbage collector will get them later.
+- Changed pointer substitution to return the number of pointers changed.
+
 March 13, 2008 (Changes by Graf Zahl)
 - Fixed: Pointer substitution changed the pointers for the thinker ring list
   resulting in a freeze.
@@ -7,7 +15,7 @@ March 12, 2008
 - Loading a save game now initiates a collection.
 - Added a finalization cost to the sweep stage.
 - Fixed: D_dehacked.cpp/PatchThing() allocated an actor on the stack.
-- Changed the sentinels in the thinker lists into a proper thinker. The old
+- Changed the sentinels in the thinker lists into proper thinkers. The old
   way wasn't playing well with the write barriers.
 - Fixed: DFrameBuffer::WriteSavePic needs to fix the canvas in place while
   using it.

src/d_player.h

@@ -191,7 +191,7 @@ public:
 	player_s();
 
 	void Serialize (FArchive &arc);
-	void FixPointers (const DObject *obj, DObject *replacement);
+	size_t FixPointers (const DObject *obj, DObject *replacement);
 	size_t PropagateMark();
 
 	void SetLogNumber (int num);

src/dobject.cpp

@@ -432,10 +432,11 @@ size_t DObject::PropagateMark()
 	return info->Size;
 }
 
-void DObject::PointerSubstitution (DObject *old, DObject *notOld)
+size_t DObject::PointerSubstitution (DObject *old, DObject *notOld)
 {
 	const PClass *info = GetClass();
 	const size_t *offsets = info->FlatPointers;
+	size_t changed = 0;
 	if (offsets == NULL)
 	{
 		const_cast<PClass *>(info)->BuildFlatPointers();
@@ -446,20 +447,23 @@ void DObject::PointerSubstitution (DObject *old, DObject *notOld)
 		if (*(DObject **)((BYTE *)this + *offsets) == old)
 		{
 			*(DObject **)((BYTE *)this + *offsets) = notOld;
+			changed++;
 		}
 		offsets++;
 	}
+	return changed;
 }
 
-void DObject::StaticPointerSubstitution (DObject *old, DObject *notOld)
+size_t DObject::StaticPointerSubstitution (DObject *old, DObject *notOld)
 {
 	DObject *probe;
+	size_t changed = 0;
 	int i;
 
 	// Go through all objects.
 	for (probe = GC::Root; probe != NULL; probe = probe->ObjNext)
 	{
-		probe->PointerSubstitution(old, notOld);
+		changed += probe->PointerSubstitution(old, notOld);
 	}
 
 	// Go through the bodyque.
@@ -468,6 +472,7 @@ void DObject::StaticPointerSubstitution (DObject *old, DObject *notOld)
 		if (bodyque[i] == old)
 		{
 			bodyque[i] = static_cast<AActor *>(notOld);
+			changed++;
 		}
 	}
 
@@ -475,7 +480,7 @@ void DObject::StaticPointerSubstitution (DObject *old, DObject *notOld)
 	for (i = 0; i < MAXPLAYERS; i++)
 	{
 		if (playeringame[i])
-			players[i].FixPointers (old, notOld);
+			changed += players[i].FixPointers (old, notOld);
 	}
 
 	// Go through sectors.
@@ -484,7 +489,7 @@ void DObject::StaticPointerSubstitution (DObject *old, DObject *notOld)
 		for (i = 0; i < numsectors; ++i)
 		{
 #define SECTOR_CHECK(f,t) \
-	if (sectors[i].f == static_cast<t *>(old)) { sectors[i].f = static_cast<t *>(notOld); }
+	if (sectors[i].f == static_cast<t *>(old)) { sectors[i].f = static_cast<t *>(notOld); changed++; }
 			SECTOR_CHECK( SoundTarget, AActor );
 			SECTOR_CHECK( CeilingSkyBox, ASkyViewpoint );
 			SECTOR_CHECK( FloorSkyBox, ASkyViewpoint );
@@ -495,6 +500,7 @@ void DObject::StaticPointerSubstitution (DObject *old, DObject *notOld)
 #undef SECTOR_CHECK
 		}
 	}
+	return changed;
 }
 
 void DObject::Serialize (FArchive &arc)

src/dobject.h

@@ -453,8 +453,8 @@ public:
 	// If you need to replace one object with another and want to
 	// change any pointers from the old object to the new object,
 	// use this method.
-	virtual void PointerSubstitution (DObject *old, DObject *notOld);
+	virtual size_t PointerSubstitution (DObject *old, DObject *notOld);
-	static void StaticPointerSubstitution (DObject *old, DObject *notOld);
+	static size_t StaticPointerSubstitution (DObject *old, DObject *notOld);
 
 	PClass *GetClass() const
 	{

src/dthinker.cpp

@@ -44,14 +44,11 @@ static cycle_t ThinkCycles;
 extern cycle_t BotSupportCycles;
 extern cycle_t BotWTG;
 
-IMPLEMENT_POINTY_CLASS (DThinker)
+IMPLEMENT_CLASS (DThinker)
- DECLARE_POINTER(NextThinker)
- DECLARE_POINTER(PrevThinker)
-END_POINTERS
 
 static DThinker *NextToThink;
 
-FThinkerList DThinker::Thinkers[MAX_STATNUM+1];
+FThinkerList DThinker::Thinkers[MAX_STATNUM+2];
 FThinkerList DThinker::FreshThinkers[MAX_STATNUM+1];
 bool DThinker::bSerialOverride = false;
 
@@ -65,6 +62,7 @@ void FThinkerList::AddTail(DThinker *thinker)
 		Sentinel->PrevThinker = Sentinel;
 		GC::WriteBarrier(Sentinel);
 	}
+	assert(thinker->PrevThinker == NULL && thinker->NextThinker == NULL);
 	DThinker *tail = Sentinel->PrevThinker;
 	assert(tail->NextThinker == Sentinel);
 	thinker->PrevThinker = tail;
@@ -83,6 +81,7 @@ DThinker *FThinkerList::GetHead() const
 	{
 		return NULL;
 	}
+	assert(Sentinel->NextThinker->PrevThinker == Sentinel);
 	return Sentinel->NextThinker;
 }
 
@@ -106,6 +105,7 @@ void DThinker::SaveList(FArchive &arc, DThinker *node)
 	{
 		while (!(node->ObjectFlags & OF_Sentinel))
 		{
+			assert(node->NextThinker != NULL && !(node->NextThinker->ObjectFlags & OF_EuthanizeMe));
 			arc << node;
 			node = node->NextThinker;
 		}
@@ -164,6 +164,12 @@ void DThinker::SerializeAll(FArchive &arc, bool hubLoad)
 				arc << stat << thinker;
 				while (thinker != NULL)
 				{
+					// This may be a player stored in their ancillary list. Remove
+					// them first before inserting them into the new list.
+					if (thinker->NextThinker != NULL)
+					{
+						thinker->Remove();
+					}
 					// Thinkers with the OF_JustSpawned flag set go in the FreshThinkers
 					// list. Anything else goes in the regular Thinkers list.
 					if (thinker->ObjectFlags & OF_JustSpawned)
@@ -191,10 +197,10 @@ void DThinker::SerializeAll(FArchive &arc, bool hubLoad)
 
 DThinker::DThinker (int statnum) throw()
 {
+	NextThinker = NULL;
+	PrevThinker = NULL;
 	if (bSerialOverride)
 	{ // The serializer will insert us into the right list
-		NextThinker = NULL;
-		PrevThinker = NULL;
 		return;
 	}
 
@@ -218,6 +224,8 @@ DThinker::~DThinker ()
 
 void DThinker::Destroy ()
 {
+	assert((NextThinker != NULL && PrevThinker != NULL) ||
+		   (NextThinker == NULL && PrevThinker == NULL));
 	if (NextThinker != NULL)
 	{
 		Remove();
@@ -234,6 +242,9 @@ void DThinker::Remove()
 	DThinker *prev = PrevThinker;
 	DThinker *next = NextThinker;
 	assert(prev != NULL && next != NULL);
+	assert((ObjectFlags & OF_Sentinel) || (prev != this && next != this));
+	assert(prev->NextThinker == this);
+	assert(next->PrevThinker == this);
 	prev->NextThinker = next;
 	next->PrevThinker = prev;
 	GC::WriteBarrier(prev, next);
@@ -270,6 +281,9 @@ void DThinker::ChangeStatNum (int statnum)
 {
 	FThinkerList *list;
 
+	// This thinker should already be in a list; verify that.
+	assert(NextThinker != NULL && PrevThinker != NULL);
+
 	if ((unsigned)statnum > MAX_STATNUM)
 	{
 		statnum = MAX_STATNUM;
@@ -289,7 +303,7 @@ void DThinker::ChangeStatNum (int statnum)
 // Mark the first thinker of each list
 void DThinker::MarkRoots()
 {
-	for (int i = 0; i <= MAX_STATNUM; ++i)
+	for (int i = 0; i <= MAX_STATNUM+1; ++i)
 	{
 		GC::Mark(Thinkers[i].Sentinel);
 		GC::Mark(FreshThinkers[i].Sentinel);
@@ -301,7 +315,7 @@ void DThinker::DestroyAllThinkers ()
 {
 	int i;
 
-	for (i = 0; i <= MAX_STATNUM; i++)
+	for (i = 0; i <= MAX_STATNUM+1; i++)
 	{
 		if (i != STAT_TRAVELLING)
 		{
@@ -353,13 +367,13 @@ void DThinker::DestroyMostThinkersInList (FThinkerList &list, int stat)
 		DestroyThinkersInList (list);
 	}
 	else if (list.Sentinel != NULL)
-	{
+	{ // Move all players to an ancillary list to ensure
-		DThinker *node = list.Sentinel->NextThinker;
+	  // that the collector won't consider them dead.
-		while (node != list.Sentinel)
+		while (list.Sentinel->NextThinker != list.Sentinel)
 		{
-			DThinker *next = node->NextThinker;
+			DThinker *thinker = list.Sentinel->NextThinker;
-			node->Remove();
+			thinker->Remove();
-			node = next;
+			Thinkers[MAX_STATNUM+1].AddTail(thinker);
 		}
 		list.Sentinel->Destroy();
 		list.Sentinel = NULL;
@@ -435,15 +449,13 @@ void DThinker::Tick ()
 {
 }
 
-void DThinker::PointerSubstitution(DObject *old, DObject *notOld)
+size_t DThinker::PropagateMark()
 {
-	// Pointer substitution must not, under any circumstances, change 
+	assert(NextThinker != NULL && !(NextThinker->ObjectFlags & OF_EuthanizeMe));
-	// the linked thinker list or the game will freeze badly.
+	assert(PrevThinker != NULL && !(PrevThinker->ObjectFlags & OF_EuthanizeMe));
-	DThinker *next = NextThinker;
+	GC::Mark(NextThinker);
-	DThinker *prev = PrevThinker;
+	GC::Mark(PrevThinker);
-	Super::PointerSubstitution(old, notOld);
+	return Super::PropagateMark();
-	NextThinker = next;
-	PrevThinker = prev;
 }
 
 FThinkerIterator::FThinkerIterator (const PClass *type, int statnum)

src/dthinker.h

@@ -62,14 +62,13 @@ struct FThinkerList
 class DThinker : public DObject
 {
 	DECLARE_CLASS (DThinker, DObject)
-	HAS_OBJECT_POINTERS
 public:
 	DThinker (int statnum = MAX_STATNUM) throw();
 	void Destroy ();
 	virtual ~DThinker ();
 	virtual void Tick ();
 	virtual void PostBeginPlay ();	// Called just before the first tick
-	void PointerSubstitution(DObject *old, DObject *notOld);
+	size_t PropagateMark();
 	
 	void ChangeStatNum (int statnum);
 
@@ -91,7 +90,7 @@ private:
 	static void SaveList(FArchive &arc, DThinker *node);
 	void Remove();
 
-	static FThinkerList Thinkers[MAX_STATNUM+1];		// Current thinkers
+	static FThinkerList Thinkers[MAX_STATNUM+2];		// Current thinkers
 	static FThinkerList FreshThinkers[MAX_STATNUM+1];	// Newly created thinkers
 	static bool bSerialOverride;
 

src/farchive.cpp

@@ -1005,10 +1005,6 @@ FArchive &FArchive::SerializeObject (DObject *&object, PClass *type)
 {
 	if (IsStoring ())
 	{
-		if (object != (DObject*)~0)
-		{
-			GC::ReadBarrier(object);
-		}
 		return WriteObject (object);
 	}
 	else
@@ -1032,6 +1028,13 @@ FArchive &FArchive::WriteObject (DObject *obj)
 		id[0] = M1_OBJ;
 		Write (id, 1);
 	}
+	else if (obj->ObjectFlags & OF_EuthanizeMe)
+	{
+		// Objects that want to die are not saved to the archive, but
+		// we leave the pointers to them alone.
+		id[0] = NULL_OBJ;
+		Write (id, 1);
+	}
 	else
 	{
 		const PClass *type = RUNTIME_TYPE(obj);
@@ -1159,11 +1162,20 @@ FArchive &FArchive::ReadObject (DObject* &obj, PClass *wanttype)
 			{
 				// When the temporary player's inventory items were loaded,
 				// they became owned by the real player. Undo that now.
-				for (AInventory *item = tempobj->Inventory;
+				AInventory *item;
-					item != NULL; item = item->Inventory)
+
+				for (item = tempobj->Inventory; item != NULL; item = item->Inventory)
 				{
 					item->Owner = tempobj;
 				}
+				item = tempobj->Inventory;
+				tempobj->Inventory = NULL;
+#ifdef _DEBUG
+				// The only references to this inventory list should be from the
+				// temporary player, so they will be freed when a collection occurs.
+				size_t a = 0;
+				assert(item == NULL || (a = DObject::StaticPointerSubstitution(item, NULL)) == 0);
+#endif
 				tempobj->Destroy ();
 			}
 			else

src/p_user.cpp

@@ -299,21 +299,23 @@ player_s::player_s()
 // that match the pointer passed in. If you add any pointers that point to
 // DObject (or a subclass), add them here too.
 
-void player_s::FixPointers (const DObject *old, DObject *rep)
+size_t player_s::FixPointers (const DObject *old, DObject *rep)
 {
 	APlayerPawn *replacement = static_cast<APlayerPawn *>(rep);
-	if (mo == old)				mo = replacement;
+	size_t changed = 0;
-	if (poisoner == old)		poisoner = replacement;
+	if (mo == old)				mo = replacement, changed++;
-	if (attacker == old)		attacker = replacement;
+	if (poisoner == old)		poisoner = replacement, changed++;
-	if (camera == old)			camera = replacement;
+	if (attacker == old)		attacker = replacement, changed++;
-	if (dest == old)			dest = replacement;
+	if (camera == old)			camera = replacement, changed++;
-	if (prev == old)			prev = replacement;
+	if (dest == old)			dest = replacement, changed++;
-	if (enemy == old)			enemy = replacement;
+	if (prev == old)			prev = replacement, changed++;
-	if (missile == old)			missile = replacement;
+	if (enemy == old)			enemy = replacement, changed++;
-	if (mate == old)			mate = replacement;
+	if (missile == old)			missile = replacement, changed++;
-	if (last_mate == old)		last_mate = replacement;
+	if (mate == old)			mate = replacement, changed++;
-	if (ReadyWeapon == old)		ReadyWeapon = static_cast<AWeapon *>(rep);
+	if (last_mate == old)		last_mate = replacement, changed++;
-	if (PendingWeapon == old)	PendingWeapon = static_cast<AWeapon *>(rep);
+	if (ReadyWeapon == old)		ReadyWeapon = static_cast<AWeapon *>(rep), changed++;
+	if (PendingWeapon == old)	PendingWeapon = static_cast<AWeapon *>(rep), changed++;
+	return changed;
 }
 
 size_t player_s::PropagateMark()