Stack reversing logic now matches H3

client/battle/BattleStacksController.cpp

@@ -411,15 +411,14 @@ void BattleStacksController::stacksAreAttacked(std::vector<StackAttackedInfo> at
 		if (!attackedInfo.attacker)
 			continue;
 
-		bool needsReverse =
-				owner.curInt->cb->isToReverse(
-					attackedInfo.defender->getPosition(),
-					attackedInfo.attacker->getPosition(),
-					facingRight(attackedInfo.defender),
-					attackedInfo.attacker->doubleWide(),
-					facingRight(attackedInfo.attacker));
-
-		if (needsReverse)
+		// In H3, attacked stack will not reverse on ranged attack
+		if (attackedInfo.indirectAttack)
+			continue;
+
+		// defender need to face in direction opposited to out attacker
+		bool needsReverse = shouldAttackFacingRight(attackedInfo.attacker, attackedInfo.defender) == facingRight(attackedInfo.defender);
+
+		if (needsReverse && !attackedInfo.defender->isFrozen())
 		{
 			owner.executeOnAnimationCondition(EAnimationEvents::MOVEMENT, true, [=]()
 			{
@@ -494,23 +493,38 @@ void BattleStacksController::stackMoved(const CStack *stack, std::vector<BattleH
 	owner.waitForAnimationCondition(EAnimationEvents::ACTION, false);
 }
 
+bool BattleStacksController::shouldAttackFacingRight(const CStack * attacker, const CStack * defender)
+{
+	bool mustReverse = owner.curInt->cb->isToReverse(
+				attacker->getPosition(),
+				attacker,
+				defender);
+
+	if (attacker->side == BattleSide::ATTACKER)
+		return !mustReverse;
+	else
+		return mustReverse;
+}
+
 void BattleStacksController::stackAttacking( const StackAttackInfo & info )
 {
 	assert(owner.getAnimationCondition(EAnimationEvents::ACTION) == false);
 
-	bool needsReverse =
-			owner.curInt->cb->isToReverse(
-				info.attacker->getPosition(),
-				info.defender->getPosition(),
-				facingRight(info.attacker),
-				info.attacker->doubleWide(),
-				facingRight(info.defender));
-
 	auto attacker    = info.attacker;
 	auto defender    = info.defender;
 	auto tile        = info.tile;
 	auto spellEffect = info.spellEffect;
 	auto multiAttack = !info.secondaryDefender.empty();
+	bool needsReverse = false;
+
+	if (info.indirectAttack)
+	{
+		needsReverse = shouldRotate(attacker, attacker->position, info.tile);
+	}
+	else
+	{
+		needsReverse = shouldAttackFacingRight(attacker, defender) != facingRight(attacker);
+	}
 
 	if (needsReverse)
 	{

client/battle/BattleStacksController.h

@@ -100,6 +100,8 @@ class BattleStacksController
 
 	std::vector<const CStack *> selectHoveredStacks();
 
+	bool shouldAttackFacingRight(const CStack * attacker, const CStack * defender);
+
 public:
 	BattleStacksController(BattleInterface & owner);
 

lib/battle/BattleHex.cpp

@@ -151,12 +151,12 @@ std::vector<BattleHex> BattleHex::allNeighbouringTiles() const
 	return ret;
 }
 
-signed char BattleHex::mutualPosition(BattleHex hex1, BattleHex hex2)
+BattleHex::EDir BattleHex::mutualPosition(BattleHex hex1, BattleHex hex2)
 {
 	for(EDir dir = EDir(0); dir <= EDir(5); dir = EDir(dir+1))
 		if(hex2 == hex1.cloneInDirection(dir,false))
 			return dir;
-	return INVALID;
+	return NONE;
 }
 
 char BattleHex::getDistance(BattleHex hex1, BattleHex hex2)

lib/battle/BattleHex.h

@@ -15,7 +15,7 @@ VCMI_LIB_NAMESPACE_BEGIN
 
 namespace BattleSide
 {
-	enum
+	enum Type
 	{
 		ATTACKER = 0,
 		DEFENDER = 1
@@ -47,13 +47,14 @@ struct DLL_LINKAGE BattleHex //TODO: decide if this should be changed to class f
 	static const si16 INVALID = -1;
 	enum EDir
 	{
+		NONE = -1,
+
 		TOP_LEFT,
 		TOP_RIGHT,
 		RIGHT,
 		BOTTOM_RIGHT,
 		BOTTOM_LEFT,
 		LEFT,
-		NONE
 	};
 
 	BattleHex();
@@ -82,7 +83,7 @@ struct DLL_LINKAGE BattleHex //TODO: decide if this should be changed to class f
 	/// order of returned tiles matches EDir enim
 	std::vector<BattleHex> allNeighbouringTiles() const;
 
-	static signed char mutualPosition(BattleHex hex1, BattleHex hex2);
+	static EDir mutualPosition(BattleHex hex1, BattleHex hex2);
 	static char getDistance(BattleHex hex1, BattleHex hex2);
 	static void checkAndPush(BattleHex tile, std::vector<BattleHex> & ret);
 	static BattleHex getClosestTile(ui8 side, BattleHex initialPos, std::set<BattleHex> & possibilities); //TODO: vector or set? copying one to another is bad

lib/battle/CBattleInfoCallback.cpp

@@ -1382,8 +1382,12 @@ AttackableTiles CBattleInfoCallback::getPotentiallyAttackableHexes (const  battl
 	const int WN = GameConstants::BFIELD_WIDTH;
 	BattleHex hex = (attackerPos != BattleHex::INVALID) ? attackerPos : attacker->getPosition(); //real or hypothetical (cursor) position
 
+	auto defender = battleGetUnitByPos(hex, true);
+	if (!defender)
+		return at; // can't attack thin air
+
 	//FIXME: dragons or cerbers can rotate before attack, making their base hex different (#1124)
-	bool reverse = isToReverse(hex, destinationTile, isAttacker, attacker->doubleWide(), isAttacker);
+	bool reverse = isToReverse(destinationTile, attacker, defender);
 	if(reverse && attacker->doubleWide())
 	{
 		hex = attacker->occupiedHex(hex); //the other hex stack stands on
@@ -1537,60 +1541,50 @@ std::set<const CStack*> CBattleInfoCallback::getAttackedCreatures(const CStack*
 	return attackedCres;
 }
 
-//TODO: this should apply also to mechanics and cursor interface
-bool CBattleInfoCallback::isToReverseHlp (BattleHex hexFrom, BattleHex hexTo, bool curDir) const
+static bool isHexInFront(BattleHex hex, BattleHex testHex, BattleSide::Type side )
 {
-	int fromX = hexFrom.getX();
-	int fromY = hexFrom.getY();
-	int toX = hexTo.getX();
-	int toY = hexTo.getY();
+	static const std::set<BattleHex::EDir> rightDirs { BattleHex::BOTTOM_RIGHT, BattleHex::TOP_RIGHT, BattleHex::RIGHT };
+	static const std::set<BattleHex::EDir> leftDirs  { BattleHex::BOTTOM_LEFT, BattleHex::TOP_LEFT, BattleHex::LEFT };
 
-	if (curDir) // attacker, facing right
-	{
-		if (fromX < toX)
-			return false;
-		if (fromX > toX)
-			return true;
+	auto mutualPos = BattleHex::mutualPosition(hex, testHex);
 
-		if (fromY % 2 == 0 && toY % 2 == 1)
-
-			return true;
-		return false;
-	}
-	else // defender, facing left
-	{
-		if(fromX < toX)
-			return true;
-		if(fromX > toX)
-			return false;
-
-		if (fromY % 2 == 1 && toY % 2 == 0)
-			return true;
-		return false;
-	}
+	if (side == BattleSide::ATTACKER)
+		return rightDirs.count(mutualPos);
+	else
+		return leftDirs.count(mutualPos);
 }
 
 //TODO: this should apply also to mechanics and cursor interface
-bool CBattleInfoCallback::isToReverse (BattleHex hexFrom, BattleHex hexTo, bool curDir, bool toDoubleWide, bool toDir) const
+bool CBattleInfoCallback::isToReverse (BattleHex attackerHex, const battle::Unit * attacker, const battle::Unit * defender) const
 {
-	if (hexTo < 0 || hexFrom < 0) //turret
+	if (attackerHex < 0 ) //turret
 		return false;
 
-	if (toDoubleWide)
-	{
-		if (isToReverseHlp (hexFrom, hexTo, curDir))
-		{
-			if (toDir)
-				return isToReverseHlp (hexFrom, hexTo-1, curDir);
-			else
-				return isToReverseHlp (hexFrom, hexTo+1, curDir);
-		}
+	BattleHex defenderHex = defender->getPosition();
+
+	if (isHexInFront(attackerHex, defenderHex, BattleSide::Type(attacker->unitSide())))
 		return false;
+
+	if (defender->doubleWide())
+	{
+		if (isHexInFront(attackerHex,defender->occupiedHex(), BattleSide::Type(attacker->unitSide())))
+			return false;
 	}
-	else
+
+	if (attacker->doubleWide())
+	{
+		if (isHexInFront(attacker->occupiedHex(), defenderHex, BattleSide::Type(attacker->unitSide())))
+			return false;
+	}
+
+	// a bit weird case since here defender is slightly behind attacker, so reversing seems preferable,
+	// but this is how H3 handles it which is important, e.g. for direction of dragon breath attacks
+	if (attacker->doubleWide() && defender->doubleWide())
 	{
-		return isToReverseHlp(hexFrom, hexTo, curDir);
+		if (isHexInFront(attacker->occupiedHex(), defender->occupiedHex(), BattleSide::Type(attacker->unitSide())))
+			return false;
 	}
+	return true;
 }
 
 ReachabilityInfo::TDistances CBattleInfoCallback::battleGetDistances(const battle::Unit * unit, BattleHex assumedPosition) const

lib/battle/CBattleInfoCallback.h

@@ -136,8 +136,7 @@ public:
 	AttackableTiles getPotentiallyShootableHexes(const  battle::Unit* attacker, BattleHex destinationTile, BattleHex attackerPos) const;
 	std::vector<const battle::Unit *> getAttackedBattleUnits(const battle::Unit* attacker, BattleHex destinationTile, bool rangedAttack, BattleHex attackerPos = BattleHex::INVALID) const; //calculates range of multi-hex attacks
 	std::set<const CStack*> getAttackedCreatures(const CStack* attacker, BattleHex destinationTile, bool rangedAttack, BattleHex attackerPos = BattleHex::INVALID) const; //calculates range of multi-hex attacks
-	bool isToReverse(BattleHex hexFrom, BattleHex hexTo, bool curDir /*if true, creature is in attacker's direction*/, bool toDoubleWide, bool toDir) const; //determines if creature should be reversed (it stands on hexFrom and should 'see' hexTo)
-	bool isToReverseHlp(BattleHex hexFrom, BattleHex hexTo, bool curDir) const; //helper for isToReverse
+	bool isToReverse(BattleHex attackerHex, const battle::Unit *attacker, const battle::Unit *defender) const; //determines if attacker standing at attackerHex should reverse in order to attack defender
 
 	ReachabilityInfo getReachability(const battle::Unit * unit) const;
 	ReachabilityInfo getReachability(const ReachabilityInfo::Parameters & params) const;