Initial version of new bonus caching system

include/vcmi/FactionMember.h

@@ -44,10 +44,6 @@ public:
 	 Returns defence of creature or hero.
 	*/
 	virtual int getDefense(bool ranged) const;
-	/**
-	 Returns primskill of creature or hero.
-	*/
-	int getPrimSkillLevel(PrimarySkill id) const;
 	/**
 	 Returns morale of creature or hero. Taking absolute bonuses into account.
 	 For now, uses range from EGameSettings

lib/BasicTypes.cpp

@@ -69,14 +69,6 @@ int AFactionMember::getMaxDamage(bool ranged) const
 	return getBonusBearer()->valOfBonuses(selector, cachingStr);
 }
 
-int AFactionMember::getPrimSkillLevel(PrimarySkill id) const
-{
-	auto allSkills = getBonusBearer()->getBonusesOfType(BonusType::PRIMARY_SKILL);
-	int ret = allSkills->valOfBonuses(Selector::subtype()(BonusSubtypeID(id)));
-	int minSkillValue = VLC->engineSettings()->getVectorValue(EGameSettings::HEROES_MINIMAL_PRIMARY_SKILLS, id.getNum());
-	return std::max(ret, minSkillValue); //otherwise, some artifacts may cause negative skill value effect, sp=0 works in old saves
-}
-
 int AFactionMember::moraleValAndBonusList(TConstBonusListPtr & bonusList) const
 {
 	int32_t maxGoodMorale = VLC->engineSettings()->getVector(EGameSettings::COMBAT_GOOD_MORALE_DICE).size();

lib/CMakeLists.txt

@@ -65,6 +65,7 @@ set(lib_MAIN_SRCS
 	battle/Unit.cpp
 
 	bonuses/Bonus.cpp
+	bonuses/BonusCache.cpp
 	bonuses/BonusEnum.cpp
 	bonuses/BonusList.cpp
 	bonuses/BonusParams.cpp
@@ -435,6 +436,7 @@ set(lib_MAIN_HEADERS
 	battle/Unit.h
 
 	bonuses/Bonus.h
+	bonuses/BonusCache.h
 	bonuses/BonusEnum.h
 	bonuses/BonusList.h
 	bonuses/BonusParams.h

lib/battle/CUnitState.cpp

@@ -26,7 +26,7 @@ namespace battle
 CAmmo::CAmmo(const battle::Unit * Owner, CSelector totalSelector):
 	used(0),
 	owner(Owner),
-	totalProxy(Owner, std::move(totalSelector))
+	totalProxy(Owner, totalSelector)
 {
 	reset();
 }
@@ -34,7 +34,6 @@ CAmmo::CAmmo(const battle::Unit * Owner, CSelector totalSelector):
 CAmmo & CAmmo::operator= (const CAmmo & other)
 {
 	used = other.used;
-	totalProxy = other.totalProxy;
 	return *this;
 }
 
@@ -60,7 +59,7 @@ void CAmmo::reset()
 
 int32_t CAmmo::total() const
 {
-	return totalProxy->totalValue();
+	return totalProxy.getValue();
 }
 
 void CAmmo::use(int32_t amount)
@@ -89,13 +88,6 @@ CShots::CShots(const battle::Unit * Owner)
 {
 }
 
-CShots & CShots::operator=(const CShots & other)
-{
-	CAmmo::operator=(other);
-	shooter = other.shooter;
-	return *this;
-}
-
 bool CShots::isLimited() const
 {
 	return !shooter.getHasBonus() || !env->unitHasAmmoCart(owner);
@@ -140,7 +132,7 @@ int32_t CRetaliations::total() const
 		return 0;
 
 	//after dispel bonus should remain during current round
-	int32_t val = 1 + totalProxy->totalValue();
+	int32_t val = 1 + totalProxy.getValue();
 	vstd::amax(totalCache, val);
 	return totalCache;
 }
@@ -341,12 +333,7 @@ CUnitState::CUnitState():
 	counterAttacks(this),
 	health(this),
 	shots(this),
-	totalAttacks(this, Selector::type()(BonusType::ADDITIONAL_ATTACK), 1),
+	bonusCache(this, generateBonusSelectors()),
-	minDamage(this, Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageBoth).Or(Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageMin)), 0),
-	maxDamage(this, Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageBoth).Or(Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageMax)), 0),
-	attack(this, Selector::typeSubtype(BonusType::PRIMARY_SKILL, BonusSubtypeID(PrimarySkill::ATTACK)), 0),
-	defence(this, Selector::typeSubtype(BonusType::PRIMARY_SKILL, BonusSubtypeID(PrimarySkill::DEFENSE)), 0),
-	inFrenzy(this, Selector::type()(BonusType::IN_FRENZY)),
 	cloneLifetimeMarker(this, Selector::type()(BonusType::NONE).And(Selector::source(BonusSource::SPELL_EFFECT, BonusSourceID(SpellID(SpellID::CLONE)))), "CUnitState::cloneLifetimeMarker"),
 	cloneID(-1)
 {
@@ -374,12 +361,7 @@ CUnitState & CUnitState::operator=(const CUnitState & other)
 	counterAttacks = other.counterAttacks;
 	health = other.health;
 	shots = other.shots;
-	totalAttacks = other.totalAttacks;
+//	bonusCache = other.bonusCache;
-	minDamage = other.minDamage;
-	maxDamage = other.maxDamage;
-	attack = other.attack;
-	defence = other.defence;
-	inFrenzy = other.inFrenzy;
 	cloneLifetimeMarker = other.cloneLifetimeMarker;
 	cloneID = other.cloneID;
 	position = other.position;
@@ -695,45 +677,62 @@ BattlePhases::Type CUnitState::battleQueuePhase(int turn) const
 
 int CUnitState::getTotalAttacks(bool ranged) const
 {
-	return ranged ? totalAttacks.getRangedValue() : totalAttacks.getMeleeValue();
+	return 1 + (ranged ?
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::TOTAL_ATTACKS_RANGED):
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::TOTAL_ATTACKS_MELEE));
 }
 
 int CUnitState::getMinDamage(bool ranged) const
 {
-	return ranged ? minDamage.getRangedValue() : minDamage.getMeleeValue();
+	return ranged ?
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::MIN_DAMAGE_RANGED):
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::MIN_DAMAGE_MELEE);
+
 }
 
 int CUnitState::getMaxDamage(bool ranged) const
 {
-	return ranged ? maxDamage.getRangedValue() : maxDamage.getMeleeValue();
+	return ranged ?
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::MAX_DAMAGE_RANGED):
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::MAX_DAMAGE_MELEE);
 }
 
 int CUnitState::getAttack(bool ranged) const
 {
-	int ret = ranged ? attack.getRangedValue() : attack.getMeleeValue();
+	int attack = ranged ?
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::ATTACK_RANGED):
+		bonusCache.cache.getBonusValue(UnitBonusValuesProxy::ATTACK_MELEE);
 
-	if(!inFrenzy->empty())
+	int frenzy = bonusCache.cache.getBonusValue(UnitBonusValuesProxy::IN_FRENZY);
+	if(frenzy != 0)
 	{
-		double frenzyPower = static_cast<double>(inFrenzy->totalValue()) / 100;
+		int defence = ranged ?
-		frenzyPower *= static_cast<double>(ranged ? defence.getRangedValue() : defence.getMeleeValue());
+			bonusCache.cache.getBonusValue(UnitBonusValuesProxy::DEFENCE_RANGED):
-		ret += static_cast<int>(frenzyPower);
+			bonusCache.cache.getBonusValue(UnitBonusValuesProxy::DEFENCE_MELEE);
+
+		int frenzyBonus = frenzy * defence / 100;
+		attack += frenzyBonus;
 	}
 
-	vstd::amax(ret, 0);
+	vstd::amax(attack, 0);
-	return ret;
+	return attack;
 }
 
 int CUnitState::getDefense(bool ranged) const
 {
-	if(!inFrenzy->empty())
+	int frenzy = bonusCache.cache.getBonusValue(UnitBonusValuesProxy::IN_FRENZY);
+
+	if(frenzy != 0)
 	{
 		return 0;
 	}
 	else
 	{
-		int ret = ranged ? defence.getRangedValue() : defence.getMeleeValue();
+		int defence = ranged ?
-		vstd::amax(ret, 0);
+						  bonusCache.cache.getBonusValue(UnitBonusValuesProxy::DEFENCE_RANGED):
-		return ret;
+						  bonusCache.cache.getBonusValue(UnitBonusValuesProxy::DEFENCE_MELEE);
+		vstd::amax(defence, 0);
+		return defence;
 	}
 }
 
@@ -911,6 +910,33 @@ void CUnitState::onRemoved()
 	ghost = true;
 }
 
+const UnitBonusValuesProxy::SelectorsArray * CUnitState::generateBonusSelectors()
+{
+	static const CSelector additionalAttack = Selector::type()(BonusType::ADDITIONAL_ATTACK);
+	static const CSelector selectorMelee = Selector::effectRange()(BonusLimitEffect::NO_LIMIT).Or(Selector::effectRange()(BonusLimitEffect::ONLY_MELEE_FIGHT));
+	static const CSelector selectorRanged = Selector::effectRange()(BonusLimitEffect::NO_LIMIT).Or(Selector::effectRange()(BonusLimitEffect::ONLY_DISTANCE_FIGHT));
+	static const CSelector minDamage = Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageBoth).Or(Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageMin));
+	static const CSelector maxDamage = Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageBoth).Or(Selector::typeSubtype(BonusType::CREATURE_DAMAGE, BonusCustomSubtype::creatureDamageMin));
+	static const CSelector attack = Selector::typeSubtype(BonusType::PRIMARY_SKILL, BonusSubtypeID(PrimarySkill::ATTACK));
+	static const CSelector defence = Selector::typeSubtype(BonusType::PRIMARY_SKILL, BonusSubtypeID(PrimarySkill::ATTACK));
+
+	static const UnitBonusValuesProxy::SelectorsArray selectors = {
+		additionalAttack.And(selectorMelee), //TOTAL_ATTACKS_MELEE,
+		additionalAttack.And(selectorRanged), //TOTAL_ATTACKS_RANGED,
+		minDamage.And(selectorMelee), //MIN_DAMAGE_MELEE,
+		minDamage.And(selectorRanged), //MIN_DAMAGE_RANGED,
+		minDamage.And(selectorMelee), //MAX_DAMAGE_MELEE,
+		maxDamage.And(selectorRanged), //MAX_DAMAGE_RANGED,
+		attack.And(selectorRanged),//ATTACK_MELEE,
+		attack.And(selectorRanged),//ATTACK_RANGED,
+		defence.And(selectorRanged),//DEFENCE_MELEE,
+		defence.And(selectorRanged),//DEFENCE_RANGED,
+		Selector::type()(BonusType::IN_FRENZY),//IN_FRENZY,
+	};
+
+	return &selectors;
+}
+
 CUnitStateDetached::CUnitStateDetached(const IUnitInfo * unit_, const IBonusBearer * bonus_):
 	unit(unit_),
 	bonus(bonus_)

lib/battle/CUnitState.h

@@ -12,6 +12,7 @@
 
 #include "Unit.h"
 #include "../bonuses/CBonusProxy.h"
+#include "../bonuses/BonusCache.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -32,10 +33,6 @@ class DLL_LINKAGE CAmmo
 public:
 	explicit CAmmo(const battle::Unit * Owner, CSelector totalSelector);
 
-	//only copy construction is allowed for acquire(), serializeJson should be used for any other "assignment"
-	CAmmo(const CAmmo & other) = default;
-	CAmmo(CAmmo && other) = delete;
-
 	CAmmo & operator=(const CAmmo & other);
 	CAmmo & operator=(CAmmo && other) = delete;
 
@@ -50,15 +47,14 @@ public:
 protected:
 	int32_t used;
 	const battle::Unit * owner;
-	CBonusProxy totalProxy;
+	BonusValueCache totalProxy;
 };
 
 class DLL_LINKAGE CShots : public CAmmo
 {
 public:
 	explicit CShots(const battle::Unit * Owner);
-	CShots(const CShots & other) = default;
+
-	CShots & operator=(const CShots & other);
 	bool isLimited() const override;
 	int32_t total() const override;
 
@@ -73,16 +69,13 @@ class DLL_LINKAGE CCasts : public CAmmo
 {
 public:
 	explicit CCasts(const battle::Unit * Owner);
-	CCasts(const CCasts & other) = default;
-	CCasts & operator=(const CCasts & other) = default;
 };
 
 class DLL_LINKAGE CRetaliations : public CAmmo
 {
 public:
 	explicit CRetaliations(const battle::Unit * Owner);
-	CRetaliations(const CRetaliations & other) = default;
+
-	CRetaliations & operator=(const CRetaliations & other) = default;
 	bool isLimited() const override;
 	int32_t total() const override;
 	void reset() override;
@@ -132,6 +125,41 @@ private:
 	int32_t resurrected;
 };
 
+class UnitBonusValuesProxy
+{
+public:
+	enum ECacheKeys : uint8_t
+	{
+		TOTAL_ATTACKS_MELEE,
+		TOTAL_ATTACKS_RANGED,
+
+		MIN_DAMAGE_MELEE,
+		MIN_DAMAGE_RANGED,
+		MAX_DAMAGE_MELEE,
+		MAX_DAMAGE_RANGED,
+
+		ATTACK_MELEE,
+		ATTACK_RANGED,
+
+		DEFENCE_MELEE,
+		DEFENCE_RANGED,
+
+		IN_FRENZY,
+
+		TOTAL_KEYS,
+	};
+
+	static constexpr size_t KEYS_COUNT = static_cast<size_t>(ECacheKeys::TOTAL_KEYS);
+
+	BonusValuesArrayCache<ECacheKeys, KEYS_COUNT> cache;
+
+	using SelectorsArray = BonusValuesArrayCache<ECacheKeys, KEYS_COUNT>::SelectorsArray;
+
+	UnitBonusValuesProxy(const IBonusBearer * Target, const SelectorsArray * selectors):
+		cache(Target, selectors)
+	{}
+};
+
 class DLL_LINKAGE CUnitState : public Unit
 {
 public:
@@ -154,11 +182,6 @@ public:
 	CHealth health;
 	CShots shots;
 
-	CTotalsProxy totalAttacks;
-
-	CTotalsProxy minDamage;
-	CTotalsProxy maxDamage;
-
 	///id of alive clone of this stack clone if any
 	si32 cloneID;
 
@@ -266,12 +289,11 @@ public:
 	void onRemoved();
 
 private:
+	static const UnitBonusValuesProxy::SelectorsArray * generateBonusSelectors();
+
 	const IUnitEnvironment * env;
 
-	CTotalsProxy attack;
+	UnitBonusValuesProxy bonusCache;
-	CTotalsProxy defence;
-	CBonusProxy inFrenzy;
-
 	CCheckProxy cloneLifetimeMarker;
 
 	void reset();
@@ -282,13 +304,12 @@ class DLL_LINKAGE CUnitStateDetached : public CUnitState
 public:
 	explicit CUnitStateDetached(const IUnitInfo * unit_, const IBonusBearer * bonus_);
 
-	TConstBonusListPtr getAllBonuses(const CSelector & selector, const CSelector & limit,
+	CUnitStateDetached & operator= (const CUnitState & other);
-		const std::string & cachingStr = "") const override;
+
+	TConstBonusListPtr getAllBonuses(const CSelector & selector, const CSelector & limit, const std::string & cachingStr = "") const override;
 
 	int64_t getTreeVersion() const override;
 
-	CUnitStateDetached & operator= (const CUnitState & other);
-
 	uint32_t unitId() const override;
 	BattleSide unitSide() const override;
 
@@ -297,7 +318,6 @@ public:
 
 	SlotID unitSlot() const override;
 
-
 	int32_t unitBaseAmount() const override;
 
 	void spendMana(ServerCallback * server, const int spellCost) const override;

lib/bonuses/BonusCache.cpp

@@ -0,0 +1,81 @@
+/*
+ * BonusCache.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 "BonusCache.h"
+#include "IBonusBearer.h"
+
+#include "BonusSelector.h"
+#include "BonusList.h"
+
+#include "../VCMI_Lib.h"
+#include "../IGameSettings.h"
+
+int BonusCacheBase::getBonusValueImpl(BonusCacheEntry & currentValue, const CSelector & selector) const
+{
+	if (target->getTreeVersion() == currentValue.version)
+	{
+		return currentValue.value;
+	}
+	else
+	{
+		// NOTE: following code theoretically can fail if bonus tree was changed by another thread between two following lines
+		// However, this situation should not be possible - gamestate modification should only happen in single-treaded mode with locked gamestate mutex
+		int newValue = target->valOfBonuses(selector);
+		currentValue.value = newValue;
+		currentValue.version = target->getTreeVersion();
+
+		return newValue;
+	}
+}
+
+BonusValueCache::BonusValueCache(const IBonusBearer * target, const CSelector selector)
+	:BonusCacheBase(target),selector(selector)
+{}
+
+int BonusValueCache::getValue() const
+{
+	return getBonusValueImpl(value, selector);
+}
+
+PrimarySkillsCache::PrimarySkillsCache(const IBonusBearer * target)
+	:target(target)
+{}
+
+void PrimarySkillsCache::update() const
+{
+	static const CSelector primarySkillsSelector = Selector::type()(BonusType::PRIMARY_SKILL);
+	static const CSelector attackSelector = Selector::subtype()(PrimarySkill::ATTACK);
+	static const CSelector defenceSelector = Selector::subtype()(PrimarySkill::DEFENSE);
+	static const CSelector spellPowerSelector = Selector::subtype()(PrimarySkill::SPELL_POWER);
+	static const CSelector knowledgeSelector = Selector::subtype()(PrimarySkill::KNOWLEDGE);
+
+	std::array<int, 4> minValues = {
+		VLC->engineSettings()->getVectorValue(EGameSettings::HEROES_MINIMAL_PRIMARY_SKILLS, PrimarySkill::ATTACK),
+		VLC->engineSettings()->getVectorValue(EGameSettings::HEROES_MINIMAL_PRIMARY_SKILLS, PrimarySkill::DEFENSE),
+		VLC->engineSettings()->getVectorValue(EGameSettings::HEROES_MINIMAL_PRIMARY_SKILLS, PrimarySkill::SPELL_POWER),
+		VLC->engineSettings()->getVectorValue(EGameSettings::HEROES_MINIMAL_PRIMARY_SKILLS, PrimarySkill::KNOWLEDGE)
+	};
+
+	auto list = target->getBonuses(primarySkillsSelector);
+	skills[PrimarySkill::ATTACK] = std::max(minValues[PrimarySkill::ATTACK], list->valOfBonuses(attackSelector));
+	skills[PrimarySkill::DEFENSE] = std::max(minValues[PrimarySkill::DEFENSE], list->valOfBonuses(defenceSelector));
+	skills[PrimarySkill::SPELL_POWER] = std::max(minValues[PrimarySkill::SPELL_POWER], list->valOfBonuses(spellPowerSelector));
+	skills[PrimarySkill::KNOWLEDGE] = std::max(minValues[PrimarySkill::KNOWLEDGE], list->valOfBonuses(knowledgeSelector));
+
+	version = target->getTreeVersion();
+}
+
+const std::array<std::atomic<int32_t>, 4> & PrimarySkillsCache::getSkills() const
+{
+	if (target->getTreeVersion() != version)
+		update();
+	return skills;
+}

lib/bonuses/BonusCache.h

@@ -0,0 +1,81 @@
+/*
+ * BonusCache.h, 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
+ *
+ */
+
+#pragma once
+
+#include "BonusSelector.h"
+
+/// Internal base class with no own cache
+class BonusCacheBase
+{
+	const IBonusBearer * target;
+
+protected:
+	explicit BonusCacheBase(const IBonusBearer * target):
+		target(target)
+	{}
+
+	struct BonusCacheEntry
+	{
+		std::atomic<int64_t> version = 0;
+		std::atomic<int64_t> value = 0;
+	};
+
+	int getBonusValueImpl(BonusCacheEntry & currentValue, const CSelector & selector) const;
+};
+
+/// Cache that tracks a single query to bonus system
+class BonusValueCache : public BonusCacheBase
+{
+	CSelector selector;
+	mutable BonusCacheEntry value;
+public:
+	BonusValueCache(const IBonusBearer * target, const CSelector selector);
+	int getValue() const;
+};
+
+/// Cache that can track a list of queries to bonus system
+template<typename EnumType, size_t SIZE>
+class BonusValuesArrayCache : public BonusCacheBase
+{
+public:
+	using SelectorsArray = std::array<const CSelector, SIZE>;
+
+	BonusValuesArrayCache(const IBonusBearer * target, const SelectorsArray * selectors)
+		: BonusCacheBase(target)
+		, selectors(selectors)
+	{}
+
+	int getBonusValue(EnumType which) const
+	{
+		auto index = static_cast<size_t>(which);
+		return getBonusValueImpl(cache[index], (*selectors)[index]);
+	}
+
+private:
+	using CacheArray = std::array<BonusCacheEntry, SIZE>;
+
+	const SelectorsArray * selectors;
+	mutable CacheArray cache;
+};
+
+/// Cache that tracks values of primary skill values in bonus system
+class PrimarySkillsCache
+{
+	const IBonusBearer * target;
+	mutable std::atomic<int64_t> version = 0;
+	mutable std::array<std::atomic<int32_t>, 4> skills;
+
+	void update() const;
+public:
+	PrimarySkillsCache(const IBonusBearer * target);
+
+	const std::array<std::atomic<int32_t>, 4> & getSkills() const;
+};

