Fixed: duplicated bonusing building. Improved: opposite bonus propagation

client/Client.cpp

@@ -558,7 +558,6 @@ int CClient::sendRequest(const CPackForServer * request, PlayerColor player)
 
 void CClient::battleStarted(const BattleInfo * info)
 {
-	BattleInfo::adjustOppositeBonuses(const_cast<BattleInfo *>(info));
 	setBattle(info);
 
 	for(auto & battleCb : battleCallbacks)

client/Client.h

@@ -202,7 +202,7 @@ public:
 	bool moveStack(const StackLocation & src, const StackLocation & dst, TQuantity count = -1) override {return false;}
 
 	void removeAfterVisit(const CGObjectInstance * object) override {};
-	bool garrisonSwapOnSiege(ObjectInstanceID tid) override {return false;};
+	bool swapGarrisonOnSiege(ObjectInstanceID tid) override {return false;};
 	void giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos) override {};
 	void giveHeroArtifact(const CGHeroInstance * h, const CArtifactInstance * a, ArtifactPosition pos) override {};
 	void putArtifact(const ArtifactLocation & al, const CArtifactInstance * a) override {};

client/widgets/MiscWidgets.cpp

@@ -406,19 +406,15 @@ void MoraleLuckBox::set(const IBonusBearer * node)
 	}
 	else
 	{
-		bool isListActual = false;
 		std::string addInfo = "";
-
 		for(auto & bonus : * modifierList)
 		{
 			if(bonus->val)
-			{
 				addInfo += "\n" + bonus->Description();
-				isListActual = true;
-			}
 		}
-		text = isListActual ? text + addInfo
-			: text + CGI->generaltexth->arraytxt[noneTxtId];//no modifiers
+		text = addInfo.empty() 
+			? text + CGI->generaltexth->arraytxt[noneTxtId] 
+			: text + addInfo;
 	}
 	std::string imageName;
 	if (small)

config/creatures/necropolis.json

@@ -339,7 +339,7 @@
 			{
 				"type" : "DRAGON_NATURE"
 			},
