Fix crashes on game start, gamestate now derives from GameCallbackHolder

client/Client.cpp

@@ -118,7 +118,7 @@ void CClient::newGame(std::shared_ptr<CGameState> initializedGameState)
 	CMapService mapService;
 	assert(initializedGameState);
 	gamestate = initializedGameState;
-	gamestate->preInit(LIBRARY, this);
+	gamestate->preInit(LIBRARY);
 	logNetwork->trace("\tCreating gamestate: %i", GAME->server().th->getDiff());
 
 	initMapHandler();
@@ -133,7 +133,7 @@ void CClient::loadGame(std::shared_ptr<CGameState> initializedGameState)
 
 	logNetwork->info("Game state was transferred over network, loading.");
 	gamestate = initializedGameState;
-	gamestate->preInit(LIBRARY, this);
+	gamestate->preInit(LIBRARY);
 	gamestate->updateOnLoad(GAME->server().si.get());
 	logNetwork->info("Game loaded, initialize interfaces.");
 

lib/CArtHandler.cpp

@@ -949,9 +949,9 @@ bool CArtifactSet::isPositionFree(const ArtifactPosition & pos, bool onlyLockChe
 
 void CArtifactSet::artDeserializationFix(CBonusSystemNode *node)
 {
-	for(auto & elem : artifactsWorn)
-		if(elem.second.getArt() && !elem.second.locked)
-			node->attachToSource(*elem.second.getArt());
+	//for(auto & elem : artifactsWorn)
+	//	if(elem.second.getArt() && !elem.second.locked)
+	//		node->attachToSource(*elem.second.getArt());
 }
 
 void CArtifactSet::serializeJsonArtifacts(JsonSerializeFormat & handler, const std::string & fieldName, CMap * map)

lib/CArtifactInstance.cpp

@@ -184,9 +184,9 @@ bool CArtifactInstance::isScroll() const
 
 void CArtifactInstance::deserializationFix()
 {
-	setType(artTypeID.toArtifact());
-	for(PartInfo & part : partsInfo)
-		attachToSource(*part.getArtifact());
+//	setType(artTypeID.toArtifact());
+//	for(PartInfo & part : partsInfo)
+//		attachToSource(*part.getArtifact());
 }
 
 VCMI_LIB_NAMESPACE_END

lib/CCreatureSet.cpp

@@ -334,6 +334,15 @@ void CCreatureSet::addToSlot(const SlotID & slot, std::unique_ptr<CStackInstance
 	}
 }
 
+void CCreatureSet::deserializationFix()
+{
+	for(const auto & elem : stacks)
+	{
+		elem.second->attachTo(*getArmy());
+		elem.second->artDeserializationFix(elem.second.get());
+	}
+}
+
 bool CCreatureSet::validTypes(bool allowUnrandomized) const
 {
 	for(const auto & elem : stacks)
@@ -517,7 +526,7 @@ void CCreatureSet::putStack(const SlotID & slot, std::unique_ptr<CStackInstance>
 	assert(slot.getNum() < GameConstants::ARMY_SIZE);
 	assert(!hasStackAtSlot(slot));
 	stacks[slot] = std::move(stack);
-	stack->setArmy(getArmy());
+	stacks[slot]->setArmy(getArmy());
 	armyChanged();
 }
 
@@ -536,10 +545,7 @@ void CCreatureSet::changeStackCount(const SlotID & slot, TQuantity toAdd)
 	setStackCount(slot, getStackCount(slot) + toAdd);
 }
 
-CCreatureSet::~CCreatureSet()
-{
-	clearSlots();
-}
+CCreatureSet::~CCreatureSet() = default;
 
 void CCreatureSet::setToArmy(CSimpleArmy &src)
 {
@@ -864,19 +870,11 @@ TerrainId CStackInstance::getNativeTerrain() const
 
 	return getFactionID().toEntity(LIBRARY)->getNativeTerrain();
 }
-
 TerrainId CStackInstance::getCurrentTerrain() const
 {
 	return getArmy()->getCurrentTerrain();
 }
 
