Merge pull request #2359 from IvanSavenko/crashfixing

Fix crashes from 1.2.1 statistics on Google Play

AI/BattleAI/BattleAI.cpp

@@ -88,175 +88,211 @@ void CBattleAI::initBattleInterface(std::shared_ptr<Environment> ENV, std::share
 	playerID = *CB->getPlayerID(); //TODO should be sth in callback
 	wasWaitingForRealize = CB->waitTillRealize;
 	wasUnlockingGs = CB->unlockGsWhenWaiting;
-	CB->waitTillRealize = true;
+	CB->waitTillRealize = false;
 	CB->unlockGsWhenWaiting = false;
 	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
-		}
-
-		attemptCastingSpell();
+		const CSpell * spell = creatureSpellToCast.toSpell();
 
-		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);
 		}
+	}
+
+	return BattleAction::makeDefend(stack);
+}
+
+void CBattleAI::yourTacticPhase(int distance)
+{
+	cb->battleMakeUnitAction(BattleAction::makeEndOFTacticPhase(cb->battleGetTacticsSide()));
+}
+
+void CBattleAI::activeStack( const CStack * stack )
+{
+	LOG_TRACE_PARAMS(logAi, "stack: %s", stack->nodeName());
+
+	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(score <= EvaluationResult::INEFFECTIVE_SCORE
-			&& !stack->hasBonusOfType(BonusType::FLYING)
-			&& stack->unitSide() == BattleSide::ATTACKER
-			&& cb->battleGetSiegeLevel() >= CGTownInstance::CITADEL)
+	try
+	{
+		if(stack->creatureId() == CreatureID::CATAPULT)
 		{
-			auto brokenWallMoat = getBrokenWallMoatHexes();
+			cb->battleMakeUnitAction(useCatapult(stack));
+			return;
+		}
+		if(stack->hasBonusOfType(BonusType::SIEGE_WEAPON) && stack->hasBonusOfType(BonusType::HEALER))
+		{
+			cb->battleMakeUnitAction(useHealingTent(stack));
+			return;
+		}
 
-			if(brokenWallMoat.size())
-			{
-				movesSkippedByDefense = 0;
+		attemptCastingSpell();
 
-				if(stack->doubleWide() && vstd::contains(brokenWallMoat, stack->getPosition()))
-					result = BattleAction::makeMove(stack, stack->getPosition().cloneInDirection(BattleHex::RIGHT));
-				else
-					result = goTowardsNearest(stack, brokenWallMoat);
-			}
+		if(cb->battleIsFinished() || !stack->alive())
+		{
+			//spellcast may finish battle or kill active stack
+			//send special preudo-action
+			BattleAction cancel;
+			cancel.actionType = EActionType::CANCEL;
+			cb->battleMakeUnitAction(cancel);
+			return;
 		}
+
+		if(auto action = considerFleeingOrSurrendering())
+		{
+			cb->battleMakeUnitAction(*action);
+			return;
+		}
+
+		result = selectStackAction(stack);
 	}
 	catch(boost::thread_interrupted &)
 	{
@@ -276,7 +312,7 @@ BattleAction CBattleAI::activeStack( const CStack * stack )
 		movesSkippedByDefense = 0;
 	}
 
-	return result;
+	cb->battleMakeUnitAction(result);
 }
 
 BattleAction CBattleAI::goTowardsNearest(const CStack * stack, std::vector<BattleHex> hexes) const
@@ -730,7 +766,7 @@ void CBattleAI::attemptCastingSpell()
 		spellcast.setTarget(castToPerform.dest);
 		spellcast.side = side;
 		spellcast.stackNumber = (!side) ? -1 : -2;
-		cb->battleMakeAction(&spellcast);
+		cb->battleMakeSpellAction(spellcast);
 		movesSkippedByDefense = 0;
 	}
 	else

AI/BattleAI/BattleAI.h

@@ -71,12 +71,17 @@ public:
 
 	void evaluateCreatureSpellcast(const CStack * stack, PossibleSpellcast & ps); //for offensive damaging spells only
 
-	BattleAction activeStack(const CStack * stack) override; //called when it's turn of that stack
+	void activeStack(const CStack * stack) override; //called when it's turn of that stack
+	void yourTacticPhase(int distance) override;
 
 	std::optional<BattleAction> considerFleeingOrSurrendering();
 
 	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

AI/BattleAI/PotentialTargets.cpp

