Code cleanup

AI/BattleAI/BattleAI.cpp

@@ -18,6 +18,7 @@
 #include "../../lib/mapObjects/CGTownInstance.h"
 #include "../../lib/spells/CSpellHandler.h"
 #include "../../lib/spells/ISpellMechanics.h"
+#include "../../lib/battle/BattleAction.h"
 #include "../../lib/battle/BattleStateInfoForRetreat.h"
 #include "../../lib/battle/CObstacleInstance.h"
 #include "../../lib/CStack.h" // TODO: remove
@@ -781,7 +782,7 @@ bool CBattleAI::attemptCastingSpell()
 		LOGFL("Best spell is %s (value %d). Will cast.", castToPerform.spell->getNameTranslated() % castToPerform.value);
 		BattleAction spellcast;
 		spellcast.actionType = EActionType::HERO_SPELL;
-		spellcast.actionSubtype = castToPerform.spell->id;
+		spellcast.spell = castToPerform.spell->getId();
 		spellcast.setTarget(castToPerform.dest);
 		spellcast.side = side;
 		spellcast.stackNumber = (!side) ? -1 : -2;

AI/EmptyAI/CEmptyAI.cpp

@@ -12,6 +12,7 @@
 
 #include "../../lib/CRandomGenerator.h"
 #include "../../lib/CStack.h"
+#include "../../lib/battle/BattleAction.h"
 
 void CEmptyAI::saveGame(BinarySerializer & h, const int version)
 {
@@ -73,3 +74,8 @@ void CEmptyAI::showMapObjectSelectDialog(QueryID askID, const Component & icon,
 {
 	cb->selectionMade(0, askID);
 }
+
+std::optional<BattleAction> CEmptyAI::makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState)
+{
+	return std::nullopt;
+}

AI/EmptyAI/CEmptyAI.h

@@ -32,6 +32,7 @@ public:
 	void showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
 	void showGarrisonDialog(const CArmedInstance *up, const CGHeroInstance *down, bool removableUnits, QueryID queryID) override;
 	void showMapObjectSelectDialog(QueryID askID, const Component & icon, const MetaString & title, const MetaString & description, const std::vector<ObjectInstanceID> & objects) override;
+	std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState) override;
 };
 
 #define NAME "EmptyAI 0.1"

AI/StupidAI/StupidAI.cpp

@@ -13,6 +13,7 @@
 #include "../../lib/CStack.h"
 #include "../../CCallback.h"
 #include "../../lib/CCreatureHandler.h"
+#include "../../lib/battle/BattleAction.h"
 
 static std::shared_ptr<CBattleCallback> cbc;
 

AI/VCAI/VCAI.cpp

@@ -2890,4 +2890,7 @@ bool shouldVisit(HeroPtr h, const CGObjectInstance * obj)
 	return true;
 }
 
-
+std::optional<BattleAction> VCAI::makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState)
+{
+	return std::nullopt;
+}

AI/VCAI/VCAI.h

@@ -203,6 +203,7 @@ public:
 
 	void battleStart(const CCreatureSet * army1, const CCreatureSet * army2, int3 tile, const CGHeroInstance * hero1, const CGHeroInstance * hero2, bool side, bool replayAllowed) override;
 	void battleEnd(const BattleResult * br, QueryID queryID) override;
+	std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState) override;
 
 	void makeTurn();
 	void mainLoop();

client/CPlayerInterface.cpp

@@ -128,7 +128,6 @@ CPlayerInterface::CPlayerInterface(PlayerColor Player):
 	destinationTeleportPos = int3(-1);
 	GH.defActionsDef = 0;
 	LOCPLINT = this;
-	curAction = nullptr;
 	playerID=Player;
 	human=true;
 	battleInt = nullptr;
@@ -769,8 +768,7 @@ void CPlayerInterface::actionStarted(const BattleAction &action)
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	BATTLE_EVENT_POSSIBLE_RETURN;
 
-	curAction = new BattleAction(action);
+	battleInt->startAction(action);
-	battleInt->startAction(curAction);
 }
 
 void CPlayerInterface::actionFinished(const BattleAction &action)
@@ -778,9 +776,7 @@ void CPlayerInterface::actionFinished(const BattleAction &action)
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	BATTLE_EVENT_POSSIBLE_RETURN;
 
-	battleInt->endAction(curAction);
+	battleInt->endAction(action);
-	delete curAction;
-	curAction = nullptr;
 }
 
 void CPlayerInterface::activeStack(const CStack * stack) //called when it's turn of that stack
@@ -935,8 +931,6 @@ void CPlayerInterface::battleAttack(const BattleAttack * ba)
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	BATTLE_EVENT_POSSIBLE_RETURN;
 
