Switch STRUCTURE to use PagedEntityContainer<STRUCTURE> as backin…

…g storage

1. Split the structure storage into pages containing 256 structs
2. Disable slot reuse to guard against memory-related issues
   when some object pointers won't get updated properly,
   e.g. when transitioning between the base and offworld missions.

Signed-off-by: Pavel Solodovnikov <[email protected]>

src/display3d.cpp

@@ -363,7 +363,8 @@ struct Blueprint
 			STRUCTURE *psStruct = buildBlueprint();
 			ASSERT_OR_RETURN(, psStruct != nullptr, "buildBlueprint returned nullptr");
 			renderStructure(psStruct, viewMatrix, perspectiveViewMatrix);
-			delete psStruct;
+
+			objmemDestroy(psStruct, false);
 		}
 	}
 
@@ -692,7 +693,11 @@ STRUCTURE *getTileBlueprintStructure(int mapX, int mapY)
 	Blueprint blueprint = getTileBlueprint(mapX, mapY);
 	if (blueprint.state == SS_BLUEPRINT_PLANNED)
 	{
-		delete psStruct;  // Delete previously returned structure, if any.
+		if (psStruct)
+		{
+			// Delete previously returned structure, if any.
+			objmemDestroy(psStruct, false);
+		}
 		psStruct = blueprint.buildBlueprint();
 		return psStruct;  // This blueprint was clicked on.
 	}

src/objmem.cpp

@@ -226,14 +226,22 @@ bool objmemDestroy(BASE_OBJECT *psObj, bool checkRefs)
 	{
 		return false;
 	}
-	// Droids are managed by a separate droid container.
 	if (psObj->type == OBJ_DROID)
 	{
+		// Droids are managed by a separate droid container.
 		auto& droidContainer = GlobalDroidContainer();
 		auto it = droidContainer.find(*static_cast<DROID*>(psObj));
 		ASSERT(it != droidContainer.end(), "Droid not found in the global container!");
 		droidContainer.erase(it);
 	}
+	else if (psObj->type == OBJ_STRUCTURE)
+	{
+		// Structs are managed by a separate struct container.
+		auto& structContainer = GlobalStructContainer();
+		auto it = structContainer.find(*static_cast<STRUCTURE*>(psObj));
+		ASSERT(it != structContainer.end(), "Structure not found in the global container!");
+		structContainer.erase(it);
+	}
 	else
 	{
 		delete psObj;
@@ -452,16 +460,53 @@ void killDroid(DROID *psDel)
 	destroyObject(apsDroidLists, psDel);
 }
 
-static void freeAllDroidsImpl(PerPlayerDroidLists& droidLists)
+template <typename EntityType>
+struct GlobalEntityContainerTraits;
+
+template <>
+struct GlobalEntityContainerTraits<DROID>
 {
-	auto& droidContainer = GlobalDroidContainer();
-	for (auto& list : droidLists)
+	using StorageType = DroidContainer;
+
+	static DroidContainer& getContainer()
+	{
+		return GlobalDroidContainer();
+	}
+
+	static const char* entityName()
+	{
+		return "Droid";
+	}
+};
+
+template <>
+struct GlobalEntityContainerTraits<STRUCTURE>
+{
+	using StorageType = StructContainer;
+
+	static StructContainer& getContainer()
+	{
+		return GlobalStructContainer();
+	}
+
+	static const char* entityName()
+	{
+		return "Structure";
+	}
+};
+
+template <typename Entity, unsigned PlayerCount>
+static void freeAllEntitiesImpl(PerPlayerObjectLists<Entity, PlayerCount>& entityLists)
+{
+	using Traits = GlobalEntityContainerTraits<Entity>;
+	auto& entityContainer = Traits::getContainer();
+	for (auto& list : entityLists)
 	{
-		for (DROID* d : list)
+		for (auto* ent : list)
 		{
-			auto it = droidContainer.find(*d);
-			ASSERT(it != droidContainer.end(), "Droid not found in the global container!");
-			droidContainer.erase(it);
+			auto it = entityContainer.find(*ent);
+			ASSERT(it != entityContainer.end(), "%s not found in the global container!", Traits::entityName());
+			entityContainer.erase(it);
 		}
 		list.clear();
 	}
@@ -470,7 +515,7 @@ static void freeAllDroidsImpl(PerPlayerDroidLists& droidLists)
 /* Remove all droids */
 void freeAllDroids()
 {
-	freeAllDroidsImpl(apsDroidLists);
+	freeAllEntitiesImpl<DROID, MAX_PLAYERS>(apsDroidLists);
 }
 
 /*Remove a single Droid from a list*/
@@ -503,13 +548,13 @@ void removeDroid(DROID* psDroidToRemove, PerPlayerDroidLists& pList)
 /*Removes all droids that may be stored in the mission lists*/
 void freeAllMissionDroids()
 {
-	freeAllDroidsImpl(mission.apsDroidLists);
+	freeAllEntitiesImpl<DROID, MAX_PLAYERS>(mission.apsDroidLists);
 }
 
 /*Removes all droids that may be stored in the limbo lists*/
 void freeAllLimboDroids()
 {
-	freeAllDroidsImpl(apsLimboDroids);
+	freeAllEntitiesImpl<DROID, MAX_PLAYERS>(apsLimboDroids);
 }
 
 /**************************  STRUCTURE  *******************************/
@@ -592,7 +637,7 @@ void killStruct(STRUCTURE *psBuilding)
 /* Remove heapall structures */
 void freeAllStructs()
 {
-	releaseAllObjectsInList(apsStructLists);
+	freeAllEntitiesImpl<STRUCTURE, MAX_PLAYERS>(apsStructLists);
 }
 
 /*Remove a single Structure from a list*/

src/structure.cpp

@@ -1414,31 +1414,27 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y
 				}
 		}
 