-			"descreaseMorale" :
+			"decreaseMorale" :
 			{
 				"type" : "MORALE",
 				"effectRange": "ONLY_ENEMY_ARMY",
@@ -372,7 +372,7 @@
 			{
 				"type" : "DRAGON_NATURE"
 			},
-			"descreaseMorale" :
+			"decreaseMorale" :
 			{
 				"type" : "MORALE",
 				"effectRange": "ONLY_ENEMY_ARMY",

lib/CCreatureHandler.cpp

@@ -392,20 +392,6 @@ void CCreature::fillWarMachine()
 	warMachine = ArtifactID::NONE; //this creature is not artifact
 }
 
-void CCreature::updateOppositeBonuses()
-{
-	auto & bonusList = getExportedBonusList();
-	for(auto & bonus : bonusList)
-	{
-		if(bonus->effectRange == Bonus::ONLY_ENEMY_ARMY //Opposite Side bonuses should not be exported from CREATURE node.
-			|| (bonus->propagator && bonus->propagator->getPropagatorType() == CBonusSystemNode::BATTLE))
-		{
-			bonus->effectRange == Bonus::ONLY_ENEMY_ARMY;
-			bonus->propagator.reset();
-		}
-	}
-}
-
 CCreatureHandler::CCreatureHandler()
 	: expAfterUpgrade(0)
 {
@@ -877,10 +863,6 @@ void CCreatureHandler::loadCreatureJson(CCreature * creature, const JsonNode & c
 			if (!ability.second.isNull())
 			{
 				auto b = JsonUtils::parseBonus(ability.second);
-
-				if(b->effectRange == Bonus::ONLY_ENEMY_ARMY) //Opposite Side bonuses should not be exported from CREATURE node.
-					b->propagator.reset();
-
 				b->source = Bonus::CREATURE_ABILITY;
 				b->sid = creature->getIndex();
 				b->duration = Bonus::PERMANENT;

lib/CCreatureHandler.h

@@ -223,17 +223,12 @@ public:
 		{
 			fillWarMachine();
 		}
-		if(version < 801 && !h.saving) // Opposite bonuses are introduced
-		{
-			updateOppositeBonuses();
-		}
 	}
 
 	CCreature();
 
 private:
 	void fillWarMachine();
-	void updateOppositeBonuses();
 };
 
 class DLL_LINKAGE CCreatureHandler : public CHandlerBase<CreatureID, Creature, CCreature, CreatureService>

lib/CCreatureSet.cpp

@@ -630,38 +630,10 @@ void CStackInstance::setType(CreatureID creID)
 		setType((const CCreature*)nullptr);
 }
 
-
-void CStackInstance::copyOppositeBonusesFromCreature(const CCreature * creature)
-{
-	auto owner = _armyObj ? _armyObj->getOwner() : PlayerColor::NEUTRAL;
-
-	if(owner == PlayerColor::UNFLAGGABLE)
-		owner = PlayerColor::NEUTRAL;
-
-	auto & creatureBonuses = const_cast<CCreature *>(type)->getExportedBonusList();
-
-	for(auto & creatureBonus : creatureBonuses)
-	{
-		if(creatureBonus->effectRange == Bonus::ONLY_ENEMY_ARMY) //it has better prformance than dynamic_cast
-		{
-			auto bCopy = std::make_shared<Bonus>(*creatureBonus);
-			bCopy->propagator.reset(new CPropagatorNodeType(CBonusSystemNode::BATTLE));
-			bCopy->limiter.reset(new OppositeSideLimiter(owner));
-			this->addNewBonus(bCopy);
-		}
-	}
-}
-
-void CStackInstance::removeOppositeBonuses()
-{
-	removeBonuses(Selector::effectRange()(Bonus::ONLY_ENEMY_ARMY));
-}
-
 void CStackInstance::setType(const CCreature *c)
 {
 	if(type)
 	{
-		removeOppositeBonuses();
 		detachFrom(const_cast<CCreature*>(type));
 		if (type->isMyUpgrade(c) && VLC->modh->modules.STACK_EXP)
 			experience = static_cast<TExpType>(experience * VLC->creh->expAfterUpgrade / 100.0);
@@ -670,11 +642,7 @@ void CStackInstance::setType(const CCreature *c)
 	CStackBasicDescriptor::setType(c);
 
 	if(type)
-	{
-		auto creature = const_cast<CCreature*>(type);
-		copyOppositeBonusesFromCreature(creature);
-		attachTo(creature);
-	}
+		attachTo(const_cast<CCreature*>(type));
 }
 std::string CStackInstance::bonusToString(const std::shared_ptr<Bonus>& bonus, bool description) const
 {
@@ -700,28 +668,9 @@ void CStackInstance::setArmyObj(const CArmedInstance * ArmyObj)
 		detachFrom(const_cast<CArmedInstance*>(_armyObj));
 
 	_armyObj = ArmyObj;
-	if(ArmyObj)
-	{
-		auto owner = _armyObj->getOwner();
-
-		if(owner == PlayerColor::UNFLAGGABLE)
-			owner = PlayerColor::NEUTRAL;
-
-		auto & bonusList = getExportedBonusList();
-
-		for(auto & bonus : bonusList)
-		{
-			if(bonus->effectRange != Bonus::ONLY_ENEMY_ARMY)
-				continue;
 
-			auto limPtr = bonus->limiter.get();
-			if(limPtr)
-				static_cast<OppositeSideLimiter *>(limPtr)->owner = owner;
-			else
-				logGlobal->error("setArmyObj. Limiter has been lost. Creature is %s", type ? type->nodeShortInfo() : std::string("No creature"));
-		}
+	if(ArmyObj)
 		attachTo(const_cast<CArmedInstance*>(_armyObj));
-	}
 }
 
 std::string CStackInstance::getQuantityTXT(bool capitalized) const
@@ -764,6 +713,11 @@ std::string CStackInstance::nodeName() const
 	return oss.str();
 }
 
+PlayerColor CStackInstance::getOwner() const
+{
+	return _armyObj ? _armyObj->getOwner() : PlayerColor::NEUTRAL;
+}
+
 void CStackInstance::deserializationFix()
 {
 	const CCreature *backup = type;

lib/CCreatureSet.h

@@ -99,10 +99,7 @@ public:
 	ArtBearer::ArtBearer bearerType() const override; //from CArtifactSet
 	virtual std::string nodeName() const override; //from CBonusSystemnode
 	void deserializationFix();
-
-private:
-	void copyOppositeBonusesFromCreature(const CCreature * creature);
-	inline void removeOppositeBonuses();
+	PlayerColor getOwner() const override;
 };
 
 class DLL_LINKAGE CCommanderInstance : public CStackInstance

lib/CStack.h

@@ -20,6 +20,7 @@
 struct BattleStackAttacked;
 class BattleInfo;
 
+//Represents STACK_BATTLE nodes
 class DLL_LINKAGE CStack : public CBonusSystemNode, public battle::CUnitState, public battle::IUnitEnvironment
 {
 public:
@@ -79,6 +80,11 @@ public:
 
 	void spendMana(ServerCallback * server, const int spellCost) const override;
 
+	PlayerColor getOwner() const override
+	{
+		return this->owner;
+	}
+
 	template <typename Handler> void serialize(Handler & h, const int version)
 	{
 		//this assumes that stack objects is newly created

lib/HeroBonus.cpp

@@ -857,18 +857,7 @@ const CStackInstance * retrieveStackInstance(const CBonusSystemNode * node)
 
 PlayerColor CBonusSystemNode::retrieveNodeOwner(const CBonusSystemNode * node)
 {
-	if(!node)
-		return PlayerColor::CANNOT_DETERMINE;
-
-	const CStack * stack = retrieveStackBattle(node);
-	if(stack)
-		return stack->owner;
-
-	const CStackInstance * csi = retrieveStackInstance(node);
-	if(csi && csi->armyObj)
-		return csi->armyObj->getOwner();
-
-	return PlayerColor::NEUTRAL;
+	return node ? node->getOwner() : PlayerColor::CANNOT_DETERMINE;
 }
 
 std::shared_ptr<Bonus> CBonusSystemNode::getBonusLocalFirst(const CSelector & selector)
@@ -931,7 +920,12 @@ void CBonusSystemNode::getAllBonusesRec(BonusList &out) const
 	bonuses.getAllBonuses(beforeUpdate);
 
 	for(auto b : beforeUpdate)
-		out.push_back(update(b));
+	{
+		auto updated = b->updater 
+			? getUpdatedBonus(b, b->updater) 
+			: b;
+		out.push_back(updated);
+	}
 }
 
 TConstBonusListPtr CBonusSystemNode::getAllBonuses(const CSelector &selector, const CSelector &limit, const CBonusSystemNode *root, const std::string &cachingStr) const
@@ -1020,11 +1014,10 @@ TConstBonusListPtr CBonusSystemNode::getAllBonusesWithoutCaching(const CSelector
 	return ret;
 }
 
-std::shared_ptr<Bonus> CBonusSystemNode::update(const std::shared_ptr<Bonus> & b) const
+std::shared_ptr<Bonus> CBonusSystemNode::getUpdatedBonus(const std::shared_ptr<Bonus> & b, const TUpdaterPtr updater) const
 {
-	if(b->updater)
-		return b->updater->update(b, *this);
-	return b;
+	assert(updater);
+	return updater->createUpdatedBonus(b, * this);
 }
 
 CBonusSystemNode::CBonusSystemNode()
@@ -1215,23 +1208,25 @@ bool CBonusSystemNode::actsAsBonusSourceOnly() const
 	case CREATURE:
 	case ARTIFACT:
 	case ARTIFACT_INSTANCE:
-	case TOWN:
 		return true;
 	default:
 		return false;
 	}
 }
 
