Correctly restore bonus system on deserialization

client/windows/CMapOverview.cpp

@@ -36,6 +36,7 @@
 #include "../../lib/filesystem/Filesystem.h"
 
 #include "../../lib/StartInfo.h"
+#include "../../lib/mapObjects/CGHeroInstance.h"
 #include "../../lib/rmg/CMapGenOptions.h"
 #include "../../lib/serializer/CLoadFile.h"
 #include "../../lib/texts/CGeneralTextHandler.h"

lib/CArtHandler.cpp

@@ -18,6 +18,7 @@
 #include "json/JsonBonus.h"
 #include "mapObjectConstructors/AObjectTypeHandler.h"
 #include "mapObjectConstructors/CObjectClassesHandler.h"
+#include "gameState/CGameState.h"
 #include "mapping/CMap.h"
 #include "serializer/JsonSerializeFormat.h"
 #include "texts/CGeneralTextHandler.h"
@@ -947,11 +948,11 @@ bool CArtifactSet::isPositionFree(const ArtifactPosition & pos, bool onlyLockChe
 	return true; //no slot means not used
 }
 
-void CArtifactSet::artDeserializationFix(CBonusSystemNode *node)
+void CArtifactSet::artDeserializationFix(CGameState * gs, 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.artifactID.hasValue() && !elem.second.locked)
+			node->attachToSource(*gs->getArtInstance(elem.second.artifactID));
 }
 
 void CArtifactSet::serializeJsonArtifacts(JsonSerializeFormat & handler, const std::string & fieldName, CMap * map)

lib/CArtHandler.h

@@ -24,6 +24,7 @@ VCMI_LIB_NAMESPACE_BEGIN
 class CArtHandler;
 class CGHeroInstance;
 class CMap;
+class CGameState;
 class CArtifactSet;
 class CArtifactInstance;
 class JsonSerializeFormat;
@@ -231,7 +232,7 @@ public:
 		h & artifactsWorn;
 	}
 
-	void artDeserializationFix(CBonusSystemNode *node);
+	void artDeserializationFix(CGameState * gs, CBonusSystemNode *node);
 
 	void serializeJsonArtifacts(JsonSerializeFormat & handler, const std::string & fieldName, CMap * map);
 	const CArtifactInstance * getCombinedArtWithPart(const ArtifactID & partId) const;

lib/CArtifactInstance.cpp

@@ -14,6 +14,7 @@
 #include "ArtifactUtils.h"
 #include "CArtHandler.h"
 #include "IGameCallback.h"
+#include "gameState/CGameState.h"
 #include "networkPacks/ArtifactLocation.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
@@ -182,11 +183,10 @@ bool CArtifactInstance::isScroll() const
 	return getType()->isScroll();
 }
 
-void CArtifactInstance::deserializationFix()
+void CArtifactInstance::attachToBonusSystem(CGameState * gs)
 {
-//	setType(artTypeID.toArtifact());
-//	for(PartInfo & part : partsInfo)
-//		attachToSource(*part.getArtifact());
+	for(PartInfo & part : partsInfo)
+		attachToSource(*gs->getArtInstance(part.artifactID));
 }
 
 VCMI_LIB_NAMESPACE_END

lib/CArtifactInstance.h

@@ -15,6 +15,7 @@
 VCMI_LIB_NAMESPACE_BEGIN
 
 struct ArtifactLocation;
+class CGameState;
 
 class DLL_LINKAGE CCombinedArtifactInstance : public GameCallbackHolder
 {
@@ -90,14 +91,18 @@ public:
 	bool isCombined() const;
 	bool isScroll() const;
 	
-	void deserializationFix();
+	void attachToBonusSystem(CGameState * gs);
+
 	template <typename Handler> void serialize(Handler & h)
 	{
 		h & static_cast<CBonusSystemNode&>(*this);
 		h & static_cast<CCombinedArtifactInstance&>(*this);
 		h & artTypeID;
 		h & id;
-		BONUS_TREE_DESERIALIZATION_FIX
+
+		if(!h.saving && h.loadingGamestate)
+			setType(artTypeID.toArtifact());
+
 	}
 };
 