@@ -89,7 +89,7 @@ PotentialTargets::PotentialTargets(const battle::Unit * attacker, const Hypothet
 	{
 		auto & bestAp = possibleAttacks[0];
 
-		logGlobal->info("Battle AI best: %s -> %s at %d from %d, affects %d units: d:%lld a:%lld c:%lld s:%lld",
+		logGlobal->debug("Battle AI best: %s -> %s at %d from %d, affects %d units: d:%lld a:%lld c:%lld s:%lld",
 			bestAp.attack.attacker->unitType()->getJsonKey(),
 			state.battleGetUnitByPos(bestAp.dest)->unitType()->getJsonKey(),
 			(int)bestAp.dest, (int)bestAp.from, (int)bestAp.affectedUnits.size(),

AI/EmptyAI/CEmptyAI.cpp

@@ -11,6 +11,7 @@
 #include "CEmptyAI.h"
 
 #include "../../lib/CRandomGenerator.h"
+#include "../../lib/CStack.h"
 
 void CEmptyAI::saveGame(BinarySerializer & h, const int version)
 {
@@ -33,6 +34,16 @@ void CEmptyAI::yourTurn()
 	cb->endTurn();
 }
 
+void CEmptyAI::activeStack(const CStack * stack)
+{
+	cb->battleMakeUnitAction(BattleAction::makeDefend(stack));
+}
+
+void CEmptyAI::yourTacticPhase(int distance)
+{
+	cb->battleMakeUnitAction(BattleAction::makeEndOFTacticPhase(cb->battleGetTacticsSide()));
+}
+
 void CEmptyAI::heroGotLevel(const CGHeroInstance *hero, PrimarySkill::PrimarySkill pskill, std::vector<SecondarySkill> &skills, QueryID queryID)
 {
 	cb->selectionMade(CRandomGenerator::getDefault().nextInt((int)skills.size() - 1), queryID);

AI/EmptyAI/CEmptyAI.h

@@ -24,6 +24,8 @@ public:
 
 	void initGameInterface(std::shared_ptr<Environment> ENV, std::shared_ptr<CCallback> CB) override;
 	void yourTurn() override;
+	void yourTacticPhase(int distance) override;
+	void activeStack(const CStack * stack) override;
 	void heroGotLevel(const CGHeroInstance *hero, PrimarySkill::PrimarySkill pskill, std::vector<SecondarySkill> &skills, QueryID queryID) override;
 	void commanderGotLevel (const CCommanderInstance * commander, std::vector<ui32> skills, QueryID queryID) override;
 	void showBlockingDialog(const std::string &text, const std::vector<Component> &components, QueryID askID, const int soundID, bool selection, bool cancel) override;

AI/StupidAI/StupidAI.cpp

@@ -88,7 +88,12 @@ static bool willSecondHexBlockMoreEnemyShooters(const BattleHex &h1, const Battl
 	return shooters[0] < shooters[1];
 }
 
-BattleAction CStupidAI::activeStack( const CStack * stack )
+void CStupidAI::yourTacticPhase(int distance)
+{
+	cb->battleMakeUnitAction(BattleAction::makeEndOFTacticPhase(cb->battleGetTacticsSide()));
+}
+
+void CStupidAI::activeStack( const CStack * stack )
 {
 	//boost::this_thread::sleep(boost::posix_time::seconds(2));
 	print("activeStack called for " + stack->nodeName());
@@ -105,11 +110,13 @@ BattleAction CStupidAI::activeStack( const CStack * stack )
 		attack.side = side;
 		attack.stackNumber = stack->unitId();
 
-		return attack;
+		cb->battleMakeUnitAction(attack);
+		return;
 	}
 	else if(stack->hasBonusOfType(BonusType::SIEGE_WEAPON))
 	{
-		return BattleAction::makeDefend(stack);
+		cb->battleMakeUnitAction(BattleAction::makeDefend(stack));
+		return;
 	}
 
 	for (const CStack *s : cb->battleGetStacks(CBattleCallback::ONLY_ENEMY))
@@ -151,12 +158,14 @@ BattleAction CStupidAI::activeStack( const CStack * stack )
 	if(enemiesShootable.size())
 	{
 		const EnemyInfo &ei= *std::max_element(enemiesShootable.begin(), enemiesShootable.end(), isMoreProfitable);
-		return BattleAction::makeShotAttack(stack, ei.s);
+		cb->battleMakeUnitAction(BattleAction::makeShotAttack(stack, ei.s));
+		return;
 	}
 	else if(enemiesReachable.size())
 	{
 		const EnemyInfo &ei= *std::max_element(enemiesReachable.begin(), enemiesReachable.end(), &isMoreProfitable);
-		return BattleAction::makeMeleeAttack(stack, ei.s->getPosition(), *std::max_element(ei.attackFrom.begin(), ei.attackFrom.end(), &willSecondHexBlockMoreEnemyShooters));
+		cb->battleMakeUnitAction(BattleAction::makeMeleeAttack(stack, ei.s->getPosition(), *std::max_element(ei.attackFrom.begin(), ei.attackFrom.end(), &willSecondHexBlockMoreEnemyShooters)));
+		return;
 	}
 	else if(enemiesUnreachable.size()) //due to #955 - a buggy battle may occur when there are no enemies
 	{
@@ -167,11 +176,13 @@ BattleAction CStupidAI::activeStack( const CStack * stack )
 
 		if(dists.distToNearestNeighbour(stack, closestEnemy->s) < GameConstants::BFIELD_SIZE)
 		{
-			return goTowards(stack, closestEnemy->s->getAttackableHexes(stack));
+			cb->battleMakeUnitAction(goTowards(stack, closestEnemy->s->getAttackableHexes(stack)));
+			return;
 		}
 	}
 
-	return BattleAction::makeDefend(stack);
+	cb->battleMakeUnitAction(BattleAction::makeDefend(stack));
+	return;
 }
 
 void CStupidAI::battleAttack(const BattleAttack *ba)

AI/StupidAI/StupidAI.h

@@ -28,7 +28,8 @@ public:
 	void initBattleInterface(std::shared_ptr<Environment> ENV, std::shared_ptr<CBattleCallback> CB) 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
-	BattleAction activeStack(const CStack * stack) override; //called when it's turn of that stack
+	void activeStack(const CStack * stack) override; //called when it's turn of that stack
+	void yourTacticPhase(int distance) override;
 
 	void battleAttack(const BattleAttack *ba) override; //called when stack is performing attack
 	void battleStacksAttacked(const std::vector<BattleStackAttacked> & bsa, bool ranged) override; //called when stack receives damage (after battleAttack())

CCallback.cpp

@@ -203,12 +203,11 @@ bool CCallback::buildBuilding(const CGTownInstance *town, BuildingID buildingID)
 	return true;
 }
 
-int CBattleCallback::battleMakeAction(const BattleAction * action)
+void CBattleCallback::battleMakeSpellAction(const BattleAction & action)
 {
-	assert(action->actionType == EActionType::HERO_SPELL);
-	MakeCustomAction mca(*action);
+	assert(action.actionType == EActionType::HERO_SPELL);
+	MakeCustomAction mca(action);
 	sendRequest(&mca);
-	return 0;
 }
 
 int CBattleCallback::sendRequest(const CPackForServer * request)
@@ -378,13 +377,20 @@ CBattleCallback::CBattleCallback(std::optional<PlayerColor> Player, CClient * C)
 	cl = C;
 }
 
-bool CBattleCallback::battleMakeTacticAction( BattleAction * action )
+void CBattleCallback::battleMakeUnitAction(const BattleAction & action)
+{
+	assert(!cl->gs->curB->tacticDistance);
+	MakeAction ma;
+	ma.ba = action;
+	sendRequest(&ma);
+}
+
+void CBattleCallback::battleMakeTacticAction( const BattleAction & action )
 {
 	assert(cl->gs->curB->tacticDistance);
 	MakeAction ma;
-	ma.ba = *action;
+	ma.ba = action;
 	sendRequest(&ma);
-	return true;
 }
 
 std::optional<BattleAction> CBattleCallback::makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState)

CCallback.h

@@ -50,11 +50,12 @@ class IBattleCallback
 public:
 	virtual ~IBattleCallback() = default;
 
-	bool waitTillRealize; //if true, request functions will return after they are realized by server
-	bool unlockGsWhenWaiting;//if true after sending each request, gs mutex will be unlocked so the changes can be applied; NOTICE caller must have gs mx locked prior to any call to actiob callback!
+	bool waitTillRealize = false; //if true, request functions will return after they are realized by server
+	bool unlockGsWhenWaiting = false;//if true after sending each request, gs mutex will be unlocked so the changes can be applied; NOTICE caller must have gs mx locked prior to any call to actiob callback!
 	//battle
-	virtual int battleMakeAction(const BattleAction * action) = 0;//for casting spells by hero - DO NOT use it for moving active stack
-	virtual bool battleMakeTacticAction(BattleAction * action) = 0; // performs tactic phase actions
+	virtual void battleMakeSpellAction(const BattleAction & action) = 0;
+	virtual void battleMakeUnitAction(const BattleAction & action) = 0;
+	virtual void battleMakeTacticAction(const BattleAction & action) = 0;
 	virtual std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState) = 0;
 };
 