lib/bonuses/CBonusProxy.cpp

@@ -9,179 +9,11 @@
  */
 
 #include "StdInc.h"
-#include "BonusList.h"
 #include "CBonusProxy.h"
 #include "IBonusBearer.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-///CBonusProxy
-CBonusProxy::CBonusProxy(const IBonusBearer * Target, CSelector Selector):
-	bonusListCachedLast(0),
-	target(Target),
-	selector(std::move(Selector)),
-	currentBonusListIndex(0)
-{
-}
-
-CBonusProxy::CBonusProxy(const CBonusProxy & other):
-	bonusListCachedLast(other.bonusListCachedLast),
-	target(other.target),
-	selector(other.selector),
-	currentBonusListIndex(other.currentBonusListIndex)
-{
-	bonusList[currentBonusListIndex] = other.bonusList[currentBonusListIndex];
-}
-
-CBonusProxy::CBonusProxy(CBonusProxy && other) noexcept:
-	bonusListCachedLast(0),
-	target(other.target),
-	currentBonusListIndex(0)
-{
-	std::swap(bonusListCachedLast, other.bonusListCachedLast);
-	std::swap(selector, other.selector);
-	std::swap(bonusList, other.bonusList);
-	std::swap(currentBonusListIndex, other.currentBonusListIndex);
-}
-
-CBonusProxy & CBonusProxy::operator=(const CBonusProxy & other)
-{
-	boost::lock_guard<boost::mutex> lock(swapGuard);
-
-	selector = other.selector;
-	swapBonusList(other.bonusList[other.currentBonusListIndex]);
-	bonusListCachedLast = other.bonusListCachedLast;
-
-	return *this;
-}
-
-CBonusProxy & CBonusProxy::operator=(CBonusProxy && other) noexcept
-{
-	std::swap(bonusListCachedLast, other.bonusListCachedLast);
-	std::swap(selector, other.selector);
-	std::swap(bonusList, other.bonusList);
-	std::swap(currentBonusListIndex, other.currentBonusListIndex);
-
-	return *this;
-}
-
-void CBonusProxy::swapBonusList(TConstBonusListPtr other) const
-{
-	// The idea here is to avoid changing active bonusList while it can be read by a different thread.
-	// Because such use of shared ptr is not thread safe
-	// So to avoid this we change the second offline instance and swap active index
-	auto newCurrent = 1 - currentBonusListIndex;
-	bonusList[newCurrent] = std::move(other);
-	currentBonusListIndex = newCurrent;
-}
-
-TConstBonusListPtr CBonusProxy::getBonusList() const
-{
-	auto needUpdateBonusList = [&]() -> bool
-	{
-		return target->getTreeVersion() != bonusListCachedLast || !bonusList[currentBonusListIndex];
-	};
-
-	// avoid locking if everything is up-to-date
-	if(needUpdateBonusList())
-	{
-		boost::lock_guard<boost::mutex>lock(swapGuard);
-
-		if(needUpdateBonusList())
-		{
-			//TODO: support limiters
-			swapBonusList(target->getAllBonuses(selector, Selector::all));
-			bonusListCachedLast = target->getTreeVersion();
-		}
-	}
-
-	return bonusList[currentBonusListIndex];
-}
-
-const BonusList * CBonusProxy::operator->() const
-{
-	return getBonusList().get();
-}
-
-CTotalsProxy::CTotalsProxy(const IBonusBearer * Target, CSelector Selector, int InitialValue):
-	CBonusProxy(Target, std::move(Selector)),
-	initialValue(InitialValue),
-	meleeCachedLast(0),
-	meleeValue(0),
-	rangedCachedLast(0),
-	rangedValue(0)
-{
-}
-
-CTotalsProxy::CTotalsProxy(const CTotalsProxy & other)
-	: CBonusProxy(other),
-	initialValue(other.initialValue),
-	meleeCachedLast(other.meleeCachedLast),
-	meleeValue(other.meleeValue),
-	rangedCachedLast(other.rangedCachedLast),
-	rangedValue(other.rangedValue)
-{
-}
-
-int CTotalsProxy::getValue() const
-{
-	const auto treeVersion = target->getTreeVersion();
-
-	if(treeVersion != valueCachedLast)
-	{
-		auto bonuses = getBonusList();
-
-		value = initialValue + bonuses->totalValue();
-		valueCachedLast = treeVersion;
-	}
-	return value;
-}
-
-int CTotalsProxy::getValueAndList(TConstBonusListPtr & outBonusList) const
-{
-	const auto treeVersion = target->getTreeVersion();
-	outBonusList = getBonusList();
-
-	if(treeVersion != valueCachedLast)
-	{
-		value = initialValue + outBonusList->totalValue();
-		valueCachedLast = treeVersion;
-	}
-	return value;
-}
-
-int CTotalsProxy::getMeleeValue() const
-{
-	static const auto limit = Selector::effectRange()(BonusLimitEffect::NO_LIMIT).Or(Selector::effectRange()(BonusLimitEffect::ONLY_MELEE_FIGHT));
-
-	const auto treeVersion = target->getTreeVersion();
-
-	if(treeVersion != meleeCachedLast)
-	{
-		auto bonuses = target->getBonuses(selector, limit);
-		meleeValue = initialValue + bonuses->totalValue();
-		meleeCachedLast = treeVersion;
-	}
-
-	return meleeValue;
-}
-
-int CTotalsProxy::getRangedValue() const
-{
-	static const auto limit = Selector::effectRange()(BonusLimitEffect::NO_LIMIT).Or(Selector::effectRange()(BonusLimitEffect::ONLY_DISTANCE_FIGHT));
-
-	const auto treeVersion = target->getTreeVersion();
-
-	if(treeVersion != rangedCachedLast)
-	{
-		auto bonuses = target->getBonuses(selector, limit);
-		rangedValue = initialValue + bonuses->totalValue();
-		rangedCachedLast = treeVersion;
-	}
-
-	return rangedValue;
-}
-
 ///CCheckProxy
 CCheckProxy::CCheckProxy(const IBonusBearer * Target, BonusType bonusType):
 	target(Target),