lib/CCreatureSet.cpp

@@ -334,15 +334,6 @@ 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)

lib/CCreatureSet.h

@@ -221,9 +221,6 @@ 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
@@ -296,9 +293,6 @@ 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

@@ -910,7 +910,8 @@ const CGObjectInstance * CGameInfoCallback::getObjInstance( ObjectInstanceID oid
 
 const CArtifactSet * CGameInfoCallback::getArtSet(const ArtifactLocation & loc) const
 {
-	return gameState()->getArtSet(loc);
+	auto gs = const_cast<CGameState*>(gameState());
+	return gs->getArtSet(loc);
 }
 
 std::vector<ObjectInstanceID> CGameInfoCallback::getVisibleTeleportObjects(std::vector<ObjectInstanceID> ids, PlayerColor player) const

lib/IGameCallback.cpp

@@ -28,6 +28,7 @@
 #include "mapObjectConstructors/CObjectClassesHandler.h"
 #include "mapObjects/CGMarket.h"
 #include "mapObjects/TownBuildingInstance.h"
+#include "mapObjects/CGHeroInstance.h"
 #include "mapObjects/CGTownInstance.h"
 #include "mapObjects/CObjectHandler.h"
 #include "mapObjects/CQuest.h"

lib/bonuses/Bonus.h

@@ -54,8 +54,6 @@ public:
 	JsonNode toJsonNode() const;
 };
 
-#define BONUS_TREE_DESERIALIZATION_FIX if(!h.saving && h.loadingGamestate) deserializationFix();
-
 /// Struct for handling bonuses of several types. Can be transferred to any hero
 struct DLL_LINKAGE Bonus : public std::enable_shared_from_this<Bonus>, public Serializeable
 {

lib/bonuses/CBonusSystemNode.cpp

@@ -443,11 +443,6 @@ std::string CBonusSystemNode::nodeShortInfo() const
 	return str.str();
 }
 
-void CBonusSystemNode::deserializationFix()
-{
-	exportBonuses();
-}
-
 void CBonusSystemNode::getRedParents(TCNodes & out) const
 {
 	TCNodes lparents;

lib/bonuses/CBonusSystemNode.h

@@ -127,8 +127,6 @@ public:
 	virtual std::string nodeName() const;
 	bool isHypothetic() const { return isHypotheticNode; }
 
-	void deserializationFix();
-
 	BonusList & getExportedBonusList();
 	const BonusList & getExportedBonusList() const;
 	CBonusSystemNode::ENodeTypes getNodeType() const;
@@ -148,7 +146,9 @@ public:
 	{
 		h & nodeType;
 		h & exportedBonuses;
-		BONUS_TREE_DESERIALIZATION_FIX
+
+		if(!h.saving && h.loadingGamestate)
+			exportBonuses();
 	}
 
 	friend class CBonusProxy;

lib/gameState/CGameState.cpp

@@ -602,6 +602,10 @@ void CGameState::initHeroes()
 		auto hero = getHero(heroID);
 		assert(map->isInTheMap(hero->visitablePos()));
 		const auto & tile = map->getTile(hero->visitablePos());
+
+		if (hero->ID == Obj::PRISON)
+			continue;
+
 		if (tile.isWater())
 		{
 			auto handler = LIBRARY->objtypeh->getHandlerFor(Obj::BOAT, hero->getBoatType().getNum());
@@ -615,12 +619,6 @@ void CGameState::initHeroes()
 		}
 	}
 
-	for(auto hero : map->getObjects<CGHeroInstance>()) //prisons
-	{
-		if(hero->ID == Obj::PRISON)
-			hero->initHero(getRandomGenerator());
-	}
-
 	std::set<HeroTypeID> heroesToCreate = getUnusedAllowedHeroes(); //ids of heroes to be created and put into the pool
 
 	for(const HeroTypeID & htype : heroesToCreate) //all not used allowed heroes go with default state into the pool