-	assert(curAction);
-
 	StackAttackInfo info;
 	info.attacker = cb->battleGetStackByID(ba->stackAttacking);
 	info.defender = nullptr;
@@ -2110,3 +2104,8 @@ void CPlayerInterface::showWorldViewEx(const std::vector<ObjectPosInfo>& objectP
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	adventureInt->openWorldView(objectPositions, showTerrain );
 }
+
+std::optional<BattleAction> CPlayerInterface::makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState)
+{
+	return std::nullopt;
+}

client/CPlayerInterface.h

@@ -66,7 +66,6 @@ class CPlayerInterface : public CGameInterface, public IUpdateable
 	int autosaveCount;
 
 	std::list<std::shared_ptr<CInfoWindow>> dialogs; //queue of dialogs awaiting to be shown (not currently shown!)
-	const BattleAction *curAction; //during the battle - action currently performed by active stack (or nullptr)
 
 	ObjectInstanceID destinationTeleport; //contain -1 or object id if teleportation
 	int3 destinationTeleportPos;
@@ -173,6 +172,7 @@ 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;
+	std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState) override;
 
 public: // public interface for use by client via LOCPLINT access
 

client/Client.h

@@ -13,7 +13,6 @@
 #include <vcmi/Environment.h>
 
 #include "../lib/IGameCallback.h"
-#include "../lib/battle/BattleAction.h"
 #include "../lib/battle/CBattleInfoCallback.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
@@ -25,6 +24,7 @@ class CBattleGameInterface;
 class CGameInterface;
 class BinaryDeserializer;
 class BinarySerializer;
+class BattleAction;
 
 template<typename T> class CApplier;
 
@@ -118,7 +118,7 @@ public:
 
 	std::map<PlayerColor, std::vector<std::shared_ptr<IBattleEventsReceiver>>> additionalBattleInts;
 
-	std::optional<BattleAction> curbaction;
+	std::unique_ptr<BattleAction> currentBattleAction;
 
 	CClient();
 	~CClient();

client/NetPacksClient.cpp

@@ -784,7 +784,7 @@ void ApplyClientNetPackVisitor::visitBattleAttack(BattleAttack & pack)
 
 void ApplyFirstClientNetPackVisitor::visitStartAction(StartAction & pack)
 {
-	cl.curbaction = std::make_optional(pack.ba);
+	cl.currentBattleAction = std::make_unique<BattleAction>(pack.ba);
 	callBattleInterfaceIfPresentForBothSides(cl, &IBattleEventsReceiver::actionStarted, pack.ba);
 }
 
@@ -830,8 +830,8 @@ void ApplyClientNetPackVisitor::visitCatapultAttack(CatapultAttack & pack)
 
 void ApplyClientNetPackVisitor::visitEndAction(EndAction & pack)
 {
-	callBattleInterfaceIfPresentForBothSides(cl, &IBattleEventsReceiver::actionFinished, *cl.curbaction);
+	callBattleInterfaceIfPresentForBothSides(cl, &IBattleEventsReceiver::actionFinished, *cl.currentBattleAction);
-	cl.curbaction.reset();
+	cl.currentBattleAction.reset();
 }
 
 void ApplyClientNetPackVisitor::visitPackageApplied(PackageApplied & pack)

client/battle/BattleActionsController.cpp

