Split BattleAI::activeStack into several smaller methods

AI/BattleAI/BattleAI.cpp

@@ -93,170 +93,191 @@ void CBattleAI::initBattleInterface(std::shared_ptr<Environment> ENV, std::share
 	movesSkippedByDefense = 0;
 }
 
-BattleAction CBattleAI::activeStack( const CStack * stack )
+BattleAction CBattleAI::useHealingTent(const CStack *stack)
 {
-	LOG_TRACE_PARAMS(logAi, "stack: %s", stack->nodeName());
+	auto healingTargets = cb->battleGetStacks(CBattleInfoEssentials::ONLY_MINE);
+	std::map<int, const CStack*> woundHpToStack;
+	for(const auto * stack : healingTargets)
+	{
+		if(auto woundHp = stack->getMaxHealth() - stack->getFirstHPleft())
+			woundHpToStack[woundHp] = stack;
+	}
 
-	BattleAction result = BattleAction::makeDefend(stack);
-	setCbc(cb); //TODO: make solid sure that AIs always use their callbacks (need to take care of event handlers too)
+	if(woundHpToStack.empty())
+		return BattleAction::makeDefend(stack);
+	else
+		return BattleAction::makeHeal(stack, woundHpToStack.rbegin()->second); //last element of the woundHpToStack is the most wounded stack
+}
 
-	try
+std::optional<PossibleSpellcast> CBattleAI::findBestCreatureSpell(const CStack *stack)
+{
+	//TODO: faerie dragon type spell should be selected by server
+	SpellID creatureSpellToCast = cb->battleGetRandomStackSpell(CRandomGenerator::getDefault(), stack, CBattleInfoCallback::RANDOM_AIMED);
+	if(stack->hasBonusOfType(BonusType::SPELLCASTER) && stack->canCast() && creatureSpellToCast != SpellID::NONE)
 	{
-		if(stack->creatureId() == CreatureID::CATAPULT)
-			return useCatapult(stack);
-		if(stack->hasBonusOfType(BonusType::SIEGE_WEAPON) && stack->hasBonusOfType(BonusType::HEALER))
-		{
-			auto healingTargets = cb->battleGetStacks(CBattleInfoEssentials::ONLY_MINE);
-			std::map<int, const CStack*> woundHpToStack;
-			for(auto stack : healingTargets)
-				if(auto woundHp = stack->getMaxHealth() - stack->getFirstHPleft())
-					woundHpToStack[woundHp] = stack;
-			if(woundHpToStack.empty())
-				return BattleAction::makeDefend(stack);
-			else
-				return BattleAction::makeHeal(stack, woundHpToStack.rbegin()->second); //last element of the woundHpToStack is the most wounded stack
-		}
+		const CSpell * spell = creatureSpellToCast.toSpell();
 
-		attemptCastingSpell();
-
-		if(cb->battleIsFinished() || !stack->alive())
+		if(spell->canBeCast(getCbc().get(), spells::Mode::CREATURE_ACTIVE, stack))
 		{
-			//spellcast may finish battle or kill active stack
-			//send special preudo-action
-			BattleAction cancel;
-			cancel.actionType = EActionType::CANCEL;
-			return cancel;
-		}
+			std::vector<PossibleSpellcast> possibleCasts;
+			spells::BattleCast temp(getCbc().get(), stack, spells::Mode::CREATURE_ACTIVE, spell);
+			for(auto & target : temp.findPotentialTargets())
+			{
+				PossibleSpellcast ps;
+				ps.dest = target;
+				ps.spell = spell;
+				evaluateCreatureSpellcast(stack, ps);
+				possibleCasts.push_back(ps);
+			}
 
-		if(auto action = considerFleeingOrSurrendering())
-			return *action;
-		//best action is from effective owner point if view, we are effective owner as we received "activeStack"
+			std::sort(possibleCasts.begin(), possibleCasts.end(), [&](const PossibleSpellcast & lhs, const PossibleSpellcast & rhs) { return lhs.value > rhs.value; });
+			if(!possibleCasts.empty() && possibleCasts.front().value > 0)
+			{
+				return possibleCasts.front();
+			}
+		}
+	}
+	return std::nullopt;
+}
 