@@ -115,8 +116,9 @@ protected:
 
 public:
 	CBattleCallback(std::optional<PlayerColor> Player, CClient * C);
-	int battleMakeAction(const BattleAction * action) override;//for casting spells by hero - DO NOT use it for moving active stack
-	bool battleMakeTacticAction(BattleAction * action) override; // performs tactic phase actions
+	void battleMakeSpellAction(const BattleAction & action) override;//for casting spells by hero - DO NOT use it for moving active stack
+	void battleMakeUnitAction(const BattleAction & action) override;
+	void battleMakeTacticAction(const BattleAction & action) override; // performs tactic phase actions
 	std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState) override;
 
 #if SCRIPTING_ENABLED

client/CPlayerInterface.cpp

@@ -657,7 +657,7 @@ void CPlayerInterface::battleStart(const CCreatureSet *army1, const CCreatureSet
 	//quick combat with neutral creatures only
 	auto * army2_object = dynamic_cast<const CGObjectInstance *>(army2);
 	if((!autoBattleResultRefused && !allowBattleReplay && army2_object
-		&& army2_object->getOwner() == PlayerColor::UNFLAGGABLE
+		&& (army2_object->getOwner() == PlayerColor::UNFLAGGABLE || army2_object->getOwner() == PlayerColor::NEUTRAL)
 		&& settings["adventure"]["quickCombat"].Bool())
 		|| settings["adventure"]["alwaysSkipCombat"].Bool())
 	{
@@ -785,27 +785,29 @@ void CPlayerInterface::actionFinished(const BattleAction &action)
 	curAction = nullptr;
 }
 
-BattleAction CPlayerInterface::activeStack(const CStack * stack) //called when it's turn of that stack
+void CPlayerInterface::activeStack(const CStack * stack) //called when it's turn of that stack
 {
-	THREAD_CREATED_BY_CLIENT;
+	EVENT_HANDLER_CALLED_BY_CLIENT;
 	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!");
+
+		cb->battleMakeUnitAction(BattleAction::makeDefend(stack));
+		return ;
+	}
+
 	if (autofightingAI)
 	{
 		if (isAutoFightOn)
 		{
-			auto ret = autofightingAI->activeStack(stack);
-
-			if(cb->battleIsFinished())
-			{
-				return BattleAction::makeDefend(stack); // battle finished with spellcast
-			}
-
-			if (isAutoFightOn)
-			{
-				return ret;
-			}
+			//FIXME: we want client rendering to proceed while AI is making actions
+			// so unlock mutex while AI is busy since this might take quite a while, especially if hero has many spells
+			auto unlockPim = vstd::makeUnlockGuard(*pim);
+			autofightingAI->activeStack(stack);
+			return;
 		}
 
 		cb->unregisterBattleInterface(autofightingAI);
@@ -813,16 +815,10 @@ BattleAction CPlayerInterface::activeStack(const CStack * stack) //called when i
 	}
 
 	assert(battleInt);
-
 	if(!battleInt)
 	{
-		return BattleAction::makeDefend(stack); // probably battle is finished already
-	}
-
-	if(BattleInterface::givenCommand.get())
-	{
-		logGlobal->error("Command buffer must be clean! (we don't want to use old command)");
-		vstd::clear_pointer(BattleInterface::givenCommand.data);
+		// probably battle is finished already
+		cb->battleMakeUnitAction(BattleAction::makeDefend(stack));
 	}
 
 	{
@@ -830,29 +826,6 @@ BattleAction CPlayerInterface::activeStack(const CStack * stack) //called when i
 		battleInt->stackActivated(stack);
 		//Regeneration & mana drain go there
 	}
-	//wait till BattleInterface sets its command
-	boost::unique_lock<boost::mutex> lock(BattleInterface::givenCommand.mx);
-	while(!BattleInterface::givenCommand.data)
-	{
-		BattleInterface::givenCommand.cond.wait(lock);
-		if (!battleInt) //battle ended while we were waiting for movement (eg. because of spell)
-			throw boost::thread_interrupted(); //will shut the thread peacefully
-	}
-
-	//tidy up
-	BattleAction ret = *(BattleInterface::givenCommand.data);
-	vstd::clear_pointer(BattleInterface::givenCommand.data);
-
-	if(ret.actionType == EActionType::CANCEL)
-	{
-		if(stackId != ret.stackNumber)
-			logGlobal->error("Not current active stack action canceled");
-		logGlobal->trace("Canceled command for %s", stackName);
-	}
-	else
-		logGlobal->trace("Giving command for %s", stackName);
-
-	return ret;
 }
 
 void CPlayerInterface::battleEnd(const BattleResult *br, QueryID queryID)
@@ -1004,15 +977,7 @@ void CPlayerInterface::battleGateStateChanged(const EGateState state)
 
 void CPlayerInterface::yourTacticPhase(int distance)
 {
-	THREAD_CREATED_BY_CLIENT;
-	while(battleInt && battleInt->tacticsMode)
-		boost::this_thread::sleep(boost::posix_time::millisec(1));
-}
-
-void CPlayerInterface::forceEndTacticPhase()
-{
-	if (battleInt)
-		battleInt->tacticsMode = false;
+	EVENT_HANDLER_CALLED_BY_CLIENT;
 }
 
 void CPlayerInterface::showInfoDialog(EInfoWindowMode type, const std::string &text, const std::vector<Component> & components, int soundID)

client/CPlayerInterface.h

@@ -157,7 +157,7 @@ protected: // Call-ins from server, should not be called directly, but only via
 	//for battles
 	void actionFinished(const BattleAction& action) override;//occurs AFTER action taken by active stack or by the hero
 	void actionStarted(const BattleAction& action) override;//occurs BEFORE action taken by active stack or by the hero
-	BattleAction activeStack(const CStack * stack) override; //called when it's turn of that stack
+	void activeStack(const CStack * stack) override; //called when it's turn of that stack
 	void battleAttack(const BattleAttack *ba) override; //stack performs attack
 	void battleEnd(const BattleResult *br, QueryID queryID) override; //end of battle
 	void battleNewRoundFirst(int round) override; //called at the beginning of each turn before changes are applied; used for HP regen handling
