Unified spellcasting handling with other actions

AI/BattleAI/BattleAI.cpp

@@ -283,7 +283,8 @@ void CBattleAI::activeStack( const CStack * stack )
 			return;
 		}
 
-		attemptCastingSpell();
+		if (attemptCastingSpell())
+			return;
 
 		logAi->trace("Spellcast attempt completed in %lld", timeElapsed(start));
 
@@ -476,14 +477,14 @@ BattleAction CBattleAI::useCatapult(const CStack * stack)
 	return attack;
 }
 
-void CBattleAI::attemptCastingSpell()
+bool CBattleAI::attemptCastingSpell()
 {
 	auto hero = cb->battleGetMyHero();
 	if(!hero)
-		return;
+		return false;
 
 	if(cb->battleCanCastSpell(hero, spells::Mode::HERO) != ESpellCastProblem::OK)
-		return;
+		return false;
 
 	LOGL("Casting spells sounds like fun. Let's see...");
 	//Get all spells we can cast
@@ -522,7 +523,7 @@ void CBattleAI::attemptCastingSpell()
 	}
 	LOGFL("Found %d spell-target combinations.", possibleCasts.size());
 	if(possibleCasts.empty())
-		return;
+		return false;
 
 	using ValueMap = PossibleSpellcast::ValueMap;
 
@@ -657,7 +658,7 @@ void CBattleAI::attemptCastingSpell()
 			if(battleIsFinishedOpt)
 			{
 				print("No need to cast a spell. Battle will finish soon.");
-				return;
+				return false;
 			}
 		}
 	}
@@ -786,10 +787,12 @@ void CBattleAI::attemptCastingSpell()
 		spellcast.stackNumber = (!side) ? -1 : -2;
 		cb->battleMakeSpellAction(spellcast);
 		movesSkippedByDefense = 0;
+		return true;
 	}
 	else
 	{
 		LOGFL("Best spell is %s. But it is actually useless (value %d).", castToPerform.spell->getNameTranslated() % castToPerform.value);
+		return false;
 	}
 }
 

AI/BattleAI/BattleAI.h

@@ -68,7 +68,7 @@ public:
 	~CBattleAI();
 
 	void initBattleInterface(std::shared_ptr<Environment> ENV, std::shared_ptr<CBattleCallback> CB) override;
-	void attemptCastingSpell();
+	bool attemptCastingSpell();
 
 	void evaluateCreatureSpellcast(const CStack * stack, PossibleSpellcast & ps); //for offensive damaging spells only
 

server/battles/BattleActionProcessor.cpp

@@ -136,13 +136,8 @@ bool BattleActionProcessor::doHeroSpellAction(const BattleAction & ba)
 		return false;
 	}
 
-	StartAction start_action(ba);
-	gameHandler->sendAndApply(&start_action); //start spell casting
-
 	parameters.cast(gameHandler->spellEnv, ba.getTarget(gameHandler->gameState()->curB));
 
-	EndAction end_action;
-	gameHandler->sendAndApply(&end_action);
 	return true;
 }
 

server/battles/BattleActionProcessor.h

@@ -18,7 +18,8 @@ struct BattleHex;
 class CStack;
 enum class BonusType;
 
-namespace battle {
+namespace battle
+{
 class Unit;
 class CUnitState;
 }
@@ -28,6 +29,7 @@ VCMI_LIB_NAMESPACE_END
 class CGameHandler;
 class BattleProcessor;
 
+/// Processes incoming battle action queries and applies requested action(s)
 class BattleActionProcessor : boost::noncopyable
 {
 	using FireShieldInfo = std::vector<std::pair<const CStack *, int64_t>>;
@@ -71,7 +73,5 @@ public:
 	explicit BattleActionProcessor(BattleProcessor * owner);
 	void setGameHandler(CGameHandler * newGameHandler);
 
-	bool makeBattleAction(const BattleAction &ba);
+	bool makeBattleAction(const BattleAction & ba);
-
 };
-

server/battles/BattleFlowProcessor.cpp

@@ -147,6 +147,72 @@ void BattleFlowProcessor::onBattleStarted()
 		onTacticsEnded();
 }
 