@@ -1562,19 +1560,21 @@ void CGameState::obtainPlayersStats(SThievesGuildInfo & tgi, int level)
 void CGameState::buildBonusSystemTree()
 {
 	buildGlobalTeamPlayerTree();
-	attachArmedObjects();
+	for(auto & armed : map->getObjects<CArmedInstance>())
+		armed->attachToBonusSystem(this);
 
-	for (const auto & townID : getMap().getAllTowns())
-	{
-		auto t = getTown(townID);
-		t->deserializationFix();
-	}
+	for(auto & art : map->getArtifacts())
+		art->attachToBonusSystem(this);
 }
 
-void CGameState::deserializationFix()
+void CGameState::restoreBonusSystemTree()
 {
 	buildGlobalTeamPlayerTree();
-	attachArmedObjects();
+	for(auto & armed : map->getObjects<CArmedInstance>())
+		armed->restoreBonusSystem(this);
+
+	for(auto & art : map->getArtifacts())
+		art->attachToBonusSystem(this);
 
 	if (campaign)
 		campaign->setGamestate(this);
@@ -1596,14 +1596,6 @@ void CGameState::buildGlobalTeamPlayerTree()
 	}
 }
 
-void CGameState::attachArmedObjects()
-{
-	for(auto & armed : map->getObjects<CArmedInstance>())
-	{
-		armed->whatShouldBeAttached().attachTo(armed->whereShouldBeAttached(this));
-	}
-}
-
 bool CGameState::giveHeroArtifact(CGHeroInstance * h, const ArtifactID & aid)
 {
 	 CArtifactInstance * ai = createArtifact(aid);
@@ -1632,12 +1624,6 @@ std::set<HeroTypeID> CGameState::getUnusedAllowedHeroes(bool alsoIncludeNotAllow
 	for (auto heroID : map->getHeroesOnMap())
 		ret -= getHero(heroID)->getHeroTypeID();
 
-	for(auto hero : map->getObjects<CGHeroInstance>()) //prisons
-	{
-		if(hero && hero->ID == Obj::PRISON)
-			ret -= hero->getHeroTypeID();
-	}
-
 	return ret;
 }
 

lib/gameState/CGameState.h

@@ -184,7 +184,8 @@ public:
 		h & allocatedArtifacts;
 		h & statistic;
 
-		BONUS_TREE_DESERIALIZATION_FIX
+		if(!h.saving && h.loadingGamestate)
+			restoreBonusSystemTree();
 	}
 
 private:
@@ -213,9 +214,8 @@ private:
 	// ----- bonus system handling -----
 
 	void buildBonusSystemTree();
-	void attachArmedObjects();
 	void buildGlobalTeamPlayerTree();
-	void deserializationFix();
+	void restoreBonusSystemTree();
 
 	// ---- misc helpers -----
 

lib/mapObjects/CArmedInstance.cpp

@@ -153,6 +153,34 @@ CBonusSystemNode & CArmedInstance::whatShouldBeAttached()
 	return *this;
 }
 
