diff --git a/client/ClientCommandManager.cpp b/client/ClientCommandManager.cpp index 93a68c758..a28c55f63 100644 --- a/client/ClientCommandManager.cpp +++ b/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"); diff --git a/lib/bonuses/CBonusSystemNode.cpp b/lib/bonuses/CBonusSystemNode.cpp index b0fe30c6d..1df3a67c0 100644 --- a/lib/bonuses/CBonusSystemNode.cpp +++ b/lib/bonuses/CBonusSystemNode.cpp @@ -35,7 +35,7 @@ std::shared_ptr 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 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(nodeType), parent.nodeShortInfo(), static_cast(parent.nodeType)); } - nodeHasChanged(); + invalidateChildrenNodes(globalCounter); } void CBonusSystemNode::removeBonusesRecursive(const CSelector & s) @@ -371,7 +362,7 @@ void CBonusSystemNode::propagateBonus(const std::shared_ptr & 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); } diff --git a/lib/bonuses/CBonusSystemNode.h b/lib/bonuses/CBonusSystemNode.h index b9ff66285..4b68236a0 100644 --- a/lib/bonuses/CBonusSystemNode.h +++ b/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 & b, const CBonusSystemNode & source); void unpropagateBonus(const std::shared_ptr & 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 getFirstBonus(const CSelector & selector) const; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 425b9e6c6..d33021077 100644 --- a/test/CMakeLists.txt +++ b/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 diff --git a/test/bonus/BonusSystemTest.cpp b/test/bonus/BonusSystemTest.cpp new file mode 100644 index 000000000..ed327fffb --- /dev/null +++ b/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(BonusDuration::PERMANENT, BonusType::MORALE, BonusSource::SECONDARY_SKILL, -1, SecondarySkill(0)); + intimidationBonus->propagator = std::make_shared(BonusNodeType::BATTLE_WIDE); + intimidationBonus->propagationUpdater = std::make_shared(); + intimidationBonus->limiter = std::make_shared(); + + 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(BonusDuration::PERMANENT, BonusType::PRIMARY_SKILL, BonusSource::ARTIFACT, 1, ArtifactID(0), PrimarySkill::SPELL_POWER); + auto spellpowerBonus4 = std::make_shared(BonusDuration::PERMANENT, BonusType::PRIMARY_SKILL, BonusSource::ARTIFACT, 4, ArtifactID(1), PrimarySkill::SPELL_POWER); + auto attackBonus = std::make_shared(BonusDuration::PERMANENT, BonusType::PRIMARY_SKILL, BonusSource::ARTIFACT, 1, ArtifactID(2), PrimarySkill::ATTACK); + auto blockMagicBonus = std::make_shared(BonusDuration::PERMANENT, BonusType::BLOCK_ALL_MAGIC, BonusSource::ARTIFACT, 1, ArtifactID(3)); + auto legionBonus = std::make_shared(BonusDuration::PERMANENT, BonusType::CREATURE_GROWTH, BonusSource::ARTIFACT, 4, ArtifactID(4), BonusCustomSubtype::creatureLevel(3)); + + auto luckBonusEnemies = std::make_shared(BonusDuration::PERMANENT, BonusType::LUCK, BonusSource::CREATURE_ABILITY, -1, CreatureID(0)); + auto moraleBonusAllies = std::make_shared(BonusDuration::PERMANENT, BonusType::MORALE, BonusSource::CREATURE_ABILITY, 1, CreatureID(1)); + + luckBonusEnemies->propagator = std::make_shared(BonusNodeType::BATTLE_WIDE); + luckBonusEnemies->propagationUpdater = std::make_shared(); + luckBonusEnemies->limiter = std::make_shared(); + luckBonusEnemies->stacking = "devil"; + moraleBonusAllies->propagator = std::make_shared(BonusNodeType::ARMY); + moraleBonusAllies->stacking = "angel"; + blockMagicBonus->propagator = std::make_shared(BonusNodeType::BATTLE_WIDE); + legionBonus->propagator = std::make_shared(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); +} + +}