@@ -175,7 +175,6 @@ protected: // Call-ins from server, should not be called directly, but only via
 	void battleCatapultAttacked(const CatapultAttack & ca) override; //called when catapult makes an attack
 	void battleGateStateChanged(const EGateState state) override;
 	void yourTacticPhase(int distance) override;
-	void forceEndTacticPhase() override;
 
 public: // public interface for use by client via LOCPLINT access
 

client/CServerHandler.cpp

@@ -887,11 +887,6 @@ void CServerHandler::threadHandleConnection()
 			}
 		}
 	}
-	catch(...)
-	{
-		handleException();
-		throw;
-	}
 }
 
 void CServerHandler::visitForLobby(CPackForLobby & lobbyPack)

client/Client.cpp

@@ -373,9 +373,6 @@ void CClient::endGame()
 		logNetwork->info("Deleted mapHandler and gameState.");
 	}
 
-	//threads cleanup has to be after gs cleanup and before battleints cleanup to stop tacticThread
-	cleanThreads();
-
 	CPlayerInterface::battleInt.reset();
 	playerint.clear();
 	battleints.clear();
@@ -600,13 +597,20 @@ void CClient::battleStarted(const BattleInfo * info)
 	//Remove player interfaces for auto battle (quickCombat option)
 	if(att && att->isAutoFightOn)
 	{
+		if (att->cb->battleGetTacticDist())
+		{
+			auto side = att->cb->playerToSide(att->playerID);
+			auto action = BattleAction::makeEndOFTacticPhase(*side);
+			att->cb->battleMakeTacticAction(action);
+		}
+
 		att.reset();
 		def.reset();
 	}
 
 	if(!settings["session"]["headless"].Bool())
 	{
-		if(!!att || !!def)
+		if(att || def)
 		{
 			boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
 			CPlayerInterface::battleInt = std::make_shared<BattleInterface>(leftSide.armyObject, rightSide.armyObject, leftSide.hero, rightSide.hero, att, def);
@@ -620,35 +624,10 @@ void CClient::battleStarted(const BattleInfo * info)
 			CPlayerInterface::battleInt = std::make_shared<BattleInterface>(leftSide.armyObject, rightSide.armyObject, leftSide.hero, rightSide.hero, att, def, spectratorInt);
 		}
 	}
-
-	if(info->tacticDistance && vstd::contains(battleints, info->sides[info->tacticsSide].color))
-	{
-		PlayerColor color = info->sides[info->tacticsSide].color;
-		playerTacticThreads[color] = std::make_unique<boost::thread>(&CClient::commenceTacticPhaseForInt, this, battleints[color]);
-	}
-}
-
-void CClient::commenceTacticPhaseForInt(std::shared_ptr<CBattleGameInterface> battleInt)
-{
-	setThreadName("CClient::commenceTacticPhaseForInt");
-	try
-	{
-		battleInt->yourTacticPhase(gs->curB->tacticDistance);
-		if(gs && !!gs->curB && gs->curB->tacticDistance) //while awaiting for end of tactics phase, many things can happen (end of battle... or game)
-		{
-			MakeAction ma(BattleAction::makeEndOFTacticPhase(gs->curB->playerToSide(battleInt->playerID).value()));
-			sendRequest(&ma, battleInt->playerID);
-		}
-	}
-	catch(...)
-	{
-		handleException();
-	}
 }
 
 void CClient::battleFinished()
 {
-	stopAllBattleActions();
 	for(auto & side : gs->curB->sides)
 		if(battleCallbacks.count(side.color))
 			battleCallbacks[side.color]->setBattle(nullptr);
@@ -662,57 +641,12 @@ void CClient::battleFinished()
 
 void CClient::startPlayerBattleAction(PlayerColor color)
 {
-	stopPlayerBattleAction(color);
+	assert(vstd::contains(battleints, color));
 
 	if(vstd::contains(battleints, color))
 	{
-		auto thread = std::make_shared<boost::thread>(std::bind(&CClient::waitForMoveAndSend, this, color));
-		playerActionThreads[color] = thread;
-	}
-}
-
-void CClient::stopPlayerBattleAction(PlayerColor color)
-{
-	if(vstd::contains(playerActionThreads, color))
-	{
-		auto thread = playerActionThreads.at(color);
-		if(thread->joinable())
-		{
-			thread->interrupt();
-			thread->join();
-		}
-		playerActionThreads.erase(color);
-	}
-
-}
-
-void CClient::stopAllBattleActions()
-{
-	while(!playerActionThreads.empty())
-		stopPlayerBattleAction(playerActionThreads.begin()->first);
-}
-
-void CClient::waitForMoveAndSend(PlayerColor color)
-{
-	try
-	{
-		setThreadName("CClient::waitForMoveAndSend");
 		assert(vstd::contains(battleints, color));
-		BattleAction ba = battleints[color]->activeStack(gs->curB->battleGetStackByID(gs->curB->activeStack, false));
-		if(ba.actionType != EActionType::CANCEL)
-		{
-			logNetwork->trace("Send battle action to server: %s", ba.toString());
-			MakeAction temp_action(ba);
-			sendRequest(&temp_action, color);
-		}
-	}
-	catch(boost::thread_interrupted &)
-	{
-		logNetwork->debug("Wait for move thread was interrupted and no action will be send. Was a battle ended by spell?");
-	}
-	catch(...)
-	{
-		handleException();
+		battleints[color]->activeStack(gs->curB->battleGetStackByID(gs->curB->activeStack, false));
 	}
 }
 
@@ -782,23 +716,6 @@ void CClient::removeGUI()
 	LOCPLINT = nullptr;
 }
 
-void CClient::cleanThreads()
-{
-	stopAllBattleActions();
-
-	while (!playerTacticThreads.empty())
-	{
-		PlayerColor color = playerTacticThreads.begin()->first;
-
-		//set tacticcMode of the players to false to stop tacticThread
-		if (vstd::contains(battleints, color))
-			battleints[color]->forceEndTacticPhase();
-
-		playerTacticThreads[color]->join();
-		playerTacticThreads.erase(color);
-	}
-}
-
 #ifdef VCMI_ANDROID
 #ifndef SINGLE_PROCESS_APP
 extern "C" JNIEXPORT void JNICALL Java_eu_vcmi_vcmi_NativeMethods_notifyServerClosed(JNIEnv * env, jclass cls)

