Issue2888 (#421)

Fixed issue 2888
* Merged AFTER_ATTACK & BEFORE_ATTACK cast modes.
* Introduced new caster class for creature ability usage
* Added few tests

lib/CMakeLists.txt

@@ -96,11 +96,14 @@ set(lib_SRCS
 		serializer/JsonSerializeFormat.cpp
 		serializer/JsonSerializer.cpp
 
+		spells/AbilityCaster.cpp
 		spells/AdventureSpellMechanics.cpp
 		spells/BattleSpellMechanics.cpp
+		spells/BonusCaster.cpp
 		spells/CSpellHandler.cpp
 		spells/ISpellMechanics.cpp
 		spells/Problem.cpp
+		spells/ProxyCaster.cpp
 		spells/TargetCondition.cpp
 		spells/ViewSpellInt.cpp
 
@@ -258,13 +261,16 @@ set(lib_HEADERS
 		serializer/JsonSerializeFormat.h
 		serializer/JsonSerializer.h
 
+		spells/AbilityCaster.h
 		spells/AdventureSpellMechanics.h
 		spells/BattleSpellMechanics.h
+		spells/BonusCaster.h
 		spells/CSpellHandler.h
 		spells/ISpellMechanics.h
 		spells/Magic.h
 		spells/SpellMechanics.h
 		spells/Problem.h
+		spells/ProxyCaster.h
 		spells/TargetCondition.h
 		spells/ViewSpellInt.h
 

lib/VCMI_lib.cbp

@@ -381,10 +381,14 @@
 		<Unit filename="serializer/JsonSerializer.cpp" />
 		<Unit filename="serializer/JsonSerializer.h" />
 		<Unit filename="serializer/JsonTreeSerializer.h" />
+		<Unit filename="spells/AbilityCaster.cpp" />
+		<Unit filename="spells/AbilityCaster.h" />
 		<Unit filename="spells/AdventureSpellMechanics.cpp" />
 		<Unit filename="spells/AdventureSpellMechanics.h" />
 		<Unit filename="spells/BattleSpellMechanics.cpp" />
 		<Unit filename="spells/BattleSpellMechanics.h" />
+		<Unit filename="spells/BonusCaster.cpp" />
+		<Unit filename="spells/BonusCaster.h" />
 		<Unit filename="spells/CSpellHandler.cpp" />
 		<Unit filename="spells/CSpellHandler.h" />
 		<Unit filename="spells/ISpellMechanics.cpp" />
@@ -392,6 +396,8 @@
 		<Unit filename="spells/Magic.h" />
 		<Unit filename="spells/Problem.cpp" />
 		<Unit filename="spells/Problem.h" />
+		<Unit filename="spells/ProxyCaster.cpp" />
+		<Unit filename="spells/ProxyCaster.h" />
 		<Unit filename="spells/TargetCondition.cpp" />
 		<Unit filename="spells/TargetCondition.h" />
 		<Unit filename="spells/ViewSpellInt.cpp" />

lib/battle/CUnitState.cpp

@@ -606,7 +606,7 @@ void CUnitState::getCasterName(MetaString & text) const
 	addNameReplacement(text, true);
 }
 
-void CUnitState::getCastDescription(const spells::Spell * spell, MetaString & text) const
+void CUnitState::getCastDescription(const spells::Spell * spell, const std::vector<const Unit *> & attacked, MetaString & text) const
 {
 	text.addTxt(MetaString::GENERAL_TXT, 565);//The %s casts %s
 	//todo: use text 566 for single creature
@@ -614,11 +614,6 @@ void CUnitState::getCastDescription(const spells::Spell * spell, MetaString & te
 	text.addReplacement(MetaString::SPELL_NAME, spell->getIndex());
 }
 
-void CUnitState::getCastDescription(const spells::Spell * spell, const std::vector<const Unit *> & attacked, MetaString & text) const
-{
-	getCastDescription(spell, text);
-}
-
 bool CUnitState::ableToRetaliate() const
 {
 	return alive()

lib/battle/CUnitState.h

@@ -226,7 +226,6 @@ public:
 
 	const PlayerColor getOwner() const override;
 	void getCasterName(MetaString & text) const override;
-	void getCastDescription(const spells::Spell * spell, MetaString & text) const override;
 	void getCastDescription(const spells::Spell * spell, const std::vector<const Unit *> & attacked, MetaString & text) const override;
 
 	bool ableToRetaliate() const override;

lib/mapObjects/CGHeroInstance.cpp

@@ -699,13 +699,6 @@ void CGHeroInstance::getCasterName(MetaString & text) const
 	text.addReplacement(name);
 }
 
-void CGHeroInstance::getCastDescription(const spells::Spell * spell, MetaString & text) const
-{
-	text.addTxt(MetaString::GENERAL_TXT, 196);
-	getCasterName(text);
-	text.addReplacement(MetaString::SPELL_NAME, spell->getIndex());
-}
-
 void CGHeroInstance::getCastDescription(const spells::Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const
 {
 	const bool singleTarget = attacked.size() == 1;

lib/mapObjects/CGHeroInstance.h

@@ -247,7 +247,6 @@ public:
 	const PlayerColor getOwner() const override;
 
 	void getCasterName(MetaString & text) const override;
-	void getCastDescription(const spells::Spell * spell, MetaString & text) const override;
 	void getCastDescription(const spells::Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const override;
 	void spendMana(const spells::PacketSender * server, const int spellCost) const override;
 

lib/spells/AbilityCaster.cpp

@@ -0,0 +1,58 @@
+/*
+ * AbilityCaster.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 "AbilityCaster.h"
+
+#include "../battle/Unit.h"
+
+namespace spells
+{
+
+AbilityCaster::AbilityCaster(const battle::Unit * actualCaster_, int baseSpellLevel_)
+	: ProxyCaster(actualCaster_),
+	actualCaster(actualCaster_),
+	baseSpellLevel(baseSpellLevel_)
+{
+}
+
+AbilityCaster::~AbilityCaster() = default;
+
+ui8 AbilityCaster::getSpellSchoolLevel(const Spell * spell, int * outSelectedSchool) const
+{
+	int skill = baseSpellLevel;
+
+	if(spell->getLevel() > 0)
+	{
+		vstd::amax(skill, actualCaster->valOfBonuses(Bonus::MAGIC_SCHOOL_SKILL, 0));
+	}
+
+	vstd::amax(skill, 0);
+	vstd::amin(skill, 3);
+
+	return static_cast<ui8>(skill); //todo: unify spell school level type
+}
+
+int AbilityCaster::getEffectLevel(const Spell * spell) const
+{
+	return getSpellSchoolLevel(spell);
+}
+
+void AbilityCaster::getCastDescription(const Spell * spell, const std::vector<const battle::Unit*> & attacked, MetaString & text) const
+{
+	//do nothing
+}
+
+void AbilityCaster::spendMana(const PacketSender * server, const int spellCost) const
+{
+	//do nothing
+}
+
+} // namespace spells

lib/spells/AbilityCaster.h

@@ -0,0 +1,34 @@
+/*
+ * AbilityCaster.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 "ProxyCaster.h"
+
+namespace spells
+{
+
+class DLL_LINKAGE AbilityCaster : public ProxyCaster
+{
+public:
+	AbilityCaster(const battle::Unit * actualCaster_, int baseSpellLevel_);
+	virtual ~AbilityCaster();
+
+	ui8 getSpellSchoolLevel(const Spell * spell, int * outSelectedSchool = nullptr) const override;
+	int getEffectLevel(const Spell * spell) const override;
+	void getCastDescription(const Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const override;
+	void spendMana(const PacketSender * server, const int spellCost) const override;
+
+private:
+	const battle::Unit * actualCaster;
+	int baseSpellLevel;
+};
+
+} // namespace spells

lib/spells/BattleSpellMechanics.cpp

@@ -262,7 +262,8 @@ void BattleSpellMechanics::cast(const PacketSender * server, vstd::RNG & rng, co
 		{
 			MetaString line;
 			caster->getCastDescription(owner, affectedUnits, line);
-			sc.battleLog.push_back(line);
+			if(!line.message.empty())
+				sc.battleLog.push_back(line);
 		}
 		break;
 

lib/spells/BonusCaster.cpp

@@ -0,0 +1,57 @@
+/*
+ * BonusCaster.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 "BonusCaster.h"
+
+#include "../NetPacksBase.h"
+#include "../HeroBonus.h"
+#include "../battle/Unit.h"
+
+namespace spells
+{
+
+BonusCaster::BonusCaster(const Caster * actualCaster_, std::shared_ptr<Bonus> bonus_)
+	: ProxyCaster(actualCaster_),
+	actualCaster(actualCaster_),
+	bonus(bonus_)
+{
+
+}
+
+BonusCaster::~BonusCaster() = default;
+
+void BonusCaster::getCasterName(MetaString & text) const
+{
+	if(!bonus->description.empty())
+		text.addReplacement(bonus->description);
+	else
+		actualCaster->getCasterName(text);
+}
+
+void BonusCaster::getCastDescription(const Spell * spell, const std::vector<const battle::Unit*> & attacked, MetaString & text) const
+{
+	const bool singleTarget = attacked.size() == 1;
+	const int textIndex = singleTarget ? 195 : 196;
+
+	text.addTxt(MetaString::GENERAL_TXT, textIndex);
+	getCasterName(text);
+	text.addReplacement(MetaString::SPELL_NAME, spell->getIndex());
+	if(singleTarget)
+		attacked.at(0)->addNameReplacement(text, true);
+}
+
+void BonusCaster::spendMana(const PacketSender * server, const int spellCost) const
+{
+	logGlobal->error("Unexpected call to BonusCaster::spendMana");
+}
+
+
+} // namespace spells

lib/spells/BonusCaster.h

@@ -0,0 +1,36 @@
+/*
+ * BonusCaster.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 "ProxyCaster.h"
+
+struct Bonus;
+
+namespace spells
+{
+
+class DLL_LINKAGE BonusCaster : public ProxyCaster
+{
+public:
+	BonusCaster(const Caster * actualCaster_, std::shared_ptr<Bonus> bonus_);
+	virtual ~BonusCaster();
+
+	void getCasterName(MetaString & text) const override;
+	void getCastDescription(const Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const override;
+	void spendMana(const PacketSender * server, const int spellCost) const override;
+
+private:
+	const Caster * actualCaster;
+	std::shared_ptr<Bonus> bonus;
+};
+
+} // namespace spells
+

lib/spells/CSpellHandler.cpp

@@ -250,6 +250,11 @@ int32_t CSpell::getIndex() const
 	return id.toEnum();
 }
 
+int32_t CSpell::getLevel() const
+{
+	return level;
+}
+
 bool CSpell::isCombatSpell() const
 {
 	return combatSpell;

lib/spells/CSpellHandler.h

@@ -291,6 +291,7 @@ public:
 	void forEachSchool(const std::function<void (const spells::SchoolInfo &, bool &)> & cb) const override;
 
 	int32_t getIndex() const override;
+	int32_t getLevel() const override;
 
 	/**
 	 * Returns resource name of icon for SPELL_IMMUNITY bonus

lib/spells/ISpellMechanics.cpp

@@ -158,8 +158,7 @@ BattleCast::BattleCast(const CBattleInfoCallback * cb, const Caster * caster_, c
 	cb(cb),
 	caster(caster_),
 	mode(mode_),
-	spellLvl(),
+	magicSkillLevel(),
-	effectLevel(),
 	effectPower(),
 	effectDuration(),
 	effectValue(),
@@ -174,8 +173,7 @@ BattleCast::BattleCast(const BattleCast & orig, const Caster * caster_)
 	cb(orig.cb),
 	caster(caster_),
 	mode(Mode::MAGIC_MIRROR),
-	spellLvl(orig.spellLvl),
+	magicSkillLevel(orig.magicSkillLevel),
-	effectLevel(orig.effectLevel),
 	effectPower(orig.effectPower),
 	effectDuration(orig.effectDuration),
 	effectValue(orig.effectValue),
@@ -206,20 +204,9 @@ const CBattleInfoCallback * BattleCast::getBattle() const
 	return cb;
 }
 
-BattleCast::OptionalValue BattleCast::getEffectLevel() const
+BattleCast::OptionalValue BattleCast::getSpellLevel() const
 {
-	if(effectLevel)
+	return magicSkillLevel;
-		return effectLevel;
-	else
-		return spellLvl;
-}
-
-BattleCast::OptionalValue BattleCast::getRangeLevel() const
-{
-	if(rangeLevel)
-		return rangeLevel;
-	else
-		return spellLvl;
 }
 
 BattleCast::OptionalValue BattleCast::getEffectPower() const
@@ -249,17 +236,7 @@ boost::logic::tribool BattleCast::isMassive() const
 
 void BattleCast::setSpellLevel(BattleCast::Value value)
 {
-	spellLvl = boost::make_optional(value);
+	magicSkillLevel = boost::make_optional(value);
-}
-
-void BattleCast::setEffectLevel(BattleCast::Value value)
-{
-	effectLevel = boost::make_optional(value);
-}
-
-void BattleCast::setRangeLevel(BattleCast::Value value)
-{
-	rangeLevel = boost::make_optional(value);
 }
 
 void BattleCast::setEffectPower(BattleCast::Value value)
@@ -484,7 +461,7 @@ BaseMechanics::BaseMechanics(const IBattleCast * event)
 		casterSide = cb->playerToSide(caster->getOwner()).get();
 
 	{
-		auto value = event->getRangeLevel();
+		auto value = event->getSpellLevel();
 		if(value)
 			rangeLevel = value.get();
 		else
@@ -492,7 +469,7 @@ BaseMechanics::BaseMechanics(const IBattleCast * event)
 		vstd::abetween(rangeLevel, 0, 3);
 	}
 	{
-		auto value = event->getEffectLevel();
+		auto value = event->getSpellLevel();
         if(value)
 			effectLevel = value.get();
 		else

lib/spells/ISpellMechanics.h

@@ -88,8 +88,7 @@ public:
 	virtual const Caster * getCaster() const = 0;
 	virtual const CBattleInfoCallback * getBattle() const = 0;
 
-	virtual OptionalValue getEffectLevel() const = 0;
+	virtual OptionalValue getSpellLevel() const = 0;
-	virtual OptionalValue getRangeLevel() const = 0;
 
 	virtual OptionalValue getEffectPower() const = 0;
 	virtual OptionalValue getEffectDuration() const = 0;
@@ -123,8 +122,7 @@ public:
 	const Caster * getCaster() const override;
 	const CBattleInfoCallback * getBattle() const override;
 
-	OptionalValue getEffectLevel() const override;
+	OptionalValue getSpellLevel() const override;
-	OptionalValue getRangeLevel() const override;
 
 	OptionalValue getEffectPower() const override;
 	OptionalValue getEffectDuration() const override;
@@ -135,8 +133,6 @@ public:
 	boost::logic::tribool isMassive() const override;
 
 	void setSpellLevel(Value value);
-	void setEffectLevel(Value value);
-	void setRangeLevel(Value value);
 
 	void setEffectPower(Value value);
 	void setEffectDuration(Value value);
@@ -162,10 +158,8 @@ public:
 
 private:
 	///spell school level
-	OptionalValue spellLvl;
+	OptionalValue magicSkillLevel;
 
-	OptionalValue rangeLevel;
-	OptionalValue effectLevel;
 	///actual spell-power affecting effect values
 	OptionalValue effectPower;
 	///actual spell-power affecting effect duration

lib/spells/Magic.h

@@ -41,8 +41,6 @@ enum class Mode
 {
 	//ACTIVE, //todo: use
 	HERO, //deprecated
-	AFTER_ATTACK,
-	BEFORE_ATTACK,
 	MAGIC_MIRROR,
 	CREATURE_ACTIVE, //deprecated
 	ENCHANTER,
@@ -92,6 +90,8 @@ public:
 
 	virtual int32_t getIndex() const = 0;
 
+	virtual int32_t getLevel() const = 0;
+
 	virtual void forEachSchool(const std::function<void (const SchoolInfo &, bool &)> & cb) const = 0;
 };
 
@@ -129,7 +129,6 @@ public:
 	virtual void getCasterName(MetaString & text) const = 0;
 
 	///full default text
-	virtual void getCastDescription(const Spell * spell, MetaString & text) const = 0;
 	virtual void getCastDescription(const Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const = 0;
 
 	virtual void spendMana(const PacketSender * server, const int spellCost) const = 0;

lib/spells/ProxyCaster.cpp

@@ -0,0 +1,82 @@
+/*
+ * ProxyCaster.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 "ProxyCaster.h"
+
+#include "../GameConstants.h"
+
+namespace spells
+{
+
+ProxyCaster::ProxyCaster(const Caster * actualCaster_)
+	: actualCaster(actualCaster_)
+{
+
+}
+
+ProxyCaster::~ProxyCaster() = default;
+
+ui8 ProxyCaster::getSpellSchoolLevel(const Spell * spell, int * outSelectedSchool) const
+{
+	return actualCaster->getSpellSchoolLevel(spell, outSelectedSchool);
+}
+
+int ProxyCaster::getEffectLevel(const Spell * spell) const
+{
+	return actualCaster->getEffectLevel(spell);
+}
+
+int64_t ProxyCaster::getSpellBonus(const Spell * spell, int64_t base, const battle::Unit * affectedStack) const
+{
+	return actualCaster->getSpellBonus(spell, base, affectedStack);
+}
+
+int64_t ProxyCaster::getSpecificSpellBonus(const Spell * spell, int64_t base) const
+{
+	return actualCaster->getSpecificSpellBonus(spell, base);
+}
+
+int ProxyCaster::getEffectPower(const Spell * spell) const
+{
+	return actualCaster->getEffectPower(spell);
+}
+
+int ProxyCaster::getEnchantPower(const Spell * spell) const
+{
+	return actualCaster->getEnchantPower(spell);
+}
+
+int64_t ProxyCaster::getEffectValue(const Spell * spell) const
+{
+	return actualCaster->getEffectValue(spell);
+}
+
+const PlayerColor ProxyCaster::getOwner() const
+{
+	return actualCaster->getOwner();
+}
+
+void ProxyCaster::getCasterName(MetaString & text) const
+{
+	return actualCaster->getCasterName(text);
+}
+
+void ProxyCaster::getCastDescription(const Spell * spell, const std::vector<const battle::Unit*> & attacked, MetaString & text) const
+{
+	actualCaster->getCastDescription(spell, attacked, text);
+}
+
+void ProxyCaster::spendMana(const PacketSender * server, const int spellCost) const
+{
+	actualCaster->spendMana(server, spellCost);
+}
+
+}

lib/spells/ProxyCaster.h

@@ -0,0 +1,40 @@
+/*
+ * ProxyCaster.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 "Magic.h"
+
+namespace spells
+{
+
+class DLL_LINKAGE ProxyCaster : public Caster
+{
+public:
+	ProxyCaster(const Caster * actualCaster_);
+	virtual ~ProxyCaster();
+
+	ui8 getSpellSchoolLevel(const Spell * spell, int * outSelectedSchool = nullptr) const override;
+	int getEffectLevel(const Spell * spell) const override;
+	int64_t getSpellBonus(const Spell * spell, int64_t base, const battle::Unit * affectedStack) const override;
+	int64_t getSpecificSpellBonus(const Spell * spell, int64_t base) const override;
+	int getEffectPower(const Spell * spell) const override;
+	int getEnchantPower(const Spell * spell) const override;
+	int64_t getEffectValue(const Spell * spell) const override;
+	const PlayerColor getOwner() const override;
+	void getCasterName(MetaString & text) const override;
+	void getCastDescription(const Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const override;
+	void spendMana(const PacketSender * server, const int spellCost) const override;
+
+private:
+	const Caster * actualCaster;
+};
+
+} // namespace spells

server/CGameHandler.cpp

@@ -18,6 +18,8 @@
 #include "../lib/CArtHandler.h"
 #include "../lib/CBuildingHandler.h"
 #include "../lib/CHeroHandler.h"
+#include "../lib/spells/AbilityCaster.h"
+#include "../lib/spells/BonusCaster.h"
 #include "../lib/spells/CSpellHandler.h"
 #include "../lib/spells/ISpellMechanics.h"
 #include "../lib/spells/Problem.h"
@@ -141,11 +143,6 @@ public:
 		logGlobal->error("Unexpected call to ObstacleCasterProxy::getCasterName");
 	}
 
-	void getCastDescription(const Spell * spell, MetaString & text) const override
-	{
-		logGlobal->error("Unexpected call to ObstacleCasterProxy::getCastDescription");
-	}
-
 	void getCastDescription(const Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const override
 	{
 		logGlobal->error("Unexpected call to ObstacleCasterProxy::getCastDescription");
@@ -163,95 +160,6 @@ private:
 	const SpellCreatedObstacle * obs;
 };
 
-class BonusCasterProxy : public Caster
-{
-public:
-	BonusCasterProxy(const CGHeroInstance * hero_, std::shared_ptr<const Bonus> bonus_)
-		: hero(hero_),
-		bonus(bonus_)
-	{
-
-	}
-
-	~BonusCasterProxy() = default;
-
-	ui8 getSpellSchoolLevel(const Spell * spell, int * outSelectedSchool = nullptr) const override
-	{
-		return hero->getSpellSchoolLevel(spell, outSelectedSchool);
-	}
-
-	int getEffectLevel(const Spell * spell) const override
-	{
-		return hero->getEffectLevel(spell);
-	}
-
-	int64_t getSpellBonus(const Spell * spell, int64_t base, const battle::Unit * affectedStack) const override
-	{
-		return hero->getSpellBonus(spell, base, affectedStack);
-	}
-
-	int64_t getSpecificSpellBonus(const Spell * spell, int64_t base) const override
-	{
-		return hero->getSpecificSpellBonus(spell, base);
-	}
-
-	int getEffectPower(const Spell * spell) const override
-	{
-		return hero->getEffectPower(spell);
-	}
-
-	int getEnchantPower(const Spell * spell) const override
-	{
-		return hero->getEnchantPower(spell);
-	}
-
-	int64_t getEffectValue(const Spell * spell) const override
-	{
-		return hero->getEffectValue(spell);
-	}
-
-	const PlayerColor getOwner() const override
-	{
-		return hero->getOwner();
-	}
-
-	void getCasterName(MetaString & text) const override
-	{
-		if(!bonus->description.empty())
-			text.addReplacement(bonus->description);
-		else
-			hero->getCasterName(text);
-	}
-
-	void getCastDescription(const Spell * spell, MetaString & text) const override
-	{
-		text.addTxt(MetaString::GENERAL_TXT, 196);
-		getCasterName(text);
-		text.addReplacement(MetaString::SPELL_NAME, spell->getIndex());
-	}
-
-	void getCastDescription(const Spell * spell, const std::vector<const battle::Unit *> & attacked, MetaString & text) const override
-	{
-		const bool singleTarget = attacked.size() == 1;
-		const int textIndex = singleTarget ? 195 : 196;
-
-		text.addTxt(MetaString::GENERAL_TXT, textIndex);
-		getCasterName(text);
-		text.addReplacement(MetaString::SPELL_NAME, spell->getIndex());
-		if(singleTarget)
-			attacked.at(0)->addNameReplacement(text, true);
-	}
-
-	void spendMana(const PacketSender * server, const int spellCost) const override
-	{
-		logGlobal->error("Unexpected call to BonusCasterProxy::spendMana");
-	}
-
-private:
-	const CGHeroInstance * hero;
-	std::shared_ptr<const Bonus> bonus;
-};
-
 }//
 
 CondSh<bool> battleMadeAction(false);
@@ -5530,10 +5438,8 @@ bool CGameHandler::dig(const CGHeroInstance *h)
 	return true;
 }
 
-void CGameHandler::attackCasting(bool ranged, Bonus::BonusType attackMode, const CStack * attacker, const CStack * defender)
+void CGameHandler::attackCasting(bool ranged, Bonus::BonusType attackMode, const battle::Unit * attacker, const battle::Unit * defender)
 {
-	spells::Mode mode = (attackMode == Bonus::SPELL_AFTER_ATTACK) ? spells::Mode::AFTER_ATTACK : spells::Mode::BEFORE_ATTACK;
-
 	if(attacker->hasBonusOfType(attackMode))
 	{
 		std::set<SpellID> spellsToCast;
@@ -5563,7 +5469,9 @@ void CGameHandler::attackCasting(bool ranged, Bonus::BonusType attackMode, const
 			vstd::amin(chance, 100);
 
 			const CSpell * spell = SpellID(spellID).toSpell();
-			if(!spell->canBeCastAt(gs->curB, mode, attacker, defender->getPosition()))
+			spells::AbilityCaster caster(attacker, spellLevel);
+
+			if(!spell->canBeCastAt(gs->curB, spells::Mode::PASSIVE, &caster, defender->getPosition()))
 				continue;
 
 			//check if spell should be cast (probability handling)
@@ -5573,8 +5481,7 @@ void CGameHandler::attackCasting(bool ranged, Bonus::BonusType attackMode, const
 			//casting
 			if(castMe)
 			{
-				spells::BattleCast parameters(gs->curB, attacker, mode, spell);
+				spells::BattleCast parameters(gs->curB, &caster, spells::Mode::PASSIVE, spell);
-				parameters.setSpellLevel(spellLevel);
 				parameters.aimToUnit(defender);
 				parameters.cast(spellEnv);
 			}
@@ -5623,8 +5530,9 @@ void CGameHandler::handleAfterAttackCasting(bool ranged, const CStack * attacker
 			//TODO: death stare was not originally available for multiple-hex attacks, but...
 			const CSpell * spell = SpellID(SpellID::DEATH_STARE).toSpell();
 
-			spells::BattleCast parameters(gs->curB, attacker, spells::Mode::AFTER_ATTACK, spell);
+			spells::AbilityCaster caster(attacker, 0);
-			parameters.setSpellLevel(0);
+
+			spells::BattleCast parameters(gs->curB, &caster, spells::Mode::PASSIVE, spell);
 			parameters.aimToUnit(defender);
 			parameters.setEffectValue(staredCreatures);
 			parameters.cast(spellEnv);
@@ -5646,8 +5554,9 @@ void CGameHandler::handleAfterAttackCasting(bool ranged, const CStack * attacker
 	{
 		const CSpell * spell = SpellID(SpellID::ACID_BREATH_DAMAGE).toSpell();
 
-		spells::BattleCast parameters(gs->curB, attacker, spells::Mode::AFTER_ATTACK, spell);
+		spells::AbilityCaster caster(attacker, 0);
-		parameters.setSpellLevel(0);
+
+		spells::BattleCast parameters(gs->curB, &caster, spells::Mode::PASSIVE, spell);
 		parameters.aimToUnit(defender);
 		parameters.setEffectValue(acidDamage * attacker->getCount());
 		parameters.cast(spellEnv);
@@ -6066,7 +5975,7 @@ void CGameHandler::runBattle()
 
 			for (auto b : *bl)
 			{
-				spells::BonusCasterProxy caster(h, b);
+				spells::BonusCaster caster(h, b);
 
 				const CSpell * spell = SpellID(b->subtype).toSpell();
 

server/CGameHandler.h

@@ -289,7 +289,7 @@ public:
 	void newTurn();
 	void handleAttackBeforeCasting(bool ranged, const CStack * attacker, const CStack * defender);
 	void handleAfterAttackCasting(bool ranged, const CStack * attacker, const CStack * defender);
-	void attackCasting(bool ranged, Bonus::BonusType attackMode, const CStack * attacker, const CStack * defender);
+	void attackCasting(bool ranged, Bonus::BonusType attackMode, const battle::Unit * attacker, const battle::Unit * defender);
 	bool sacrificeArtifact(const IMarket * m, const CGHeroInstance * hero, const std::vector<ArtifactPosition> & slot);
 	void spawnWanderingMonsters(CreatureID creatureID);
 	void handleCheatCode(std::string & cheat, PlayerColor player, const CGHeroInstance * hero, const CGTownInstance * town, bool & cheated);

test/CMakeLists.txt

@@ -30,6 +30,7 @@ set(test_SRCS
  		map/CMapFormatTest.cpp
  		map/MapComparer.cpp
 
+		spells/AbilityCasterTest.cpp
  		spells/TargetConditionTest.cpp
 
 		spells/effects/EffectFixture.cpp

test/Test.cbp

@@ -109,6 +109,7 @@
 		<Unit filename="mock/mock_spells_Problem.h" />
 		<Unit filename="mock/mock_spells_Spell.h" />
 		<Unit filename="mock/mock_vstd_RNG.h" />
+		<Unit filename="spells/AbilityCasterTest.cpp" />
 		<Unit filename="spells/TargetConditionTest.cpp" />
 		<Unit filename="spells/effects/CatapultTest.cpp" />
 		<Unit filename="spells/effects/CloneTest.cpp" />

test/game/CGameStateTest.cpp

@@ -26,6 +26,7 @@
 
 #include "../../lib/spells/CSpellHandler.h"
 #include "../../lib/spells/ISpellMechanics.h"
+#include "../../lib/spells/AbilityCaster.h"
 
 class CGameStateTest : public ::testing::Test, public SpellCastEnvironment, public MapListener
 {
@@ -224,12 +225,14 @@ TEST_F(CGameStateTest, issue2765)
 	{
 		const CSpell * age = SpellID(SpellID::AGE).toSpell();
 		ASSERT_NE(age, nullptr);
-		//here tested ballista, but this applied to all war machines
-		spells::BattleCast cast(gameState->curB, att, spells::Mode::AFTER_ATTACK, age);
-		cast.aimToUnit(def);
-		cast.setSpellLevel(3);
 
-		EXPECT_FALSE(age->canBeCastAt(gameState->curB, spells::Mode::AFTER_ATTACK, att, def->getPosition()));
+		spells::AbilityCaster caster(att, 3);
+
+		//here tested ballista, but this applied to all war machines
+		spells::BattleCast cast(gameState->curB, &caster, spells::Mode::PASSIVE, age);
+		cast.aimToUnit(def);
+
+		EXPECT_FALSE(age->canBeCastAt(gameState->curB, spells::Mode::PASSIVE, &caster, def->getPosition()));
 
 		EXPECT_TRUE(cast.castIfPossible(this));//should be possible, but with no effect (change to aimed cast check?)
 

test/mock/mock_battle_Unit.h

@@ -27,7 +27,6 @@ public:
 	MOCK_CONST_METHOD1(getEffectValue, int64_t(const spells::Spell *));
 	MOCK_CONST_METHOD0(getOwner, const PlayerColor());
 	MOCK_CONST_METHOD1(getCasterName, void(MetaString &));
-	MOCK_CONST_METHOD2(getCastDescription, void(const spells::Spell *, MetaString &));
 	MOCK_CONST_METHOD3(getCastDescription, void(const spells::Spell *, const std::vector<const battle::Unit *> &, MetaString &));
 	MOCK_CONST_METHOD2(spendMana, void(const spells::PacketSender *, const int));
 

test/mock/mock_spells_Spell.h

@@ -19,6 +19,7 @@ class SpellMock : public Spell
 {
 public:
 	MOCK_CONST_METHOD0(getIndex, int32_t());
+	MOCK_CONST_METHOD0(getLevel, int32_t());
 	MOCK_CONST_METHOD1(forEachSchool, void(const std::function<void (const SchoolInfo &, bool &)> &));
 };
 

test/spells/AbilityCasterTest.cpp

@@ -0,0 +1,84 @@
+/*
+ * AbilityCasterTest.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 "mock/mock_battle_Unit.h"
+#include "mock/mock_BonusBearer.h"
+#include "mock/mock_spells_Spell.h"
+
+#include "../../../lib/NetPacksBase.h"
+#include "../../../lib/spells/AbilityCaster.h"
+
+namespace test
+{
+using namespace ::spells;
+using namespace ::testing;
+
+
+class AbilityCasterTest : public Test
+{
+public:
+	std::shared_ptr<AbilityCaster> subject;
+
+	StrictMock<UnitMock> actualCaster;
+	BonusBearerMock casterBonuses;
+	StrictMock<SpellMock> spellMock;
+
+protected:
+	void SetUp() override
+	{
+		ON_CALL(actualCaster, getAllBonuses(_, _, _, _)).WillByDefault(Invoke(&casterBonuses, &BonusBearerMock::getAllBonuses));
+		ON_CALL(actualCaster, getTreeVersion()).WillByDefault(Invoke(&casterBonuses, &BonusBearerMock::getTreeVersion));
+	}
+
+	void setupSubject(int skillLevel)
+	{
+		subject = std::make_shared<AbilityCaster>(&actualCaster, skillLevel);
+	}
+};
+
+TEST_F(AbilityCasterTest, NonMagicAbilityIgnoresBonuses)
+{
+	EXPECT_CALL(spellMock, getLevel()).WillRepeatedly(Return(0));
+	setupSubject(1);
+
+	EXPECT_EQ(subject->getSpellSchoolLevel(&spellMock), 1);
+}
+
+TEST_F(AbilityCasterTest, MagicAbilityAffectedByGenericBonus)
+{
+	EXPECT_CALL(spellMock, getLevel()).WillRepeatedly(Return(1));
+
+	casterBonuses.addNewBonus(std::make_shared<Bonus>(Bonus::ONE_BATTLE, Bonus::MAGIC_SCHOOL_SKILL, Bonus::OTHER, 2, 0, 0));
+
+	EXPECT_CALL(actualCaster, getAllBonuses(_, _, _, _)).Times(AtLeast(1));
+	EXPECT_CALL(actualCaster, getTreeVersion()).Times(AtLeast(0));
+
+	setupSubject(1);
+
+	EXPECT_EQ(subject->getSpellSchoolLevel(&spellMock), 2);
+}
+
+TEST_F(AbilityCasterTest, MagicAbilityIngoresSchoolBonus)
+{
+	EXPECT_CALL(spellMock, getLevel()).WillRepeatedly(Return(1));
+
+	casterBonuses.addNewBonus(std::make_shared<Bonus>(Bonus::ONE_BATTLE, Bonus::MAGIC_SCHOOL_SKILL, Bonus::OTHER, 2, 0, 1));
+
+	EXPECT_CALL(actualCaster, getAllBonuses(_, _, _, _)).Times(AtLeast(1));
+	EXPECT_CALL(actualCaster, getTreeVersion()).Times(AtLeast(0));
+
+	setupSubject(1);
+
+	EXPECT_EQ(subject->getSpellSchoolLevel(&spellMock), 1);
+}
+
+
+}