+BattleAction CBattleAI::selectStackAction(const CStack * stack)
+{
+	//evaluate casting spell for spellcasting stack
+	std::optional<PossibleSpellcast> bestSpellcast = findBestCreatureSpell(stack);
 
-		//evaluate casting spell for spellcasting stack
-		std::optional<PossibleSpellcast> bestSpellcast(std::nullopt);
-		//TODO: faerie dragon type spell should be selected by server
-		SpellID creatureSpellToCast = cb->battleGetRandomStackSpell(CRandomGenerator::getDefault(), stack, CBattleInfoCallback::RANDOM_AIMED);
-		if(stack->hasBonusOfType(BonusType::SPELLCASTER) && stack->canCast() && creatureSpellToCast != SpellID::NONE)
-		{
-			const CSpell * spell = creatureSpellToCast.toSpell();
+	HypotheticBattle hb(env.get(), cb);
 
-			if(spell->canBeCast(getCbc().get(), spells::Mode::CREATURE_ACTIVE, stack))
-			{
-				std::vector<PossibleSpellcast> possibleCasts;
-				spells::BattleCast temp(getCbc().get(), stack, spells::Mode::CREATURE_ACTIVE, spell);
-				for(auto & target : temp.findPotentialTargets())
-				{
-					PossibleSpellcast ps;
-					ps.dest = target;
-					ps.spell = spell;
-					evaluateCreatureSpellcast(stack, ps);
-					possibleCasts.push_back(ps);
-				}
+	PotentialTargets targets(stack, hb);
+	BattleExchangeEvaluator scoreEvaluator(cb, env);
+	auto moveTarget = scoreEvaluator.findMoveTowardsUnreachable(stack, targets, hb);
 
-				std::sort(possibleCasts.begin(), possibleCasts.end(), [&](const PossibleSpellcast & lhs, const PossibleSpellcast & rhs) { return lhs.value > rhs.value; });
-				if(!possibleCasts.empty() && possibleCasts.front().value > 0)
-				{
-					bestSpellcast = std::optional<PossibleSpellcast>(possibleCasts.front());
-				}
-			}
-		}
+	int64_t score = EvaluationResult::INEFFECTIVE_SCORE;
 
-		HypotheticBattle hb(env.get(), cb);
-		
-		PotentialTargets targets(stack, hb);
-		BattleExchangeEvaluator scoreEvaluator(cb, env);
-		auto moveTarget = scoreEvaluator.findMoveTowardsUnreachable(stack, targets, hb);
 
-		int64_t score = EvaluationResult::INEFFECTIVE_SCORE;
+	if(targets.possibleAttacks.empty() && bestSpellcast.has_value())
+	{
+		movesSkippedByDefense = 0;
+		return BattleAction::makeCreatureSpellcast(stack, bestSpellcast->dest, bestSpellcast->spell->id);
+	}
 
-		if(!targets.possibleAttacks.empty())
-		{
+	if(!targets.possibleAttacks.empty())
+	{
 #if BATTLE_TRACE_LEVEL>=1
-			logAi->trace("Evaluating attack for %s", stack->getDescription());
+		logAi->trace("Evaluating attack for %s", stack->getDescription());
 #endif
 
-			auto evaluationResult = scoreEvaluator.findBestTarget(stack, targets, hb);
-			auto & bestAttack = evaluationResult.bestAttack;
+		auto evaluationResult = scoreEvaluator.findBestTarget(stack, targets, hb);
+		auto & bestAttack = evaluationResult.bestAttack;
 
-			//TODO: consider more complex spellcast evaluation, f.e. because "re-retaliation" during enemy move in same turn for melee attack etc.
-			if(bestSpellcast.has_value() && bestSpellcast->value > bestAttack.damageDiff())
-			{
-				// return because spellcast value is damage dealt and score is dps reduce
-				movesSkippedByDefense = 0;
-				return BattleAction::makeCreatureSpellcast(stack, bestSpellcast->dest, bestSpellcast->spell->id);
-			}
+		//TODO: consider more complex spellcast evaluation, f.e. because "re-retaliation" during enemy move in same turn for melee attack etc.
+		if(bestSpellcast.has_value() && bestSpellcast->value > bestAttack.damageDiff())
+		{
+			// return because spellcast value is damage dealt and score is dps reduce
+			movesSkippedByDefense = 0;
+			return BattleAction::makeCreatureSpellcast(stack, bestSpellcast->dest, bestSpellcast->spell->id);
+		}
 
-			if(evaluationResult.score > score)
+		if(evaluationResult.score > score)
+		{
+			score = evaluationResult.score;
+
+			logAi->debug("BattleAI: %s -> %s x %d, from %d curpos %d dist %d speed %d: +%lld -%lld = %lld",
+				bestAttack.attackerState->unitType()->getJsonKey(),
+				bestAttack.affectedUnits[0]->unitType()->getJsonKey(),
+				(int)bestAttack.affectedUnits[0]->getCount(),
+				(int)bestAttack.from,
+				(int)bestAttack.attack.attacker->getPosition().hex,
+				bestAttack.attack.chargeDistance,
+				bestAttack.attack.attacker->speed(0, true),
+				bestAttack.defenderDamageReduce,
+				bestAttack.attackerDamageReduce, bestAttack.attackValue()
+			);
+
+			if (moveTarget.score <= score)
 			{
-				score = evaluationResult.score;
-				std::string action;
-
 				if(evaluationResult.wait)
 				{
-					result = BattleAction::makeWait(stack);
-					action = "wait";
+					return BattleAction::makeWait(stack);
 				}
 				else if(bestAttack.attack.shooting)
 				{
-					result = BattleAction::makeShotAttack(stack, bestAttack.attack.defender);
-					action = "shot";
 					movesSkippedByDefense = 0;
+					return BattleAction::makeShotAttack(stack, bestAttack.attack.defender);
 				}
 				else
 				{
-					result = BattleAction::makeMeleeAttack(stack, bestAttack.attack.defender->getPosition(), bestAttack.from);
-					action = "melee";
 					movesSkippedByDefense = 0;
+					return BattleAction::makeMeleeAttack(stack, bestAttack.attack.defender->getPosition(), bestAttack.from);
 				}
-
-				logAi->debug("BattleAI: %s -> %s x %d, %s, from %d curpos %d dist %d speed %d: +%lld -%lld = %lld",
-					bestAttack.attackerState->unitType()->getJsonKey(),
-					bestAttack.affectedUnits[0]->unitType()->getJsonKey(),
-					(int)bestAttack.affectedUnits[0]->getCount(), action, (int)bestAttack.from, (int)bestAttack.attack.attacker->getPosition().hex,
-					bestAttack.attack.chargeDistance, bestAttack.attack.attacker->speed(0, true),
-					bestAttack.defenderDamageReduce, bestAttack.attackerDamageReduce, bestAttack.attackValue()
-				);
 			}
 		}
-		else if(bestSpellcast.has_value())
+	}
+
+	//ThreatMap threatsToUs(stack); // These lines may be usefull but they are't used in the code.
+	if(moveTarget.score > score)
+	{
+		score = moveTarget.score;
+
+		if(stack->waited())
 		{
-			movesSkippedByDefense = 0;
-			return BattleAction::makeCreatureSpellcast(stack, bestSpellcast->dest, bestSpellcast->spell->id);
+			return goTowardsNearest(stack, moveTarget.positions);
 		}
+		else
+		{
+			return BattleAction::makeWait(stack);
+		}
+	}
+
+	if(score <= EvaluationResult::INEFFECTIVE_SCORE
+		&& !stack->hasBonusOfType(BonusType::FLYING)
+		&& stack->unitSide() == BattleSide::ATTACKER
+		&& cb->battleGetSiegeLevel() >= CGTownInstance::CITADEL)
+	{
+		auto brokenWallMoat = getBrokenWallMoatHexes();
 
-			//ThreatMap threatsToUs(stack); // These lines may be usefull but they are't used in the code.
-		if(moveTarget.score > score)
+		if(brokenWallMoat.size())
 		{
-			score = moveTarget.score;
+			movesSkippedByDefense = 0;
 
-			if(stack->waited())
-			{
-				result = goTowardsNearest(stack, moveTarget.positions);
-			}
+			if(stack->doubleWide() && vstd::contains(brokenWallMoat, stack->getPosition()))
+				return BattleAction::makeMove(stack, stack->getPosition().cloneInDirection(BattleHex::RIGHT));
 			else
-			{
-				result = BattleAction::makeWait(stack);
-			}
+				return goTowardsNearest(stack, brokenWallMoat);
 		}
+	}
 
-		if(score <= EvaluationResult::INEFFECTIVE_SCORE
-			&& !stack->hasBonusOfType(BonusType::FLYING)
-			&& stack->unitSide() == BattleSide::ATTACKER
-			&& cb->battleGetSiegeLevel() >= CGTownInstance::CITADEL)
-		{
-			auto brokenWallMoat = getBrokenWallMoatHexes();
+	return BattleAction::makeDefend(stack);
+}
 
-			if(brokenWallMoat.size())
-			{
-				movesSkippedByDefense = 0;
+BattleAction CBattleAI::activeStack( const CStack * stack )
+{
+	LOG_TRACE_PARAMS(logAi, "stack: %s", stack->nodeName());
 
-				if(stack->doubleWide() && vstd::contains(brokenWallMoat, stack->getPosition()))
-					result = BattleAction::makeMove(stack, stack->getPosition().cloneInDirection(BattleHex::RIGHT));
-				else
-					result = goTowardsNearest(stack, brokenWallMoat);
-			}
+	BattleAction result = BattleAction::makeDefend(stack);
+	setCbc(cb); //TODO: make solid sure that AIs always use their callbacks (need to take care of event handlers too)
+
+	try
+	{
+		if(stack->creatureId() == CreatureID::CATAPULT)
+			return useCatapult(stack);
+		if(stack->hasBonusOfType(BonusType::SIEGE_WEAPON) && stack->hasBonusOfType(BonusType::HEALER))
+			return useHealingTent(stack);
+
+		attemptCastingSpell();
+
+		if(cb->battleIsFinished() || !stack->alive())
+		{
+			//spellcast may finish battle or kill active stack
+			//send special preudo-action
+			BattleAction cancel;
+			cancel.actionType = EActionType::CANCEL;
+			return cancel;
 		}
+
+		if(auto action = considerFleeingOrSurrendering())
+			return *action;
+
+		result = selectStackAction(stack);
 	}
 	catch(boost::thread_interrupted &)
 	{

AI/BattleAI/BattleAI.h

@@ -77,6 +77,10 @@ public:
 
 	void print(const std::string &text) const;
 	BattleAction useCatapult(const CStack *stack);
+	BattleAction useHealingTent(const CStack *stack);
+	BattleAction selectStackAction(const CStack * stack);
+	std::optional<PossibleSpellcast> findBestCreatureSpell(const CStack *stack);
+
 	void battleStart(const CCreatureSet * army1, const CCreatureSet * army2, int3 tile, const CGHeroInstance * hero1, const CGHeroInstance * hero2, bool Side) override;
 	//void actionFinished(const BattleAction &action) override;//occurs AFTER every action taken by any stack or by the hero
 	//void actionStarted(const BattleAction &action) override;//occurs BEFORE every action taken by any stack or by the hero

client/CPlayerInterface.cpp

@@ -774,6 +774,14 @@ BattleAction CPlayerInterface::activeStack(const CStack * stack) //called when i
 	logGlobal->trace("Awaiting command for %s", stack->nodeName());
 	auto stackId = stack->unitId();
 	auto stackName = stack->nodeName();
+
+	assert(!cb->battleIsFinished());
+	if (cb->battleIsFinished())
+	{
+		logGlobal->error("Received CPlayerInterface::activeStack after battle is finished!");
+		return BattleAction::makeDefend(stack);
+	}
+
 	if (autofightingAI)
 	{
 		if (isAutoFightOn)

client/Client.cpp

@@ -683,7 +683,6 @@ void CClient::stopPlayerBattleAction(PlayerColor color)
 		}
 		playerActionThreads.erase(color);
 	}
-
 }
 
 void CClient::stopAllBattleActions()