+void CArmedInstance::attachToBonusSystem(CGameState * gs)
+{
+	CArmedInstance::restoreBonusSystem(gs);
+}
+
+void CArmedInstance::restoreBonusSystem(CGameState * gs)
+{
+	whatShouldBeAttached().attachTo(whereShouldBeAttached(gs));
+
+	for(const auto & elem : stacks)
+		elem.second->artDeserializationFix(gs, elem.second.get());
+}
+
+void CArmedInstance::detachFromBonusSystem(CGameState * gs)
+{
+	whatShouldBeAttached().detachFrom(whereShouldBeAttached(gs));
+
+// TODO: the opposite
+//	for(const auto & elem : stacks)
+//		elem.second->artDeserializationFix(elem.second.get());
+}
+
+void CArmedInstance::attachUnitsToArmy()
+{
+	for(const auto & elem : stacks)
+		elem.second->attachTo(*getArmy());
+}
+
 const IBonusBearer* CArmedInstance::getBonusBearer() const
 {
 	return this;

lib/mapObjects/CArmedInstance.h

@@ -25,6 +25,12 @@ class DLL_LINKAGE CArmedInstance: public CGObjectInstance, public CBonusSystemNo
 private:
 	BonusValueCache nonEvilAlignmentMix;
 
+	void attachUnitsToArmy();
+
+protected:
+	virtual CBonusSystemNode & whereShouldBeAttached(CGameState * gs);
+	virtual CBonusSystemNode & whatShouldBeAttached();
+
 public:
 	BattleInfo *battle; //set to the current battle, if engaged
 
@@ -38,9 +44,10 @@ public:
 	//////////////////////////////////////////////////////////////////////////
 	//IConstBonusProvider
 	const IBonusBearer* getBonusBearer() const override;
-//	int valOfGlobalBonuses(CSelector selector) const; //used only for castle interface								???
-	virtual CBonusSystemNode & whereShouldBeAttached(CGameState * gs);
-	virtual CBonusSystemNode & whatShouldBeAttached();
+
+	void attachToBonusSystem(CGameState * gs) override;
+	void detachFromBonusSystem(CGameState * gs) override;
+	void restoreBonusSystem(CGameState * gs) override;
 	//////////////////////////////////////////////////////////////////////////
 
 	CArmedInstance(IGameCallback *cb);
@@ -60,6 +67,9 @@ public:
 		h & static_cast<CGObjectInstance&>(*this);
 		h & static_cast<CBonusSystemNode&>(*this);
 		h & static_cast<CCreatureSet&>(*this);
+
+		if(!h.saving && h.loadingGamestate)
+			attachUnitsToArmy();
 	}
 };
 

lib/mapObjects/CGHeroInstance.cpp

@@ -1323,17 +1323,38 @@ void CGHeroInstance::setBoat(CGBoat* newBoat)
 	newBoat->setBoardedHero(this);
 }
 
-void CGHeroInstance::deserializationFix()
+void CGHeroInstance::restoreBonusSystem(CGameState * gs)
 {
-	artDeserializationFix(this);
-	boatDeserializationFix();
+	CArmedInstance::restoreBonusSystem(gs);
+	artDeserializationFix(gs, this);
+	if (boardedBoat.hasValue())
+	{
+		auto boat = gs->getObjInstance(boardedBoat);
+		if (boat)
+			attachTo(dynamic_cast<CGBoat&>(*boat));
+	}
+}
+
+void CGHeroInstance::attachToBonusSystem(CGameState * gs)
+{
+	CArmedInstance::attachToBonusSystem(gs);
+	if (boardedBoat.hasValue())
+	{
+		auto boat = gs->getObjInstance(boardedBoat);
+		if (boat)
+			attachTo(dynamic_cast<CGBoat&>(*boat));
+	}
 }
 
-void CGHeroInstance::boatDeserializationFix()
+void CGHeroInstance::detachFromBonusSystem(CGameState * gs)
 {
-//	auto boat = cb->gameState()->getObjInstance(boardedBoat);
-//	if (boat)
-//		attachTo(dynamic_cast<CGBoat&>(*boat));
+	CArmedInstance::attachToBonusSystem(gs);
+	if (boardedBoat.hasValue())
+	{
+		auto boat = gs->getObjInstance(boardedBoat);
+		if (boat)
+			detachFrom(dynamic_cast<CGBoat&>(*boat));
+	}
 }
 
 CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(const bool isBattleOutsideTown) const
@@ -1357,14 +1378,15 @@ CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(CGameState * gs)
 
 CBonusSystemNode & CGHeroInstance::whereShouldBeAttached(CGameState * gs)
 {
-//	if(getVisitedTown())
-//	{
-//		if(isGarrisoned())
-//			return *getVisitedTown();
-//		else
-//			return getVisitedTown()->townAndVis;
-//	}
-//	else
+	if(visitedTown.hasValue())
+	{
+		auto town = gs->getTown(visitedTown);
+		if(isGarrisoned())
+			return *town;
+		else
+			return town->townAndVis;
+	}
+	else
 		return CArmedInstance::whereShouldBeAttached(gs);
 }
 