-void CBonusSystemNode::propagateBonus(std::shared_ptr<Bonus> b)
+void CBonusSystemNode::propagateBonus(std::shared_ptr<Bonus> b, const CBonusSystemNode & source)
 {
 	if(b->propagator->shouldBeAttached(this))
 	{
-		bonuses.push_back(b);
-		logBonus->trace("#$# %s #propagated to# %s",  b->Description(), nodeName());
+		auto propagated = b->propagationUpdater 
+			? source.getUpdatedBonus(b, b->propagationUpdater)
+			: b;
+		bonuses.push_back(propagated);
+		logBonus->trace("#$# %s #propagated to# %s",  propagated->Description(), nodeName());
 	}
 
 	FOREACH_RED_CHILD(child)
-		child->propagateBonus(b);
+		child->propagateBonus(b, source);
 }
 
 void CBonusSystemNode::unpropagateBonus(std::shared_ptr<Bonus> b)
@@ -1340,30 +1335,17 @@ void CBonusSystemNode::newRedDescendant(CBonusSystemNode * descendant)
 	for(auto b : exportedBonuses)
 	{
 		if(b->propagator)
-			descendant->propagateBonus(b);
+			descendant->propagateBonus(b, *this);
 	}
 	TNodes redParents;
 	getRedAncestors(redParents); //get all red parents recursively
 
-	//it's not in the 'getRedParents' due to infinite recursion loop.
-	//it's actual only for battle, otherwise, when garrison hero is linking to the town, town's 'children' is already empty
-	if(descendant->nodeType == BATTLE && nodeType == TOWN) //acts as a pure bonus source, but has bonus bearers.
-	{
-		for(auto child : children)
-		{
-			for(auto b : child->exportedBonuses)
-			{
-				if(b->propagator && b->propagator->getPropagatorType() == BATTLE)
-					descendant->propagateBonus(b);
-			}
-		}
-	}
 	for(auto parent : redParents)
 	{
 		for(auto b : parent->exportedBonuses)
 		{
 			if(b->propagator)
-				descendant->propagateBonus(b);
+				descendant->propagateBonus(b, *this);
 		}
 	}
 }
@@ -1406,7 +1388,7 @@ void CBonusSystemNode::getRedDescendants(TNodes &out)
 void CBonusSystemNode::exportBonus(std::shared_ptr<Bonus> b)
 {
 	if(b->propagator)
-		propagateBonus(b);
+		propagateBonus(b, *this);
 	else
 		bonuses.push_back(b);
 
@@ -2336,24 +2318,13 @@ std::shared_ptr<Bonus> Bonus::addUpdater(TUpdaterPtr Updater)
 	return this->shared_from_this();
 }
 
