Better support for Adela specialty (+new modding functionality for it)

Fixes Adela specialty that was apparently broken back in #1518 and
replaced with logic that was clearly not tested - it was neither
functional, nor it was following H3 behavior.

- `HAS_ANOTHER_BONUS_LIMITER` now accepts `null` in place of bonus type,
for cases when limiting is needed by bonus source or bonus subtype. This
allows Adela Bless specialty to always work, irregardless of which
bonuses are provided by Bless.
- Implemented `DIVIDE_STACK_LEVEL` updater that functions same as
`TIMES_STACK_LEVEL`, but it divides bonus value, instead of multiplying
it (to make Adela specialty weaker for high-tier units, as in H3)
- Implemented `TIMES_HERO_LEVEL_DIVIDE_STACK_LEVEL` updater that
combines two existing updaters, to implement `val * heroLevel /
unitLevel` formula needed for Adela specialty
- Removed deprecated `ARMY_MOVEMENT` updater. Its functionality has
already been removed in 1.6.X releases, and it was remaining only as a
placeholder
- Updated modding documentation to account for these changes & to remove
some TODO's

Fixed regression from #777 that could led to either duplicated bonuses
or to multiple application of updaters. It introduced double-recursion -
node parents were gathered recursively, and then bonuses were also
collected recursively within each parent. This created situation where
updater could be applied different number of times. For example, hero
bonus that is propagated to unit in combat could be selected directly,
or via hero->combat unit chain, or via hero->garrison unit->combat unit
chains, leading to different calls to updaters if updater handles
garrison unit node type

config/heroes/castle.json

@@ -182,7 +182,7 @@
 						{
 							"type" : "HAS_ANOTHER_BONUS_LIMITER",
 							"parameters" : [
-								"GENERAL_DAMAGE_PREMY",
+								null,
 								null,
 								{
 									"type" : "SPELL_EFFECT",
@@ -191,7 +191,7 @@
 							]
 						}
 					],
-					"updater" : "TIMES_HERO_LEVEL",
+					"updater" : "TIMES_HERO_LEVEL_DIVIDE_STACK_LEVEL",
 					"val" : 3
 				}
 			}

config/schemas/bonusInstance.json