lib/mapObjects/CGHeroInstance.h

@@ -319,8 +319,6 @@ public:
 	void getCastDescription(const spells::Spell * spell, const battle::Units & attacked, MetaString & text) const override;
 	void spendMana(ServerCallback * server, const int spellCost) const override;
 
-	void boatDeserializationFix();
-	void deserializationFix();
 	void updateAppearance();
 
 	void pickRandomObject(vstd::RNG & rand) override;
@@ -333,6 +331,9 @@ public:
 
 	void afterAddToMap(CMap * map) override;
 	void afterRemoveFromMap(CMap * map) override;
+	void attachToBonusSystem(CGameState * gs) override;
+	void detachFromBonusSystem(CGameState * gs) override;
+	void restoreBonusSystem(CGameState * gs) override;
 
 	void updateFrom(const JsonNode & data) override;
 
@@ -381,7 +382,6 @@ public:
 		h & boardedBoat;
 		h & commander;
 		h & visitedObjects;
-		BONUS_TREE_DESERIALIZATION_FIX
 	}
 };
 

lib/mapObjects/CGObjectInstance.cpp

@@ -380,16 +380,6 @@ void CGObjectInstance::serializeJson(JsonSerializeFormat & handler)
 	}
 }
 
-void CGObjectInstance::afterAddToMap(CMap * map)
-{
-	//nothing here
-}
-
-void CGObjectInstance::afterRemoveFromMap(CMap * map)
-{
-	//nothing here
-}
-
 void CGObjectInstance::serializeJsonOptions(JsonSerializeFormat & handler)
 {
 	//nothing here

lib/mapObjects/CGObjectInstance.h

@@ -22,6 +22,7 @@ struct Component;
 class JsonSerializeFormat;
 class ObjectTemplate;
 class CMap;
+class CGameState;
 class AObjectTypeHandler;
 using TObjectTypeHandler = std::shared_ptr<AObjectTypeHandler>;
 
@@ -136,8 +137,11 @@ public:
 	/// method for synchronous update. Note: For new properties classes should override setPropertyDer instead
 	void setProperty(ObjProperty what, ObjPropertyID identifier) final;
 
-	virtual void afterAddToMap(CMap * map);
-	virtual void afterRemoveFromMap(CMap * map);
+	virtual void afterAddToMap(CMap * map){};
+	virtual void afterRemoveFromMap(CMap * map){};
+	virtual void attachToBonusSystem(CGameState * gs){};
+	virtual void detachFromBonusSystem(CGameState * gs){};
+	virtual void restoreBonusSystem(CGameState * gs){};
 
 	///Entry point of binary (de-)serialization
 	template <typename Handler> void serialize(Handler &h)

lib/mapObjects/CGTownInstance.cpp

@@ -274,7 +274,8 @@ CGTownInstance::CGTownInstance(IGameCallback *cb):
 	spellResearchAcceptedCounter(0),
 	spellResearchAllowed(true)
 {
-	this->setNodeType(CBonusSystemNode::TOWN);
+	setNodeType(CBonusSystemNode::TOWN);
+	attachTo(townAndVis);
 }
 
 CGTownInstance::~CGTownInstance() = default;
@@ -705,11 +706,6 @@ std::string CGTownInstance::nodeName() const
 	return "Town (" + getTown()->faction->getNameTranslated() + ") of " + getNameTranslated();
 }
 