@@ -286,7 +286,7 @@ void BattleActionsController::castThisSpell(SpellID spellID)
 {
 	heroSpellToCast = std::make_shared<BattleAction>();
 	heroSpellToCast->actionType = EActionType::HERO_SPELL;
-	heroSpellToCast->actionSubtype = spellID; //spell number
+	heroSpellToCast->spell = spellID;
 	heroSpellToCast->stackNumber = (owner.attackingHeroInstance->tempOwner == owner.curInt->playerID) ? -1 : -2;
 	heroSpellToCast->side = owner.defendingHeroInstance ? (owner.curInt->playerID == owner.defendingHeroInstance->tempOwner) : false;
 
@@ -314,7 +314,7 @@ void BattleActionsController::castThisSpell(SpellID spellID)
 const CSpell * BattleActionsController::getHeroSpellToCast( ) const
 {
 	if (heroSpellToCast)
-		return SpellID(heroSpellToCast->actionSubtype).toSpell();
+		return heroSpellToCast->spell.toSpell();
 	return nullptr;
 }
 

client/battle/BattleEffectsController.cpp

@@ -94,13 +94,13 @@ void BattleEffectsController::battleTriggerEffect(const BattleTriggerEffect & bt
 	owner.waitForAnimations();
 }
 
-void BattleEffectsController::startAction(const BattleAction* action)
+void BattleEffectsController::startAction(const BattleAction & action)
 {
 	owner.checkForAnimations();
 
-	const CStack *stack = owner.curInt->cb->battleGetStackByID(action->stackNumber);
+	const CStack *stack = owner.curInt->cb->battleGetStackByID(action.stackNumber);
 
-	switch(action->actionType)
+	switch(action.actionType)
 	{
 	case EActionType::WAIT:
 		owner.appendBattleLog(stack->formatGeneralMessage(136));

client/battle/BattleEffectsController.h

@@ -60,7 +60,7 @@ public:
 
 	BattleEffectsController(BattleInterface & owner);
 
-	void startAction(const BattleAction* action);
+	void startAction(const BattleAction & action);
 
 	//displays custom effect on the battlefield
 	void displayEffect(EBattleEffect effect, const BattleHex & destTile);

client/battle/BattleInterface.cpp

@@ -234,7 +234,7 @@ void BattleInterface::newRound(int number)
 	console->addText(CGI->generaltexth->allTexts[412]);
 }
 
-void BattleInterface::giveCommand(EActionType action, BattleHex tile, si32 additional)
+void BattleInterface::giveCommand(EActionType action, BattleHex tile, SpellID spell)
 {
 	const CStack * actor = nullptr;
 	if(action != EActionType::HERO_SPELL && action != EActionType::RETREAT && action != EActionType::SURRENDER)
@@ -253,7 +253,7 @@ void BattleInterface::giveCommand(EActionType action, BattleHex tile, si32 addit
 	ba.side = side.value();
 	ba.actionType = action;
 	ba.aimToHex(tile);
-	ba.actionSubtype = additional;
+	ba.spell = spell;
 
 	sendCommand(ba, actor);
 }
@@ -567,12 +567,12 @@ bool BattleInterface::makingTurn() const
 	return stacksController->getActiveStack() != nullptr;
 }
 
-void BattleInterface::endAction(const BattleAction* action)
+void BattleInterface::endAction(const BattleAction &action)
 {
 	// it is possible that tactics mode ended while opening music is still playing
 	waitForAnimations();
 
-	const CStack *stack = curInt->cb->battleGetStackByID(action->stackNumber);
+	const CStack *stack = curInt->cb->battleGetStackByID(action.stackNumber);
 
 	// Activate stack from stackToActivate because this might have been temporary disabled, e.g., during spell cast
 	activateStack();
@@ -585,7 +585,7 @@ void BattleInterface::endAction(const BattleAction* action)
 		tacticNextStack(stack);
 
 	//we have activated next stack after sending request that has been just realized -> blockmap due to movement has changed
-	if(action->actionType == EActionType::HERO_SPELL)
+	if(action.actionType == EActionType::HERO_SPELL)
 		fieldController->redrawBackgroundWithHexes();
 }
 
@@ -594,15 +594,15 @@ void BattleInterface::appendBattleLog(const std::string & newEntry)
 	console->addText(newEntry);
 }
 
-void BattleInterface::startAction(const BattleAction* action)
+void BattleInterface::startAction(const BattleAction & action)
 {
-	if(action->actionType == EActionType::END_TACTIC_PHASE)
+	if(action.actionType == EActionType::END_TACTIC_PHASE)
 	{
 		windowObject->tacticPhaseEnded();
 		return;
 	}
 
-	const CStack *stack = curInt->cb->battleGetStackByID(action->stackNumber);
+	const CStack *stack = curInt->cb->battleGetStackByID(action.stackNumber);
 
 	if (stack)
 	{
@@ -610,17 +610,17 @@ void BattleInterface::startAction(const BattleAction* action)
 	}
 	else
 	{
-		assert(action->actionType == EActionType::HERO_SPELL); //only cast spell is valid action without acting stack number
+		assert(action.actionType == EActionType::HERO_SPELL); //only cast spell is valid action without acting stack number
 	}
 
 	stacksController->startAction(action);
 
-	if(action->actionType == EActionType::HERO_SPELL) //when hero casts spell
+	if(action.actionType == EActionType::HERO_SPELL) //when hero casts spell
 		return;
 
 	if (!stack)
 	{
-		logGlobal->error("Something wrong with stackNumber in actionStarted. Stack number: %d", action->stackNumber);
+		logGlobal->error("Something wrong with stackNumber in actionStarted. Stack number: %d", action.stackNumber);
 		return;
 	}
 

client/battle/BattleInterface.h

@@ -156,7 +156,7 @@ public:
 	void activateStack(); //sets activeStack to stackToActivate etc. //FIXME: No, it's not clear at all
 	void requestAutofightingAIToTakeAction();
 
-	void giveCommand(EActionType action, BattleHex tile = BattleHex(), si32 additional = -1);
+	void giveCommand(EActionType action, BattleHex tile = BattleHex(), SpellID spell = SpellID::NONE);
 	void sendCommand(BattleAction command, const CStack * actor = nullptr);
 
 	const CGHeroInstance *getActiveHero(); //returns hero that can currently cast a spell
@@ -188,7 +188,7 @@ public:
 	void addToAnimationStage( EAnimationEvents event, const AwaitingAnimationAction & action);
 
 	//call-ins
-	void startAction(const BattleAction* action);
+	void startAction(const BattleAction & action);
 	void stackReset(const CStack * stack);
 	void stackAdded(const CStack * stack); //new stack appeared on battlefield
 	void stackRemoved(uint32_t stackID); //stack disappeared from batlefiled
@@ -211,7 +211,7 @@ public:
 	void displaySpellEffect(const CSpell * spell, BattleHex destinationTile); //displays spell`s affected animation
 	void displaySpellHit(const CSpell * spell, BattleHex destinationTile); //displays spell`s affected animation
 
-	void endAction(const BattleAction* action);
+	void endAction(const BattleAction & action);
 
 	void obstaclePlaced(const std::vector<std::shared_ptr<const CObstacleInstance>> oi);
 	void obstacleRemoved(const std::vector<ObstacleChanges> & obstacles);

client/battle/BattleStacksController.cpp

@@ -31,6 +31,7 @@
 
 #include "../../CCallback.h"
 #include "../../lib/spells/ISpellMechanics.h"
+#include "../../lib/battle/BattleAction.h"
 #include "../../lib/battle/BattleHex.h"
 #include "../../lib/CStack.h"
 #include "../../lib/CondSh.h"
@@ -663,7 +664,7 @@ bool BattleStacksController::shouldRotate(const CStack * stack, const BattleHex
 	return false;
 }
 
-void BattleStacksController::endAction(const BattleAction* action)
+void BattleStacksController::endAction(const BattleAction & action)
 {
 	owner.checkForAnimations();
 
@@ -688,7 +689,7 @@ void BattleStacksController::endAction(const BattleAction* action)
 	removeExpiredColorFilters();
 }
 
-void BattleStacksController::startAction(const BattleAction* action)
+void BattleStacksController::startAction(const BattleAction & action)
 {
 	removeExpiredColorFilters();
 }

client/battle/BattleStacksController.h

@@ -115,8 +115,8 @@ public:
 	void stacksAreAttacked(std::vector<StackAttackedInfo> attackedInfos); //called when a certain amount of stacks has been attacked
 	void stackAttacking(const StackAttackInfo & info); //called when stack with id ID is attacking something on hex dest
 
-	void startAction(const BattleAction* action);
+	void startAction(const BattleAction & action);
-	void endAction(const BattleAction* action);
+	void endAction(const BattleAction & action);
 
 	void deactivateStack(); //copy activeStack to stackToActivate, then set activeStack to nullptr to temporary disable current stack
 

lib/CGameInterface.h

@@ -9,7 +9,6 @@
  */
 #pragma once
 
-#include "battle/BattleAction.h"
 #include "IGameEventsReceiver.h"
 
 #include "spells/ViewSpellInt.h"
@@ -36,6 +35,7 @@ class CCreatureSet;
 class CArmedInstance;
 class IShipyard;
 class IMarket;
+class BattleAction;
 struct BattleResult;
 struct BattleAttack;
 struct BattleStackAttacked;
@@ -107,10 +107,7 @@ public:
 
 	virtual void showWorldViewEx(const std::vector<ObjectPosInfo> & objectPositions, bool showTerrain){};
 
-	virtual std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState)
+	virtual std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleStateInfoForRetreat & battleState) = 0;
-	{
-		return std::nullopt;
-	}
 
 	virtual void saveGame(BinarySerializer & h, const int version) = 0;
 	virtual void loadGame(BinaryDeserializer & h, const int version) = 0;