+void BattleFlowProcessor::trySummonGuardians(const CStack * stack)
+{
+	if (!stack->hasBonusOfType(BonusType::SUMMON_GUARDIANS))
+		return;
+
+	std::shared_ptr<const Bonus> summonInfo = stack->getBonus(Selector::type()(BonusType::SUMMON_GUARDIANS));
+	auto accessibility = gameHandler->getAccesibility();
+	CreatureID creatureData = CreatureID(summonInfo->subtype);
+	std::vector<BattleHex> targetHexes;
+	const bool targetIsBig = stack->unitType()->isDoubleWide(); //target = creature to guard
+	const bool guardianIsBig = creatureData.toCreature()->isDoubleWide();
+
+	/*Chosen idea for two hex units was to cover all possible surrounding hexes of target unit with as small number of stacks as possible.
+		For one-hex targets there are four guardians - front, back and one per side (up + down).
+		Two-hex targets are wider and the difference is there are two guardians per side to cover 3 hexes + extra hex in the front
+		Additionally, there are special cases for starting positions etc., where guardians would be outside of battlefield if spawned normally*/
+	if (!guardianIsBig)
+		targetHexes = stack->getSurroundingHexes();
+	else
+		summonGuardiansHelper(targetHexes, stack->getPosition(), stack->unitSide(), targetIsBig);
+
+	for(auto hex : targetHexes)
+	{
+		if(accessibility.accessible(hex, guardianIsBig, stack->unitSide())) //without this multiple creatures can occupy one hex
+		{
+			battle::UnitInfo info;
+			info.id = gameHandler->gameState()->curB->battleNextUnitId();
+			info.count =  std::max(1, (int)(stack->getCount() * 0.01 * summonInfo->val));
+			info.type = creatureData;
+			info.side = stack->unitSide();
+			info.position = hex;
+			info.summoned = true;
+
+			BattleUnitsChanged pack;
+			pack.changedStacks.emplace_back(info.id, UnitChanges::EOperation::ADD);
+			info.save(pack.changedStacks.back().data);
+			gameHandler->sendAndApply(&pack);
+		}
+	}
+}
+
+void BattleFlowProcessor::castOpeningSpells()
+{
+	for (int i = 0; i < 2; ++i)
+	{
+		auto h = gameHandler->gameState()->curB->battleGetFightingHero(i);
+		if (!h)
+			continue;
+
+		TConstBonusListPtr bl = h->getBonuses(Selector::type()(BonusType::OPENING_BATTLE_SPELL));
+
+		for (auto b : *bl)
+		{
+			spells::BonusCaster caster(h, b);
+
+			const CSpell * spell = SpellID(b->subtype).toSpell();
+
+			spells::BattleCast parameters(gameHandler->gameState()->curB, &caster, spells::Mode::PASSIVE, spell);
+			parameters.setSpellLevel(3);
+			parameters.setEffectDuration(b->val);
+			parameters.massive = true;
+			parameters.castIfPossible(gameHandler->spellEnv, spells::Target());
+		}
+	}
+}
+
 void BattleFlowProcessor::onTacticsEnded()
 {
 	//initial stacks appearance triggers, e.g. built-in bonus spells
@@ -154,73 +220,17 @@ void BattleFlowProcessor::onTacticsEnded()
 
 	for (CStack * stack : initialStacks)
 	{
-		if (stack->hasBonusOfType(BonusType::SUMMON_GUARDIANS))
+		trySummonGuardians(stack);
-		{
-			std::shared_ptr<const Bonus> summonInfo = stack->getBonus(Selector::type()(BonusType::SUMMON_GUARDIANS));
-			auto accessibility = gameHandler->getAccesibility();
-			CreatureID creatureData = CreatureID(summonInfo->subtype);
-			std::vector<BattleHex> targetHexes;
-			const bool targetIsBig = stack->unitType()->isDoubleWide(); //target = creature to guard
-			const bool guardianIsBig = creatureData.toCreature()->isDoubleWide();
-
-			/*Chosen idea for two hex units was to cover all possible surrounding hexes of target unit with as small number of stacks as possible.
-			For one-hex targets there are four guardians - front, back and one per side (up + down).
-			Two-hex targets are wider and the difference is there are two guardians per side to cover 3 hexes + extra hex in the front
-			Additionally, there are special cases for starting positions etc., where guardians would be outside of battlefield if spawned normally*/
-			if (!guardianIsBig)
-				targetHexes = stack->getSurroundingHexes();
-			else
-				summonGuardiansHelper(targetHexes, stack->getPosition(), stack->unitSide(), targetIsBig);
-
-			for(auto hex : targetHexes)
-			{
-				if(accessibility.accessible(hex, guardianIsBig, stack->unitSide())) //without this multiple creatures can occupy one hex
-				{
-					battle::UnitInfo info;
-					info.id = gameHandler->gameState()->curB->battleNextUnitId();
-					info.count =  std::max(1, (int)(stack->getCount() * 0.01 * summonInfo->val));
-					info.type = creatureData;
-					info.side = stack->unitSide();
-					info.position = hex;
-					info.summoned = true;
-
-					BattleUnitsChanged pack;
-					pack.changedStacks.emplace_back(info.id, UnitChanges::EOperation::ADD);
-					info.save(pack.changedStacks.back().data);
-					gameHandler->sendAndApply(&pack);
-				}
-			}
-		}
-
 		stackEnchantedTrigger(stack);
 	}
 
-	//spells opening battle
+	castOpeningSpells();
-	for (int i = 0; i < 2; ++i)
-	{
-		auto h = gameHandler->gameState()->curB->battleGetFightingHero(i);
-		if (h)
-		{
-			TConstBonusListPtr bl = h->getBonuses(Selector::type()(BonusType::OPENING_BATTLE_SPELL));
 
-			for (auto b : *bl)
-			{
-				spells::BonusCaster caster(h, b);
-
-				const CSpell * spell = SpellID(b->subtype).toSpell();
-
-				spells::BattleCast parameters(gameHandler->gameState()->curB, &caster, spells::Mode::PASSIVE, spell);
-				parameters.setSpellLevel(3);
-				parameters.setEffectDuration(b->val);
-				parameters.massive = true;
-				parameters.castIfPossible(gameHandler->spellEnv, spells::Target());
-			}
-		}
-	}
 	// it is possible that due to opening spells one side was eliminated -> check for end of battle
 	owner->checkBattleStateChanges();
 
 	startNextRound(true);
+	activateNextStack();
 }
 
 void BattleFlowProcessor::startNextRound(bool isFirstRound)