-void Bonus::createOppositeLimiter()
+// Update ONLY_ENEMY_ARMY bonuses from old saves to make them workable.
+// Also, we should foreseen possible errors in bonus configuration and fix them.
+void Bonus::updateOppositeBonuses()
 {
-	if(limiter)
-	{
-		if(!dynamic_cast<OppositeSideLimiter *>(limiter.get()))
-		{
-			logMod->error("Wrong Limiter will be ignored: The 'ONLY_ENEMY_ARMY' effectRange is only compatible with the 'OPPOSITE_SIDE' limiter.");
-			limiter.reset(new OppositeSideLimiter());
-		}
-	}
-	else
-	{
-		limiter = std::make_shared<OppositeSideLimiter>();
-	}
-}
+	if(effectRange != Bonus::ONLY_ENEMY_ARMY)
+		return;
 
-void Bonus::createBattlePropagator()
-{
 	if(propagator)
 	{
 		if(propagator->getPropagatorType() != CBonusSystemNode::BATTLE)
@@ -2366,27 +2337,26 @@ void Bonus::createBattlePropagator()
 	{
 		propagator = std::make_shared<CPropagatorNodeType>(CBonusSystemNode::BATTLE);
 	}
-}
-
-void Bonus::updateOppositeBonuses()
-{
-	if(effectRange == Bonus::ONLY_ENEMY_ARMY)
+	if(limiter)
 	{
-		createBattlePropagator();
-		createOppositeLimiter();
+		if(!dynamic_cast<OppositeSideLimiter*>(limiter.get()))
+		{
+			logMod->error("Wrong Limiter will be ignored: The 'ONLY_ENEMY_ARMY' effectRange is only compatible with the 'OPPOSITE_SIDE' limiter.");
+			limiter.reset(new OppositeSideLimiter());
+		}
 	}
-	else if(limiter && dynamic_cast<OppositeSideLimiter *>(limiter.get()))
+	else
 	{
-		createBattlePropagator();
-		effectRange = Bonus::ONLY_ENEMY_ARMY;
+		limiter = std::make_shared<OppositeSideLimiter>();
 	}
+	propagationUpdater = std::make_shared<OwnerUpdater>();
 }
 
 IUpdater::~IUpdater()
 {
 }
 
-std::shared_ptr<Bonus> IUpdater::update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
+std::shared_ptr<Bonus> IUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
 {
 	return b;
 }
@@ -2409,7 +2379,7 @@ GrowsWithLevelUpdater::GrowsWithLevelUpdater(int valPer20, int stepSize) : valPe
 {
 }
 
-std::shared_ptr<Bonus> GrowsWithLevelUpdater::update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
+std::shared_ptr<Bonus> GrowsWithLevelUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
 {
 	if(context.getNodeType() == CBonusSystemNode::HERO)
 	{
@@ -2446,7 +2416,7 @@ TimesHeroLevelUpdater::TimesHeroLevelUpdater()
 {
 }
 
-std::shared_ptr<Bonus> TimesHeroLevelUpdater::update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
+std::shared_ptr<Bonus> TimesHeroLevelUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
 {
 	if(context.getNodeType() == CBonusSystemNode::HERO)
 	{
@@ -2472,7 +2442,7 @@ TimesStackLevelUpdater::TimesStackLevelUpdater()
 {
 }
 
-std::shared_ptr<Bonus> TimesStackLevelUpdater::update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
+std::shared_ptr<Bonus> TimesStackLevelUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
 {
 	if(context.getNodeType() == CBonusSystemNode::STACK_INSTANCE)
 	{
@@ -2506,3 +2476,30 @@ JsonNode TimesStackLevelUpdater::toJsonNode() const
 {
 	return JsonUtils::stringNode("TIMES_STACK_LEVEL");
 }
+
+OwnerUpdater::OwnerUpdater()
+{
+}
+
+std::string OwnerUpdater::toString() const
+{
+	return "OwnerUpdater";
+}
+
+JsonNode OwnerUpdater::toJsonNode() const
+{
+	return JsonUtils::stringNode("BONUS_OWNER_UPDATER");
+}
+
+std::shared_ptr<Bonus> OwnerUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
+{
+	auto owner = CBonusSystemNode::retrieveNodeOwner(&context);
+
+	if(owner == PlayerColor::UNFLAGGABLE)
+		owner = PlayerColor::NEUTRAL;
+
+	std::shared_ptr<Bonus> updated = std::make_shared<Bonus>(
+		(Bonus::BonusDuration)b->duration, b->type, b->source, b->val, b->sid, b->subtype, b->valType);
+	updated->limiter = std::make_shared<OppositeSideLimiter>(owner);
+	return updated;
+}

lib/HeroBonus.h

@@ -417,6 +417,7 @@ struct DLL_LINKAGE Bonus : public std::enable_shared_from_this<Bonus>
 	TLimiterPtr limiter;
 	TPropagatorPtr propagator;
 	TUpdaterPtr updater;
+	TUpdaterPtr propagationUpdater;
 
 	std::string description;
 
@@ -455,6 +456,10 @@ struct DLL_LINKAGE Bonus : public std::enable_shared_from_this<Bonus>
 		{
 			h & updater;
 		}
+		if(version >= 801)
+		{
+			h & propagationUpdater;
+		}
 		if(version < 801 && !h.saving) //Opposite Side bonuses are introduced
 		{
 			updateOppositeBonuses();
@@ -526,9 +531,6 @@ struct DLL_LINKAGE Bonus : public std::enable_shared_from_this<Bonus>
 	std::shared_ptr<Bonus> addLimiter(TLimiterPtr Limiter); //returns this for convenient chain-calls
 	std::shared_ptr<Bonus> addPropagator(TPropagatorPtr Propagator); //returns this for convenient chain-calls
 	std::shared_ptr<Bonus> addUpdater(TUpdaterPtr Updater); //returns this for convenient chain-calls
-
-	inline void createOppositeLimiter();
-	inline void createBattlePropagator();
 	void updateOppositeBonuses();
 };
 
@@ -774,7 +776,7 @@ private:
 
 	void getAllBonusesRec(BonusList &out) const;
 	TConstBonusListPtr getAllBonusesWithoutCaching(const CSelector &selector, const CSelector &limit, const CBonusSystemNode *root = nullptr) const;
-	std::shared_ptr<Bonus> update(const std::shared_ptr<Bonus> & b) const;
+	std::shared_ptr<Bonus> getUpdatedBonus(const std::shared_ptr<Bonus> & b, const TUpdaterPtr updater) const;
 
 public:
 	explicit CBonusSystemNode();
@@ -808,7 +810,7 @@ public:
 
 	void newChildAttached(CBonusSystemNode *child);
 	void childDetached(CBonusSystemNode *child);
-	void propagateBonus(std::shared_ptr<Bonus> b);
+	void propagateBonus(std::shared_ptr<Bonus> b, const CBonusSystemNode & source);
 	void unpropagateBonus(std::shared_ptr<Bonus> b);
 	void removeBonus(const std::shared_ptr<Bonus>& b);
 	void removeBonuses(const CSelector & selector);
@@ -843,6 +845,11 @@ public:
 
 	int64_t getTreeVersion() const override;
 
+	virtual PlayerColor getOwner() const
+	{
+		return PlayerColor::NEUTRAL;
+	}
+
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{
 //		h & bonuses;
@@ -1225,7 +1232,7 @@ class DLL_LINKAGE IUpdater
 public:
 	virtual ~IUpdater();
 
-	virtual std::shared_ptr<Bonus> update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const;
+	virtual std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const;
 	virtual std::string toString() const;
 	virtual JsonNode toJsonNode() const;
 
@@ -1250,7 +1257,7 @@ public:
 		h & stepSize;
 	}
 
-	std::shared_ptr<Bonus> update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const override;
+	std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const override;
 	virtual std::string toString() const override;
 	virtual JsonNode toJsonNode() const override;
 };
@@ -1265,7 +1272,7 @@ public:
 		h & static_cast<IUpdater &>(*this);
 	}
 
-	std::shared_ptr<Bonus> update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const override;
+	std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const override;
 	virtual std::string toString() const override;
 	virtual JsonNode toJsonNode() const override;
 };
@@ -1280,7 +1287,22 @@ public:
 		h & static_cast<IUpdater &>(*this);
 	}
 
-	std::shared_ptr<Bonus> update(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const override;
+	std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const override;
+	virtual std::string toString() const override;
+	virtual JsonNode toJsonNode() const override;
+};
+
+class DLL_LINKAGE OwnerUpdater : public IUpdater
+{
+public:
+	OwnerUpdater();
+
+	template <typename Handler> void serialize(Handler& h, const int version)
+	{
+		h & static_cast<IUpdater &>(*this);
+	}
+
+	std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus>& b, const CBonusSystemNode& context) const override;
 	virtual std::string toString() const override;
 	virtual JsonNode toJsonNode() const override;
 };

lib/IGameCallback.h

@@ -98,7 +98,7 @@ public:
 	virtual void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, bool creatureBank = false)=0; //if any of armies is hero, hero will be used
 	virtual void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, bool creatureBank = false)=0; //if any of armies is hero, hero will be used, visitable tile of second obj is place of battle
 	virtual bool moveHero(ObjectInstanceID hid, int3 dst, ui8 teleporting, bool transit = false, PlayerColor asker = PlayerColor::NEUTRAL)=0;