client/Client.h

@@ -15,22 +15,14 @@
 #include "../lib/IGameCallback.h"
 #include "../lib/battle/BattleAction.h"
 #include "../lib/battle/CBattleInfoCallback.h"
-#include "../lib/CStopWatch.h"
-#include "../lib/int3.h"
-#include "../lib/CondSh.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
 struct CPack;
 struct CPackForServer;
-class CampaignState;
-class IGameEventsReceiver;
 class IBattleEventsReceiver;
 class CBattleGameInterface;
-class CGameState;
 class CGameInterface;
-class BattleAction;
-struct CPathsInfo;
 class BinaryDeserializer;
 class BinarySerializer;
 
@@ -60,9 +52,8 @@ namespace boost { class thread; }
 template<typename T>
 class ThreadSafeVector
 {
-	using TVector = std::vector<T>;
 	using TLock = boost::unique_lock<boost::mutex>;
-	TVector items;
+	std::vector<T> items;
 	boost::mutex mx;
 	boost::condition_variable cond;
 
@@ -81,28 +72,16 @@ public:
 		cond.notify_all();
 	}
 
-// 	//to access list, caller must present a lock used to lock mx
-// 	TVector &getList(TLock &lockedLock)
-// 	{
-// 		assert(lockedLock.owns_lock() && lockedLock.mutex() == &mx);
-// 		return items;
-// 	}
-
-	TLock getLock()
-	{
-		return TLock(mx);
-	}
-
 	void waitWhileContains(const T & item)
 	{
-		auto lock = getLock();
+		TLock lock(mx);
 		while(vstd::contains(items, item))
 			cond.wait(lock);
 	}
 
 	bool tryRemovingElement(const T & item) //returns false if element was not present
 	{
-		auto lock = getLock();
+		TLock lock(mx);
 		auto itr = vstd::find(items, item);
 		if(itr == items.end()) //not in container
 		{
@@ -172,11 +151,8 @@ public:
 	int sendRequest(const CPackForServer * request, PlayerColor player); //returns ID given to that request
 
 	void battleStarted(const BattleInfo * info);
-	void commenceTacticPhaseForInt(std::shared_ptr<CBattleGameInterface> battleInt); //will be called as separate thread
 	void battleFinished();
 	void startPlayerBattleAction(PlayerColor color);
-	void stopPlayerBattleAction(PlayerColor color);
-	void stopAllBattleActions();
 
 	void invalidatePaths();
 	std::shared_ptr<const CPathsInfo> getPathsInfo(const CGHeroInstance * h);
@@ -243,8 +219,6 @@ public:
 	void showInfoDialog(const std::string & msg, PlayerColor player) override {};
 	void removeGUI();
 
-	void cleanThreads();
-
 #if SCRIPTING_ENABLED
 	scripting::Pool * getGlobalContextPool() const override;
 	scripting::Pool * getContextPool() const override;
@@ -264,10 +238,5 @@ private:
 	mutable boost::mutex pathCacheMutex;
 	std::map<const CGHeroInstance *, std::shared_ptr<CPathsInfo>> pathCache;
 
-	std::map<PlayerColor, std::shared_ptr<boost::thread>> playerActionThreads;
-
-	std::map<PlayerColor, std::unique_ptr<boost::thread>> playerTacticThreads;
-
-	void waitForMoveAndSend(PlayerColor color);
 	void reinitScripting();
 };

client/battle/BattleActionsController.cpp

@@ -298,7 +298,7 @@ void BattleActionsController::castThisSpell(SpellID spellID)
 	if (spellSelMode.get() == PossiblePlayerBattleAction::NO_LOCATION) //user does not have to select location
 	{
 		heroSpellToCast->aimToHex(BattleHex::INVALID);
-		owner.curInt->cb->battleMakeAction(heroSpellToCast.get());
+		owner.curInt->cb->battleMakeSpellAction(*heroSpellToCast);
 		endCastingSpell();
 	}
 	else
@@ -673,7 +673,7 @@ void BattleActionsController::actionRealize(PossiblePlayerBattleAction action, B
 			BattleHex attackFromHex = owner.fieldController->fromWhichHexAttack(targetHex);
 			if(attackFromHex.isValid()) //we can be in this line when unreachable creature is L - clicked (as of revision 1308)
 			{
-				auto command = new BattleAction(BattleAction::makeMeleeAttack(owner.stacksController->getActiveStack(), targetHex, attackFromHex, returnAfterAttack));
+				BattleAction command = BattleAction::makeMeleeAttack(owner.stacksController->getActiveStack(), targetHex, attackFromHex, returnAfterAttack);
 				owner.sendCommand(command, owner.stacksController->getActiveStack());
 			}
 			return;
@@ -763,7 +763,7 @@ void BattleActionsController::actionRealize(PossiblePlayerBattleAction action, B
 						heroSpellToCast->aimToHex(targetHex);
 						break;
 				}
-				owner.curInt->cb->battleMakeAction(heroSpellToCast.get());
+				owner.curInt->cb->battleMakeSpellAction(*heroSpellToCast);
 				endCastingSpell();
 			}
 			selectedStack = nullptr;

client/battle/BattleInterface.cpp

@@ -44,8 +44,6 @@
 #include "../../lib/UnlockGuard.h"
 #include "../../lib/TerrainHandler.h"
 
-CondSh<BattleAction *> BattleInterface::givenCommand(nullptr);
-
 BattleInterface::BattleInterface(const CCreatureSet *army1, const CCreatureSet *army2,
 		const CGHeroInstance *hero1, const CGHeroInstance *hero2,
 		std::shared_ptr<CPlayerInterface> att,
@@ -68,8 +66,6 @@ BattleInterface::BattleInterface(const CCreatureSet *army1, const CCreatureSet *
 		curInt = defenderInt;
 	}
 
-	givenCommand.setn(nullptr);
-
 	//hot-seat -> check tactics for both players (defender may be local human)
 	if(attackerInt && attackerInt->cb->battleGetTacticDist())
 		tacticianInterface = attackerInt;
@@ -158,11 +154,11 @@ void BattleInterface::openingEnd()
 BattleInterface::~BattleInterface()
 {
 	CPlayerInterface::battleInt = nullptr;
-	givenCommand.cond.notify_all(); //that two lines should make any stacksController->getActiveStack() waiting thread to finish
 
 	if (adventureInt)
 		adventureInt->onAudioResumed();
 
+	awaitingEvents.clear();
 	onAnimationsFinished();
 }
 
@@ -253,29 +249,28 @@ void BattleInterface::giveCommand(EActionType action, BattleHex tile, si32 addit
 		return;
 	}
 
-	auto ba = new BattleAction(); //is deleted in CPlayerInterface::stacksController->getActiveStack()()
-	ba->side = side.value();
-	ba->actionType = action;
-	ba->aimToHex(tile);
-	ba->actionSubtype = additional;
+	BattleAction ba;
+	ba.side = side.value();
+	ba.actionType = action;
+	ba.aimToHex(tile);
+	ba.actionSubtype = additional;
 
 	sendCommand(ba, actor);
 }
 
-void BattleInterface::sendCommand(BattleAction *& command, const CStack * actor)
+void BattleInterface::sendCommand(BattleAction command, const CStack * actor)
 {
-	command->stackNumber = actor ? actor->unitId() : ((command->side == BattleSide::ATTACKER) ? -1 : -2);
+	command.stackNumber = actor ? actor->unitId() : ((command.side == BattleSide::ATTACKER) ? -1 : -2);
 
 	if(!tacticsMode)
 	{
 		logGlobal->trace("Setting command for %s", (actor ? actor->nodeName() : "hero"));
 		stacksController->setActiveStack(nullptr);
-		givenCommand.setn(command);
+		LOCPLINT->cb->battleMakeUnitAction(command);
 	}
 	else
 	{
 		curInt->cb->battleMakeTacticAction(command);
-		vstd::clear_pointer(command);
 		stacksController->setActiveStack(nullptr);
 		//next stack will be activated when action ends
 	}
@@ -634,6 +629,11 @@ void BattleInterface::tacticPhaseEnd()
 {
 	stacksController->setActiveStack(nullptr);
 	tacticsMode = false;
+
+	auto side = tacticianInterface->cb->playerToSide(tacticianInterface->playerID);
+	auto action = BattleAction::makeEndOFTacticPhase(*side);
+
+	tacticianInterface->cb->battleMakeTacticAction(action);
 }
 
 static bool immobile(const CStack *s)
@@ -701,38 +701,39 @@ void BattleInterface::requestAutofightingAIToTakeAction()
 {
 	assert(curInt->isAutoFightOn);
 
-	boost::thread aiThread([&]()
+	if(curInt->cb->battleIsFinished())
 	{
-		if(curInt->cb->battleIsFinished())
-		{
-			return; // battle finished with spellcast
-		}
+		return; // battle finished with spellcast
+	}
+
+	if (tacticsMode)
+	{
+		// Always end tactics mode. Player interface is blocked currently, so it's not possible that
+		// the AI can take any action except end tactics phase (AI actions won't be triggered)
+		//TODO implement the possibility that the AI will be triggered for further actions
+		//TODO any solution to merge tactics phase & normal phase in the way it is handled by the player and battle interface?
+		tacticPhaseEnd();
+		stacksController->setActiveStack(nullptr);
+	}
+	else
+	{
+		const CStack* activeStack = stacksController->getActiveStack();
 
-		if (tacticsMode)
+		// If enemy is moving, activeStack can be null
+		if (activeStack)
 		{
-			// Always end tactics mode. Player interface is blocked currently, so it's not possible that
-			// the AI can take any action except end tactics phase (AI actions won't be triggered)
-			//TODO implement the possibility that the AI will be triggered for further actions
-			//TODO any solution to merge tactics phase & normal phase in the way it is handled by the player and battle interface?
 			stacksController->setActiveStack(nullptr);
-			tacticsMode = false;
-		}
-		else
-		{
-			const CStack* activeStack = stacksController->getActiveStack();
 
-			// If enemy is moving, activeStack can be null
-			if (activeStack)
+			// FIXME: unsafe
+			// Run task in separate thread to avoid UI lock while AI is making turn (which might take some time)
+			// HOWEVER this thread won't atttempt to lock game state, potentially leading to races
+			boost::thread aiThread([&]()
 			{
-				auto ba = std::make_unique<BattleAction>(curInt->autofightingAI->activeStack(activeStack));
-				givenCommand.setn(ba.release());
-			}
-
-			stacksController->setActiveStack(nullptr);
+				curInt->autofightingAI->activeStack(activeStack);
+			});
+			aiThread.detach();
 		}
-	});
-
-	aiThread.detach();
+	}
 }
 
 void BattleInterface::castThisSpell(SpellID spellID)