@@ -245,8 +255,6 @@ void BattleFlowProcessor::startNextRound(bool isFirstRound)
 		if(stack->alive() && !isFirstRound)
 			stackEnchantedTrigger(stack);
 	}
-
-	activateNextStack();
 }
 
 const CStack * BattleFlowProcessor::getNextStack()
@@ -296,7 +304,13 @@ void BattleFlowProcessor::activateNextStack()
 		const CStack * next = getNextStack();
 
 		if (!next)
-			return;
+		{
+			// No stacks to move - start next round
+			startNextRound(false);
+			next = getNextStack();
+			if (!next)
+				throw std::runtime_error("Failed to find valid stack to act!");
+		}
 
 		BattleUnitsChanged removeGhosts;
 
@@ -499,27 +513,41 @@ bool BattleFlowProcessor::rollGoodMorale(const CStack * next)
 
 void BattleFlowProcessor::onActionMade(const BattleAction &ba)
 {
-	const CStack * next = gameHandler->gameState()->curB->battleGetStackByID(ba.stackNumber);
+	const auto & battle = gameHandler->gameState()->curB;
+
+	const CStack * actedStack = battle->battleGetStackByID(ba.stackNumber);
+	const CStack * activeStack = battle->battleGetStackByID(battle->getActiveStackID());
+	assert(activeStack != nullptr);
 
 	//we're after action, all results applied
 	owner->checkBattleStateChanges(); //check if this action ended the battle
 
-	if(next == nullptr)
+	bool heroAction = ba.actionType == EActionType::HERO_SPELL || ba.actionType ==EActionType::SURRENDER || ba.actionType ==EActionType::RETREAT || ba.actionType ==EActionType::END_TACTIC_PHASE;
-		return;
 
-	bool heroAction = ba.actionType == EActionType::HERO_SPELL || ba.actionType ==EActionType::SURRENDER|| ba.actionType ==EActionType::RETREAT;
+	if (heroAction)
-
-	if (heroAction && next->alive())
 	{
-		// this is action made by hero AND unit is alive (e.g. not killed by casted spell)
+		if (activeStack->alive())
-		// keep current active stack for next action
+		{
-		return;
+			// this is action made by hero AND unit is alive (e.g. not killed by casted spell)
+			// keep current active stack for next action
+			BattleSetActiveStack sas;
+			sas.stack = activeStack->unitId();
+			gameHandler->sendAndApply(&sas);
+			return;
+		}
 	}
-
+	else
-	if (rollGoodMorale(next))
 	{
-		// Good morale - same stack makes 2nd turn
+		assert(actedStack != nullptr);
-		return;
+
+		if (rollGoodMorale(actedStack))
+		{
+			// Good morale - same stack makes 2nd turn
+			BattleSetActiveStack sas;
+			sas.stack = actedStack->unitId();
+			gameHandler->sendAndApply(&sas);
+			return;
+		}
 	}
 
 	activateNextStack();

server/battles/BattleFlowProcessor.h

@@ -19,6 +19,7 @@ VCMI_LIB_NAMESPACE_END
 class CGameHandler;
 class BattleProcessor;
 
+/// Controls flow of battles - battle startup actions and switching to next stack or next round after actions
 class BattleFlowProcessor : boost::noncopyable
 {
 	BattleProcessor * owner;
@@ -30,15 +31,17 @@ class BattleFlowProcessor : boost::noncopyable
 	bool tryMakeAutomaticAction(const CStack * stack);
 
 	void summonGuardiansHelper(std::vector<BattleHex> & output, const BattleHex & targetPosition, ui8 side, bool targetIsTwoHex);
+	void trySummonGuardians(const CStack * stack);
+	void castOpeningSpells();
 	void activateNextStack();
 	void startNextRound(bool isFirstRound);
 
 	void stackEnchantedTrigger(const CStack * stack);
-	void removeObstacle(const CObstacleInstance &obstacle);
+	void removeObstacle(const CObstacleInstance & obstacle);
-	void stackTurnTrigger(const CStack *stack);
+	void stackTurnTrigger(const CStack * stack);
 
 	void makeStackDoNothing(const CStack * next);
-	bool makeAutomaticAction(const CStack *stack, BattleAction &ba); //used when action is taken by stack without volition of player (eg. unguided catapult attack)
+	bool makeAutomaticAction(const CStack * stack, BattleAction & ba); //used when action is taken by stack without volition of player (eg. unguided catapult attack)
 
 public:
 	explicit BattleFlowProcessor(BattleProcessor * owner);
@@ -46,5 +49,5 @@ public:
 
 	void onBattleStarted();
 	void onTacticsEnded();
-	void onActionMade(const BattleAction &ba);
+	void onActionMade(const BattleAction & ba);
 };

server/battles/BattleProcessor.h

@@ -27,6 +27,7 @@ class BattleActionProcessor;
 class BattleFlowProcessor;
 class BattleResultProcessor;
 
+/// Main class for battle handling. Contains all public interface for battles that is accessible from outside, e.g. for CGameHandler
 class BattleProcessor : boost::noncopyable
 {
 	friend class BattleActionProcessor;
@@ -39,14 +40,15 @@ class BattleProcessor : boost::noncopyable
 	std::unique_ptr<BattleResultProcessor> resultProcessor;
 
 	void updateGateState();
-	void engageIntoBattle( PlayerColor player );
+	void engageIntoBattle(PlayerColor player);
 
 	void checkBattleStateChanges();
 	void setupBattle(int3 tile, const CArmedInstance *armies[2], const CGHeroInstance *heroes[2], bool creatureBank, const CGTownInstance *town);
 
-	bool makeBattleAction(const BattleAction &ba);
+	bool makeBattleAction(const BattleAction & ba);
 
 	void setBattleResult(EBattleResult resultType, int victoriusSide);
+
 public:
 	explicit BattleProcessor(CGameHandler * gameHandler);
 	BattleProcessor();
@@ -54,14 +56,20 @@ public:
 
 	void setGameHandler(CGameHandler * gameHandler);
 
-	void startBattlePrimary(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, const CGHeroInstance *hero1, const CGHeroInstance *hero2, bool creatureBank = false, const CGTownInstance *town = nullptr); //use hero=nullptr for no hero
+	/// Starts battle with specified parameters
-	void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, bool creatureBank = false); //if any of armies is hero, hero will be used
+	void startBattlePrimary(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, const CGHeroInstance *hero1, const CGHeroInstance *hero2, bool creatureBank = false, const CGTownInstance *town = nullptr);
-	void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, bool creatureBank = false); //if any of armies is hero, hero will be used, visitable tile of second obj is place of battle
+	/// Starts battle between two armies (which can also be heroes) at specified tile
+	void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, bool creatureBank = false);
+	/// Starts battle between two armies (which can also be heroes) at position of 2nd object
+	void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, bool creatureBank = false);
 