lib/NetPacksLib.cpp

@@ -2289,7 +2289,7 @@ void StartAction::applyGs(CGameState *gs)
 	assert(st || heroAction); // stack must exists for all non-hero actions
 
 	if(ba.actionType == EActionType::HERO_SPELL)
-		gs->curB->sides[ba.side].usedSpellsHistory.emplace_back(ba.actionSubtype);
+		gs->curB->sides[ba.side].usedSpellsHistory.push_back(ba.spell);
 
 	switch(ba.actionType)
 	{

lib/battle/BattleAction.cpp

@@ -20,8 +20,7 @@ static const int32_t INVALID_UNIT_ID = -1000;
 BattleAction::BattleAction():
 	side(-1),
 	stackNumber(-1),
-	actionType(EActionType::NO_ACTION),
+	actionType(EActionType::NO_ACTION)
-	actionSubtype(-1)
 {
 }
 
@@ -80,7 +79,7 @@ BattleAction BattleAction::makeCreatureSpellcast(const battle::Unit * stack, con
 {
 	BattleAction ba;
 	ba.actionType = EActionType::MONSTER_SPELL;
-	ba.actionSubtype = spellID;
+	ba.spell = spellID;
 	ba.setTarget(target);
 	ba.side = stack->unitSide();
 	ba.stackNumber = stack->unitId();
@@ -144,7 +143,7 @@ std::string BattleAction::toString() const
 	}
 
 	boost::format fmt("{BattleAction: side '%d', stackNumber '%d', actionType '%s', actionSubtype '%d', target {%s}}");
-	fmt % static_cast<int>(side) % stackNumber % actionTypeStream.str() % actionSubtype % targetStream.str();
+	fmt % static_cast<int>(side) % stackNumber % actionTypeStream.str() % spell.getNum() % targetStream.str();
 	return fmt.str();
 }
 
@@ -193,6 +192,44 @@ void BattleAction::setTarget(const battle::Target & target_)
 	}
 }
 