-	virtual bool garrisonSwapOnSiege(ObjectInstanceID tid)=0;
+	virtual bool swapGarrisonOnSiege(ObjectInstanceID tid)=0;
 	virtual void giveHeroBonus(GiveBonus * bonus)=0;
 	virtual void setMovePoints(SetMovePoints * smp)=0;
 	virtual void setManaPoints(ObjectInstanceID hid, int val)=0;

lib/battle/BattleInfo.cpp

@@ -186,48 +186,6 @@ struct RangeGenerator
 	std::function<int()> myRand;
 };
 
-void BattleInfo::adjustOppositeBonuses(CBonusSystemNode * node, PlayerColor ownerColor)
-{
-	auto & bonusList = node->getExportedBonusList();
-	if(bonusList.empty())
-		return;
-
-	for(const auto & bonus : bonusList)
-	{
-		if(bonus->effectRange != Bonus::ONLY_ENEMY_ARMY)
-			continue;
-
-		auto limPtr = bonus->limiter.get();
-		if(limPtr)
-			static_cast<OppositeSideLimiter *>(limPtr)->owner = ownerColor;
-		else
-			logGlobal->error("adjustOppositeBonuses. Limiter has been lost. Node is %s", node->nodeShortInfo());
-	}
-}
-
-void BattleInfo::adjustOppositeBonuses(BattleInfo * curB)
-{
-	for(int i = 0; i < 2; i++)
-	{
-		TNodes nodes;
-		auto * army = curB->battleGetArmyObject(i);
-		auto ownerColor = curB->sides[i].color;
-
-		if(army->getNodeType() == CBonusSystemNode::HERO)
-			adjustOppositeBonuses(army, ownerColor);
-			
-		army->getRedAncestors(nodes);
-
-		for(auto node : nodes)
-		{
-			auto currentNodeType = node->getNodeType();
-
-			if(currentNodeType == CBonusSystemNode::ARTIFACT || currentNodeType == CBonusSystemNode::TOWN)
-				adjustOppositeBonuses(node, ownerColor);
-		}
-	}
-}
-
 BattleInfo * BattleInfo::setupBattle(int3 tile, ETerrainType terrain, BFieldType battlefieldType, const CArmedInstance * armies[2], const CGHeroInstance * heroes[2], bool creatureBank, const CGTownInstance * town)
 {
 	CMP_stack cmpst;
@@ -597,7 +555,6 @@ BattleInfo * BattleInfo::setupBattle(int3 tile, ETerrainType terrain, BFieldType
 	else
 		curB->tacticDistance = 0;
 
-	adjustOppositeBonuses(curB);
 	return curB;
 }
 

lib/battle/BattleInfo.h

@@ -137,7 +137,6 @@ public:
 	const CGHeroInstance * getHero(PlayerColor player) const; //returns fighting hero that belongs to given player
 
 	void localInit();
-	static void adjustOppositeBonuses(BattleInfo * curB);
 	static BattleInfo * setupBattle(int3 tile, ETerrainType terrain, BFieldType battlefieldType, const CArmedInstance * armies[2], const CGHeroInstance * heroes[2], bool creatureBank, const CGTownInstance * town);
 
 	ui8 whatSide(PlayerColor player) const;
@@ -146,9 +145,6 @@ public:
 
 protected:
 	scripting::Pool * getContextPool() const override;
-
-private:
-	inline static void adjustOppositeBonuses(CBonusSystemNode * node, PlayerColor ownerColor);
 };
 
 

