Add tests for bonus system propagation, fix discovered issues

client/ClientCommandManager.cpp

@@ -467,7 +467,7 @@ void ClientCommandManager::handleBonusesCommand(std::istringstream & singleWordB
 
 	printCommandMessage("\nInherited bonuses:\n");
 	TCNodes parents;
-		GAME->interface()->localState->getCurrentArmy()->getParents(parents);
+		GAME->interface()->localState->getCurrentArmy()->getDirectParents(parents);
 	for(const CBonusSystemNode *parent : parents)
 	{
 		printCommandMessage(std::string("\nBonuses from ") + typeid(*parent).name() + "\n" + format(*parent->getAllBonuses(Selector::all)) + "\n");

lib/bonuses/CBonusSystemNode.cpp

@@ -35,7 +35,7 @@ std::shared_ptr<const Bonus> CBonusSystemNode::getFirstBonus(const CSelector & s
 		return ret;
 
 	TCNodes lparents;
-	getParents(lparents);
+	getDirectParents(lparents);
 	for(const CBonusSystemNode *pname : lparents)
 	{
 		ret = pname->getFirstBonus(selector);
@@ -46,25 +46,12 @@ std::shared_ptr<const Bonus> CBonusSystemNode::getFirstBonus(const CSelector & s
 	return nullptr;
 }
 
-void CBonusSystemNode::getParents(TCNodes & out) const /*retrieves list of parent nodes (nodes to inherit bonuses from) */
+void CBonusSystemNode::getDirectParents(TCNodes & out) const /*retrieves list of parent nodes (nodes to inherit bonuses from) */
 {
 	for(const auto * elem : parentsToInherit)
 		out.insert(elem);
 }
 
-void CBonusSystemNode::getAllParents(TCNodes & out) const //retrieves list of parent nodes (nodes to inherit bonuses from)
-{
-	for(auto * parent : parentsToInherit)
-	{
-		// Diamond found! One of the parents of the targeted node can be discovered in two ways.
-		// For example, a hero has been attached to both the player node and the global node (to which the player node is also attached).
-		// This is illegal and can be a source of duplicate bonuses.
-		assert(out.count(parent) == 0);
-		out.insert(parent);
-		parent->getAllParents(out);
-	}
-}
-
 void CBonusSystemNode::getAllBonusesRec(BonusList &out) const
 {
 	BonusList beforeUpdate;
@@ -219,13 +206,15 @@ void CBonusSystemNode::attachToSource(const CBonusSystemNode & parent)
 	assert(!vstd::contains(parentsToInherit, &parent));
 	parentsToInherit.push_back(&parent);
 
+	++globalCounter;
+
 	if(!isHypothetic())
 	{
 		if(parent.actsAsBonusSourceOnly())
 			parent.newRedDescendant(*this);
 	}
 
-	nodeHasChanged();
+	invalidateChildrenNodes(globalCounter);
 }
 
 void CBonusSystemNode::detachFrom(CBonusSystemNode & parent)
@@ -268,6 +257,8 @@ void CBonusSystemNode::detachFromSource(const CBonusSystemNode & parent)
 {
 	assert(vstd::contains(parentsToInherit, &parent));
 
+	++globalCounter;
+
 	if(!isHypothetic())
 	{
 		if(parent.actsAsBonusSourceOnly())
@@ -284,7 +275,7 @@ void CBonusSystemNode::detachFromSource(const CBonusSystemNode & parent)
 			nodeShortInfo(), static_cast<int>(nodeType), parent.nodeShortInfo(), static_cast<int>(parent.nodeType));
 	}
 
-	nodeHasChanged();
+	invalidateChildrenNodes(globalCounter);
 }
 
 void CBonusSystemNode::removeBonusesRecursive(const CSelector & s)
@@ -371,7 +362,7 @@ void CBonusSystemNode::propagateBonus(const std::shared_ptr<Bonus> & b, const CB
 {
 	if(b->propagator->shouldBeAttached(this))
 	{
-		auto propagated = b->propagationUpdater 
+		auto propagated = b->propagationUpdater
 			? source.getUpdatedBonus(b, b->propagationUpdater)
 			: b;
 		bonuses.push_back(propagated);
@@ -442,7 +433,7 @@ std::string CBonusSystemNode::nodeShortInfo() const
 void CBonusSystemNode::getRedParents(TCNodes & out) const
 {
 	TCNodes lparents;
-	getParents(lparents);
+	getDirectParents(lparents);
 	for(const CBonusSystemNode *pname : lparents)
 	{
 		if(pname->actsAsBonusSourceOnly())
@@ -606,23 +597,6 @@ void CBonusSystemNode::nodeHasChanged()
 	invalidateChildrenNodes(++globalCounter);
 }
 
-void CBonusSystemNode::recomputePropagationUpdaters(const CBonusSystemNode & source)
-{
-	for(const auto & b : exportedBonuses)
-	{
-		if (b->propagator && b->propagationUpdater)
-		{
-			unpropagateBonus(b);
-			propagateBonus(b,  source);
-		}
-	}
-
-	for(const CBonusSystemNode * parent : source.parentsToInherit)
-		if (parent->actsAsBonusSourceOnly())
-			recomputePropagationUpdaters(*parent);
-
-}
-
 void CBonusSystemNode::invalidateChildrenNodes(int32_t changeCounter)
 {
 	if (nodeChanged == changeCounter)
@@ -630,8 +604,6 @@ void CBonusSystemNode::invalidateChildrenNodes(int32_t changeCounter)
 
 	nodeChanged = changeCounter;
 
-	recomputePropagationUpdaters(*this);
-
 	for(CBonusSystemNode * child : children)
 		child->invalidateChildrenNodes(changeCounter);
 }

lib/bonuses/CBonusSystemNode.h

@@ -39,8 +39,12 @@ public:
 	};
 
 private:
-	BonusList bonuses; //wielded bonuses (local or up-propagated here)
-	BonusList exportedBonuses; //bonuses coming from this node (wielded or propagated away)
+	/// List of bonuses that affect this node, whether local, or propagated to this node
+	BonusList bonuses;
+
+	/// List of bonuses that ar ecoming from this node.
+	/// Also includes nodes that are propagated away from this node, and might not affect this node itself
+	BonusList exportedBonuses;
 
 	TCNodesVector parentsToInherit; // we inherit bonuses from them
 	TNodesVector parentsToPropagate; // we may attach our bonuses to them
@@ -71,11 +75,8 @@ private:
 	void getRedAncestors(TCNodes &out) const;
 	void getRedChildren(TNodes &out);
 
-	void getAllParents(TCNodes & out) const;
-
 	void propagateBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & source);
 	void unpropagateBonus(const std::shared_ptr<Bonus> & b);
-	void recomputePropagationUpdaters(const CBonusSystemNode & source);
 	bool actsAsBonusSourceOnly() const;
 
 	void newRedDescendant(CBonusSystemNode & descendant) const; //propagation needed
@@ -95,7 +96,7 @@ public:
 	virtual ~CBonusSystemNode();
 
 	TConstBonusListPtr getAllBonuses(const CSelector &selector, const std::string &cachingStr = "") const override;
-	void getParents(TCNodes &out) const;  //retrieves list of parent nodes (nodes to inherit bonuses from),
+	void getDirectParents(TCNodes &out) const;  //retrieves list of parent nodes (nodes to inherit bonuses from),
 
 	/// Returns first bonus matching selector
 	std::shared_ptr<const Bonus> getFirstBonus(const CSelector & selector) const;

test/CMakeLists.txt

@@ -23,6 +23,8 @@ set(test_SRCS
 		battle/CUnitStateMagicTest.cpp
 		battle/battle_UnitTest.cpp
 
+		bonus/BonusSystemTest.cpp
+
 		entity/CArtifactTest.cpp
 		entity/CCreatureTest.cpp
 		entity/CFactionTest.cpp

test/bonus/BonusSystemTest.cpp

@@ -0,0 +1,302 @@
+/*
+ * BattleHexTest.cpp, part of VCMI engine
+ *
+ * Authors: listed in file AUTHORS in main folder
+ *
+ * License: GNU General Public License v2.0 or later
+ * Full text of license available in license.txt file, in main folder
+ *
+ */
+
+#include "StdInc.h"
+#include "../lib/bonuses/CBonusSystemNode.h"
+#include "../lib/bonuses/BonusEnum.h"
+#include "../lib/bonuses/Limiters.h"
+#include "../lib/bonuses/Propagators.h"
+#include "../lib/bonuses/Updaters.h"
+
+namespace test
+{
+
+using namespace ::testing;
+
+class TestBonusSystemNode : public CBonusSystemNode
+{
+public:
+	using CBonusSystemNode::CBonusSystemNode;
+
+	MOCK_CONST_METHOD0(getOwner, PlayerColor());
+
+	void setPlayer(PlayerColor player)
+	{
+		EXPECT_CALL(*this, getOwner()).WillRepeatedly(Return(player));
+	}
+
+	void giveIntimidationSkill()
+	{
+		auto intimidationBonus = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::MORALE, BonusSource::SECONDARY_SKILL, -1, SecondarySkill(0));
+		intimidationBonus->propagator = std::make_shared<CPropagatorNodeType>(BonusNodeType::BATTLE_WIDE);
+		intimidationBonus->propagationUpdater = std::make_shared<OwnerUpdater>();
+		intimidationBonus->limiter = std::make_shared<OppositeSideLimiter>();
+
+		addNewBonus(intimidationBonus);
+	}
+};
+
+class BonusSystemTest : public Test
+{
+protected:
+
+	CBonusSystemNode global{BonusNodeType::GLOBAL_EFFECTS};
+	CBonusSystemNode battle{BonusNodeType::BATTLE_WIDE};
+
+	TestBonusSystemNode playerRed{BonusNodeType::PLAYER};
+	TestBonusSystemNode playerBlue{BonusNodeType::PLAYER};
+
+	TestBonusSystemNode heroAine{BonusNodeType::HERO};
+	TestBonusSystemNode heroBron{BonusNodeType::HERO};
+
+	CBonusSystemNode artifactTypeRing{BonusNodeType::ARTIFACT};
+	CBonusSystemNode artifactTypeSword{BonusNodeType::ARTIFACT};
+	CBonusSystemNode artifactTypeArmor{BonusNodeType::ARTIFACT};
+	CBonusSystemNode artifactTypeOrb{BonusNodeType::ARTIFACT};
+	CBonusSystemNode artifactTypeLegion{BonusNodeType::ARTIFACT};
+
+	CBonusSystemNode creatureAngel{BonusNodeType::CREATURE};
+	CBonusSystemNode creatureDevil{BonusNodeType::CREATURE};
+	CBonusSystemNode creaturePikeman{BonusNodeType::CREATURE};
+
+	void startBattle()
+	{
+		heroAine.attachTo(battle);
+		heroBron.attachTo(battle);
+	}
+
+	void SetUp() override
+	{
+		playerRed.attachTo(global);
+		playerBlue.attachTo(global);
+
+		heroAine.attachTo(playerRed);
+		heroBron.attachTo(playerBlue);
+
+		playerRed.setPlayer(PlayerColor(0));
+		playerBlue.setPlayer(PlayerColor(1));
+		heroAine.setPlayer(PlayerColor(0));
+		heroBron.setPlayer(PlayerColor(1));
+
+		auto spellpowerBonus = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::PRIMARY_SKILL, BonusSource::ARTIFACT, 1, ArtifactID(0), PrimarySkill::SPELL_POWER);
+		auto spellpowerBonus4 = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::PRIMARY_SKILL, BonusSource::ARTIFACT, 4, ArtifactID(1), PrimarySkill::SPELL_POWER);
+		auto attackBonus = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::PRIMARY_SKILL, BonusSource::ARTIFACT, 1, ArtifactID(2), PrimarySkill::ATTACK);
+		auto blockMagicBonus = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::BLOCK_ALL_MAGIC, BonusSource::ARTIFACT, 1, ArtifactID(3));
+		auto legionBonus = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::CREATURE_GROWTH, BonusSource::ARTIFACT, 4, ArtifactID(4), BonusCustomSubtype::creatureLevel(3));
+
+		auto luckBonusEnemies = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::LUCK, BonusSource::CREATURE_ABILITY, -1, CreatureID(0));
+		auto moraleBonusAllies = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::MORALE, BonusSource::CREATURE_ABILITY, 1, CreatureID(1));
+
+		luckBonusEnemies->propagator = std::make_shared<CPropagatorNodeType>(BonusNodeType::BATTLE_WIDE);
+		luckBonusEnemies->propagationUpdater = std::make_shared<OwnerUpdater>();
+		luckBonusEnemies->limiter = std::make_shared<OppositeSideLimiter>();
+		luckBonusEnemies->stacking = "devil";
+		moraleBonusAllies->propagator = std::make_shared<CPropagatorNodeType>(BonusNodeType::ARMY);
+		moraleBonusAllies->stacking = "angel";
+		blockMagicBonus->propagator = std::make_shared<CPropagatorNodeType>(BonusNodeType::BATTLE_WIDE);
+		legionBonus->propagator = std::make_shared<CPropagatorNodeType>(BonusNodeType::TOWN_AND_VISITOR);
+
+		artifactTypeRing.addNewBonus(spellpowerBonus);
+		artifactTypeSword.addNewBonus(attackBonus);
+		artifactTypeArmor.addNewBonus(spellpowerBonus4);
+		artifactTypeOrb.addNewBonus(blockMagicBonus);
+		artifactTypeLegion.addNewBonus(legionBonus);
+		creatureAngel.addNewBonus(moraleBonusAllies);
+		creatureDevil.addNewBonus(luckBonusEnemies);
+	}
+};
+
+TEST_F(BonusSystemTest, multipleBonusSources)
+{
+	CBonusSystemNode ring1{BonusNodeType::ARTIFACT_INSTANCE};
+	CBonusSystemNode ring2{BonusNodeType::ARTIFACT_INSTANCE};
+	CBonusSystemNode armor{BonusNodeType::ARTIFACT_INSTANCE};
+
+	ring1.attachToSource(artifactTypeRing);
+	ring2.attachToSource(artifactTypeRing);
+	armor.attachToSource(artifactTypeArmor);
+
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::PRIMARY_SKILL, PrimarySkill::SPELL_POWER), 0);
+
+	// single bonus produces single effect
+	heroAine.attachToSource(ring1);
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::PRIMARY_SKILL, PrimarySkill::SPELL_POWER), 1);
+
+	// same bonus applied twice produces single effect
+	heroAine.attachToSource(ring2);
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::PRIMARY_SKILL, PrimarySkill::SPELL_POWER), 1);
+
+	// different bonuses stack
+	heroAine.attachToSource(armor);
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::PRIMARY_SKILL, PrimarySkill::SPELL_POWER), 1+4);
+
+	heroAine.detachFromSource(ring1);
+	heroAine.detachFromSource(ring2);
+	heroAine.detachFromSource(armor);
+}
+
+TEST_F(BonusSystemTest, battlewidePropagationToAllies)
+{
+	CBonusSystemNode angel1{BonusNodeType::STACK_INSTANCE};
+	CBonusSystemNode angel2{BonusNodeType::STACK_INSTANCE};
+
+	CBonusSystemNode pikemanAlly{BonusNodeType::STACK_INSTANCE};
+	CBonusSystemNode pikemanEnemy{BonusNodeType::STACK_INSTANCE};
+
+	angel1.attachToSource(creatureAngel);
+	angel2.attachToSource(creatureAngel);
+
+	pikemanAlly.attachToSource(creaturePikeman);
+	pikemanEnemy.attachToSource(creaturePikeman);
+
+	pikemanAlly.attachTo(heroAine);
+	pikemanEnemy.attachTo(heroBron);
+
+	startBattle();
+
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::MORALE), 0);
+
+	angel1.attachTo(heroAine);
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::MORALE), 1);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::MORALE), 1);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::MORALE), 0);
+
+	angel2.attachTo(heroAine);
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::MORALE), 1);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::MORALE), 1);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::MORALE), 0);
+}
+
+TEST_F(BonusSystemTest, battlewidePropagationToAll)
+{
+	CBonusSystemNode orb{BonusNodeType::ARTIFACT_INSTANCE};
+	orb.attachToSource(artifactTypeOrb);
+
+	heroAine.attachToSource(orb);
+	startBattle();
+
+	EXPECT_TRUE(heroAine.hasBonusOfType(BonusType::BLOCK_ALL_MAGIC));
+	EXPECT_TRUE(heroBron.hasBonusOfType(BonusType::BLOCK_ALL_MAGIC));
+}
+
+TEST_F(BonusSystemTest, battlewidePropagationToEnemies)
+{
+	TestBonusSystemNode devil1{BonusNodeType::STACK_INSTANCE};
+	TestBonusSystemNode devil2{BonusNodeType::STACK_INSTANCE};
+
+	TestBonusSystemNode pikemanAlly{BonusNodeType::STACK_INSTANCE};
+	TestBonusSystemNode pikemanEnemy{BonusNodeType::STACK_INSTANCE};
+
+	devil1.setPlayer(PlayerColor(0));
+	devil2.setPlayer(PlayerColor(0));
+	pikemanAlly.setPlayer(PlayerColor(0));
+	pikemanEnemy.setPlayer(PlayerColor(1));
+
+	devil1.attachToSource(creatureDevil);
+	devil2.attachToSource(creatureDevil);
+
+	pikemanAlly.attachToSource(creaturePikeman);
+	pikemanEnemy.attachToSource(creaturePikeman);
+
+	pikemanAlly.attachTo(heroAine);
+	pikemanEnemy.attachTo(heroBron);
+
+	startBattle();
+
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::LUCK), 0);
+
+	devil1.attachTo(heroAine);
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::LUCK), -1);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::LUCK), -1);
+
+	devil2.attachTo(heroAine);
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::LUCK), -1);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::LUCK), -1);
+
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::LUCK), -1);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::LUCK), 0);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::LUCK), -1);
+}
+
+TEST_F(BonusSystemTest, battlewideSkillPropagationToEnemies)
+{
+	TestBonusSystemNode pikemanAlly{BonusNodeType::STACK_INSTANCE};
+	TestBonusSystemNode pikemanEnemy{BonusNodeType::STACK_INSTANCE};
+
+	// #5975 - skill that affects only enemies in battle is broken,
+	// but only if hero has *any* artifact (including obligatory catapult/spellbook, so always)
+	CBonusSystemNode armor{BonusNodeType::ARTIFACT_INSTANCE};
+	armor.attachToSource(artifactTypeArmor);
+	heroAine.attachToSource(armor);
+
+	pikemanAlly.setPlayer(PlayerColor(0));
+	pikemanEnemy.setPlayer(PlayerColor(1));
+
+	pikemanAlly.attachToSource(creaturePikeman);
+	pikemanEnemy.attachToSource(creaturePikeman);
+
+	heroAine.giveIntimidationSkill();
+
+	pikemanAlly.attachTo(heroAine);
+	pikemanEnemy.attachTo(heroBron);
+
+	startBattle();
+
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::MORALE), -1);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::MORALE), -1);
+
+	heroAine.nodeHasChanged();
+
+	EXPECT_EQ(heroAine.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(heroBron.valOfBonuses(BonusType::MORALE), -1);
+	EXPECT_EQ(pikemanAlly.valOfBonuses(BonusType::MORALE), 0);
+	EXPECT_EQ(pikemanEnemy.valOfBonuses(BonusType::MORALE), -1);
+}
+
+TEST_F(BonusSystemTest, legionPieces)
+{
+	TestBonusSystemNode townAndVisitor{BonusNodeType::TOWN_AND_VISITOR};
+	TestBonusSystemNode town{BonusNodeType::TOWN_AND_VISITOR};
+	town.attachTo(townAndVisitor);
+
+	CBonusSystemNode legion{BonusNodeType::ARTIFACT_INSTANCE};
+	legion.attachToSource(artifactTypeLegion);
+
+	heroAine.attachToSource(legion);
+
+	heroAine.attachTo(townAndVisitor);
+	EXPECT_EQ(town.valOfBonuses(BonusType::CREATURE_GROWTH, BonusCustomSubtype::creatureLevel(3)), 4);
+
+	heroAine.detachFromSource(legion);
+	EXPECT_EQ(town.valOfBonuses(BonusType::CREATURE_GROWTH, BonusCustomSubtype::creatureLevel(3)), 0);
+
+	heroAine.attachToSource(legion);
+	EXPECT_EQ(town.valOfBonuses(BonusType::CREATURE_GROWTH, BonusCustomSubtype::creatureLevel(3)), 4);
+
+	heroAine.detachFrom(townAndVisitor);
+	EXPECT_EQ(town.valOfBonuses(BonusType::CREATURE_GROWTH, BonusCustomSubtype::creatureLevel(3)), 0);
+}
+
+}