+bool BattleAction::isUnitAction() const
+{
+	static const std::array<EActionType, 9> actions = {
+		EActionType::WALK,
+		EActionType::WAIT,
+		EActionType::DEFEND,
+		EActionType::WALK_AND_ATTACK,
+		EActionType::SHOOT,
+		EActionType::CATAPULT,
+		EActionType::MONSTER_SPELL,
+		EActionType::BAD_MORALE,
+		EActionType::STACK_HEAL
+	};
+
+	return vstd::contains(actions, actionType);
+}
+
+bool BattleAction::isSpellAction() const
+{
+	static const std::array<EActionType, 2> actions = {
+		EActionType::HERO_SPELL,
+		EActionType::MONSTER_SPELL
+	};
+
+	return vstd::contains(actions, actionType);
+}
+
+bool BattleAction::isTacticsAction() const
+{
+	static const std::array<EActionType, 9> actions = {
+		EActionType::WALK,
+		EActionType::END_TACTIC_PHASE,
+		EActionType::RETREAT,
+		EActionType::SURRENDER
+	};
+
+	return vstd::contains(actions, actionType);
+}
 
 std::ostream & operator<<(std::ostream & os, const BattleAction & ba)
 {

lib/battle/BattleAction.h

@@ -28,7 +28,7 @@ public:
 	ui32 stackNumber; //stack ID, -1 left hero, -2 right hero,
 	EActionType actionType; //use ActionType enum for values
 
-	si32 actionSubtype;
+	SpellID spell;
 
 	BattleAction();
 
@@ -43,6 +43,9 @@ public:
 	static BattleAction makeRetreat(ui8 side);
 	static BattleAction makeSurrender(ui8 side);
 
+	bool isTacticsAction() const;
+	bool isUnitAction() const;
+	bool isSpellAction() const;
 	std::string toString() const;
 
 	void aimToHex(const BattleHex & destination);
@@ -56,7 +59,7 @@ public:
 		h & side;
 		h & stackNumber;
 		h & actionType;
-		h & actionSubtype;
+		h & spell;
 		h & target;
 	}
 private:

server/battles/BattleActionProcessor.cpp

@@ -58,16 +58,21 @@ bool BattleActionProcessor::doEmptyAction(const BattleAction & ba)
 
 bool BattleActionProcessor::doEndTacticsAction(const BattleAction & ba)
 {
+	if (gameHandler->gameState()->curB->tacticDistance == 0)
+	{
+		gameHandler->complain("Cannot end tactics mode - no tactics!");
+		return false;
+	}
 	return true;
 }
 
 bool BattleActionProcessor::doWaitAction(const BattleAction & ba)
 {
-	return true;
+	const CStack * stack = gameHandler->battleGetStackByID(ba.stackNumber);
-}
+
+	if (!canStackAct(stack))
+		return false;
 
-bool BattleActionProcessor::doBadMoraleAction(const BattleAction & ba)
-{
 	return true;
 }
 
@@ -113,10 +118,10 @@ bool BattleActionProcessor::doHeroSpellAction(const BattleAction & ba)
 		return false;
 	}
 
-	const CSpell * s = SpellID(ba.actionSubtype).toSpell();
+	const CSpell * s = ba.spell.toSpell();
 	if (!s)
 	{
-		logGlobal->error("Wrong spell id (%d)!", ba.actionSubtype);
+		logGlobal->error("Wrong spell id (%d)!", ba.spell.getNum());
 		return false;
 	}
 