lib/mapObjects/CArmedInstance.h

@@ -38,6 +38,11 @@ public:
 	CArmedInstance();
 	CArmedInstance(bool isHypotetic);
 
+	PlayerColor getOwner() const override
+	{
+		return this->tempOwner;
+	}
+
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{
 		h & static_cast<CGObjectInstance&>(*this);

lib/mapObjects/CGTownInstance.cpp

@@ -665,8 +665,8 @@ void CGTownInstance::onHeroVisit(const CGHeroInstance * h) const
 		if(armedGarrison() || visitingHero)
 		{
 			const CGHeroInstance * defendingHero = visitingHero ? visitingHero : garrisonHero;
-			const CArmedInstance * defendingArmy = defendingHero ? (CArmedInstance *)defendingHero : (CArmedInstance *)this;
-			const bool isBattleOutside = isBattleOutsideTown(defendingHero); //defendingHero && garrisonHero && defendingHero != garrisonHero;
+			const CArmedInstance * defendingArmy = defendingHero ? (CArmedInstance *)defendingHero : this;
+			const bool isBattleOutside = isBattleOutsideTown(defendingHero);
 
 			if(!isBattleOutside && visitingHero && defendingHero == visitingHero)
 			{
@@ -674,7 +674,7 @@ void CGTownInstance::onHeroVisit(const CGHeroInstance * h) const
 				auto nodeSiege = defendingHero->whereShouldBeAttachedOnSiege(isBattleOutside);
 
 				if(nodeSiege == (CBonusSystemNode *)this)
-					cb->garrisonSwapOnSiege(this->id);
+					cb->swapGarrisonOnSiege(this->id);
 
 				const_cast<CGHeroInstance *>(defendingHero)->inTownGarrison = false; //hack to return visitor from garrison after battle
 			}
@@ -742,15 +742,6 @@ bool CGTownInstance::townEnvisagesBuilding(BuildingSubID::EBuildingSubID subId)
 	return town->getBuildingType(subId) != BuildingID::NONE;
 }
 
-//it does not check hasBuilt because this check is in the OnHeroVisit handler
-void CGTownInstance::tryAddOnePerWeekBonus(BuildingSubID::EBuildingSubID subID)
-{
-	auto bid = town->getBuildingType(subID);
-
-	if(bid != BuildingID::NONE)
-		bonusingBuildings.push_back(new COPWBonus(bid, subID, this));
-}
-
 void CGTownInstance::initOverriddenBids()
 {
 	for(const auto & bid : builtBuildings)
@@ -762,11 +753,30 @@ void CGTownInstance::initOverriddenBids()
 	}
 }
 