-		// allocate memory for and initialize a structure object
-		psBuilding = new STRUCTURE(id, player);
-		if (psBuilding == nullptr)
-		{
-			return nullptr;
-		}
+		// initialize the structure object
+		STRUCTURE building(id, player);
 
 		//fill in other details
-		psBuilding->pStructureType = pStructureType;
+		building.pStructureType = pStructureType;
 
-		psBuilding->pos.x = x;
-		psBuilding->pos.y = y;
-		psBuilding->rot.direction = snapDirection(direction);
-		psBuilding->rot.pitch = 0;
-		psBuilding->rot.roll = 0;
+		building.pos.x = x;
+		building.pos.y = y;
+		building.rot.direction = snapDirection(direction);
+		building.rot.pitch = 0;
+		building.rot.roll = 0;
 
 		//This needs to be done before the functionality bit...
 		//load into the map data and structure list if not an upgrade
 		Vector2i map = map_coord(Vector2i(x, y)) - size / 2;
 
 		//set up the imd to use for the display
-		psBuilding->sDisplay.imd = pStructureType->pIMD[0];
+		building.sDisplay.imd = pStructureType->pIMD[0];
 
-		psBuilding->state = SAS_NORMAL;
-		psBuilding->lastStateTime = gameTime;
+		building.state = SAS_NORMAL;
+		building.lastStateTime = gameTime;
 
 		/* if resource extractor - need to remove oil feature first, but do not do any
 		 * consistency checking here - save games do not have any feature to remove
@@ -1452,7 +1448,6 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y
 				if (fireOnLocation(psFeature->pos.x, psFeature->pos.y))
 				{
 					// Can't build on burning oil resource
-					delete psBuilding;
 					return nullptr;
 				}
 				// remove it from the map
@@ -1486,16 +1481,18 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y
 #if defined(WZ_CC_GNU) && !defined(WZ_CC_INTEL) && !defined(WZ_CC_CLANG) && (7 <= __GNUC__)
 # pragma GCC diagnostic pop
 #endif
-					delete psBuilding;
 					return nullptr;
 				}
 			}
 		}
+		// Emplace the structure being built in the global storage to obtain stable address.
+		STRUCTURE& stableBuilding = GlobalStructContainer().emplace(std::move(building));
+		psBuilding = &stableBuilding;
 		for (int tileY = map.y; tileY < map.y + size.y; ++tileY)
 		{
 			for (int tileX = map.x; tileX < map.x + size.x; ++tileX)
 			{
-				// We now know the previous loop didn't return early, so it is safe to save references to psBuilding now.
+				// We now know the previous loop didn't return early, so it is safe to save references to `stableBuilding` now.
 				MAPTILE *psTile = mapTile(tileX, tileY);
 				psTile->psObject = psBuilding;
 
@@ -1612,7 +1609,7 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y
 		if (!setFunctionality(psBuilding, pStructureType->type))
 		{
 			removeStructFromMap(psBuilding);
-			delete psBuilding;
+			objmemDestroy(psBuilding, false);
 			//better reset these if you couldn't build the structure!
 			if (FromSave && player == selectedPlayer && missionLimboExpand())
 			{
@@ -1834,8 +1831,6 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y
 
 STRUCTURE *buildBlueprint(STRUCTURE_STATS const *psStats, Vector3i pos, uint16_t direction, unsigned moduleIndex, STRUCT_STATES state, uint8_t ownerPlayer)
 {
-	STRUCTURE *blueprint = nullptr;
-
 	ASSERT_OR_RETURN(nullptr, psStats != nullptr, "No blueprint stats");
 	ASSERT_OR_RETURN(nullptr, psStats->pIMD[0] != nullptr, "No blueprint model for %s", getStatsName(psStats));
 	ASSERT_OR_RETURN(nullptr, ownerPlayer < MAX_PLAYERS, "invalid ownerPlayer: %" PRIu8 "", ownerPlayer);
@@ -1866,55 +1861,57 @@ STRUCTURE *buildBlueprint(STRUCTURE_STATS const *psStats, Vector3i pos, uint16_t
 		}
 	}
 
-	blueprint = new STRUCTURE(0, ownerPlayer);
+	STRUCTURE blueprint(0, ownerPlayer);
 	// construct the fake structure
-	blueprint->pStructureType = const_cast<STRUCTURE_STATS *>(psStats);  // Couldn't be bothered to fix const correctness everywhere.
+	blueprint.pStructureType = const_cast<STRUCTURE_STATS *>(psStats);  // Couldn't be bothered to fix const correctness everywhere.
 	if (selectedPlayer < MAX_PLAYERS)
 	{
-		blueprint->visible[selectedPlayer] = UBYTE_MAX;
+		blueprint.visible[selectedPlayer] = UBYTE_MAX;
 	}
-	blueprint->sDisplay.imd = (*pIMD)[std::min<int>(moduleNumber, pIMD->size() - 1)];
-	blueprint->pos = pos;
-	blueprint->rot = rot;
-	blueprint->selected = false;
+	blueprint.sDisplay.imd = (*pIMD)[std::min<int>(moduleNumber, pIMD->size() - 1)];
+	blueprint.pos = pos;
+	blueprint.rot = rot;
+	blueprint.selected = false;
 
-	blueprint->numWeaps = 0;
-	blueprint->asWeaps[0].nStat = 0;
+	blueprint.numWeaps = 0;
+	blueprint.asWeaps[0].nStat = 0;
 
 	// give defensive structures a weapon
 	if (psStats->psWeapStat[0])
 	{
-		blueprint->asWeaps[0].nStat = psStats->psWeapStat[0] - asWeaponStats.data();
+		blueprint.asWeaps[0].nStat = psStats->psWeapStat[0] - asWeaponStats.data();
 	}
 	// things with sensors or ecm (or repair facilities) need these set, even if they have no official weapon
-	blueprint->numWeaps = 0;
-	blueprint->asWeaps[0].lastFired = 0;
-	blueprint->asWeaps[0].rot.pitch = 0;
-	blueprint->asWeaps[0].rot.direction = 0;
-	blueprint->asWeaps[0].rot.roll = 0;
-	blueprint->asWeaps[0].prevRot = blueprint->asWeaps[0].rot;
+	blueprint.numWeaps = 0;
+	blueprint.asWeaps[0].lastFired = 0;
+	blueprint.asWeaps[0].rot.pitch = 0;
+	blueprint.asWeaps[0].rot.direction = 0;
+	blueprint.asWeaps[0].rot.roll = 0;
+	blueprint.asWeaps[0].prevRot = blueprint.asWeaps[0].rot;
 
-	blueprint->expectedDamage = 0;
+	blueprint.expectedDamage = 0;
 
 	// Times must be different, but don't otherwise matter.
-	blueprint->time = 23;
-	blueprint->prevTime = 42;
+	blueprint.time = 23;
+	blueprint.prevTime = 42;
 
-	blueprint->status = state;
+	blueprint.status = state;
 
 	// Rotate wall if needed.
-	if (blueprint->pStructureType->type == REF_WALL || blueprint->pStructureType->type == REF_GATE)
+	if (blueprint.pStructureType->type == REF_WALL || blueprint.pStructureType->type == REF_GATE)
 	{
-		WallOrientation scanType = structChooseWallTypeBlueprint(map_coord(blueprint->pos.xy()));
+		WallOrientation scanType = structChooseWallTypeBlueprint(map_coord(blueprint.pos.xy()));
 		unsigned type = wallType(scanType);
 		if (scanType != WallConnectNone)
 		{
-			blueprint->rot.direction = wallDir(scanType);
-			blueprint->sDisplay.imd = blueprint->pStructureType->pIMD[std::min<unsigned>(type, blueprint->pStructureType->pIMD.size() - 1)];
+			blueprint.rot.direction = wallDir(scanType);
+			blueprint.sDisplay.imd = blueprint.pStructureType->pIMD[std::min<unsigned>(type, blueprint.pStructureType->pIMD.size() - 1)];
 		}
 	}
 
-	return blueprint;
+	auto& stableBlueprint = GlobalStructContainer().emplace(std::move(blueprint));
+
+	return &stableBlueprint;
 }
 
 static Vector2i defaultAssemblyPointPos(STRUCTURE *psBuilding)
@@ -7216,3 +7213,9 @@ LineBuild calcLineBuild(STRUCTURE_STATS const *stats, uint16_t direction, Vector
 {
 	return calcLineBuild(stats->size(direction), stats->type, pos, pos2);
 }
+
+StructContainer& GlobalStructContainer()
+{
+	static StructContainer instance;
+	return instance;
+}

src/structure.h

@@ -496,4 +496,10 @@ struct LineBuild
 LineBuild calcLineBuild(Vector2i size, STRUCTURE_TYPE type, Vector2i pos, Vector2i pos2);
 LineBuild calcLineBuild(STRUCTURE_STATS const *stats, uint16_t direction, Vector2i pos, Vector2i pos2);
 
+// Split the struct storage into pages containing 256 structs, disable slot reuse
+// to guard against memory-related issues when some object pointers won't get
+// updated properly, e.g. when transitioning between the base and offworld missions.
+using StructContainer = PagedEntityContainer<STRUCTURE, 256, false>;
+StructContainer& GlobalStructContainer();
+
 #endif // __INCLUDED_SRC_STRUCTURE_H__