Make bonus stacking configurable + fix duplicate propagation/inheritance (#433)

Addresses several related problems:
* Propagation / unpropagation of duplicate bonuses is inconsistent, causing bugs
* Duplicate bonuses never stack, which is not always intended behaviour (e.g. multiple copies of resource generating artifacts)
* Different bonuses always stack, which is not always intended behaviour (e.g. Angel + Archangel morale bonuses)

This is addressed as follows:
* Duplicate bonuses are never eliminated during propagation/inheritance.
* Unpropagation eliminates only a single copy of duplicated bonus
* Bonus receives a new field stacking that determines stacking behaviour:
* * empty string = no stacking with duplicates (default)
* * "ALWAYS" = stacks with duplicates & everything else
* * some other value = no stacking with bonuses with same stacking value
Also Morale/Luck window now hides non-stacking bonuses.

client/CMT.cpp

@@ -794,15 +794,24 @@ void processCommand(const std::string &message)
 	}
 	else if(cn == "bonuses")
 	{
+		bool jsonFormat = (message == "bonuses json");
+		auto format = [jsonFormat](const BonusList & b) -> std::string
+		{
+			if(jsonFormat)
+				return b.toJsonNode().toJson(true);
+			std::ostringstream ss;
+			ss << b;
+			return ss.str();
+		};
 		std::cout << "Bonuses of " << adventureInt->selection->getObjectName() << std::endl
-			<< adventureInt->selection->getBonusList() << std::endl;
+			<< format(adventureInt->selection->getBonusList()) << std::endl;
 
 		std::cout << "\nInherited bonuses:\n";
 		TCNodes parents;
 		adventureInt->selection->getParents(parents);
 		for(const CBonusSystemNode *parent : parents)
 		{
-			std::cout << "\nBonuses from " << typeid(*parent).name() << std::endl << parent->getBonusList() << std::endl;
+			std::cout << "\nBonuses from " << typeid(*parent).name() << std::endl << format(*parent->getAllBonuses(Selector::all, Selector::all)) << std::endl;
 		}
 	}
 	else if(cn == "not dialog")

config/artifacts.json

@@ -1549,7 +1549,8 @@
 				"subtype" : "resource.crystal",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 1,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 109,
@@ -1562,7 +1563,8 @@
 				"subtype" : "resource.gems",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 1,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 110,
@@ -1575,7 +1577,8 @@
 				"subtype" : "resource.mercury",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 1,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 111,
@@ -1588,7 +1591,8 @@
 				"subtype" : "resource.ore",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 1,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 112,
@@ -1601,7 +1605,8 @@
 				"subtype" : "resource.sulfur",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 1,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 113,
@@ -1614,7 +1619,8 @@
 				"subtype" : "resource.wood",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 1,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 114,
@@ -1627,7 +1633,8 @@
 				"subtype" : "resource.gold",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 1000,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 115,
@@ -1640,7 +1647,8 @@
 				"subtype" : "resource.gold",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 750,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 116,
@@ -1653,7 +1661,8 @@
 				"subtype" : "resource.gold",
 				"type" : "GENERATE_RESOURCE",
 				"val" : 500,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "BASE_NUMBER",
+				"stacking" : "ALWAYS"
 			}
 		],
 		"index" : 117,

config/creatures/castle.json

@@ -306,6 +306,10 @@
 				"type" : "HATE",
 				"subtype" : "creature.archDevil",
 				"val" : 50
+			},
+			"const_raises_morale" :
+			{
+				"stacking" : "Angels"
 			}
 		},
 		"upgrades": ["archangel"],
@@ -357,6 +361,10 @@
 				"type" : "HATE",
 				"subtype" : "creature.archDevil",
 				"val" : 50
+			},
+			"const_raises_morale" :
+			{
+				"stacking" : "Angels"
 			}
 		},
 		"graphics" :

config/creatures/inferno.json