@@ -411,7 +416,7 @@ bool BattleActionProcessor::doUnitSpellAction(const BattleAction & ba)
 {
 	const CStack * stack = gameHandler->gameState()->curB->battleGetStackByID(ba.stackNumber);
 	battle::Target target = ba.getTarget(gameHandler->gameState()->curB);
-	SpellID spellID = SpellID(ba.actionSubtype);
+	SpellID spellID = ba.spell;
 
 	if (!canStackAct(stack))
 		return false;
@@ -479,8 +484,6 @@ bool BattleActionProcessor::doHealAction(const BattleAction & ba)
 
 bool BattleActionProcessor::canStackAct(const CStack * stack)
 {
-	const bool isAboutActiveStack = stack->unitId() == gameHandler->gameState()->curB->getActiveStackID();
-
 	if (!stack)
 	{
 		gameHandler->complain("No such stack!");
@@ -500,11 +503,14 @@ bool BattleActionProcessor::canStackAct(const CStack * stack)
 			return false;
 		}
 	}
-	else if (!isAboutActiveStack)
+	else
+	{
+		if (stack->unitId() != gameHandler->gameState()->curB->getActiveStackID())
 		{
 			gameHandler->complain("Action has to be about active stack!");
 			return false;
 		}
+	}
 	return true;
 }
 
@@ -536,8 +542,6 @@ bool BattleActionProcessor::dispatchBattleAction(const BattleAction & ba)
 			return doCatapultAction(ba);
 		case EActionType::MONSTER_SPELL:
 			return doUnitSpellAction(ba);
-		case EActionType::BAD_MORALE:
-			return doBadMoraleAction(ba);
 		case EActionType::STACK_HEAL:
 			return doHealAction(ba);
 	}
@@ -545,7 +549,7 @@ bool BattleActionProcessor::dispatchBattleAction(const BattleAction & ba)
 	return false;
 }
 
-bool BattleActionProcessor::makeBattleAction(const BattleAction &ba)
+bool BattleActionProcessor::makeBattleActionImpl(const BattleAction &ba)
 {
 	logGlobal->trace("Making action: %s", ba.toString());
 	const CStack * stack = gameHandler->gameState()->curB->battleGetStackByID(ba.stackNumber);
@@ -1394,3 +1398,53 @@ void BattleActionProcessor::addGenericKilledLog(BattleLogMessage & blm, const CS
 		blm.lines.push_back(std::move(line));
 	}
 }
+
+bool BattleActionProcessor::makeAutomaticBattleAction(const BattleAction & ba)
+{
+	return makeBattleActionImpl(ba);
+}
+
+bool BattleActionProcessor::makePlayerBattleAction(PlayerColor player, const BattleAction &ba)
+{
+	const BattleInfo * battle = gameHandler->gameState()->curB;
+
+	if(!battle && gameHandler->complain("Can not make action - there is no battle ongoing!"))
+		return false;
+
+	if (ba.side != 0 && ba.side != 1 && gameHandler->complain("Can not make action - invalid battle side!"))
+		return false;
+
+	if(battle->tacticDistance != 0)
+	{
+		if(!ba.isTacticsAction())
+		{
+			gameHandler->complain("Can not make actions while in tactics mode!");
+			return false;
+		}
+
+		if(player != battle->sides[ba.side].color)
+		{
+			gameHandler->complain("Can not make actions in battles you are not part of!");
+			return false;
+		}
+	}
+	else
+	{
+		if (ba.isUnitAction() && ba.stackNumber != battle->getActiveStackID())
+		{
+			gameHandler->complain("Can not make actions - stack is not active!");
+			return false;
+		}
+
+		auto active = battle->battleActiveUnit();
+		if(!active && gameHandler->complain("No active unit in battle!"))
+			return false;
+
+		auto unitOwner = battle->battleGetOwner(active);
+
+		if(player != unitOwner && gameHandler->complain("Can not make actions in battles you are not part of!"))
+			return false;
+	}
+
+	return makeBattleActionImpl(ba);
+}

server/battles/BattleActionProcessor.h

@@ -16,6 +16,7 @@ struct BattleAttack;
 class BattleAction;
 struct BattleHex;
 class CStack;
+class PlayerColor;
 enum class BonusType;
 
 namespace battle
@@ -64,14 +65,15 @@ class BattleActionProcessor : boost::noncopyable
 	bool doShootAction(const BattleAction & ba);
 	bool doCatapultAction(const BattleAction & ba);
 	bool doUnitSpellAction(const BattleAction & ba);