+bool CGTownInstance::isBonusingBuildingAdded(BuildingID::EBuildingID bid) const
+{
+	auto present = std::find_if(bonusingBuildings.begin(), bonusingBuildings.end(), [&](CGTownBuilding* building)
+		{
+			return building->getBuildingType().num == bid;
+		});
+
+	return present != bonusingBuildings.end();
+}
+
+//it does not check hasBuilt because this check is in the OnHeroVisit handler
+void CGTownInstance::tryAddOnePerWeekBonus(BuildingSubID::EBuildingSubID subID)
+{
+	auto bid = town->getBuildingType(subID);
+
+	if(bid != BuildingID::NONE && !isBonusingBuildingAdded(bid))
+		bonusingBuildings.push_back(new COPWBonus(bid, subID, this));
+}
+
 void CGTownInstance::tryAddVisitingBonus(BuildingSubID::EBuildingSubID subID)
 {
 	auto bid = town->getBuildingType(subID);
 
-	if(bid != BuildingID::NONE)
+	if(bid != BuildingID::NONE && !isBonusingBuildingAdded(bid))
 		bonusingBuildings.push_back(new CTownBonus(bid, subID, this));
 }
 
@@ -785,6 +795,28 @@ void CGTownInstance::addTownBonuses()
 	}
 }
 
+void CGTownInstance::fixBonusingDuplicates() //For versions 794-800
+{
+	std::map<BuildingID::EBuildingID, int> bids;
+
+	for(auto i = 0; i != bonusingBuildings.size(); i++)
+	{
+		auto bid = bonusingBuildings[i]->getBuildingType();
+		if(!bids.count(bid))
+			bids.insert({ bid, 0 });
+		else
+			bids[bid]++;
+	}
+	for(auto & pair : bids)
+	{
+		if(!pair.second)
+			continue;
+
+		for(auto i = 0; i < pair.second; i++)
+			deleteTownBonus(pair.first);
+	}
+}
+
 void CGTownInstance::deleteTownBonus(BuildingID::EBuildingID bid)
 {
 	size_t i = 0;

lib/mapObjects/CGTownInstance.h

@@ -275,6 +275,9 @@ public:
 		else if(!h.saving)
 			updateTown794();
 
+		if(!h.saving && (version >= 794 && version < 801))
+			fixBonusingDuplicates();
+
 		if(!h.saving)
 			this->setNodeType(CBonusSystemNode::TOWN);
 	}
@@ -366,9 +369,11 @@ private:
 	void updateBonusingBuildings();
 	bool hasBuiltInOldWay(ETownType::ETownType type, BuildingID bid) const;
 	bool townEnvisagesBuilding(BuildingSubID::EBuildingSubID bid) const;
+	bool isBonusingBuildingAdded(BuildingID::EBuildingID bid) const;
 	void tryAddOnePerWeekBonus(BuildingSubID::EBuildingSubID subID);
 	void tryAddVisitingBonus(BuildingSubID::EBuildingSubID subID);
 	void initOverriddenBids();
 	void addTownBonuses();
 	void updateTown794(); //populate overriddenBuildings and vanila bonuses for old saves 
+	void fixBonusingDuplicates(); //For versions 794-800.
 };

lib/mapObjects/CObjectHandler.cpp

@@ -119,14 +119,6 @@ CObjectHandler::CObjectHandler()
 	logGlobal->trace("\t\tDone loading resource prices!");
 }
 
-PlayerColor CGObjectInstance::getOwner() const
-{
-	//if (state)
-	//	return state->owner;
-	//else
-		return tempOwner; //won't have owner
-}
-
 CGObjectInstance::CGObjectInstance():
 	pos(-1,-1,-1),
 	ID(Obj::NO_OBJ),