-void CStackInstance::deserializationFix()
-{
-	const CArmedInstance *armyBackup = getArmy();
-	armyInstanceID = {};
-	setArmy(armyBackup);
-	artDeserializationFix(this);
-}
 
 CreatureID CStackInstance::getCreatureID() const
 {

lib/CCreatureSet.h

@@ -100,9 +100,6 @@ public:
 		h & static_cast<CArtifactSet&>(*this);
 		h & armyInstanceID;
 		h & experience;
-
-		if(!h.saving)
-			deserializationFix();
 	}
 
 	void serializeJson(JsonSerializeFormat & handler);
@@ -137,7 +134,6 @@ public:
 	void removeArtifact(const ArtifactPosition & pos) override;
 	ArtBearer::ArtBearer bearerType() const override; //from CArtifactSet
 	std::string nodeName() const override; //from CBonusSystemnode
-	void deserializationFix();
 	PlayerColor getOwner() const override;
 
 	int32_t getInitiative(int turn = 0) const final;
@@ -225,6 +221,9 @@ class DLL_LINKAGE CCreatureSet : public IArmyDescriptor, public virtual Serializ
 	CCreatureSet(const CCreatureSet &) = delete;
 	CCreatureSet &operator=(const CCreatureSet&);
 
+
+	void deserializationFix();
+
 public:
 	TSlots stacks; //slots[slot_id]->> pair(creature_id,creature_quantity)
 	EArmyFormation formation = EArmyFormation::LOOSE; //0 - wide, 1 - tight
@@ -297,6 +296,9 @@ public:
 	{
 		h & stacks;
 		h & formation;
+
+		if(!h.saving)
+			deserializationFix();
 	}
 
 	void serializeJson(JsonSerializeFormat & handler, const std::string & armyFieldName, const std::optional<int> fixedSize = std::nullopt);

lib/CGameInfoCallback.cpp