-void CGTownInstance::deserializationFix()
-{
-	attachTo(townAndVis);
-}
-
 void CGTownInstance::updateMoraleBonusFromArmy()
 {
 	auto b = getExportedBonusList().getFirst(Selector::sourceType()(BonusSource::ARMY).And(Selector::type()(BonusType::MORALE)));
@@ -1255,7 +1251,6 @@ void CGTownInstance::fillUpgradeInfo(UpgradeInfo & info, const CStackInstance &s
 
 void CGTownInstance::postDeserialize()
 {
-	setNodeType(CBonusSystemNode::TOWN);
 	for(auto & building : rewardableBuildings)
 		building.second->town = this;
 

lib/mapObjects/CGTownInstance.h

@@ -99,7 +99,6 @@ public:
 		h & spellResearchAllowed;
 		h & rewardableBuildings;
 		h & townAndVis;
-		BONUS_TREE_DESERIALIZATION_FIX
 
 		if(!h.saving)
 			postDeserialize();
@@ -109,7 +108,6 @@ public:
 	CBonusSystemNode & whatShouldBeAttached() override;
 	std::string nodeName() const override;
 	void updateMoraleBonusFromArmy() override;
-	void deserializationFix();
 	void postDeserialize();
 	void recreateBuildingsBonuses();
 	void setVisitingHero(CGHeroInstance *h);

lib/mapping/CMap.h

@@ -23,6 +23,7 @@ class CArtifactSet;
 class CGObjectInstance;
 class CGHeroInstance;
 class CCommanderInstance;
+class CGameState;
 class CGCreature;
 class CQuest;
 class CGTownInstance;
@@ -140,6 +141,8 @@ public:
 	CGObjectInstance * getObject(ObjectInstanceID obj);
 	const CGObjectInstance * getObject(ObjectInstanceID obj) const;
 
+	void attachToBonusSystem(CGameState * gs);
+
 	template<typename ObjectType = CGObjectInstance>
 	std::vector<const ObjectType *> getObjects() const
 	{
@@ -166,6 +169,16 @@ public:
 		return result;
 	}
 
+	std::vector<CArtifactInstance *> getArtifacts()
+	{
+		std::vector<CArtifactInstance *> result;
+		for (const auto & art : artInstances)
+			if (art)
+				result.push_back(art.get());
+
+		return result;
+	}
+
 	bool isWaterMap() const;
 	bool calculateWaterContent();
 	void banWaterArtifacts();
@@ -183,8 +196,7 @@ public:
 	CGHeroInstance * getHero(HeroTypeID heroId);
 
 	/// Returns ID's of all heroes that are currently present on map
-	/// All garrisoned heroes are included from this list
-	/// All prisons are excluded from this list
+	/// Includes all garrisoned and imprisoned heroes
 	const std::vector<ObjectInstanceID> & getHeroesOnMap();
 
 	/// Returns ID's of all towns present on map

lib/networkPacks/NetPacksLib.cpp

@@ -1493,7 +1493,7 @@ void NewObject::applyGs(CGameState *gs)
 	// attach newly spawned wandering monster to global bonus system node
 	auto newArmy = std::dynamic_pointer_cast<CArmedInstance>(newObject);
 	if (newArmy)
-		newArmy->whatShouldBeAttached().attachTo(newArmy->whereShouldBeAttached(gs));
+		newArmy->attachToBonusSystem(gs);
 
 	logGlobal->debug("Added object id=%d; name=%s", newObject->id, newObject->getObjectName());
 }
@@ -1977,10 +1977,9 @@ void SetObjectProperty::applyGs(CGameState *gs)
 			}
 		}
 
-		CBonusSystemNode & nodeToMove = cai->whatShouldBeAttached();
-		nodeToMove.detachFrom(cai->whereShouldBeAttached(gs));
+		cai->detachFromBonusSystem(gs);
 		obj->setProperty(what, identifier);
-		nodeToMove.attachTo(cai->whereShouldBeAttached(gs));
+		cai->attachToBonusSystem(gs);
 	}
 	else //not an armed instance
 	{
@@ -2476,7 +2475,7 @@ ArtSlotInfo::ArtSlotInfo(const CArtifactInstance * artifact, bool locked)
 
 const CArtifactInstance * ArtSlotInfo::getArt() const
 {
-	if(locked)
+	if(locked || !artifactID.hasValue())
 		return nullptr;
 	return cb->getArtInstance(artifactID);
 }