@@ -781,8 +782,19 @@ void BattleInterface::onAnimationsFinished()
 
 void BattleInterface::waitForAnimations()
 {
-	auto unlockPim = vstd::makeUnlockGuard(*CPlayerInterface::pim);
-	ongoingAnimationsState.waitUntil(false);
+	{
+		auto unlockPim = vstd::makeUnlockGuard(*CPlayerInterface::pim);
+		ongoingAnimationsState.waitUntil(false);
+	}
+
+	assert(!hasAnimations());
+	assert(awaitingEvents.empty());
+
+	if (!awaitingEvents.empty())
+	{
+		logGlobal->error("Wait for animations finished but we still have awaiting events!");
+		awaitingEvents.clear();
+	}
 }
 
 bool BattleInterface::hasAnimations()

client/battle/BattleInterface.h

@@ -144,8 +144,6 @@ public:
 	std::shared_ptr<BattleHero> attackingHero;
 	std::shared_ptr<BattleHero> defendingHero;
 
-	static CondSh<BattleAction *> givenCommand; //data != nullptr if we have i.e. moved current unit
-
 	bool openingPlaying();
 	void openingEnd();
 
@@ -159,7 +157,7 @@ public:
 	void requestAutofightingAIToTakeAction();
 
 	void giveCommand(EActionType action, BattleHex tile = BattleHex(), si32 additional = -1);
-	void sendCommand(BattleAction *& command, const CStack * actor = nullptr);
+	void sendCommand(BattleAction command, const CStack * actor = nullptr);
 
 	const CGHeroInstance *getActiveHero(); //returns hero that can currently cast a spell
 

client/battle/BattleStacksController.cpp

@@ -375,13 +375,11 @@ void BattleStacksController::updateBattleAnimations(uint32_t msPassed)
 	tickFrameBattleAnimations(msPassed);
 	vstd::erase(currentAnimations, nullptr);
 
-	if (hadAnimations && currentAnimations.empty())
-	{
-		//stackAmountBoxHidden.clear();
+	if (currentAnimations.empty())
 		owner.executeStagedAnimations();
-		if (currentAnimations.empty())
-			owner.onAnimationsFinished();
-	}
+
+	if (hadAnimations && currentAnimations.empty())
+		owner.onAnimationsFinished();
 
 	initializeBattleAnimations();
 }
