Significantly simplified threading model in battles

AI/BattleAI/BattleAI.cpp

@@ -88,7 +88,7 @@ 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;
 }
@@ -249,7 +249,7 @@ BattleAction CBattleAI::selectStackAction(const CStack * stack)
 	return BattleAction::makeDefend(stack);
 }
 
-BattleAction CBattleAI::activeStack( const CStack * stack )
+void CBattleAI::activeStack( const CStack * stack )
 {
 	LOG_TRACE_PARAMS(logAi, "stack: %s", stack->nodeName());
 
@@ -259,9 +259,15 @@ BattleAction CBattleAI::activeStack( const CStack * stack )
 	try
 	{
 		if(stack->creatureId() == CreatureID::CATAPULT)
-			return useCatapult(stack);
+		{
+			cb->battleMakeUnitAction(useCatapult(stack));
+			return;
+		}
 		if(stack->hasBonusOfType(BonusType::SIEGE_WEAPON) && stack->hasBonusOfType(BonusType::HEALER))
-			return useHealingTent(stack);
+		{
+			cb->battleMakeUnitAction(useHealingTent(stack));
+			return;
+		}
 
 		attemptCastingSpell();
 
@@ -271,11 +277,15 @@ BattleAction CBattleAI::activeStack( const CStack * stack )
 			//send special preudo-action
 			BattleAction cancel;
 			cancel.actionType = EActionType::CANCEL;
-			return cancel;
+			cb->battleMakeUnitAction(cancel);
+			return;
 		}
 
 		if(auto action = considerFleeingOrSurrendering())
-			return *action;
+		{
+			cb->battleMakeUnitAction(*action);
+			return;
+		}
 
 		result = selectStackAction(stack);
 	}
@@ -297,7 +307,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
@@ -751,7 +761,7 @@ void CBattleAI::attemptCastingSpell()
 		spellcast.setTarget(castToPerform.dest);
 		spellcast.side = side;
 		spellcast.stackNumber = (!side) ? -1 : -2;
-		cb->battleMakeAction(&spellcast);
+		cb->battleMakeUnitAction(spellcast);
 		movesSkippedByDefense = 0;
 	}
 	else

AI/BattleAI/BattleAI.h

@@ -71,7 +71,7 @@ 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
 
 	std::optional<BattleAction> considerFleeingOrSurrendering();
 

AI/StupidAI/StupidAI.cpp

@@ -88,7 +88,7 @@ static bool willSecondHexBlockMoreEnemyShooters(const BattleHex &h1, const Battl
 	return shooters[0] < shooters[1];
 }
 
-BattleAction CStupidAI::activeStack( const CStack * stack )
+void CStupidAI::activeStack( const CStack * stack )
 {
 	//boost::this_thread::sleep(boost::posix_time::seconds(2));
 	print("activeStack called for " + stack->nodeName());
@@ -105,11 +105,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 +153,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 +171,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,7 @@ 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 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

@@ -768,52 +768,34 @@ 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!");
-		return BattleAction::makeDefend(stack);
+
+		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;
-			}
-		}
+			autofightingAI->activeStack(stack);
 
 		cb->unregisterBattleInterface(autofightingAI);
 		autofightingAI.reset();
 	}
 
 	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));
 	}
 
 	{
@@ -821,29 +803,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)

client/CPlayerInterface.h

@@ -158,7 +158,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

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();
@@ -606,7 +603,7 @@ void CClient::battleStarted(const BattleInfo * info)
 
 	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 +617,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,56 +634,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));
 	}
 }
 
@@ -781,23 +709,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;
 
@@ -159,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);
@@ -230,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;
@@ -251,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,7 +154,6 @@ 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();
@@ -254,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
 	}
@@ -724,10 +718,7 @@ void BattleInterface::requestAutofightingAIToTakeAction()
 
 			// If enemy is moving, activeStack can be null
 			if (activeStack)
-			{
-				auto ba = std::make_unique<BattleAction>(curInt->autofightingAI->activeStack(activeStack));
-				givenCommand.setn(ba.release());
-			}
+				curInt->autofightingAI->activeStack(activeStack);
 
 			stacksController->setActiveStack(nullptr);
 		}

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

@@ -401,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);
 	}
 }

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,12 +152,12 @@ std::shared_ptr<scripting::Module> CDynLibHandler::getNewScriptingModule(const b
 }
 #endif
 
-BattleAction CGlobalAI::activeStack(const CStack * stack)
+void CGlobalAI::activeStack(const CStack * stack)
 {
 	BattleAction ba;
 	ba.actionType = EActionType::DEFEND;
 	ba.stackNumber = stack->unitId();
-	return ba;
+	assert(0);
 }
 
 CGlobalAI::CGlobalAI()
@@ -241,9 +241,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,7 +78,7 @@ 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 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
 };
@@ -132,7 +132,7 @@ class DLL_LINKAGE CGlobalAI : public CGameInterface // AI class (to derivate)
 public:
 	std::shared_ptr<Environment> env;
 	CGlobalAI();
-	virtual BattleAction activeStack(const CStack * stack) override;
+	virtual void activeStack(const CStack * stack) override;
 };
 
 //class to  be inherited by adventure-only AIs, it cedes battle actions to given battle-AI
@@ -147,7 +147,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;