-	bool makeBattleAction(PlayerColor player, BattleAction &ba);
+	/// Processing of incoming battle action netpack
+	bool makeBattleAction(PlayerColor player, BattleAction & ba);
 
+	/// Applies results of a battle once player agrees to them
 	void endBattleConfirm(const BattleInfo * battleInfo);
-	void battleAfterLevelUp(const BattleResult &result);
+	/// Applies results of a battle after potential levelup
+	void battleAfterLevelUp(const BattleResult & result);
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{

server/battles/BattleResultProcessor.h

@@ -24,7 +24,7 @@ struct CasualtiesAfterBattle
 {
 	using TStackAndItsNewCount = std::pair<StackLocation, int>;
 	using TSummoned = std::map<CreatureID, TQuantity>;
-	enum {ERASE = -1};
+	//	enum {ERASE = -1};
 	const CArmedInstance * army;
 	std::vector<TStackAndItsNewCount> newStackCounts;
 	std::vector<ArtifactLocation> removedWarMachines;
@@ -32,7 +32,7 @@ struct CasualtiesAfterBattle
 	ObjectInstanceID heroWithDeadCommander; //TODO: unify stack locations
 
 	CasualtiesAfterBattle(const SideInBattle & battleSide, const BattleInfo * bat);
-	void updateArmy(CGameHandler *gh);
+	void updateArmy(CGameHandler * gh);
 };
 
 struct FinishingBattleHelper
@@ -61,7 +61,7 @@ struct FinishingBattleHelper
 
 class BattleResultProcessor : boost::noncopyable
 {
-//	BattleProcessor * owner;
+	//	BattleProcessor * owner;
 	CGameHandler * gameHandler;
 
 	std::unique_ptr<BattleResult> battleResult;
@@ -74,5 +74,5 @@ public:
 	void setBattleResult(EBattleResult resultType, int victoriusSide);
 	void endBattle(int3 tile, const CGHeroInstance * hero1, const CGHeroInstance * hero2); //ends battle
 	void endBattleConfirm(const BattleInfo * battleInfo);
-	void battleAfterLevelUp(const BattleResult &result);
+	void battleAfterLevelUp(const BattleResult & result);
 };