@@ -403,11 +401,12 @@ void BattleStacksController::stackRemoved(uint32_t stackID)
 {
 	if (getActiveStack() && getActiveStack()->unitId() == stackID)
 	{
-		BattleAction *action = new BattleAction();
-		action->side = owner.defendingHeroInstance ? (owner.curInt->playerID == owner.defendingHeroInstance->tempOwner) : false;
-		action->actionType = EActionType::CANCEL;
-		action->stackNumber = getActiveStack()->unitId();
-		owner.givenCommand.setn(action);
+		BattleAction action;
+		action.side = owner.defendingHeroInstance ? (owner.curInt->playerID == owner.defendingHeroInstance->tempOwner) : false;
+		action.actionType = EActionType::CANCEL;
+		action.stackNumber = getActiveStack()->unitId();
+
+		LOCPLINT->cb->battleMakeUnitAction(action);
 		setActiveStack(nullptr);
 	}
 }

client/gui/WindowHandler.cpp

@@ -99,6 +99,12 @@ bool WindowHandler::isTopWindow(IShowActivatable * window) const
 
 void WindowHandler::totalRedraw()
 {
+	totalRedrawRequested = true;
+}
+
+void WindowHandler::totalRedrawImpl()
+{
+	logGlobal->debug("totalRedraw requested!");
 	CSDL_Ext::fillSurface(screen2, Colors::BLACK);
 
 	Canvas target = Canvas::createFromSurface(screen2);
@@ -109,6 +115,16 @@ void WindowHandler::totalRedraw()
 }
 
 void WindowHandler::simpleRedraw()
+{
+	if (totalRedrawRequested)
+		totalRedrawImpl();
+	else
+		simpleRedrawImpl();
+
+	totalRedrawRequested = false;
+}
+
+void WindowHandler::simpleRedrawImpl()
 {
 	//update only top interface and draw background
 	if(windowsStack.size() > 1)

client/gui/WindowHandler.h

@@ -20,9 +20,17 @@ class WindowHandler
 	/// Temporary list of recently popped windows
 	std::vector<std::shared_ptr<IShowActivatable>> disposed;
 
+	bool totalRedrawRequested = false;
+
 	/// returns top windows
 	std::shared_ptr<IShowActivatable> topWindowImpl() const;
 
+	/// forces total redraw (using showAll), sets a flag, method gets called at the end of the rendering
+	void totalRedrawImpl();
+
+	/// update only top windows and draw background from buffer, sets a flag, method gets called at the end of the rendering
+	void simpleRedrawImpl();
+
 public:
 	/// forces total redraw (using showAll), sets a flag, method gets called at the end of the rendering
 	void totalRedraw();

client/mainmenu/CMainMenu.cpp

@@ -337,13 +337,7 @@ void CMainMenu::update()
 
 	// Handles mouse and key input
 	GH.handleEvents();
-
-	Canvas canvas = Canvas::createFromSurface(screen);
-
-	// check for null othervice crash on finishing a campaign
-	// /FIXME: find out why GH.windows().listInt is empty to begin with
-	if(GH.windows().topWindow<CIntObject>())
-		GH.windows().topWindow<CIntObject>()->show(canvas);
+	GH.windows().simpleRedraw();
 }
 
 void CMainMenu::openLobby(ESelectionScreen screenType, bool host, const std::vector<std::string> * names, ELoadMode loadMode)

client/mapView/MapRenderer.cpp

@@ -145,6 +145,13 @@ void MapRendererTerrain::renderTile(IMapRendererContext & context, Canvas & targ
 
 	const auto & image = storage.find(terrainIndex, rotationIndex, imageIndex);
 
+	assert(image);
+	if (!image)
+	{
+		logGlobal->error("Failed to find image %d for terrain %s on tile %s", imageIndex, mapTile.terType->getNameTranslated(), coordinates.toString());
+		return;
+	}
+
 	for( auto const & element : mapTile.terType->paletteAnimation)
 		image->shiftPalette(element.start, element.length, context.terrainImageIndex(element.length));
 

client/windows/GUIClasses.cpp

@@ -621,51 +621,6 @@ static bool isQuickExchangeLayoutAvailable()
 	return CResourceHandler::get()->existsResource(ResourceID(std::string("SPRITES/") + QUICK_EXCHANGE_BG, EResType::IMAGE));
 }
 
-// Runs a task asynchronously with gamestate locking and waitTillRealize set to true
-class GsThread
-{
-private:
-	std::function<void()> action;
-	std::shared_ptr<CCallback> cb;
-
-public:
-
-	static void run(std::function<void()> action)
-	{
-		std::shared_ptr<GsThread> instance(new GsThread(action));
-
-
-		boost::thread(std::bind(&GsThread::staticRun, instance));
-	}
-
-private:
-	GsThread(std::function<void()> action)
-		:action(action), cb(LOCPLINT->cb)
-	{
-	}
-
-	static void staticRun(std::shared_ptr<GsThread> instance)
-	{
-		instance->run();
-	}
-
-	void run()
-	{
-		boost::shared_lock<boost::shared_mutex> gsLock(CGameState::mutex);
-
-		auto originalWaitTillRealize = cb->waitTillRealize;
-		auto originalUnlockGsWhenWating = cb->unlockGsWhenWaiting;
-
-		cb->waitTillRealize = true;
-		cb->unlockGsWhenWaiting = true;
-
-		action();
-
-		cb->waitTillRealize = originalWaitTillRealize;
-		cb->unlockGsWhenWaiting = originalUnlockGsWhenWating;
-	}
-};
-
 CExchangeController::CExchangeController(CExchangeWindow * view, ObjectInstanceID hero1, ObjectInstanceID hero2)
 	:left(LOCPLINT->cb->getHero(hero1)), right(LOCPLINT->cb->getHero(hero2)), cb(LOCPLINT->cb), view(view)
 {
@@ -697,10 +652,7 @@ std::function<void()> CExchangeController::onSwapArtifacts()
 {
 	return [&]()
 	{
-		GsThread::run([=]
-		{
-			cb->bulkMoveArtifacts(left->id, right->id, true);
-		});
+		cb->bulkMoveArtifacts(left->id, right->id, true);
 	};
 }
 
@@ -725,39 +677,42 @@ std::function<void()> CExchangeController::onSwapArmy()
 {
 	return [&]()
 	{
-		GsThread::run([=]
+		if(left->tempOwner != cb->getMyColor()
+		   || right->tempOwner != cb->getMyColor())
 		{
-			if(left->tempOwner != cb->getMyColor()
-				|| right->tempOwner != cb->getMyColor())
-			{
-				return;
-			}
+			return;
+		}
 
-			auto leftSlots = getStacks(left);
-			auto rightSlots = getStacks(right);
+		auto leftSlots = getStacks(left);
+		auto rightSlots = getStacks(right);
 
-			auto i = leftSlots.begin(), j = rightSlots.begin();
+		auto i = leftSlots.begin(), j = rightSlots.begin();
 
-			for(; i != leftSlots.end() && j != rightSlots.end(); i++, j++)
-			{
-				cb->swapCreatures(left, right, i->first, j->first);
-			}
+		for(; i != leftSlots.end() && j != rightSlots.end(); i++, j++)
+		{
+			cb->swapCreatures(left, right, i->first, j->first);
+		}
+
+		if(i != leftSlots.end())
+		{
+			auto freeSlots = right->getFreeSlots();
+			auto slot = freeSlots.begin();
 
-			if(i != leftSlots.end())
+			for(; i != leftSlots.end() && slot != freeSlots.end(); i++, slot++)
 			{
-				for(; i != leftSlots.end(); i++)
-				{
-					cb->swapCreatures(left, right, i->first, right->getFreeSlot());
-				}
+				cb->swapCreatures(left, right, i->first, *slot);
 			}
-			else if(j != rightSlots.end())
+		}
+		else if(j != rightSlots.end())
+		{
+			auto freeSlots = left->getFreeSlots();
+			auto slot = freeSlots.begin();
+
+			for(; j != rightSlots.end() && slot != freeSlots.end(); j++, slot++)
 			{
-				for(; j != rightSlots.end(); j++)
-				{
-					cb->swapCreatures(left, right, left->getFreeSlot(), j->first);
-				}
+				cb->swapCreatures(left, right, *slot, j->first);
 			}
-		});
+		}
 	};
 }
 