lib/bonuses/CBonusProxy.h

@@ -10,66 +10,10 @@
 
 #pragma once
 
-#include "Bonus.h"
 #include "BonusSelector.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-class DLL_LINKAGE CBonusProxy
-{
-public:
-	CBonusProxy(const IBonusBearer * Target, CSelector Selector);
-	CBonusProxy(const CBonusProxy & other);
-	CBonusProxy(CBonusProxy && other) noexcept;
-
-	CBonusProxy & operator=(CBonusProxy && other) noexcept;
-	CBonusProxy & operator=(const CBonusProxy & other);
-	const BonusList * operator->() const;
-	TConstBonusListPtr getBonusList() const;
-
-protected:
-	CSelector selector;
-	const IBonusBearer * target;
-	mutable int64_t bonusListCachedLast;
-	mutable TConstBonusListPtr bonusList[2];
-	mutable int currentBonusListIndex;
-	mutable boost::mutex swapGuard;
-	void swapBonusList(TConstBonusListPtr other) const;
-};
-
-class DLL_LINKAGE CTotalsProxy : public CBonusProxy
-{
-public:
-	CTotalsProxy(const IBonusBearer * Target, CSelector Selector, int InitialValue);
-	CTotalsProxy(const CTotalsProxy & other);
-	CTotalsProxy(CTotalsProxy && other) = delete;
-
-	CTotalsProxy & operator=(const CTotalsProxy & other) = default;
-	CTotalsProxy & operator=(CTotalsProxy && other) = delete;
-
-	int getMeleeValue() const;
-	int getRangedValue() const;
-	int getValue() const;
-	/**
-	Returns total value of all selected bonuses and sets bonusList as a pointer to the list of selected bonuses
-	@param bonusList is the out list of all selected bonuses
-	@return total value of all selected bonuses and 0 otherwise
-	*/
-	int getValueAndList(TConstBonusListPtr & bonusList) const;
-
-private:
-	int initialValue;
-
-	mutable int64_t valueCachedLast = 0;
-	mutable int value = 0;
-
-	mutable int64_t meleeCachedLast;
-	mutable int meleeValue;
-
-	mutable int64_t rangedCachedLast;
-	mutable int rangedValue;
-};
-
 class DLL_LINKAGE CCheckProxy
 {
 public:

lib/mapObjects/CGHeroInstance.cpp

@@ -293,6 +293,7 @@ CGHeroInstance::CGHeroInstance(IGameCallback * cb)
 	level(1),
 	exp(UNINITIALIZED_EXPERIENCE),
 	gender(EHeroGender::DEFAULT),
+	primarySkills(this),
 	lowestCreatureSpeed(0)
 {
 	setNodeType(HERO);
@@ -704,40 +705,20 @@ void CGHeroInstance::setPropertyDer(ObjProperty what, ObjPropertyID identifier)
 		setStackCount(SlotID(0), identifier.getNum());
 }
 
-std::array<int, 4> CGHeroInstance::getPrimarySkills() const
+int CGHeroInstance::getPrimSkillLevel(PrimarySkill id) const
 {
-	std::array<int, 4> result;
+	return primarySkills.getSkills()[id];
-
-	auto allSkills = getBonusBearer()->getBonusesOfType(BonusType::PRIMARY_SKILL);
-	for (auto skill : PrimarySkill::ALL_SKILLS())
-	{
-		int ret = allSkills->valOfBonuses(Selector::subtype()(BonusSubtypeID(skill)));
-		int minSkillValue = VLC->engineSettings()->getVectorValue(EGameSettings::HEROES_MINIMAL_PRIMARY_SKILLS, skill.getNum());
-		result[skill] = std::max(ret, minSkillValue); //otherwise, some artifacts may cause negative skill value effect
-	}
-
-	return result;
 }
 
 double CGHeroInstance::getFightingStrength() const
 {
-	const auto & primarySkills = getPrimarySkills();
+	const auto & skillValues = primarySkills.getSkills();
-	return getFightingStrengthImpl(primarySkills);
+	return sqrt((1.0 + 0.05*skillValues[PrimarySkill::ATTACK]) * (1.0 + 0.05*skillValues[PrimarySkill::DEFENSE]));
-}
-
-double CGHeroInstance::getFightingStrengthImpl(const std::array<int, 4> & primarySkills) const
-{
-	return sqrt((1.0 + 0.05*primarySkills[PrimarySkill::ATTACK]) * (1.0 + 0.05*primarySkills[PrimarySkill::DEFENSE]));
 }
 
 double CGHeroInstance::getMagicStrength() const
 {
-	const auto & primarySkills = getPrimarySkills();
+	const auto & skillValues = primarySkills.getSkills();
-	return getMagicStrengthImpl(primarySkills);
-}
-
-double CGHeroInstance::getMagicStrengthImpl(const std::array<int, 4> & primarySkills) const
-{
 	if (!hasSpellbook())
 		return 1;
 	bool atLeastOneCombatSpell = false;
@@ -751,13 +732,12 @@ double CGHeroInstance::getMagicStrengthImpl(const std::array<int, 4> & primarySk
 	}
 	if (!atLeastOneCombatSpell)
 		return 1;
-	return sqrt((1.0 + 0.05*primarySkills[PrimarySkill::KNOWLEDGE] * mana / manaLimit()) * (1.0 + 0.05*primarySkills[PrimarySkill::SPELL_POWER] * mana / manaLimit()));
+	return sqrt((1.0 + 0.05*skillValues[PrimarySkill::KNOWLEDGE] * mana / manaLimit()) * (1.0 + 0.05*skillValues[PrimarySkill::SPELL_POWER] * mana / manaLimit()));
 }
 
 double CGHeroInstance::getHeroStrength() const
 {
-	const auto & primarySkills = getPrimarySkills();
+	return getFightingStrength() * getMagicStrength();
-	return getFightingStrengthImpl(primarySkills) * getMagicStrengthImpl(primarySkills);
 }
 
 uint64_t CGHeroInstance::getValueForDiplomacy() const

lib/mapObjects/CGHeroInstance.h

@@ -14,6 +14,7 @@
 #include "CArmedInstance.h"
 #include "IOwnableObject.h"
 
+#include "../bonuses/BonusCache.h"
 #include "../entities/hero/EHeroGender.h"
 #include "../CArtHandler.h" // For CArtifactSet
 
@@ -58,13 +59,12 @@ class DLL_LINKAGE CGHeroInstance : public CArmedInstance, public IBoatGenerator,
 	friend class CMapFormatJson;
 
 private:
+	PrimarySkillsCache primarySkills;
+
 	std::set<SpellID> spells; //known spells (spell IDs)
 	mutable int lowestCreatureSpeed;
 	ui32 movement; //remaining movement points
 
-	double getFightingStrengthImpl(const std::array<int, 4> & primarySkills) const;
-	double getMagicStrengthImpl(const std::array<int, 4> & primarySkills) const;
-
 public:
 
 	//////////////////////////////////////////////////////////////////////////
@@ -204,7 +204,7 @@ public:
 	std::vector<SecondarySkill> getLevelUpProposedSecondarySkills(vstd::RNG & rand) const;
 
 	ui8 getSecSkillLevel(const SecondarySkill & skill) const; //0 - no skill
-	std::array<int, 4> getPrimarySkills() const;
+	int getPrimSkillLevel(PrimarySkill id) const;
 
 	/// Returns true if hero has free secondary skill slot.
 	bool canLearnSkill() const;

server/battles/BattleActionProcessor.cpp

@@ -257,7 +257,7 @@ bool BattleActionProcessor::doAttackAction(const CBattleInfoCallback & battle, c
 	}
 
 	//attack
-	int totalAttacks = stack->totalAttacks.getMeleeValue();
+	int totalAttacks = stack->getTotalAttacks(false);
 
 	//TODO: move to CUnitState
 	const auto * attackingHero = battle.battleGetFightingHero(ba.side);
@@ -378,7 +378,7 @@ bool BattleActionProcessor::doShootAction(const CBattleInfoCallback & battle, co
 	}
 	//allow more than one additional attack
 
-	int totalRangedAttacks = stack->totalAttacks.getRangedValue();
+	int totalRangedAttacks = stack->getTotalAttacks(true);
 
 	//TODO: move to CUnitState
 	const auto * attackingHero = battle.battleGetFightingHero(ba.side);