@@ -72,7 +72,7 @@
 			"anyOf" : [
 				{
 					"type" : "string",
-					"enum" : [ "TIMES_HERO_LEVEL", "TIMES_STACK_LEVEL", "ARMY_MOVEMENT", "BONUS_OWNER_UPDATER" ]
+					"enum" : [ "TIMES_HERO_LEVEL", "TIMES_STACK_LEVEL", "DIVIDE_STACK_LEVEL", "BONUS_OWNER_UPDATER", "TIMES_HERO_LEVEL_DIVIDE_STACK_LEVEL" ]
 				},
 				{
 					"description" : "updater",
@@ -82,7 +82,7 @@
 					"properties" : {
 						"type" : {
 							"type" : "string",
-							"enum" : [ "GROWS_WITH_LEVEL", "ARMY_MOVEMENT" ],
+							"enum" : [ "GROWS_WITH_LEVEL" ],
 							"description" : "type"
 						},
 						"parameters" : {

docs/modders/Bonus/Bonus_Updaters.md

@@ -63,20 +63,27 @@ Usage:
 
 Remark: The stack level for war machines is 0.
 
-## ARMY_MOVEMENT
+## DIVIDE_STACK_LEVEL
 
-- Type: Complex
-- Parameters: basePerSpeed, dividePerSpeed, additionalMultiplier, maxValue
-- Effect: Updates val to `val+= max((floor(basePerSpeed / dividePerSpeed) * additionalMultiplier), maxValue)`
-- Remark: this updater is designed for MOVEMENT bonus to match H3 army movement rules (in the example - actual movement updater, which produces values same as in default movement.txt).
-- Example:
+- Type: Simple
+- Effect: Updates val to `val / stackLevel`
 
-```json
-"updater" : {
-    "parameters" : [ 20, 3, 10, 700 ],
-    "type" : "ARMY_MOVEMENT"
-}
-```
+Usage:
+
+`"updater" : "DIVIDE_STACK_LEVEL"`
+
+Remark: The stack level for war machines is 0.
+
+## TIMES_HERO_LEVEL_DIVIDE_STACK_LEVEL
+
+- Type: Simple
+- Effect: Same effect as `TIMES_HERO_LEVEL` combined with `DIVIDE_STACK_LEVEL`: `val * heroLevel / stackLevel`
+
+Intended to be used as hero bonus (such as specialty, skill, or artifact), for bonuses that affect units (Example: Adela Bless specialty)
+
+Usage:
+
+`"updater" : "TIMES_HERO_LEVEL_DIVIDE_STACK_LEVEL"`
 
 ## BONUS_OWNER_UPDATER
 

docs/modders/Bonus_Format.md

@@ -40,25 +40,39 @@ All parameters but type are optional.
 	// TODO
 	"effectRange" : "EFFECT_RANGE",
 
-	// TODO
+	// This sections allows to define 'limiter', which allows to limit bonus and only apply it under specific conditions
+	// Typical conditions are "affect only specific creature", or "affect only if target has another, specific bonus"
+	// See Bonus Limiters list below for full list of supported limiters 
 	"limiters" : [
 		"PREDEFINED_LIMITER", optional_parameters (...), //whhich one is preferred?
 		{"type" : LIMITER_TYPE, "parameters" : [1,2,3]}
 	],
 	
-	// TODO
+	// Normally, only entities that are located "below" bonus source are affected by the bonus
+	// For example, bonus on creature will only affect creature itself,
+	// bonus on hero will affect hero itself and all its units, and player bonus will affect all objects owned by player
+	// Propagator allows bonus to affect "upwards" entities. 
+	// For example, creature that has bonus with battle-wide propagator will affect all units in combat, and not just unit itself
+	// See Bonus Propagators list below for full list of supported propagators
 	"propagator" : 	["PROPAGATOR_TYPE", optional_parameters (...)],
 	
-	// TODO
+	// Updaters allow to modify bonus depending on context. 
+	// For example, it can be used to scale bonus based on hero or unit level
+	// See Bonus Updaters list below for full list of supported updaters
 	"updater" :	    {Bonus Updater},
 	
-	// TODO
+	// This is special type of propagator, that is only activated when bonus is being propagated upwards,
+	// using its propagator. It has no effect on bonuses without propagator
 	"propagationUpdater" :	{Bonus Updater, but works during propagation},
 	
 	// TODO
 	"description" : "",
 	
-	// TODO
+	// Stacking string allows to block stacking of bonuses from different entities
+	// For example, devils and archdevils (different entities) both have battle-wide debuff to luck
+	// Normally, having both such units in combat would result in bonus stacking, providing -2 debuff to luck in total
+	// If such behavior is undesired, both such unit must have same, non-empty stacking string
+	// String can contain any text, as long as it not empty and is same for both of such creatures
 	"stacking" :    ""
 }
 ```

lib/bonuses/CBonusSystemNode.cpp

@@ -55,6 +55,10 @@ void CBonusSystemNode::getAllParents(TCNodes & out) const //retrieves list of pa
 {
 	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);
 	}
@@ -63,13 +67,10 @@ void CBonusSystemNode::getAllParents(TCNodes & out) const //retrieves list of pa
 void CBonusSystemNode::getAllBonusesRec(BonusList &out, const CSelector & selector) const
 {
 	BonusList beforeUpdate;
-	TCNodes lparents;
-	getAllParents(lparents);
 
-	for(const auto * parent : lparents)
-	{
+	for(const auto * parent : parentsToInherit)
 		parent->getAllBonusesRec(beforeUpdate, selector);
-	}
+
 	bonuses.getAllBonuses(beforeUpdate);
 
 	for(const auto & b : beforeUpdate)
@@ -79,18 +80,7 @@ void CBonusSystemNode::getAllBonusesRec(BonusList &out, const CSelector & select
 			? getUpdatedBonus(b, b->updater)
 			: b;
 
-		//do not add bonus with updater
-		bool bonusExists = false;
-		for(const auto & bonus : out)
-		{
-			if (bonus == updated)
-				bonusExists = true;
-			if (bonus->updater && bonus->updater == updated->updater)
-				bonusExists = true;
-		}
-
-		if (!bonusExists)
-			out.push_back(updated);
+		out.push_back(updated);
 	}
 }
 

lib/bonuses/Limiters.cpp

@@ -166,15 +166,21 @@ HasAnotherBonusLimiter::HasAnotherBonusLimiter(BonusType bonus, BonusSubtypeID _
 
 ILimiter::EDecision HasAnotherBonusLimiter::limit(const BonusLimitationContext &context) const
 {
-	//TODO: proper selector config with parsing of JSON
-	auto mySelector = Selector::type()(type);
+	boost::container::static_vector<CSelector, 4> selectorSegments;
 
+	if (type != BonusType::NONE)
+		selectorSegments.push_back(Selector::type()(type));
 	if(isSubtypeRelevant)
-		mySelector = mySelector.And(Selector::subtype()(subtype));
+		selectorSegments.push_back(Selector::subtype()(subtype));
 	if(isSourceRelevant && isSourceIDRelevant)
-		mySelector = mySelector.And(Selector::source(source, sid));
+		selectorSegments.push_back(Selector::source(source, sid));
 	else if (isSourceRelevant)
-		mySelector = mySelector.And(Selector::sourceTypeSel(source));
+		selectorSegments.push_back(Selector::sourceTypeSel(source));
+
+	auto mySelector = selectorSegments.empty() ? Selector::none : selectorSegments[0];
+
+	for (size_t i = 1; i <selectorSegments.size(); ++i)
+		mySelector = mySelector.And(selectorSegments[i]);
 
 	//if we have a bonus of required type accepted, limiter should accept also this bonus
 	if(context.alreadyAccepted.getFirst(mySelector))

lib/bonuses/Updaters.cpp

@@ -108,72 +108,53 @@ JsonNode TimesHeroLevelUpdater::toJsonNode() const
 	return JsonNode("TIMES_HERO_LEVEL");
 }
 
-ArmyMovementUpdater::ArmyMovementUpdater():
-	base(20),
-	divider(3),
-	multiplier(10),
-	max(700)
-{
-}
-
-ArmyMovementUpdater::ArmyMovementUpdater(int base, int divider, int multiplier, int max):
-	base(base),
-	divider(divider),
-	multiplier(multiplier),
-	max(max)
-{
-}
-
-std::shared_ptr<Bonus> ArmyMovementUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context)
+std::shared_ptr<Bonus> TimesHeroLevelDivideStackLevelUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context)
 {
+	if(context.getNodeType() == CBonusSystemNode::HERO)
+	{
+		auto newBonus = TimesHeroLevelUpdater::createUpdatedBonus(b, context);
+		newBonus->updater = divideStackLevel;
+		return newBonus;
+	}
 	return b;
 }
 
-std::string ArmyMovementUpdater::toString() const
+std::string TimesHeroLevelDivideStackLevelUpdater::toString() const
 {
-	return "ArmyMovementUpdater";
+	return "TimesHeroLevelDivideStackLevelUpdater";
 }
 
-JsonNode ArmyMovementUpdater::toJsonNode() const
+JsonNode TimesHeroLevelDivideStackLevelUpdater::toJsonNode() const
 {
-	JsonNode root;
-
-	root["type"].String() = "ARMY_MOVEMENT";
-	root["parameters"].Vector().emplace_back(base);
-	root["parameters"].Vector().emplace_back(divider);
-	root["parameters"].Vector().emplace_back(multiplier);
-	root["parameters"].Vector().emplace_back(max);
-
-	return root;
+	return JsonNode("TIMES_HERO_LEVEL_DIVIDE_STACK_LEVEL");
 }
+
+std::shared_ptr<Bonus> TimesStackLevelUpdater::apply(const std::shared_ptr<Bonus> & b, int level) const
+{
+	auto newBonus = std::make_shared<Bonus>(*b);
+	newBonus->val *= level;
+	newBonus->updater = nullptr; // prevent double-apply
+	return newBonus;
+}
+
 std::shared_ptr<Bonus> TimesStackLevelUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context)
 {
 	if(context.getNodeType() == CBonusSystemNode::STACK_INSTANCE || context.getNodeType() == CBonusSystemNode::COMMANDER)
 	{
 		int level = dynamic_cast<const CStackInstance &>(context).getLevel();
-		auto newBonus = std::make_shared<Bonus>(*b);
-		newBonus->val *= level;
-		return newBonus;
+		return apply(b, level);
 	}
-	else if(context.getNodeType() == CBonusSystemNode::STACK_BATTLE)
+
+	if(context.getNodeType() == CBonusSystemNode::STACK_BATTLE)
 	{
 		const auto & stack = dynamic_cast<const CStack &>(context);
 		//update if stack doesn't have an instance (summons, war machines)
 		if(stack.base == nullptr)
-		{
-			int level = stack.unitType()->getLevel();
-			auto newBonus = std::make_shared<Bonus>(*b);
-			newBonus->val *= level;
-			return newBonus;
-		}
+			return apply(b, stack.unitType()->getLevel());
+
 		// If these are not handled here, the final outcome may potentially be incorrect.
-		else
-		{
-			int level = dynamic_cast<const CStackInstance*>(stack.base)->getLevel();
-			auto newBonus = std::make_shared<Bonus>(*b);
-			newBonus->val *= level;
-			return newBonus;
-		}
+		int level = dynamic_cast<const CStackInstance*>(stack.base)->getLevel();
+		return apply(b, level);
 	}
 	return b;
 }
@@ -188,6 +169,46 @@ JsonNode TimesStackLevelUpdater::toJsonNode() const
 	return JsonNode("TIMES_STACK_LEVEL");
 }
 
+std::shared_ptr<Bonus> DivideStackLevelUpdater::apply(const std::shared_ptr<Bonus> & b, int level) const
+{
+	auto newBonus = std::make_shared<Bonus>(*b);
+	newBonus->val /= level;
+	newBonus->updater = nullptr; // prevent double-apply
+	return newBonus;
+}
+
+std::shared_ptr<Bonus> DivideStackLevelUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context)
+{
+	if(context.getNodeType() == CBonusSystemNode::STACK_INSTANCE || context.getNodeType() == CBonusSystemNode::COMMANDER)
+	{
+		int level = dynamic_cast<const CStackInstance &>(context).getLevel();
+		return apply(b, level);
+	}
+
+	if(context.getNodeType() == CBonusSystemNode::STACK_BATTLE)
+	{
+		const auto & stack = dynamic_cast<const CStack &>(context);
+		//update if stack doesn't have an instance (summons, war machines)
+		if(stack.base == nullptr)
+			return apply(b, stack.unitType()->getLevel());
+
+		// If these are not handled here, the final outcome may potentially be incorrect.
+		int level = dynamic_cast<const CStackInstance*>(stack.base)->getLevel();
+		return apply(b, level);
+	}
+	return b;
+}
+
+std::string DivideStackLevelUpdater::toString() const
+{
+	return "DivideStackLevelUpdater";
+}
+
+JsonNode DivideStackLevelUpdater::toJsonNode() const
+{
+	return JsonNode("DIVIDE_STACK_LEVEL");
+}
+
 std::string OwnerUpdater::toString() const
 {
 	return "OwnerUpdater";

lib/bonuses/Updaters.h

@@ -88,6 +88,7 @@ public:
 
 class DLL_LINKAGE TimesStackLevelUpdater : public IUpdater
 {
+	std::shared_ptr<Bonus> apply(const std::shared_ptr<Bonus> & b, int level) const;
 public:
 	template <typename Handler> void serialize(Handler & h)
 	{
@@ -99,22 +100,13 @@ public:
 	JsonNode toJsonNode() const override;
 };
 
-class DLL_LINKAGE ArmyMovementUpdater : public IUpdater
+class DLL_LINKAGE DivideStackLevelUpdater : public IUpdater
 {
+	std::shared_ptr<Bonus> apply(const std::shared_ptr<Bonus> & b, int level) const;
 public:
-	si32 base;
-	si32 divider;
-	si32 multiplier;
-	si32 max;
-	ArmyMovementUpdater();
-	ArmyMovementUpdater(int base, int divider, int multiplier, int max);
 	template <typename Handler> void serialize(Handler & h)
 	{
 		h & static_cast<IUpdater &>(*this);
-		h & base;
-		h & divider;
-		h & multiplier;
-		h & max;
 	}
 
 	std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) override;
@@ -122,6 +114,25 @@ public:
 	JsonNode toJsonNode() const override;
 };
 
+class DLL_LINKAGE TimesHeroLevelDivideStackLevelUpdater : public TimesHeroLevelUpdater
+{
+	std::shared_ptr<DivideStackLevelUpdater> divideStackLevel;
+public:
+	template <typename Handler> void serialize(Handler & h)
+	{
+		h & static_cast<TimesHeroLevelUpdater &>(*this);
+		h & divideStackLevel;
+	}
+
+	TimesHeroLevelDivideStackLevelUpdater()
+		: divideStackLevel(std::make_shared<DivideStackLevelUpdater>())
+	{}
+
+	std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) override;
+	std::string toString() const override;
+	JsonNode toJsonNode() const override;
+};
+
 class DLL_LINKAGE OwnerUpdater : public IUpdater
 {
 protected:

lib/json/JsonBonus.cpp

@@ -328,8 +328,9 @@ static TUpdaterPtr parseUpdater(const JsonNode & updaterJson)
 	const std::map<std::string, std::function<TUpdaterPtr()>> bonusUpdaterMap =
 	{
 			{"TIMES_HERO_LEVEL", std::make_shared<TimesHeroLevelUpdater>},
+			{"TIMES_HERO_LEVEL_DIVIDE_STACK_LEVEL", std::make_shared<TimesHeroLevelDivideStackLevelUpdater>},
+			{"DIVIDE_STACK_LEVEL", std::make_shared<DivideStackLevelUpdater>},
 			{"TIMES_STACK_LEVEL", std::make_shared<TimesStackLevelUpdater>},
-			{"ARMY_MOVEMENT", std::make_shared<ArmyMovementUpdater>},
 			{"BONUS_OWNER_UPDATER", std::make_shared<OwnerUpdater>}
 	};
 
@@ -354,24 +355,6 @@ static TUpdaterPtr parseUpdater(const JsonNode & updaterJson)
 				updater->stepSize = static_cast<int>(param[1].Integer());
 			return updater;
 		}
-		else if (updaterJson["type"].String() == "ARMY_MOVEMENT")
-		{
-			auto updater = std::make_shared<ArmyMovementUpdater>();
-			if(updaterJson["parameters"].isVector())
-			{
-				const auto & param = updaterJson["parameters"].Vector();
-				if(param.size() < 4)
-					logMod->warn("Invalid ARMY_MOVEMENT parameters, using default!");
-				else
-				{
-					updater->base = static_cast<si32>(param.at(0).Integer());
-					updater->divider = static_cast<si32>(param.at(1).Integer());
-					updater->multiplier = static_cast<si32>(param.at(2).Integer());
-					updater->max = static_cast<si32>(param.at(3).Integer());
-				}
-				return updater;
-			}
-		}
 		else
 			logMod->warn("Unknown updater type \"%s\"", updaterJson["type"].String());
 		break;
@@ -522,47 +505,52 @@ std::shared_ptr<ILimiter> JsonUtils::parseLimiter(const JsonNode & limiter)
 			}
 			else if(limiterType == "HAS_ANOTHER_BONUS_LIMITER")
 			{
-				std::string anotherBonusType = parameters[0].String();
-				auto it = bonusNameMap.find(anotherBonusType);
-				if(it == bonusNameMap.end())
+				auto bonusLimiter = std::make_shared<HasAnotherBonusLimiter>();
+
+				if (!parameters[0].isNull())
 				{
-					logMod->error("Error: invalid ability type %s.", anotherBonusType);
-				}
-				else
-				{
-					auto bonusLimiter = std::make_shared<HasAnotherBonusLimiter>();
-					bonusLimiter->type = it->second;
-					auto findSource = [&](const JsonNode & parameter)
+					std::string anotherBonusType = parameters[0].String();
+					auto it = bonusNameMap.find(anotherBonusType);
+					if(it != bonusNameMap.end())
 					{
-						if(parameter.getType() == JsonNode::JsonType::DATA_STRUCT)
+						bonusLimiter->type = it->second;
+					}
+					else
+					{
+						logMod->error("Error: invalid ability type %s.", anotherBonusType);
+					}
+				}
+
+				auto findSource = [&](const JsonNode & parameter)
+				{
+					if(parameter.getType() == JsonNode::JsonType::DATA_STRUCT)
+					{
+						auto sourceIt = bonusSourceMap.find(parameter["type"].String());
+						if(sourceIt != bonusSourceMap.end())
 						{
-							auto sourceIt = bonusSourceMap.find(parameter["type"].String());
-							if(sourceIt != bonusSourceMap.end())
-							{
-								bonusLimiter->source = sourceIt->second;
-								bonusLimiter->isSourceRelevant = true;
-								if(!parameter["id"].isNull()) {
-									loadBonusSourceInstance(bonusLimiter->sid, bonusLimiter->source, parameter["id"]);
-									bonusLimiter->isSourceIDRelevant = true;
-								}
+							bonusLimiter->source = sourceIt->second;
+							bonusLimiter->isSourceRelevant = true;
+							if(!parameter["id"].isNull()) {
+								loadBonusSourceInstance(bonusLimiter->sid, bonusLimiter->source, parameter["id"]);
+								bonusLimiter->isSourceIDRelevant = true;
 							}
 						}
-						return false;
-					};
-					if(parameters.size() > 1)
-					{
-						if(findSource(parameters[1]) && parameters.size() == 2)
-							return bonusLimiter;
-						else
-						{
-							loadBonusSubtype(bonusLimiter->subtype, bonusLimiter->type, parameters[1]);
-							bonusLimiter->isSubtypeRelevant = true;
-							if(parameters.size() > 2)
-								findSource(parameters[2]);
-						}
 					}
-					return bonusLimiter;
+					return false;
+				};
+				if(parameters.size() > 1)
+				{
+					if(findSource(parameters[1]) && parameters.size() == 2)
+						return bonusLimiter;
+					else
+					{
+						loadBonusSubtype(bonusLimiter->subtype, bonusLimiter->type, parameters[1]);
+						bonusLimiter->isSubtypeRelevant = true;
+						if(parameters.size() > 2)
+							findSource(parameters[2]);
+					}
 				}
+				return bonusLimiter;
 			}
 			else if(limiterType == "CREATURE_ALIGNMENT_LIMITER")
 			{

lib/serializer/RegisterTypes.h

@@ -104,7 +104,6 @@ void registerTypes(Serializer &s)
 	s.template registerType<TimesHeroLevelUpdater>(44);
 	s.template registerType<TimesStackLevelUpdater>(45);
 	s.template registerType<OwnerUpdater>(46);
-	s.template registerType<ArmyMovementUpdater>(47);
 	s.template registerType<ILimiter>(48);
 	s.template registerType<AnyOfLimiter>(49);
 	s.template registerType<NoneOfLimiter>(50);
@@ -294,6 +293,8 @@ void registerTypes(Serializer &s)
 	s.template registerType<SetResearchedSpells>(242);
 	s.template registerType<SaveLocalState>(243);
 	s.template registerType<LobbyDelete>(244);
+	s.template registerType<TimesHeroLevelDivideStackLevelUpdater>(245);
+	s.template registerType<DivideStackLevelUpdater>(246);
 }
 
 VCMI_LIB_NAMESPACE_END