@@ -361,7 +361,8 @@
 			{
 				"type" : "LUCK",
 				"effectRange" : "ONLY_ENEMY_ARMY",
-				"val" : -1
+				"val" : -1,
+				"stacking" : "Devils"
 			},
 			"blockRetaliation" :
 			{
@@ -412,7 +413,8 @@
 			{
 				"type" : "LUCK",
 				"effectRange" : "ONLY_ENEMY_ARMY",
-				"val" : -1
+				"val" : -1,
+				"stacking" : "Devils"
 			},
 			"blockRetaliation" :
 			{

config/creatures/necropolis.json

@@ -338,6 +338,10 @@
 			"dragon" :
 			{
 				"type" : "DRAGON_NATURE"
+			},
+			"const_lowers_morale" :
+			{
+				"stacking" : "Undead Dragons"
 			}
 		},
 		"upgrades": ["ghostDragon"],
@@ -365,6 +369,10 @@
 			{
 				"type" : "DRAGON_NATURE"
 			},
+			"const_lowers_morale" :
+			{
+				"stacking" : "Undead Dragons"
+			},
 			"age" :
 			{
 				"type" : "SPELL_AFTER_ATTACK",

config/schemas/bonus.json

@@ -111,6 +111,10 @@
 			"type":"string",
 			"description": "sourceType"
 		},
+		"stacking" : {
+			"type" : "string",
+			"description" : "stacking"
+		},
 		"subtype": {
 			"anyOf" : [
 				{ "type" : "string" },

lib/HeroBonus.cpp

@@ -139,7 +139,6 @@ TBonusListPtr CBonusProxy::get() const
 	{
 		//TODO: support limiters
 		data = target->getAllBonuses(selector, Selector::all);
-		data->eliminateDuplicates();
 		cachedLast = target->getTreeVersion();
 	}
 	return data;
@@ -246,6 +245,45 @@ void BonusList::changed()
 		CBonusSystemNode::treeHasChanged();
 }
 
+void BonusList::stackBonuses()
+{
+	boost::sort(bonuses, [](std::shared_ptr<Bonus> b1, std::shared_ptr<Bonus> b2) -> bool
+	{
+		if(b1 == b2)
+			return false;
+#define COMPARE_ATT(ATT) if(b1->ATT != b2->ATT) return b1->ATT < b2->ATT
+		COMPARE_ATT(stacking);
+		COMPARE_ATT(type);
+		COMPARE_ATT(subtype);
+		COMPARE_ATT(valType);
+#undef COMPARE_ATT
+		return b1->val > b2->val;
+	});
+	// remove non-stacking
+	size_t next = 1;
+	while(next < bonuses.size())
+	{
+		bool remove;
+		const std::shared_ptr<Bonus> last = bonuses[next-1];
+		const std::shared_ptr<Bonus> current = bonuses[next];
+
+		if(current->stacking.empty())
+			remove = current == last;
+		else if(current->stacking == "ALWAYS")
+			remove = false;
+		else
+			remove = current->stacking == last->stacking
+				&& current->type == last->type
+				&& current->subtype == last->subtype
+				&& current->valType == last->valType;
+
+		if(remove)
+			bonuses.erase(bonuses.begin() + next);
+		else
+			next++;
+	}
+}
+
 int BonusList::totalValue() const
 {
 	int base = 0;
@@ -257,7 +295,7 @@ int BonusList::totalValue() const
 	int indepMin = 0;
 	bool hasIndepMin = false;
 
-	for (auto& b : bonuses)
+	for(std::shared_ptr<Bonus> b : bonuses)
 	{
 		switch(b->valType)
 		{
@@ -375,14 +413,15 @@ int BonusList::valOfBonuses(const CSelector &select) const
 	BonusList ret;
 	CSelector limit = nullptr;
 	getBonuses(ret, select, limit);
-	ret.eliminateDuplicates();
 	return ret.totalValue();
 }
 
-void BonusList::eliminateDuplicates()
+JsonNode BonusList::toJsonNode() const
 {
-	sort( bonuses.begin(), bonuses.end() );
-	bonuses.erase( unique( bonuses.begin(), bonuses.end() ), bonuses.end() );
+	JsonNode node(JsonNode::JsonType::DATA_VECTOR);
+	for(std::shared_ptr<Bonus> b : bonuses)
+		node.Vector().push_back(b->toJsonNode());
+	return node;
 }
 
 void BonusList::push_back(std::shared_ptr<Bonus> x)
@@ -688,8 +727,8 @@ const TBonusListPtr CBonusSystemNode::getAllBonuses(const CSelector &selector, c
 
 			BonusList allBonuses;
 			getAllBonusesRec(allBonuses);
-			allBonuses.eliminateDuplicates();
 			limitBonuses(allBonuses, cachedBonuses);
+			cachedBonuses.stackBonuses();
 
 			cachedLast = treeChanged;
 		}
@@ -730,12 +769,10 @@ const TBonusListPtr CBonusSystemNode::getAllBonusesWithoutCaching(const CSelecto
 	// Get bonus results without caching enabled.
 	BonusList beforeLimiting, afterLimiting;
 	getAllBonusesRec(beforeLimiting);
-	beforeLimiting.eliminateDuplicates();
 
 	if(!root || root == this)
 	{
 		limitBonuses(beforeLimiting, afterLimiting);
-		afterLimiting.getBonuses(*ret, selector, limit);
 	}
 	else if(root)
 	{
@@ -747,15 +784,15 @@ const TBonusListPtr CBonusSystemNode::getAllBonusesWithoutCaching(const CSelecto
 		for(auto b : beforeLimiting)
 			rootBonuses.push_back(b);
 
-		rootBonuses.eliminateDuplicates();
 		root->limitBonuses(rootBonuses, limitedRootBonuses);
 
 		for(auto b : beforeLimiting)
 			if(vstd::contains(limitedRootBonuses, b))
 				afterLimiting.push_back(b);
 
-		afterLimiting.getBonuses(*ret, selector, limit);
 	}
+	afterLimiting.getBonuses(*ret, selector, limit);
+	ret->stackBonuses();
 	return ret;
 }
 
@@ -944,11 +981,6 @@ void CBonusSystemNode::unpropagateBonus(std::shared_ptr<Bonus> b)
 	if(b->propagator->shouldBeAttached(this))
 	{
 		bonuses -= b;
-		while(vstd::contains(bonuses, b))
-		{
-			logBonus->error("Bonus was duplicated (%s) at %s", b->Description(), nodeName());
-			bonuses -= b;
-		}
 		logBonus->trace("#$# %s #is no longer propagated to# %s",  b->Description(), nodeName());
 	}
 
@@ -1198,27 +1230,36 @@ std::string Bonus::Description() const
 	std::ostringstream str;
 
 	if(description.empty())
-		switch(source)
+	{
+		if(stacking.empty() || stacking == "ALWAYS")
 		{
-		case ARTIFACT:
-			str << VLC->arth->artifacts[sid]->Name();
-			break;
-		case SPELL_EFFECT:
-			str << SpellID(sid).toSpell()->name;
-			break;
-		case CREATURE_ABILITY:
-			str << VLC->creh->creatures[sid]->namePl;
-			break;
-		case SECONDARY_SKILL:
-			str << VLC->skillh->skillName(sid);
-			break;
-		default:
-			//todo: handle all possible sources
-			str << "Unknown";
-			break;
+			switch(source)
+			{
+			case ARTIFACT:
+				str << VLC->arth->artifacts[sid]->Name();
+				break;
+			case SPELL_EFFECT:
+				str << SpellID(sid).toSpell()->name;
+				break;
+			case CREATURE_ABILITY:
+				str << VLC->creh->creatures[sid]->namePl;
+				break;
+			case SECONDARY_SKILL:
+				str << VLC->skillh->skillName(sid);
+				break;
+			default:
+				//todo: handle all possible sources
+				str << "Unknown";
+				break;
+			}
 		}
+		else
+			str << stacking;
+	}
 	else
+	{
 		str << description;
+	}
 
 	if(val != 0)
 		str << " " << std::showpos << val;
@@ -1240,6 +1281,7 @@ JsonNode subtypeToJson(Bonus::BonusType type, int subtype)
 	case Bonus::MAXED_SPELL:
 	case Bonus::SPECIAL_PECULIAR_ENCHANT:
 		return JsonUtils::stringNode("spell." + (*VLC->spellh)[SpellID::ESpellID(subtype)]->identifier);
+	case Bonus::IMPROVED_NECROMANCY:
 	case Bonus::SPECIAL_UPGRADE:
 		return JsonUtils::stringNode("creature." + CreatureID::encode(subtype));
 	case Bonus::GENERATE_RESOURCE:
@@ -1256,24 +1298,35 @@ JsonNode additionalInfoToJson(Bonus::BonusType type, CAddInfo addInfo)
 	case Bonus::SPECIAL_UPGRADE:
 		return JsonUtils::stringNode("creature." + CreatureID::encode(addInfo[0]));
 	default:
-		if(addInfo.size() <= 1)
-		{
-			return JsonUtils::intNode(addInfo[0]);
-		}
-		else
-		{
-			JsonNode vecNode(JsonNode::JsonType::DATA_VECTOR);
-			for(si32 value : addInfo)
-				vecNode.Vector().push_back(JsonUtils::intNode(value));
-			return vecNode;
-		}
+		return addInfo.toJsonNode();
+	}
+}
+
+JsonNode durationToJson(ui16 duration)
+{
+	std::vector<std::string> durationNames;
+	for(ui16 durBit = 1; durBit; durBit = durBit << 1)
+	{
+		if(duration & durBit)
+			durationNames.push_back(vstd::findKey(bonusDurationMap, durBit));
+	}
+	if(durationNames.size() == 1)
+	{
+		return JsonUtils::stringNode(durationNames[0]);
+	}
+	else
+	{
+		JsonNode node(JsonNode::JsonType::DATA_VECTOR);
+		for(std::string dur : durationNames)
+			node.Vector().push_back(JsonUtils::stringNode(dur));
+		return node;
 	}
 }
 
 JsonNode Bonus::toJsonNode() const
 {
 	JsonNode root(JsonNode::JsonType::DATA_STRUCT);
-
+	// only add values that might reasonably be found in config files
 	root["type"].String() = vstd::findKey(bonusNameMap, type);
 	if(subtype != -1)
 		root["subtype"] = subtypeToJson(type, subtype);
@@ -1283,10 +1336,22 @@ JsonNode Bonus::toJsonNode() const
 		root["val"].Integer() = val;
 	if(valType != ADDITIVE_VALUE)
 		root["valueType"].String() = vstd::findKey(bonusValueMap, valType);
+	if(stacking != "")
+		root["stacking"].String() = stacking;
+	if(description != "")
+		root["description"].String() = description;
+	if(effectRange != NO_LIMIT)
+		root["effectRange"].String() = vstd::findKey(bonusLimitEffect, effectRange);
+	if(duration != PERMANENT)
+		root["duration"] = durationToJson(duration);
+	if(turnsRemain)
+		root["turns"].Integer() = turnsRemain;
 	if(limiter)
 		root["limiters"].Vector().push_back(limiter->toJsonNode());
 	if(updater)
 		root["updater"] = updater->toJsonNode();
+	if(propagator)
+		root["propagator"].String() = vstd::findKey(bonusPropagatorMap, propagator);
 	return root;
 }
 
@@ -1474,6 +1539,8 @@ DLL_LINKAGE std::ostream & operator<<(std::ostream &out, const Bonus &bonus)
 		out << "\taddInfo: " << bonus.additionalInfo.toString() << "\n";
 	printField(turnsRemain);
 	printField(valType);
+	if(!bonus.stacking.empty())
+		out << "\tstacking: \"" << bonus.stacking << "\"\n";
 	printField(effectRange);
 #undef printField
 

lib/HeroBonus.h

@@ -355,6 +355,7 @@ struct DLL_LINKAGE Bonus : public std::enable_shared_from_this<Bonus>
 	si32 val;
 	ui32 sid; //source id: id of object/artifact/spell
 	ValueType valType;
+	std::string stacking; // bonuses with the same stacking value don't stack (e.g. Angel/Archangel morale bonus)
 
 	CAddInfo additionalInfo;
 	LimitEffect effectRange; //if not NO_LIMIT, bonus will be omitted by default
@@ -389,6 +390,10 @@ struct DLL_LINKAGE Bonus : public std::enable_shared_from_this<Bonus>
 		}
 		h & turnsRemain;
 		h & valType;
+		if(version >= 784)
+		{
+			h & stacking;
+		}
 		h & effectRange;
 		h & limiter;
 		h & propagator;
@@ -506,6 +511,7 @@ public:
 	TInternalContainer::size_type operator-=(std::shared_ptr<Bonus> const &i);
 
 	// BonusList functions
+	void stackBonuses();
 	int totalValue() const;
 	void getBonuses(BonusList &out, const CSelector &selector, const CSelector &limit) const;
 	void getAllBonuses(BonusList &out) const;
@@ -517,7 +523,8 @@ public:
 	const std::shared_ptr<Bonus> getFirst(const CSelector &select) const;
 	int valOfBonuses(const CSelector &select) const;
 
-	void eliminateDuplicates();
+	// conversion / output
+	JsonNode toJsonNode() const;
 
 	// remove_if implementation for STL vector types
 	template <class Predicate>

lib/JsonNode.cpp

@@ -598,6 +598,8 @@ bool JsonUtils::parseBonus(const JsonNode &ability, Bonus *b)
 	if (!value->isNull())
 		b->valType = static_cast<Bonus::ValueType>(parseByMap(bonusValueMap, value, "value type "));
 
+	b->stacking = ability["stacking"].String();
+
 	resolveAddInfo(b->additionalInfo, ability);
 
 	b->turnsRemain = ability["turns"].Float();
@@ -742,29 +744,6 @@ Key reverseMapFirst(const Val & val, const std::map<Key, Val> & map)
 	return "";
 }
 
-void JsonUtils::unparseBonus( JsonNode &node, const std::shared_ptr<Bonus>& bonus )
-{
-	node["type"].String() = reverseMapFirst<std::string, Bonus::BonusType>(bonus->type, bonusNameMap);
-	node["subtype"].Float() = bonus->subtype;
-	node["val"].Float() = bonus->val;
-	node["valueType"].String() = reverseMapFirst<std::string, Bonus::ValueType>(bonus->valType, bonusValueMap);
-	node["additionalInfo"] = bonus->additionalInfo.toJsonNode();
-	node["turns"].Float() = bonus->turnsRemain;
-	node["sourceID"].Float() = bonus->source;
-	node["description"].String() = bonus->description;
-	node["effectRange"].String() = reverseMapFirst<std::string, Bonus::LimitEffect>(bonus->effectRange, bonusLimitEffect);
-	node["duration"].String() = reverseMapFirst<std::string, ui16>(bonus->duration, bonusDurationMap);
-	node["source"].String() = reverseMapFirst<std::string, Bonus::BonusSource>(bonus->source, bonusSourceMap);
-	if(bonus->limiter)
-	{
-		node["limiter"].String() = reverseMapFirst<std::string, TLimiterPtr>(bonus->limiter, bonusLimiterMap);
-	}
-	if(bonus->propagator)
-	{
-		node["propagator"].String() = reverseMapFirst<std::string, TPropagatorPtr>(bonus->propagator, bonusPropagatorMap);
-	}
-}
-
 void minimizeNode(JsonNode & node, const JsonNode & schema)
 {
 	if (schema["type"].String() == "object")

lib/JsonNode.h

@@ -168,7 +168,6 @@ namespace JsonUtils
 	DLL_LINKAGE std::shared_ptr<Bonus> parseBonus(const JsonVector &ability_vec);
 	DLL_LINKAGE std::shared_ptr<Bonus> parseBonus(const JsonNode &ability);
 	DLL_LINKAGE bool parseBonus(const JsonNode &ability, Bonus *placement);
-	DLL_LINKAGE void unparseBonus (JsonNode &node, const std::shared_ptr<Bonus>& bonus);
 	DLL_LINKAGE void resolveIdentifier(si32 &var, const JsonNode &node, std::string name);
 	DLL_LINKAGE void resolveIdentifier(const JsonNode &node, si32 &var);
 	DLL_LINKAGE void resolveAddInfo(CAddInfo & var, const JsonNode & node);

lib/serializer/CSerializer.h

@@ -12,7 +12,7 @@
 #include "../ConstTransitivePtr.h"
 #include "../GameConstants.h"
 
-const ui32 SERIALIZATION_VERSION = 783;
+const ui32 SERIALIZATION_VERSION = 784;
 const ui32 MINIMAL_SERIALIZATION_VERSION = 753;
 const std::string SAVEGAME_MAGIC = "VCMISVG";
 

test/mock/mock_BonusBearer.cpp

@@ -29,7 +29,7 @@ const TBonusListPtr BonusBearerMock::getAllBonuses(const CSelector & selector, c
 {
 	if(cachedLast != treeVersion)
 	{
-		bonuses.eliminateDuplicates();
+		bonuses.stackBonuses();
 		cachedLast = treeVersion;
 	}