-	bool doBadMoraleAction(const BattleAction & ba);
 	bool doHealAction(const BattleAction & ba);
 
 	bool dispatchBattleAction(const BattleAction & ba);
+	bool makeBattleActionImpl(const BattleAction & ba);
 
 public:
 	explicit BattleActionProcessor(BattleProcessor * owner);
 	void setGameHandler(CGameHandler * newGameHandler);
 
-	bool makeBattleAction(const BattleAction & ba);
+	bool makeAutomaticBattleAction(const BattleAction & ba);
+	bool makePlayerBattleAction(PlayerColor player, const BattleAction & ba);
 };

server/battles/BattleFlowProcessor.cpp

@@ -126,12 +126,8 @@ void BattleFlowProcessor::summonGuardiansHelper(std::vector<BattleHex> & output,
 	}
 }
 
-void BattleFlowProcessor::onBattleStarted()
+void BattleFlowProcessor::tryPlaceMoats()
 {
-	gameHandler->setBattle(gameHandler->gameState()->curB);
-	assert(gameHandler->gameState()->curB);
-	//TODO: pre-tactic stuff, call scripts etc.
-
 	//Moat should be initialized here, because only here we can use spellcasting
 	if (gameHandler->gameState()->curB->town && gameHandler->gameState()->curB->town->fortLevel() >= CGTownInstance::CITADEL)
 	{
@@ -142,6 +138,14 @@ void BattleFlowProcessor::onBattleStarted()
 		auto target = spells::Target();
 		cast.cast(gameHandler->spellEnv, target);
 	}
+}
+
+void BattleFlowProcessor::onBattleStarted()
+{
+	gameHandler->setBattle(gameHandler->gameState()->curB);
+	assert(gameHandler->gameState()->curB);
+
+	tryPlaceMoats();
 
 	if (gameHandler->gameState()->curB->tacticDistance == 0)
 		onTacticsEnded();
@@ -330,11 +334,7 @@ void BattleFlowProcessor::activateNextStack()
 
 		if (!tryMakeAutomaticAction(next))
 		{
-			logGlobal->trace("Activating %s", next->nodeName());
+			setActiveStack(next);
-			auto nextId = next->unitId();
-			BattleSetActiveStack sas;
-			sas.stack = nextId;
-			gameHandler->sendAndApply(&sas);
 			break;
 		}
 	}
@@ -529,36 +529,25 @@ void BattleFlowProcessor::onActionMade(const BattleAction &ba)
 	if(owner->checkBattleStateChanges())
 		return;
 
-	bool heroAction = ba.actionType == EActionType::HERO_SPELL || ba.actionType ==EActionType::SURRENDER || ba.actionType ==EActionType::RETREAT;
+	if (ba.isUnitAction())
-	bool tacticsAction = ba.actionType == EActionType::END_TACTIC_PHASE;
-
-	if (activeStack == nullptr && !tacticsAction)
-	{
-		throw std::runtime_error("Unexpected action - no active stack!");
-	}
-
-	if (heroAction || tacticsAction)
-	{
-		if (!tacticsAction && activeStack->alive())
-		{
-			// this is action made by hero AND unit is alive (e.g. not killed by casted spell)
-			// keep current active stack for next action
-			BattleSetActiveStack sas;
-			sas.stack = activeStack->unitId();
-			gameHandler->sendAndApply(&sas);
-			return;
-		}
-	}
-	else
 	{
+		assert(activeStack != nullptr);
 		assert(actedStack != nullptr);
 
 		if (rollGoodMorale(actedStack))
 		{
 			// Good morale - same stack makes 2nd turn
-			BattleSetActiveStack sas;
+			setActiveStack(actedStack);
-			sas.stack = actedStack->unitId();
+			return;
-			gameHandler->sendAndApply(&sas);
+		}
+	}
+	else
+	{
+		if (activeStack && activeStack->alive())
+		{
+			// this is action made by hero AND unit is alive (e.g. not killed by casted spell)
+			// keep current active stack for next action
+			setActiveStack(actedStack);
 			return;
 		}
 	}
@@ -752,3 +741,13 @@ void BattleFlowProcessor::stackTurnTrigger(const CStack *st)
 		}
 	}
 }
+
+void BattleFlowProcessor::setActiveStack(const CStack * stack)
+{
+	assert(stack);
+
+	logGlobal->trace("Activating %s", stack->nodeName());
+	BattleSetActiveStack sas;
+	sas.stack = stack->unitId();
+	gameHandler->sendAndApply(&sas);
+}

server/battles/BattleFlowProcessor.h