@@ -854,10 +809,7 @@ void CExchangeController::moveArtifacts(bool leftToRight)
 		return;
 	}
 
-	GsThread::run([=]
-	{
-		cb->bulkMoveArtifacts(source->id, target->id, false);
-	});
+	cb->bulkMoveArtifacts(source->id, target->id, false);
 }
 
 void CExchangeController::moveArtifact(

config/gameConfig.json

@@ -310,7 +310,7 @@
 			// if enabled, neutral dwellings will accumulate creatures 
 			"accumulateWhenNeutral" : false,
 			// if enabled, dwellings owned by players will accumulate creatures 
-			"accumulateWhenOwned" : false
+			"accumulateWhenOwned" : false,
 			// if enabled, game will attempt to merge slots in army on recruit if all slots in hero army are in use
 			"mergeOnRecruit" : true
 		},

include/vcmi/spells/Caster.h

@@ -16,6 +16,7 @@ class PlayerColor;
 class MetaString;
 class ServerCallback;
 class CGHeroInstance;
+class Spell;
 
 namespace battle
 {

lib/CGameInterface.cpp

@@ -152,14 +152,6 @@ std::shared_ptr<scripting::Module> CDynLibHandler::getNewScriptingModule(const b
 }
 #endif
 
-BattleAction CGlobalAI::activeStack(const CStack * stack)
-{
-	BattleAction ba;
-	ba.actionType = EActionType::DEFEND;
-	ba.stackNumber = stack->unitId();
-	return ba;
-}
-
 CGlobalAI::CGlobalAI()
 {
 	human = false;
@@ -241,9 +233,9 @@ void CAdventureAI::battleUnitsChanged(const std::vector<UnitChanges> & units)
 	battleAI->battleUnitsChanged(units);
 }
 
-BattleAction CAdventureAI::activeStack(const CStack * stack)
+void CAdventureAI::activeStack(const CStack * stack)
 {
-	return battleAI->activeStack(stack);
+	battleAI->activeStack(stack);
 }
 
 void CAdventureAI::yourTacticPhase(int distance)

lib/CGameInterface.h

@@ -78,9 +78,8 @@ public:
 	virtual void initBattleInterface(std::shared_ptr<Environment> ENV, std::shared_ptr<CBattleCallback> CB){};
 
 	//battle call-ins
-	virtual BattleAction activeStack(const CStack * stack)=0; //called when it's turn of that stack
-	virtual void yourTacticPhase(int distance){}; //called when interface has opportunity to use Tactics skill -> use cb->battleMakeTacticAction from this function
-	virtual void forceEndTacticPhase(){}; //force the tatic phase to end to clean up the tactic phase thread
+	virtual void activeStack(const CStack * stack)=0; //called when it's turn of that stack
+	virtual void yourTacticPhase(int distance)=0; //called when interface has opportunity to use Tactics skill -> use cb->battleMakeTacticAction from this function
 };
 
 /// Central class for managing human player / AI interface logic
@@ -132,7 +131,6 @@ class DLL_LINKAGE CGlobalAI : public CGameInterface // AI class (to derivate)
 public:
 	std::shared_ptr<Environment> env;
 	CGlobalAI();
-	virtual BattleAction activeStack(const CStack * stack) override;
 };
 
 //class to  be inherited by adventure-only AIs, it cedes battle actions to given battle-AI
@@ -147,7 +145,7 @@ public:
 	virtual std::string getBattleAIName() const = 0; //has to return name of the battle AI to be used
 
 	//battle interface
-	virtual BattleAction activeStack(const CStack * stack) override;
+	virtual void activeStack(const CStack * stack) override;
 	virtual void yourTacticPhase(int distance) override;
 	virtual void battleNewRound(int round) override;
 	virtual void battleCatapultAttacked(const CatapultAttack & ca) override;

lib/NetPacksLib.cpp

@@ -1474,6 +1474,7 @@ void NewObject::applyGs(CGameState *gs)
 	terrainType = t.terType->getId();
 
 	auto handler = VLC->objtypeh->getHandlerFor(ID, subID);
+
 	CGObjectInstance * o = handler->create();
 	handler->configureObject(o, gs->getRandomGenerator());
 	
@@ -1491,6 +1492,11 @@ void NewObject::applyGs(CGameState *gs)
 	}
 
 	assert(!handler->getTemplates(terrainType).empty());
+	if (handler->getTemplates().empty())
+	{
+		logGlobal->error("Attempt to create object (%d %d) with no templates!", ID, subID);
+		return;
+	}
 
 	if (!handler->getTemplates(terrainType).empty())
 		o->appearance = handler->getTemplates(terrainType).front();