@@ -131,6 +131,13 @@ TurnTimerInfo CGameInfoCallback::getPlayerTurnTime(PlayerColor color) const
 
 const CGObjectInstance* CGameInfoCallback::getObj(ObjectInstanceID objid, bool verbose) const
 {
+	if (!objid.hasValue())
+	{
+		if(verbose)
+			logGlobal->error("Cannot get object with id %d. No such object", objid.getNum());
+		return nullptr;
+	}
+
 	const CGObjectInstance *ret = gameState()->getMap().getObject(objid);
 	if(!ret)
 	{

lib/gameState/CGameState.cpp

@@ -139,7 +139,8 @@ int CGameState::getDate(Date mode) const
 	return getDate(day, mode);
 }
 
-CGameState::CGameState()
+CGameState::CGameState(IGameCallback * callback)
+	: GameCallbackHolder(callback)
 {
 	heroesPool = std::make_unique<TavernHeroesPool>(this);
 	globalEffects.setNodeType(CBonusSystemNode::GLOBAL_EFFECTS);
@@ -156,16 +157,15 @@ const IGameSettings & CGameState::getSettings() const
 	return map->getSettings();
 }
 
-void CGameState::preInit(Services * newServices, IGameCallback * newCallback)
+void CGameState::preInit(Services * newServices)
 {
 	services = newServices;
-	callback = newCallback;
 }
 
 void CGameState::init(const IMapService * mapService, StartInfo * si, Load::ProgressAccumulator & progressTracking, bool allowSavingRandomMap)
 {
 	assert(services);
-	assert(callback);
+	assert(cb);
 	scenarioOps = CMemorySerializer::deepCopy(*si);
 	initialOpts = CMemorySerializer::deepCopy(*si);
 	si = nullptr;
@@ -271,7 +271,7 @@ void CGameState::updateEntity(Metatype metatype, int32_t index, const JsonNode &
 void CGameState::updateOnLoad(StartInfo * si)
 {
 	assert(services);
-	assert(callback);
+	assert(cb);
 	scenarioOps->playerInfos = si->playerInfos;
 	for(auto & i : si->playerInfos)
 		players.at(i.first).human = i.second.isControlledByHuman();
@@ -288,7 +288,7 @@ void CGameState::initNewGame(const IMapService * mapService, bool allowSavingRan
 		CStopWatch sw;
 
 		// Gen map
-		CMapGenerator mapGenerator(*scenarioOps->mapGenOptions, callback, getRandomGenerator().nextInt());
+		CMapGenerator mapGenerator(*scenarioOps->mapGenOptions, cb, getRandomGenerator().nextInt());
 		progressTracking.include(mapGenerator);
 
 		std::unique_ptr<CMap> randomMap = mapGenerator.generate();
@@ -351,7 +351,7 @@ void CGameState::initNewGame(const IMapService * mapService, bool allowSavingRan
 	{
 		logGlobal->info("Open map file: %s", scenarioOps->mapname);
 		const ResourcePath mapURI(scenarioOps->mapname, EResType::MAP);
-		map = mapService->loadMap(mapURI, callback);
+		map = mapService->loadMap(mapURI, cb);
 	}
 }
 
@@ -514,6 +514,7 @@ void CGameState::initPlayerStates()
 	logGlobal->debug("\tCreating player entries in gs");
 	for(auto & elem : scenarioOps->playerInfos)
 	{
+		players.try_emplace(elem.first, cb);
 		PlayerState & p = players.at(elem.first);
 		p.color=elem.first;
 		p.human = elem.second.isControlledByHuman();
@@ -536,7 +537,7 @@ void CGameState::placeStartingHero(const PlayerColor & playerColor, const HeroTy
 	}
 
 	auto handler = LIBRARY->objtypeh->getHandlerFor(Obj::HERO, heroTypeId.toHeroType()->heroClass->getIndex());
-	auto object = handler->create(callback, handler->getTemplates().front());
+	auto object = handler->create(cb, handler->getTemplates().front());
 	auto hero = std::dynamic_pointer_cast<CGHeroInstance>(object);
 
 	hero->ID = Obj::HERO;
@@ -604,7 +605,7 @@ void CGameState::initHeroes()
 		if (tile.isWater())
 		{
 			auto handler = LIBRARY->objtypeh->getHandlerFor(Obj::BOAT, hero->getBoatType().getNum());
-			auto boat = std::dynamic_pointer_cast<CGBoat>(handler->create(callback, nullptr));
+			auto boat = std::dynamic_pointer_cast<CGBoat>(handler->create(cb, nullptr));
 			handler->configureObject(boat.get(), getRandomGenerator());
 
 			boat->setAnchorPos(hero->anchorPos());
@@ -632,7 +633,7 @@ void CGameState::initHeroes()
 		}
 		else
 		{
-			auto vhi = std::make_shared<CGHeroInstance>(callback);
+			vhi = std::make_shared<CGHeroInstance>(cb);
 			vhi->initHero(getRandomGenerator(), htype);
 		}
 
@@ -924,7 +925,7 @@ void CGameState::initMapObjects()
 		if (q->ID ==Obj::QUEST_GUARD || q->ID ==Obj::SEER_HUT)
 			q->setObjToKill();
 	}
-	CGSubterraneanGate::postInit(callback); //pairing subterranean gates
+	CGSubterraneanGate::postInit(cb); //pairing subterranean gates
 
 	map->calculateGuardingGreaturePositions(); //calculate once again when all the guards are placed and initialized
 }
@@ -1572,8 +1573,10 @@ void CGameState::buildBonusSystemTree()
 
 void CGameState::deserializationFix()
 {
+	assert(cb != nullptr);
+
 	for(auto & player : players)
-		player.second.cb = callback;
+		player.second.cb = cb;
 
 	buildGlobalTeamPlayerTree();
 	attachArmedObjects();
@@ -1668,7 +1671,7 @@ TeamState::TeamState()
 
 vstd::RNG & CGameState::getRandomGenerator()
 {
-	return callback->getRandomGenerator();
+	return cb->getRandomGenerator();
 }
 
 ArtifactID CGameState::pickRandomArtifact(vstd::RNG & rand, int flags, std::function<bool(ArtifactID)> accepts)

lib/gameState/CGameState.h

@@ -11,6 +11,7 @@
 
 #include "../bonuses/CBonusSystemNode.h"
 #include "../IGameCallback.h"
+#include "../GameCallbackHolder.h"
 #include "../LoadProgress.h"
 
 #include "RumorState.h"
@@ -42,7 +43,7 @@ class UpgradeInfo;
 
 DLL_LINKAGE std::ostream & operator<<(std::ostream & os, const EVictoryLossCheckResult & victoryLossCheckResult);
 
-class DLL_LINKAGE CGameState : public CNonConstInfoCallback, public Serializeable
+class DLL_LINKAGE CGameState : public CNonConstInfoCallback, public Serializeable, public GameCallbackHolder
 {
 	friend class CGameStateCampaign;
 
@@ -64,15 +65,13 @@ public:
 	/// list of players currently making turn. Usually - just one, except for simturns
 	std::set<PlayerColor> actingPlayers;
 
-	IGameCallback * callback;
-
-	CGameState();
+	CGameState(IGameCallback * callback);
 	virtual ~CGameState();
 
 	CGameState * gameState() final { return this; }
 	const CGameState * gameState() const final { return this; }
 
-	void preInit(Services * services, IGameCallback * callback);
+	void preInit(Services * services);
 
 	void init(const IMapService * mapService, StartInfo * si, Load::ProgressAccumulator &, bool allowSavingRandomMap = true);
 	void updateOnLoad(StartInfo * si);

lib/gameState/CGameStateCampaign.cpp

@@ -684,7 +684,7 @@ bool CGameStateCampaign::playerHasStartingHero(PlayerColor playerColor) const
 
 std::unique_ptr<CMap> CGameStateCampaign::getCurrentMap()
 {
-	return gameState->scenarioOps->campState->getMap(CampaignScenarioID::NONE, gameState->callback);
+	return gameState->scenarioOps->campState->getMap(CampaignScenarioID::NONE, gameState->cb);
 }
 
 VCMI_LIB_NAMESPACE_END

lib/gameState/TavernHeroesPool.cpp

@@ -110,7 +110,7 @@ std::shared_ptr<CGHeroInstance> TavernHeroesPool::takeHeroFromPool(HeroTypeID he
 		return entry.hero == hero;
 	});
 
-	return owner->getMap().tryTakeFromHeroPool(hero);;
+	return owner->getMap().tryTakeFromHeroPool(hero);
 }
 
 void TavernHeroesPool::onNewDay()

lib/mapObjects/CGHeroInstance.cpp

@@ -1324,9 +1324,9 @@ void CGHeroInstance::deserializationFix()
 
 void CGHeroInstance::boatDeserializationFix()
 {
-	auto boat = cb->gameState()->getObjInstance(boardedBoat);
-	if (boat)
-		attachTo(dynamic_cast<CGBoat&>(*boat));
+//	auto boat = cb->gameState()->getObjInstance(boardedBoat);
+//	if (boat)
+//		attachTo(dynamic_cast<CGBoat&>(*boat));
 }
 
 CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(const bool isBattleOutsideTown) const
@@ -1349,14 +1349,14 @@ CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(CGameState * gs)
 
 CBonusSystemNode & CGHeroInstance::whereShouldBeAttached(CGameState * gs)
 {
-	if(getVisitedTown())
-	{
-		if(isGarrisoned())
-			return *getVisitedTown();
-		else
-			return getVisitedTown()->townAndVis;
-	}
-	else
+//	if(getVisitedTown())
+//	{
+//		if(isGarrisoned())
+//			return *getVisitedTown();
+//		else
+//			return getVisitedTown()->townAndVis;
+//	}
+//	else
 		return CArmedInstance::whereShouldBeAttached(gs);
 }
 

lib/mapObjects/CGTownInstance.cpp

@@ -708,13 +708,6 @@ std::string CGTownInstance::nodeName() const
 void CGTownInstance::deserializationFix()
 {
 	attachTo(townAndVis);
-
-	//Hero is already handled by CGameState::attachArmedObjects
-
-// 	if(getVisitingHero())
-// 		getVisitingHero()->attachTo(&townAndVis);
-// 	if(getGarrisonHero())
-// 		getGarrisonHero()->attachTo(this);
 }
 
 void CGTownInstance::updateMoraleBonusFromArmy()

lib/mapping/CMap.cpp

@@ -505,7 +505,10 @@ void CMap::generateUniqueInstanceName(CGObjectInstance * target)
 
 void CMap::addNewObject(std::shared_ptr<CGObjectInstance> obj)
 {
-	if(obj->id != ObjectInstanceID(static_cast<si32>(objects.size())))
+	if (!obj->id.hasValue())
+		obj->id = ObjectInstanceID(objects.size());
+
+	if(obj->id != ObjectInstanceID(objects.size()) && objects.at(obj->id.getNum()) != nullptr)
 		throw std::runtime_error("Invalid object instance id");
 
 	if(obj->instanceName.empty())
@@ -766,17 +769,27 @@ CArtifactInstance * CMap::createScroll(const SpellID & spellId)
 
 CArtifactInstance * CMap::createSingleArtifact(const ArtifactID & artId, const SpellID & spellId)
 {
-	return new CArtifactInstance(cb);
+	auto newArtifact = artId.hasValue() ?
+		std::make_shared<CArtifactInstance>(cb, artId.toArtifact()):
+		std::make_shared<CArtifactInstance>(cb);
+
+	newArtifact->setId(ArtifactInstanceID(artInstances.size()));
+	artInstances.push_back(newArtifact);
+	return newArtifact.get();
 }
 
 CArtifactInstance * CMap::createArtifact(const ArtifactID & artID, const SpellID & spellId)
 {
 	if(!artID.hasValue())
-		return new CArtifactInstance(cb); // random, empty //TODO: make this illegal & remove?
+	{
+		// random, empty
+		// TODO: make this illegal & remove? Such artifact can't be randomized as combined artifact later
+		return createSingleArtifact(artID, spellId);
+	}
 
 	auto art = artID.toArtifact();
 
-	auto artInst = new CArtifactInstance(cb, art);
+	auto artInst = createSingleArtifact(artID, spellId);
 	if(art->isCombined() && !art->isFused())
 	{
 		for(const auto & part : art->getConstituents())

lib/mapping/CMap.h

@@ -254,6 +254,7 @@ public:
 
 		h & objects;
 		h & heroesOnMap;
+		h & heroesPool;
 		h & teleportChannels;
 		h & towns;
 		h & artInstances;

lib/networkPacks/NetPacksLib.cpp

@@ -1555,7 +1555,7 @@ void SwapStacks::applyGs(CGameState *gs)
 void InsertNewStack::applyGs(CGameState *gs)
 {
 	if(auto * obj = gs->getArmyInstance(army))
-		obj->putStack(slot, std::make_unique<CStackInstance>(gs->callback, type, count));
+		obj->putStack(slot, std::make_unique<CStackInstance>(gs->cb, type, count));
 	else
 		throw std::runtime_error("InsertNewStack: invalid army object " + std::to_string(army.getNum()) + ", possible game state corruption.");
 }

lib/serializer/Connection.cpp

@@ -132,7 +132,7 @@ void CConnection::setCallback(IGameCallback * cb)
 
 void CConnection::enterGameplayConnectionMode(CGameState * gs)
 {
-	setCallback(gs->callback);
+	setCallback(gs->cb);
 }
 
 void CConnection::setSerializationVersion(ESerializationVersion version)

server/CGameHandler.cpp

@@ -544,8 +544,8 @@ void CGameHandler::init(StartInfo *si, Load::ProgressAccumulator & progressTrack
 	logGlobal->info("Using random seed: %d", randomNumberGenerator->nextInt());
 
 	CMapService mapService;
-	gs = std::make_shared<CGameState>();
-	gs->preInit(LIBRARY, this);
+	gs = std::make_shared<CGameState>(this);
+	gs->preInit(LIBRARY);
 	logGlobal->info("Gamestate created!");
 	gs->init(&mapService, si, progressTracking);
 	logGlobal->info("Gamestate initialized!");
@@ -1613,7 +1613,7 @@ bool CGameHandler::load(const std::string & filename)
 		gameLobby().announceMessage(str);
 		return false;
 	}
-	gameState()->preInit(LIBRARY, this);
+	gameState()->preInit(LIBRARY);
 	gameState()->updateOnLoad(gameLobby().si.get());
 	return true;
 }
@@ -2678,7 +2678,7 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 	auto & slotsDstSrc = ma.artsPack1;
 
 	// Temporary fitting set for artifacts. Used to select available slots before sending data.
-	CArtifactFittingSet artFittingSet(gameState()->callback, pdstSet->bearerType());
+	CArtifactFittingSet artFittingSet(gameState()->cb, pdstSet->bearerType());
 
 	auto moveArtifact = [this, &artFittingSet, dstId](const CArtifactInstance * artifact,
 		ArtifactPosition srcSlot, std::vector<MoveArtifactInfo> & slots) -> void
@@ -4259,7 +4259,7 @@ std::shared_ptr<CGObjectInstance> CGameHandler::createNewObject(const int3 & vis
 
 	auto handler = LIBRARY->objtypeh->getHandlerFor(objectID, subID);
 
-	auto o = handler->create(gameState()->callback, nullptr);
+	auto o = handler->create(gameState()->cb, nullptr);
 	handler->configureObject(o.get(), getRandomGenerator());
 	assert(o->ID == objectID);
 
@@ -4290,7 +4290,7 @@ void CGameHandler::createWanderingMonster(const int3 & visitablePosition, Creatu
 	cre->character = 2;
 	cre->gainedArtifact = ArtifactID::NONE;
 	cre->identifier = -1;
-	cre->addToSlot(SlotID(0), std::make_unique<CStackInstance>(gameState()->callback, creature, -1)); //add placeholder stack
+	cre->addToSlot(SlotID(0), std::make_unique<CStackInstance>(gameState()->cb, creature, -1)); //add placeholder stack
 
 	newObject(createdObject, PlayerColor::NEUTRAL);
 }

server/battles/BattleProcessor.cpp

@@ -175,7 +175,7 @@ BattleID BattleProcessor::setupBattle(int3 tile, BattleSideArray<const CArmedIns
 
 	//send info about battles
 	BattleStart bs;
-	bs.info = BattleInfo::setupBattle(gameHandler->gameState()->callback, tile, terrain, battlefieldType, armies, heroes, layout, town);
+	bs.info = BattleInfo::setupBattle(gameHandler->gameState()->cb, tile, terrain, battlefieldType, armies, heroes, layout, town);
 	bs.battleID = gameHandler->gameState()->nextBattleID;
 
 	engageIntoBattle(bs.info->getSide(BattleSide::ATTACKER).color);

test/game/CGameStateTest.cpp

@@ -48,9 +48,9 @@ public:
 
 	void SetUp() override
 	{
-		gameState = std::make_shared<CGameState>();
+		gameState = std::make_shared<CGameState>(gameCallback.get());
 		gameCallback->setGameState(gameState);
-		gameState->preInit(&services, gameCallback.get());
+		gameState->preInit(&services);
 	}
 
 	void TearDown() override
@@ -196,11 +196,11 @@ public:
 
 		auto terrain = t.getTerrainID();
 		BattleField terType(0);
-		BattleLayout layout = BattleLayout::createDefaultLayout(gameState->callback, attacker, defender);
+		BattleLayout layout = BattleLayout::createDefaultLayout(gameState->cb, attacker, defender);
 
 		//send info about battles
 
-		auto battle = BattleInfo::setupBattle(gameState->callback, tile, terrain, terType, armedInstancies, heroes, layout, nullptr);
+		auto battle = BattleInfo::setupBattle(gameState->cb, tile, terrain, terType, armedInstancies, heroes, layout, nullptr);
 
 		BattleStart bs;
 		bs.info = std::move(battle);

test/netpacks/NetPackFixture.cpp

@@ -24,7 +24,7 @@ NetPackFixture::~NetPackFixture() = default;
 
 void NetPackFixture::setUp()
 {
-	gameState = std::make_shared<GameStateFake>();
+    gameState = std::make_shared<GameStateFake>(nullptr);
 
 }
 

test/netpacks/NetPackFixture.h

@@ -18,6 +18,8 @@ namespace test
 class GameStateFake : public CGameState
 {
 public:
+	using CGameState::CGameState;
+
 	MOCK_METHOD3(updateEntity, void(Metatype, int32_t, const JsonNode &));
 };