From 9b17dea7274d667e191fb6e5572e6009e50b875a Mon Sep 17 00:00:00 2001 From: Randy Heit Date: Thu, 13 Mar 2008 23:00:33 +0000 Subject: [PATCH] - 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 | 10 ++++++++- src/d_player.h | 2 +- src/dobject.cpp | 16 ++++++++----- src/dobject.h | 4 ++-- src/dthinker.cpp | 58 +++++++++++++++++++++++++++++------------------- src/dthinker.h | 5 ++--- src/farchive.cpp | 24 +++++++++++++++----- src/p_user.cpp | 28 ++++++++++++----------- 8 files changed, 93 insertions(+), 54 deletions(-) diff --git a/docs/rh-log.txt b/docs/rh-log.txt index 20d7126423..1366d8ee05 100644 --- a/docs/rh-log.txt +++ b/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. diff --git a/src/d_player.h b/src/d_player.h index 27553ea406..934adbf9ca 100644 --- a/src/d_player.h +++ b/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); diff --git a/src/dobject.cpp b/src/dobject.cpp index 711fd934c4..304d1c87d1 100644 --- a/src/dobject.cpp +++ b/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(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(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(old)) { sectors[i].f = static_cast(notOld); } + if (sectors[i].f == static_cast(old)) { sectors[i].f = static_cast(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) diff --git a/src/dobject.h b/src/dobject.h index b44e763a51..f7d5811a2b 100644 --- a/src/dobject.h +++ b/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); - static void StaticPointerSubstitution (DObject *old, DObject *notOld); + virtual size_t PointerSubstitution (DObject *old, DObject *notOld); + static size_t StaticPointerSubstitution (DObject *old, DObject *notOld); PClass *GetClass() const { diff --git a/src/dthinker.cpp b/src/dthinker.cpp index 92003bb5c6..94fc9646f4 100644 --- a/src/dthinker.cpp +++ b/src/dthinker.cpp @@ -44,14 +44,11 @@ static cycle_t ThinkCycles; extern cycle_t BotSupportCycles; extern cycle_t BotWTG; -IMPLEMENT_POINTY_CLASS (DThinker) - DECLARE_POINTER(NextThinker) - DECLARE_POINTER(PrevThinker) -END_POINTERS +IMPLEMENT_CLASS (DThinker) 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) - { - DThinker *node = list.Sentinel->NextThinker; - while (node != list.Sentinel) + { // Move all players to an ancillary list to ensure + // that the collector won't consider them dead. + while (list.Sentinel->NextThinker != list.Sentinel) { - DThinker *next = node->NextThinker; - node->Remove(); - node = next; + DThinker *thinker = list.Sentinel->NextThinker; + thinker->Remove(); + 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 - // the linked thinker list or the game will freeze badly. - DThinker *next = NextThinker; - DThinker *prev = PrevThinker; - Super::PointerSubstitution(old, notOld); - NextThinker = next; - PrevThinker = prev; + assert(NextThinker != NULL && !(NextThinker->ObjectFlags & OF_EuthanizeMe)); + assert(PrevThinker != NULL && !(PrevThinker->ObjectFlags & OF_EuthanizeMe)); + GC::Mark(NextThinker); + GC::Mark(PrevThinker); + return Super::PropagateMark(); } FThinkerIterator::FThinkerIterator (const PClass *type, int statnum) diff --git a/src/dthinker.h b/src/dthinker.h index a9fc91e72e..4d0dc24797 100644 --- a/src/dthinker.h +++ b/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; diff --git a/src/farchive.cpp b/src/farchive.cpp index 2087b7bed5..a5fd352ed4 100644 --- a/src/farchive.cpp +++ b/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; - item != NULL; item = item->Inventory) + AInventory *item; + + 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 diff --git a/src/p_user.cpp b/src/p_user.cpp index 4d2dccac43..89104a50d5 100644 --- a/src/p_user.cpp +++ b/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(rep); - if (mo == old) mo = replacement; - if (poisoner == old) poisoner = replacement; - if (attacker == old) attacker = replacement; - if (camera == old) camera = replacement; - if (dest == old) dest = replacement; - if (prev == old) prev = replacement; - if (enemy == old) enemy = replacement; - if (missile == old) missile = replacement; - if (mate == old) mate = replacement; - if (last_mate == old) last_mate = replacement; - if (ReadyWeapon == old) ReadyWeapon = static_cast(rep); - if (PendingWeapon == old) PendingWeapon = static_cast(rep); + size_t changed = 0; + if (mo == old) mo = replacement, changed++; + if (poisoner == old) poisoner = replacement, changed++; + if (attacker == old) attacker = replacement, changed++; + if (camera == old) camera = replacement, changed++; + if (dest == old) dest = replacement, changed++; + if (prev == old) prev = replacement, changed++; + if (enemy == old) enemy = replacement, changed++; + if (missile == old) missile = replacement, changed++; + if (mate == old) mate = replacement, changed++; + if (last_mate == old) last_mate = replacement, changed++; + if (ReadyWeapon == old) ReadyWeapon = static_cast(rep), changed++; + if (PendingWeapon == old) PendingWeapon = static_cast(rep), changed++; + return changed; } size_t player_s::PropagateMark()