lib/mapObjects/CObjectHandler.h

@@ -118,12 +118,12 @@ public:
 	Obj ID;
 	/// Subtype of object, depends on type
 	si32 subID;
+	/// Current owner of an object (when below PLAYER_LIMIT)
+	PlayerColor tempOwner;
 	/// Index of object in map's list of objects
 	ObjectInstanceID id;
 	/// Defines appearance of object on map (animation, blocked tiles, blit order, etc)
 	ObjectTemplate appearance;
-	/// Current owner of an object (when below PLAYER_LIMIT)
-	PlayerColor tempOwner;
 	/// If true hero can visit this object only from neighbouring tiles and can't stand on this object
 	bool blockVisit;
 
@@ -140,7 +140,10 @@ public:
 	/// "center" tile from which the sight distance is calculated
 	int3 getSightCenter() const;
 
-	PlayerColor getOwner() const override;
+	PlayerColor getOwner() const override 
+	{
+		return this->tempOwner;
+	}
 	void setOwner(PlayerColor ow);
 
 	/** APPEARANCE ACCESSORS **/
@@ -213,8 +216,8 @@ public:
 
 	///Entry point of Json (de-)serialization
 	void serializeJson(JsonSerializeFormat & handler);
-
 	virtual void updateFrom(const JsonNode & data);
+
 protected:
 	/// virtual method that allows synchronously update object state on server and all clients
 	virtual void setPropertyDer(ui8 what, ui32 val);

lib/registerTypes/RegisterTypes.h

@@ -144,6 +144,7 @@ void registerTypesMapObjectTypes(Serializer &s)
 	s.template registerType<ILimiter, AnyOfLimiter>();
 	s.template registerType<ILimiter, NoneOfLimiter>();
 	s.template registerType<ILimiter, OppositeSideLimiter>();
+	s.template registerType<IUpdater, OwnerUpdater>();
 	//new types (other than netpacks) must register here
 	//order of type registration is critical for loading old savegames
 }

server/CGameHandler.cpp

@@ -901,7 +901,7 @@ void CGameHandler::endBattle(int3 tile, const CGHeroInstance * heroAttacker, con
 		&& !heroDefender->inTownGarrison
 		&& heroDefender->visitedTown->garrisonHero == heroDefender)
 	{
-		garrisonSwapOnSiege(heroDefender->visitedTown->id); //return defending visitor from garrison to its rightful place
+		swapGarrisonOnSiege(heroDefender->visitedTown->id); //return defending visitor from garrison to its rightful place
 	}
 	//give exp
 	if(battleResult.data->exp[0] && heroAttacker && battleResult.get()->winner == BattleSide::ATTACKER)
@@ -3492,7 +3492,7 @@ void CGameHandler::moveArmy(const CArmedInstance *src, const CArmedInstance *dst
 	}
 }
 
-bool CGameHandler::garrisonSwapOnSiege(ObjectInstanceID tid)
+bool CGameHandler::swapGarrisonOnSiege(ObjectInstanceID tid)
 {
 	const CGTownInstance * town = getTown(tid);
 

server/CGameHandler.h

@@ -242,7 +242,7 @@ public:
 	//void lootArtifacts (TArtHolder source, TArtHolder dest, std::vector<ui32> &arts); //after battle - move al arts to winer
 	bool buySecSkill( const IMarket *m, const CGHeroInstance *h, SecondarySkill skill);
 	bool garrisonSwap(ObjectInstanceID tid);
-	bool garrisonSwapOnSiege(ObjectInstanceID tid);
+	bool swapGarrisonOnSiege(ObjectInstanceID tid);
 	bool upgradeCreature( ObjectInstanceID objid, SlotID pos, CreatureID upgID );
 	bool recruitCreatures(ObjectInstanceID objid, ObjectInstanceID dst, CreatureID crid, ui32 cram, si32 level);
 	bool buildStructure(ObjectInstanceID tid, BuildingID bid, bool force=false);//force - for events: no cost, no checkings

test/mock/mock_IGameCallback.h

@@ -78,7 +78,7 @@ public:
 	void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, bool creatureBank = false) override {}; //if any of armies is hero, hero will be used
 	void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, bool creatureBank = false) override {}; //if any of armies is hero, hero will be used, visitable tile of second obj is place of battle
 	bool moveHero(ObjectInstanceID hid, int3 dst, ui8 teleporting, bool transit = false, PlayerColor asker = PlayerColor::NEUTRAL) override {return false;};
-	bool garrisonSwapOnSiege(ObjectInstanceID tid) override {};
+	bool swapGarrisonOnSiege(ObjectInstanceID tid) override {};
 	void giveHeroBonus(GiveBonus * bonus) override {};
 	void setMovePoints(SetMovePoints * smp) override {};
 	void setManaPoints(ObjectInstanceID hid, int val) override {};