@@ -32,6 +32,7 @@ class BattleFlowProcessor : boost::noncopyable
 
 	void summonGuardiansHelper(std::vector<BattleHex> & output, const BattleHex & targetPosition, ui8 side, bool targetIsTwoHex);
 	void trySummonGuardians(const CStack * stack);
+	void tryPlaceMoats();
 	void castOpeningSpells();
 	void activateNextStack();
 	void startNextRound(bool isFirstRound);
@@ -39,6 +40,7 @@ class BattleFlowProcessor : boost::noncopyable
 	void stackEnchantedTrigger(const CStack * stack);
 	void removeObstacle(const CObstacleInstance & obstacle);
 	void stackTurnTrigger(const CStack * stack);
+	void setActiveStack(const CStack * stack);
 
 	void makeStackDoNothing(const CStack * next);
 	bool makeAutomaticAction(const CStack * stack, BattleAction & ba); //used when action is taken by stack without volition of player (eg. unguided catapult attack)

server/battles/BattleProcessor.cpp

@@ -151,7 +151,6 @@ void BattleProcessor::setupBattle(int3 tile, const CArmedInstance *armies[2], co
 	if (heroes[0] && heroes[0]->boat && heroes[1] && heroes[1]->boat)
 		terType = BattleField(*VLC->identifiers()->getIdentifier("core", "battlefield.ship_to_ship"));
 
-
 	//send info about battles
 	BattleStart bs;
 	bs.info = BattleInfo::setupBattle(tile, terrain, terType, armies, heroes, creatureBank, town);
@@ -236,52 +235,9 @@ void BattleProcessor::updateGateState()
 		gameHandler->sendAndApply(&db);
 }
 
-bool BattleProcessor::makePlayerBattleAction(PlayerColor player, BattleAction &ba)
+bool BattleProcessor::makePlayerBattleAction(PlayerColor player, const BattleAction &ba)
 {
-	const BattleInfo * b = gameHandler->gameState()->curB;
+	bool result = actionsProcessor->makePlayerBattleAction(player, ba);
-
-	if(!b && gameHandler->complain("Can not make action - there is no battle ongoing!"))
-		return false;
-
-	if (ba.side != 0 && ba.side != 1 && gameHandler->complain("Can not make action - invalid battle side!"))
-		return false;
-
-	if(b->tacticDistance)
-	{
-		if(ba.actionType != EActionType::WALK && ba.actionType != EActionType::END_TACTIC_PHASE
-			&& ba.actionType != EActionType::RETREAT && ba.actionType != EActionType::SURRENDER)
-		{
-			gameHandler->complain("Can not make actions while in tactics mode!");
-			return false;
-		}
-
-		if(player != b->sides[ba.side].color)
-		{
-			gameHandler->complain("Can not make actions in battles you are not part of!");
-			return false;
-		}
-	}
-	else
-	{
-		bool heroAction = ba.actionType == EActionType::HERO_SPELL || ba.actionType ==EActionType::SURRENDER || ba.actionType ==EActionType::RETREAT || ba.actionType == EActionType::END_TACTIC_PHASE;
-
-		if (ba.stackNumber != b->getActiveStackID() && heroAction == false)
-		{
-			gameHandler->complain("Can not make actions - stack is not active!");
-			return false;
-		}
-
-		auto active = b->battleActiveUnit();
-		if(!active && gameHandler->complain("No active unit in battle!"))
-			return false;
-
-		auto unitOwner = b->battleGetOwner(active);
-
-		if(player != unitOwner && gameHandler->complain("Can not make actions in battles you are not part of!"))
-			return false;
-	}
-
-	bool result = actionsProcessor->makeBattleAction(ba);
 	flowProcessor->onActionMade(ba);
 	return result;
 }
@@ -294,7 +250,7 @@ void BattleProcessor::setBattleResult(EBattleResult resultType, int victoriusSid
 
 bool BattleProcessor::makeAutomaticBattleAction(const BattleAction &ba)
 {
-	return actionsProcessor->makeBattleAction(ba);
+	return actionsProcessor->makeAutomaticBattleAction(ba);
 }
 
 void BattleProcessor::endBattleConfirm(const BattleInfo * battleInfo)

server/battles/BattleProcessor.h

@@ -64,7 +64,7 @@ public:
 	void startBattleI(const CArmedInstance *army1, const CArmedInstance *army2, bool creatureBank = false);
 
 	/// Processing of incoming battle action netpack
-	bool makePlayerBattleAction(PlayerColor player, BattleAction & ba);
+	bool makePlayerBattleAction(PlayerColor player, const BattleAction & ba);
 
 	/// Applies results of a battle once player agrees to them
 	void endBattleConfirm(const BattleInfo * battleInfo);