Merge pull request #1423 from IvanSavenko/siege_mechanics_fix

Siege mechanics fixes

AI/BattleAI/BattleAI.cpp

@@ -46,14 +46,14 @@ std::vector<BattleHex> CBattleAI::getBrokenWallMoatHexes() const
 {
 	std::vector<BattleHex> result;
 
-	for(int wallPart = EWallPart::BOTTOM_WALL; wallPart < EWallPart::UPPER_WALL; wallPart++)
+	for(EWallPart wallPart : { EWallPart::BOTTOM_WALL, EWallPart::BELOW_GATE, EWallPart::OVER_GATE, EWallPart::UPPER_WALL })
 	{
 		auto state = cb->battleGetWallState(wallPart);
 
 		if(state != EWallState::DESTROYED)
 			continue;
 
-		auto wallHex = cb->wallPartToBattleHex((EWallPart::EWallPart)wallPart);
+		auto wallHex = cb->wallPartToBattleHex((EWallPart)wallPart);
 		auto moatHex = wallHex.cloneInDirection(BattleHex::LEFT);
 
 		result.push_back(moatHex);
@@ -364,7 +364,7 @@ BattleAction CBattleAI::useCatapult(const CStack * stack)
 	}
 	else
 	{
-		EWallPart::EWallPart wallParts[] = {
+		EWallPart wallParts[] = {
 			EWallPart::KEEP,
 			EWallPart::BOTTOM_TOWER,
 			EWallPart::UPPER_TOWER,
@@ -378,10 +378,9 @@ BattleAction CBattleAI::useCatapult(const CStack * stack)
 		{
 			auto wallState = cb->battleGetWallState(wallPart);
 
-			if(wallState == EWallState::INTACT || wallState == EWallState::DAMAGED)
+			if(wallState == EWallState::REINFORCED || wallState == EWallState::INTACT || wallState == EWallState::DAMAGED)
 			{
 				targetHex = cb->wallPartToBattleHex(wallPart);
-
 				break;
 			}
 		}

AI/BattleAI/StackWithBonuses.cpp

@@ -403,7 +403,7 @@ void HypotheticBattle::removeUnitBonus(uint32_t id, const std::vector<Bonus> & b
 	bonusTreeVersion++;
 }
 
-void HypotheticBattle::setWallState(int partOfWall, si8 state)
+void HypotheticBattle::setWallState(EWallPart partOfWall, EWallState state)
 {
 	//TODO:HypotheticBattle::setWallState
 }

AI/BattleAI/StackWithBonuses.h

@@ -130,7 +130,7 @@ public:
 	void updateUnitBonus(uint32_t id, const std::vector<Bonus> & bonus) override;
 	void removeUnitBonus(uint32_t id, const std::vector<Bonus> & bonus) override;
 
-	void setWallState(int partOfWall, si8 state) override;
+	void setWallState(EWallPart partOfWall, EWallState state) override;
 
 	void addObstacle(const ObstacleChanges & changes) override;
 	void updateObstacle(const ObstacleChanges& changes) override;

client/battle/BattleSiegeController.cpp

@@ -28,22 +28,29 @@
 #include "../../lib/CStack.h"
 #include "../../lib/mapObjects/CGTownInstance.h"
 
-std::string BattleSiegeController::getWallPieceImageName(EWallVisual::EWallVisual what, EWallState::EWallState state) const
+std::string BattleSiegeController::getWallPieceImageName(EWallVisual::EWallVisual what, EWallState state) const
 {
 	auto getImageIndex = [&]() -> int
 	{
+		bool isTower = (what == EWallVisual::KEEP || what == EWallVisual::BOTTOM_TOWER || what == EWallVisual::UPPER_TOWER);
+
 		switch (state)
 		{
-		case EWallState::INTACT :
+		case EWallState::REINFORCED :
 			return 1;
+		case EWallState::INTACT :
+			if (town->hasBuilt(BuildingID::CASTLE))
+				return 2; // reinforced walls were damaged
+			else
+				return 1;
 		case EWallState::DAMAGED :
 			// towers don't have separate image here - INTACT and DAMAGED is 1, DESTROYED is 2
-			if(what == EWallVisual::KEEP || what == EWallVisual::BOTTOM_TOWER || what == EWallVisual::UPPER_TOWER)
+			if (isTower)
 				return 1;
 			else
 				return 2;
 		case EWallState::DESTROYED :
-			if (what == EWallVisual::KEEP || what == EWallVisual::BOTTOM_TOWER || what == EWallVisual::UPPER_TOWER)
+			if (isTower)
 				return 2;
 			else
 				return 3;
@@ -130,9 +137,9 @@ bool BattleSiegeController::getWallPieceExistance(EWallVisual::EWallVisual what)
 	{
 	case EWallVisual::MOAT:              return town->hasBuilt(BuildingID::CITADEL) && town->town->faction->index != ETownType::TOWER;
 	case EWallVisual::MOAT_BANK:         return town->hasBuilt(BuildingID::CITADEL) && town->town->faction->index != ETownType::TOWER && town->town->faction->index != ETownType::NECROPOLIS;
-	case EWallVisual::KEEP_BATTLEMENT:   return town->hasBuilt(BuildingID::CITADEL) && EWallState::EWallState(owner.curInt->cb->battleGetWallState(EWallPart::KEEP)) != EWallState::DESTROYED;
-	case EWallVisual::UPPER_BATTLEMENT:  return town->hasBuilt(BuildingID::CASTLE) && EWallState::EWallState(owner.curInt->cb->battleGetWallState(EWallPart::UPPER_TOWER)) != EWallState::DESTROYED;
-	case EWallVisual::BOTTOM_BATTLEMENT: return town->hasBuilt(BuildingID::CASTLE) && EWallState::EWallState(owner.curInt->cb->battleGetWallState(EWallPart::BOTTOM_TOWER)) != EWallState::DESTROYED;
+	case EWallVisual::KEEP_BATTLEMENT:   return town->hasBuilt(BuildingID::CITADEL) && owner.curInt->cb->battleGetWallState(EWallPart::KEEP) != EWallState::DESTROYED;
+	case EWallVisual::UPPER_BATTLEMENT:  return town->hasBuilt(BuildingID::CASTLE) && owner.curInt->cb->battleGetWallState(EWallPart::UPPER_TOWER) != EWallState::DESTROYED;
+	case EWallVisual::BOTTOM_BATTLEMENT: return town->hasBuilt(BuildingID::CASTLE) && owner.curInt->cb->battleGetWallState(EWallPart::BOTTOM_TOWER) != EWallState::DESTROYED;
 	default:                             return true;
 	}
 }
@@ -177,7 +184,7 @@ BattleSiegeController::BattleSiegeController(BattleInterface & owner, const CGTo
 		if ( !getWallPieceExistance(EWallVisual::EWallVisual(g)) )
 			continue;
 
-		wallPieceImages[g] = IImage::createFromFile(getWallPieceImageName(EWallVisual::EWallVisual(g), EWallState::INTACT));
+		wallPieceImages[g] = IImage::createFromFile(getWallPieceImageName(EWallVisual::EWallVisual(g), EWallState::REINFORCED));
 	}
 }
 
@@ -321,7 +328,7 @@ bool BattleSiegeController::isAttackableByCatapult(BattleHex hex) const
 	if (!owner.curInt->cb->isWallPartPotentiallyAttackable(wallPart))
 		return false;
 
-	auto state = owner.curInt->cb->battleGetWallState(static_cast<int>(wallPart));
+	auto state = owner.curInt->cb->battleGetWallState(wallPart);
 	return state != EWallState::DESTROYED && state != EWallState::NONE;
 }
 
@@ -354,12 +361,12 @@ void BattleSiegeController::stackIsCatapulting(const CatapultAttack & ca)
 
 	for (auto attackInfo : ca.attackedParts)
 	{
-		int wallId = attackInfo.attackedPart + EWallVisual::DESTRUCTIBLE_FIRST;
+		int wallId = static_cast<int>(attackInfo.attackedPart) + EWallVisual::DESTRUCTIBLE_FIRST;
 		//gate state changing handled separately
 		if (wallId == EWallVisual::GATE)
 			continue;
 
-		auto wallState = EWallState::EWallState(owner.curInt->cb->battleGetWallState(attackInfo.attackedPart));
+		auto wallState = EWallState(owner.curInt->cb->battleGetWallState(attackInfo.attackedPart));
 
 		wallPieceImages[wallId] = IImage::createFromFile(getWallPieceImageName(EWallVisual::EWallVisual(wallId), wallState));
 	}

client/battle/BattleSiegeController.h

@@ -76,7 +76,7 @@ class BattleSiegeController
 	std::array<std::shared_ptr<IImage>, EWallVisual::WALL_LAST + 1> wallPieceImages;
 
 	/// return URI for image for a wall piece
-	std::string getWallPieceImageName(EWallVisual::EWallVisual what, EWallState::EWallState state) const;
+	std::string getWallPieceImageName(EWallVisual::EWallVisual what, EWallState state) const;
 
 	/// returns BattleHex to which chosen wall piece is bound
 	BattleHex getWallPiecePosition(EWallVisual::EWallVisual what) const;

lib/CTownHandler.cpp

@@ -173,12 +173,6 @@ CTown::~CTown()
 		str.dellNull();
 }
 
-std::vector<BattleHex> CTown::defaultMoatHexes()
-{
-	static const std::vector<BattleHex> moatHexes = {11, 28, 44, 61, 77, 111, 129, 146, 164, 181};
-	return moatHexes;
-}
-
 std::string CTown::getLocalizedFactionName() const
 {
 	if(faction == nullptr)
@@ -851,12 +845,7 @@ void CTownHandler::loadTown(CTown * town, const JsonNode & source)
 	warMachinesToLoad[town] = source["warMachine"];
 
 	town->moatDamage = static_cast<si32>(source["moatDamage"].Float());
-
-	// Compatibility for <= 0.98f mods
-	if(source["moatHexes"].isNull())
-		town->moatHexes = CTown::defaultMoatHexes();
-	else
-		town->moatHexes = source["moatHexes"].convertTo<std::vector<BattleHex> >();
+	town->moatHexes = source["moatHexes"].convertTo<std::vector<BattleHex> >();
 
 	town->mageLevel = static_cast<ui32>(source["mageGuild"].Float());
 	town->names = source["names"].convertTo<std::vector<std::string> >();

lib/CTownHandler.h

@@ -231,8 +231,6 @@ class DLL_LINKAGE CTown
 public:
 	CTown();
 	~CTown();
-	// TODO: remove once save and mod compatability not needed
-	static std::vector<BattleHex> defaultMoatHexes();
 
 	std::string getLocalizedFactionName() const;
 	std::string getBuildingScope() const;

lib/GameConstants.h

@@ -681,32 +681,27 @@ namespace ECommander
 	const int MAX_SKILL_LEVEL = 5;
 }
 
-namespace EWallPart
+enum class EWallPart : int8_t
 {
-	enum EWallPart
-	{
-		INDESTRUCTIBLE_PART_OF_GATE = -3, INDESTRUCTIBLE_PART = -2, INVALID = -1,
-		KEEP = 0, BOTTOM_TOWER, BOTTOM_WALL, BELOW_GATE, OVER_GATE, UPPER_WALL, UPPER_TOWER, GATE,
-		PARTS_COUNT /* This constant SHOULD always stay as the last item in the enum. */
-	};
-}
+	INDESTRUCTIBLE_PART_OF_GATE = -3, INDESTRUCTIBLE_PART = -2, INVALID = -1,
+	KEEP = 0, BOTTOM_TOWER, BOTTOM_WALL, BELOW_GATE, OVER_GATE, UPPER_WALL, UPPER_TOWER, GATE,
+	PARTS_COUNT /* This constant SHOULD always stay as the last item in the enum. */
+};
 
-namespace EWallState
+enum class EWallState : int8_t
 {
-	enum EWallState
-	{
-		NONE = -1, //no wall
-		DESTROYED,
-		DAMAGED,
-		INTACT
-	};
-}
+	NONE = -1, //no wall
+	DESTROYED,
+	DAMAGED,
+	INTACT,
+	REINFORCED, // walls in towns with castle
+};
 
-enum class EGateState : ui8
+enum class EGateState : uint8_t
 {
 	NONE,
 	CLOSED,
-	BLOCKED, //dead or alive stack blocking from outside
+	BLOCKED, // gate is blocked in closed state, e.g. by creature
 	OPENED,
 	DESTROYED
 };

lib/NetPacks.h

@@ -1827,7 +1827,7 @@ struct ELF_VISIBILITY CatapultAttack : public CPackForClient
 	struct AttackInfo
 	{
 		si16 destinationTile;
-		ui8 attackedPart;
+		EWallPart attackedPart;
 		ui8 damageDealt;
 
 		template <typename Handler> void serialize(Handler & h, const int version)

lib/NetPacksLib.cpp

@@ -1708,7 +1708,7 @@ DLL_LINKAGE void CatapultAttack::applyBattle(IBattleState * battleState)
 
 	for(const auto & part : attackedParts)
 	{
-		auto newWallState = SiegeInfo::applyDamage(EWallState::EWallState(battleState->getWallState(part.attackedPart)), part.damageDealt);
+		auto newWallState = SiegeInfo::applyDamage(battleState->getWallState(part.attackedPart), part.damageDealt);
 		battleState->setWallState(part.attackedPart, newWallState);
 	}
 }

lib/battle/BattleHex.cpp

@@ -159,7 +159,7 @@ BattleHex::EDir BattleHex::mutualPosition(BattleHex hex1, BattleHex hex2)
 	return NONE;
 }
 
-char BattleHex::getDistance(BattleHex hex1, BattleHex hex2)
+uint8_t BattleHex::getDistance(BattleHex hex1, BattleHex hex2)
 {
 	int y1 = hex1.getY(), y2 = hex2.getY();
 

lib/battle/BattleHex.h

@@ -88,7 +88,7 @@ struct DLL_LINKAGE BattleHex //TODO: decide if this should be changed to class f
 	std::vector<BattleHex> allNeighbouringTiles() const;
 
 	static EDir mutualPosition(BattleHex hex1, BattleHex hex2);
-	static char getDistance(BattleHex hex1, BattleHex hex2);
+	static uint8_t 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/BattleInfo.cpp

@@ -222,20 +222,23 @@ BattleInfo * BattleInfo::setupBattle(const int3 & tile, TerrainId terrain, const
 	{
 		curB->si.gateState = EGateState::CLOSED;
 
-		for (int b = 0; b < curB->si.wallState.size(); ++b)
-		{
-			curB->si.wallState[b] = EWallState::INTACT;
-		}
+		curB->si.wallState[EWallPart::GATE] = EWallState::INTACT;
 
-		if (!town->hasBuilt(BuildingID::CITADEL))
+		for (auto const wall : {EWallPart::BOTTOM_WALL, EWallPart::BELOW_GATE, EWallPart::OVER_GATE, EWallPart::UPPER_WALL} )
 		{
-			curB->si.wallState[EWallPart::KEEP] = EWallState::NONE;
+			if (town->hasBuilt(BuildingID::CASTLE))
+				curB->si.wallState[wall] = EWallState::REINFORCED;
+			else
+				curB->si.wallState[wall] = EWallState::INTACT;
 		}
 
-		if (!town->hasBuilt(BuildingID::CASTLE))
+		if (town->hasBuilt(BuildingID::CITADEL))
+			curB->si.wallState[EWallPart::KEEP] = EWallState::INTACT;
+
+		if (town->hasBuilt(BuildingID::CASTLE))
 		{
-			curB->si.wallState[EWallPart::UPPER_TOWER] = EWallState::NONE;
-			curB->si.wallState[EWallPart::BOTTOM_TOWER] = EWallState::NONE;
+			curB->si.wallState[EWallPart::UPPER_TOWER] = EWallState::INTACT;
+			curB->si.wallState[EWallPart::BOTTOM_TOWER] = EWallState::INTACT;
 		}
 	}
 
@@ -609,7 +612,7 @@ const CGTownInstance * BattleInfo::getDefendedTown() const
 	return town;
 }
 
-si8 BattleInfo::getWallState(int partOfWall) const
+EWallState BattleInfo::getWallState(EWallPart partOfWall) const
 {
 	return si.wallState.at(partOfWall);
 }
@@ -912,9 +915,9 @@ void BattleInfo::addOrUpdateUnitBonus(CStack * sta, const Bonus & value, bool fo
 	}
 }
 
-void BattleInfo::setWallState(int partOfWall, si8 state)
+void BattleInfo::setWallState(EWallPart partOfWall, EWallState state)
 {
-	si.wallState.at(partOfWall) = state;
+	si.wallState[partOfWall] = state;
 }
 
 void BattleInfo::addObstacle(const ObstacleChanges & changes)

lib/battle/BattleInfo.h

@@ -87,7 +87,7 @@ public:
 	ui8 getTacticsSide() const override;
 
 	const CGTownInstance * getDefendedTown() const override;
-	si8 getWallState(int partOfWall) const override;
+	EWallState getWallState(EWallPart partOfWall) const override;
 	EGateState getGateState() const override;
 
 	uint32_t getCastSpells(ui8 side) const override;
@@ -115,7 +115,7 @@ public:
 	void updateUnitBonus(uint32_t id, const std::vector<Bonus> & bonus) override;
 	void removeUnitBonus(uint32_t id, const std::vector<Bonus> & bonus) override;
 
-	void setWallState(int partOfWall, si8 state) override;
+	void setWallState(EWallPart partOfWall, EWallState state) override;
 
 	void addObstacle(const ObstacleChanges & changes) override;
 	void updateObstacle(const ObstacleChanges& changes) override;

lib/battle/BattleProxy.cpp

@@ -88,7 +88,7 @@ const CGTownInstance * BattleProxy::getDefendedTown() const
 	return subject->battleGetDefendedTown();
 }
 
-si8 BattleProxy::getWallState(int partOfWall) const
+EWallState BattleProxy::getWallState(EWallPart partOfWall) const
 {
 	return subject->battleGetWallState(partOfWall);
 }

lib/battle/BattleProxy.h

@@ -44,7 +44,7 @@ public:
 	ui8 getTacticsSide() const override;
 
 	const CGTownInstance * getDefendedTown() const override;
-	si8 getWallState(int partOfWall) const override;
+	EWallState getWallState(EWallPart partOfWall) const override;
 	EGateState getGateState() const override;
 
 	uint32_t getCastSpells(ui8 side) const override;

lib/battle/CBattleInfoCallback.cpp

@@ -24,44 +24,37 @@ VCMI_LIB_NAMESPACE_BEGIN
 
 namespace SiegeStuffThatShouldBeMovedToHandlers // <=== TODO
 {
-/*
- *Here are 2 explanations how below algorithm should work in H3, looks like they are not 100% accurate as it results in one damage number, not min/max range:
- *
- *1. http://heroes.thelazy.net/wiki/Arrow_tower
- *
- *2. All towns' turrets do the same damage. If Fort, Citadel or Castle is built damage of the Middle turret is 15, and 7,5 for others.
- *Buildings increase turrets' damage, but only those buildings that are new in town view, not upgrades to the existing. So, every building save:
- *- dwellings' upgrades
- *- Mage Guild upgrades
- *- Horde buildings
- *- income upgrades
- *- some special ones
- *increases middle Turret damage by 3, and 1,5 for the other two.
- *Damage is almost always the maximum one (right click on the Turret), sometimes +1/2 points, and it does not depend on the target. Nothing can influence it, except the mentioned above (but it will be roughly double if the defender has Armorer or Air Shield).
- *Maximum damage for Castle, Conflux is 120, Necropolis, Inferno, Fortress 125, Stronghold, Turret, and Dungeon 130 (for all three Turrets).
- *Artillery allows the player to control the Turrets.
- */
+
 static void retrieveTurretDamageRange(const CGTownInstance * town, const battle::Unit * turret, double & outMinDmg, double & outMaxDmg)//does not match OH3 yet, but damage is somewhat close
 {
+	// http://heroes.thelazy.net/wiki/Arrow_tower
 	assert(turret->creatureIndex() == CreatureID::ARROW_TOWERS);
 	assert(town);
 	assert(turret->getPosition() >= -4 && turret->getPosition() <= -2);
 
-	const float multiplier = (turret->getPosition() == -2) ? 1.0f : 0.5f;
+	// base damage, irregardless of town level
+	static const int baseDamageKeep = 10;
+	static const int baseDamageTower = 6;
+
+	// extra damage, for each building in town
+	static const int extraDamage = 2;
+
+	const int townLevel = town->getTownLevel();
 
-	//Revised - Where do below values come from?
-	/*int baseMin = 6;
-	int baseMax = 10;*/
+	int minDamage;
 
-	const int baseDamage = 15;
+	if (turret->getPosition() == BattleHex::CASTLE_CENTRAL_TOWER)
+		minDamage = baseDamageKeep + townLevel * extraDamage;
+	else
+		minDamage = baseDamageTower + townLevel / 2 * extraDamage;
 
-	outMinDmg = multiplier * (baseDamage + town->getTownLevel() * 3);
-	outMaxDmg = outMinDmg;
+	outMinDmg = minDamage;
+	outMaxDmg = minDamage * 2;
 }
 
 static BattleHex lineToWallHex(int line) //returns hex with wall in given line (y coordinate)
 {
-	static const BattleHex lineToHex[] = {12, 29, 45, 62, 78, 95, 112, 130, 147, 165, 182};
+	static const BattleHex lineToHex[] = {12, 29, 45, 62, 78, 96, 112, 130, 147, 165, 182};
 
 	return lineToHex[line];
 }
@@ -78,7 +71,7 @@ static bool sameSideOfWall(BattleHex pos1, BattleHex pos2)
 }
 
 // parts of wall
-static const std::pair<int, EWallPart::EWallPart> wallParts[] =
+static const std::pair<int, EWallPart> wallParts[] =
 {
 	std::make_pair(50, EWallPart::KEEP),
 	std::make_pair(183, EWallPart::BOTTOM_TOWER),
@@ -96,7 +89,7 @@ static const std::pair<int, EWallPart::EWallPart> wallParts[] =
 	std::make_pair(165, EWallPart::INDESTRUCTIBLE_PART)
 };
 
-static EWallPart::EWallPart hexToWallPart(BattleHex hex)
+static EWallPart hexToWallPart(BattleHex hex)
 {
 	for(auto & elem : wallParts)
 	{
@@ -107,7 +100,7 @@ static EWallPart::EWallPart hexToWallPart(BattleHex hex)
 	return EWallPart::INVALID; //not found!
 }
 
-static BattleHex WallPartToHex(EWallPart::EWallPart part)
+static BattleHex WallPartToHex(EWallPart part)
 {
 	for(auto & elem : wallParts)
 	{
@@ -164,8 +157,98 @@ ESpellCastProblem::ESpellCastProblem CBattleInfoCallback::battleCanCastSpell(con
 	return ESpellCastProblem::OK;
 }
 
+struct Point
+{
+	int x,y;
+};
+
+/// Algorithm to test whether line segment between points line1-line2 will intersect with
+/// rectangle specified by top-left and bottom-right points
+/// Note that in order to avoid floating point rounding errors algorithm uses integers with no divisions
+static bool intersectionSegmentRect(Point line1, Point line2, Point rectTL, Point rectBR)
+{
+	assert(rectTL.x < rectBR.x);
+	assert(rectTL.y < rectBR.y);
+
+	// check whether segment is located to the left of our AABB
+	if (line1.x < rectTL.x && line2.x < rectTL.x)
+		return false;
+
+	// check whether segment is located to the right of our AABB
+	if (line1.x > rectBR.x && line2.x > rectBR.x)
+		return false;
+
+	// check whether segment is located on top of our AABB
+	if (line1.y < rectTL.y && line2.y < rectTL.y)
+		return false;
+
+	// check whether segment is located below of our AABB
+	if (line1.y > rectBR.y && line2.y > rectBR.y)
+		return false;
+
+	Point vector { line2.x - line1.x, line2.y - line1.y};
+
+	// compute position of AABB corners relative to our line
+	int tlTest = vector.y*rectTL.x - vector.x*rectTL.y + (line2.x*line1.y-line1.x*line2.y);
+	int trTest = vector.y*rectBR.x - vector.x*rectTL.y + (line2.x*line1.y-line1.x*line2.y);
+	int blTest = vector.y*rectTL.x - vector.x*rectBR.y + (line2.x*line1.y-line1.x*line2.y);
+	int brTest = vector.y*rectBR.x - vector.x*rectBR.y + (line2.x*line1.y-line1.x*line2.y);
+
+	// if all points are on the left of our line then there is no intersection
+	if ( tlTest > 0 && trTest > 0 && blTest > 0 && brTest > 0 )
+		return false;
+
+	// if all points are on the right of our line then there is no intersection
+	if ( tlTest < 0 && trTest < 0 && blTest < 0 && brTest < 0 )
+		return false;
+
+	// if all previous checks failed, this means that there is an intersection between line and AABB
+	return true;
+}
+
 bool CBattleInfoCallback::battleHasWallPenalty(const IBonusBearer * shooter, BattleHex shooterPosition, BattleHex destHex) const
 {
+	auto isTileBlocked = [&](BattleHex tile)
+	{
+		EWallPart wallPart = battleHexToWallPart(tile);
+		if (wallPart == EWallPart::INDESTRUCTIBLE_PART_OF_GATE)
+			return false; // does not blocks ranged attacks
+		if (wallPart == EWallPart::INDESTRUCTIBLE_PART)
+			return true; // always blocks ranged attacks
+
+		assert(isWallPartPotentiallyAttackable(wallPart));
+
+		EWallState state = battleGetWallState(wallPart);
+
+		return state != EWallState::DESTROYED;
+	};
+
+	auto needWallPenalty = [&](BattleHex from, BattleHex dest)
+	{
+		// arbitrary selected cell size for virtual grid
+		// any even number can be selected (for division by two)
+		static const int cellSize = 10;
+
+		// create line that goes from center of shooter cell to center of target cell
+		Point line1{ from.getX()*cellSize+cellSize/2, from.getY()*cellSize+cellSize/2};
+		Point line2{ dest.getX()*cellSize+cellSize/2, dest.getY()*cellSize+cellSize/2};
+
+		for (int y = 0; y < GameConstants::BFIELD_HEIGHT; ++y)
+		{
+			BattleHex obstacle = lineToWallHex(y);
+			if (!isTileBlocked(obstacle))
+				continue;
+
+			// create rect around cell with an obstacle
+			Point rectTL{ obstacle.getX()*cellSize, obstacle.getY()*cellSize };
+			Point recrBR{ obstacle.getX()*(cellSize+1), obstacle.getY()*(cellSize+1) };
+
+			if ( intersectionSegmentRect(line1, line2, rectTL, recrBR))
+				return true;
+		}
+		return false;
+	};
+
 	RETURN_IF_NOT_BATTLE(false);
 	if(!battleGetSiegeLevel())
 		return false;
@@ -177,21 +260,9 @@ bool CBattleInfoCallback::battleHasWallPenalty(const IBonusBearer * shooter, Bat
 		return false;
 
 	const int wallInStackLine = lineToWallHex(shooterPosition.getY());
-	const int wallInDestLine = lineToWallHex(destHex.getY());
-
-	const bool stackLeft = shooterPosition < wallInStackLine;
-	const bool destRight = destHex > wallInDestLine;
-
-	if (stackLeft && destRight) //shooting from outside to inside
-	{
-		int row = (shooterPosition + destHex) / (2 * GameConstants::BFIELD_WIDTH);
-		if (shooterPosition > destHex && ((destHex % GameConstants::BFIELD_WIDTH - shooterPosition % GameConstants::BFIELD_WIDTH) < 2)) //shooting up high
-			row -= 2;
-		const int wallPos = lineToWallHex(row);
-		if (!isWallPartPotentiallyAttackable(battleHexToWallPart(wallPos))) return true;
-	}
+	const bool shooterOutsideWalls = shooterPosition < wallInStackLine;
 
-	return false;
+	return shooterOutsideWalls && needWallPenalty(shooterPosition, destHex);
 }
 
 si8 CBattleInfoCallback::battleCanTeleportTo(const battle::Unit * stack, BattleHex destHex, int telportLevel) const
@@ -1128,13 +1199,13 @@ AccessibilityInfo CBattleInfoCallback::getAccesibility() const
 			ret[hex] = EAccessibility::UNAVAILABLE;
 
 		//TODO likely duplicated logic
-		static const std::pair<int, BattleHex> lockedIfNotDestroyed[] =
+		static const std::pair<EWallPart, BattleHex> lockedIfNotDestroyed[] =
 		{
 			//which part of wall, which hex is blocked if this part of wall is not destroyed
-			std::make_pair(2, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_4)),
-			std::make_pair(3, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_3)),
-			std::make_pair(4, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_2)),
-			std::make_pair(5, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_1))
+			std::make_pair(EWallPart::BOTTOM_WALL, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_4)),
+			std::make_pair(EWallPart::BELOW_GATE, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_3)),
+			std::make_pair(EWallPart::OVER_GATE, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_2)),
+			std::make_pair(EWallPart::UPPER_WALL, BattleHex(ESiegeHex::DESTRUCTIBLE_WALL_1))
 		};
 
 		for(auto & elem : lockedIfNotDestroyed)
@@ -1636,19 +1707,19 @@ bool CBattleInfoCallback::isEnemyUnitWithinSpecifiedRange(BattleHex attackerPosi
 	return false;
 }
 
-BattleHex CBattleInfoCallback::wallPartToBattleHex(EWallPart::EWallPart part) const
+BattleHex CBattleInfoCallback::wallPartToBattleHex(EWallPart part) const
 {
 	RETURN_IF_NOT_BATTLE(BattleHex::INVALID);
 	return WallPartToHex(part);
 }
 
-EWallPart::EWallPart CBattleInfoCallback::battleHexToWallPart(BattleHex hex) const
+EWallPart CBattleInfoCallback::battleHexToWallPart(BattleHex hex) const
 {
 	RETURN_IF_NOT_BATTLE(EWallPart::INVALID);
 	return hexToWallPart(hex);
 }
 
-bool CBattleInfoCallback::isWallPartPotentiallyAttackable(EWallPart::EWallPart wallPart) const
+bool CBattleInfoCallback::isWallPartPotentiallyAttackable(EWallPart wallPart) const
 {
 	RETURN_IF_NOT_BATTLE(false);
 	return wallPart != EWallPart::INDESTRUCTIBLE_PART && wallPart != EWallPart::INDESTRUCTIBLE_PART_OF_GATE &&
@@ -1664,8 +1735,8 @@ std::vector<BattleHex> CBattleInfoCallback::getAttackableBattleHexes() const
 	{
 		if(isWallPartPotentiallyAttackable(wallPartPair.second))
 		{
-			auto wallState = static_cast<EWallState::EWallState>(battleGetWallState(static_cast<int>(wallPartPair.second)));
-			if(wallState == EWallState::INTACT || wallState == EWallState::DAMAGED)
+			auto wallState = battleGetWallState(wallPartPair.second);
+			if(wallState == EWallState::REINFORCED || wallState == EWallState::INTACT || wallState == EWallState::DAMAGED)
 			{
 				attackableBattleHexes.push_back(BattleHex(wallPartPair.first));
 			}

lib/battle/CBattleInfoCallback.h

@@ -111,9 +111,9 @@ public:
 	bool battleHasWallPenalty(const IBonusBearer * shooter, BattleHex shooterPosition, BattleHex destHex) const;
 	bool battleHasShootingPenalty(const battle::Unit * shooter, BattleHex destHex) const;
 
-	BattleHex wallPartToBattleHex(EWallPart::EWallPart part) const;
-	EWallPart::EWallPart battleHexToWallPart(BattleHex hex) const; //returns part of destructible wall / gate / keep under given hex or -1 if not found
-	bool isWallPartPotentiallyAttackable(EWallPart::EWallPart wallPart) const; // returns true if the wall part is potentially attackable (independent of wall state), false if not
+	BattleHex wallPartToBattleHex(EWallPart part) const;
+	EWallPart battleHexToWallPart(BattleHex hex) const; //returns part of destructible wall / gate / keep under given hex or -1 if not found
+	bool isWallPartPotentiallyAttackable(EWallPart wallPart) const; // returns true if the wall part is potentially attackable (independent of wall state), false if not
 	std::vector<BattleHex> getAttackableBattleHexes() const;
 
 	si8 battleMinSpellLevel(ui8 side) const; //calculates maximum spell level possible to be cast on battlefield - takes into account artifacts of both heroes; if no effects are set, 0 is returned

lib/battle/CBattleInfoEssentials.cpp

@@ -364,7 +364,7 @@ bool CBattleInfoEssentials::battleHasHero(ui8 side) const
 	return getBattle()->getSideHero(side) != nullptr;
 }
 
-si8 CBattleInfoEssentials::battleGetWallState(int partOfWall) const
+EWallState CBattleInfoEssentials::battleGetWallState(EWallPart partOfWall) const
 {
 	RETURN_IF_NOT_BATTLE(EWallState::NONE);
 	if(battleGetSiegeLevel() == CGTownInstance::NONE)

lib/battle/CBattleInfoEssentials.h

@@ -94,7 +94,7 @@ public:
 
 	// for determining state of a part of the wall; format: parameter [0] - keep, [1] - bottom tower, [2] - bottom wall,
 	// [3] - below gate, [4] - over gate, [5] - upper wall, [6] - uppert tower, [7] - gate; returned value: 1 - intact, 2 - damaged, 3 - destroyed; 0 - no battle
-	si8 battleGetWallState(int partOfWall) const;
+	EWallState battleGetWallState(EWallPart partOfWall) const;
 	EGateState battleGetGateState() const;
 
 	//helpers

lib/battle/IBattleState.h

@@ -49,7 +49,7 @@ public:
 	virtual ObstacleCList getAllObstacles() const = 0;
 
 	virtual const CGTownInstance * getDefendedTown() const = 0;
-	virtual si8 getWallState(int partOfWall) const = 0;
+	virtual EWallState getWallState(EWallPart partOfWall) const = 0;
 	virtual EGateState getGateState() const = 0;
 
 	virtual PlayerColor getSidePlayer(ui8 side) const = 0;
@@ -87,7 +87,7 @@ public:
 	virtual void updateUnitBonus(uint32_t id, const std::vector<Bonus> & bonus) = 0;
 	virtual void removeUnitBonus(uint32_t id, const std::vector<Bonus> & bonus) = 0;
 
-	virtual void setWallState(int partOfWall, si8 state) = 0;
+	virtual void setWallState(EWallPart partOfWall, EWallState state) = 0;
 
 	virtual void addObstacle(const ObstacleChanges & changes) = 0;
 	virtual void updateObstacle(const ObstacleChanges & changes) = 0;

lib/battle/SiegeInfo.cpp

@@ -15,20 +15,22 @@ VCMI_LIB_NAMESPACE_BEGIN
 
 SiegeInfo::SiegeInfo()
 {
-	for(int i = 0; i < wallState.size(); ++i)
+	for(int i = 0; i < int(EWallPart::PARTS_COUNT); ++i)
 	{
-		wallState[i] = EWallState::NONE;
+		wallState[EWallPart(i)] = EWallState::NONE;
 	}
 	gateState = EGateState::NONE;
 }
 
-EWallState::EWallState SiegeInfo::applyDamage(EWallState::EWallState state, unsigned int value)
+EWallState SiegeInfo::applyDamage(EWallState state, unsigned int value)
 {
 	if(value == 0)
 		return state;
 
 	switch(applyDamage(state, value - 1))
 	{
+	case EWallState::REINFORCED:
+		return EWallState::INTACT;
 	case EWallState::INTACT:
 		return EWallState::DAMAGED;
 	case EWallState::DAMAGED:

lib/battle/SiegeInfo.h

@@ -15,13 +15,13 @@ VCMI_LIB_NAMESPACE_BEGIN
 //only for use in BattleInfo
 struct DLL_LINKAGE SiegeInfo
 {
-	std::array<si8, EWallPart::PARTS_COUNT> wallState;
+	std::map<EWallPart, EWallState> wallState;
 	EGateState gateState;
 
 	SiegeInfo();
 
 	// return EWallState decreased by value of damage points
-	static EWallState::EWallState applyDamage(EWallState::EWallState state, unsigned int value);
+	static EWallState applyDamage(EWallState state, unsigned int value);
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{

lib/spells/effects/Catapult.cpp

@@ -68,7 +68,7 @@ bool Catapult::applicable(Problem & problem, const Mechanics * m) const
 void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectTarget & /* eTarget */) const
 {
 	//start with all destructible parts
-	static const std::set<EWallPart::EWallPart> potentialTargets =
+	static const std::set<EWallPart> potentialTargets =
 	{
 		EWallPart::KEEP,
 		EWallPart::BOTTOM_TOWER,
@@ -80,9 +80,9 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 		EWallPart::GATE
 	};
 
-	assert(potentialTargets.size() == EWallPart::PARTS_COUNT);
+	assert(potentialTargets.size() == size_t(EWallPart::PARTS_COUNT));
 
-	std::set<EWallPart::EWallPart> allowedTargets;
+	std::set<EWallPart> allowedTargets;
 
 	for (auto const & target : potentialTargets)
 	{
@@ -98,16 +98,13 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 	CatapultAttack ca;
 	ca.attacker = -1;
 
-	BattleUnitsChanged removeUnits;
-
 	for(int i = 0; i < targetsToAttack; i++)
 	{
 		// Hit on any existing, not destroyed targets are allowed
 		// Multiple hit on same target are allowed.
 		// Potential overshots (more hits on same targets than remaining HP) are allowed
-		EWallPart::EWallPart target = *RandomGeneratorUtil::nextItem(allowedTargets, *server->getRNG());
+		EWallPart target = *RandomGeneratorUtil::nextItem(allowedTargets, *server->getRNG());
 
-		auto state = m->battle()->battleGetWallState(target);
 
 		auto attackInfo = ca.attackedParts.begin();
 		for ( ; attackInfo != ca.attackedParts.end(); ++attackInfo)
@@ -127,11 +124,19 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 		{
 			attackInfo->damageDealt += 1;
 		}
+	}
 
+	server->apply(&ca);
+
+	BattleUnitsChanged removeUnits;
+
+	for (auto const wallPart : { EWallPart::KEEP, EWallPart::BOTTOM_TOWER, EWallPart::UPPER_TOWER })
+	{
 		//removing creatures in turrets / keep if one is destroyed
 		BattleHex posRemove;
+		auto state = m->battle()->battleGetWallState(wallPart);
 
-		switch(target)
+		switch(wallPart)
 		{
 		case EWallPart::KEEP:
 			posRemove = BattleHex::CASTLE_CENTRAL_TOWER;
@@ -144,35 +149,19 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 			break;
 		}
 
-		if(posRemove != BattleHex::INVALID && state - attackInfo->damageDealt <= 0) //HP enum subtraction not intuitive, consider using SiegeInfo::applyDamage
+		if(state == EWallState::DESTROYED) //HP enum subtraction not intuitive, consider using SiegeInfo::applyDamage
 		{
 			auto all = m->battle()->battleGetUnitsIf([=](const battle::Unit * unit)
 			{
-				return !unit->isGhost();
+				return !unit->isGhost() && unit->getPosition() == posRemove;
 			});
 
+			assert(all.size() == 0 || all.size() == 1);
 			for(auto & elem : all)
-			{
-				if(elem->getPosition() != posRemove)
-					continue;
-
-				// if tower was hit multiple times, it may have been destroyed already
-				bool stackWasRemovedBefore = false;
-				for(auto & removed : removeUnits.changedStacks)
-				{
-					if (removed.id == elem->unitId())
-						stackWasRemovedBefore = true;
-				}
-
-				if (!stackWasRemovedBefore)
-					removeUnits.changedStacks.emplace_back(elem->unitId(), UnitChanges::EOperation::REMOVE);
-				break;
-			}
+				removeUnits.changedStacks.emplace_back(elem->unitId(), UnitChanges::EOperation::REMOVE);
 		}
 	}
 
-	server->apply(&ca);
-
 	if(!removeUnits.changedStacks.empty())
 		server->apply(&removeUnits);
 }

lib/spells/effects/Obstacle.cpp

@@ -256,11 +256,13 @@ bool Obstacle::isHexAvailable(const CBattleInfoCallback * cb, const BattleHex &
 
 	if(cb->battleGetSiegeLevel() != 0)
 	{
-		EWallPart::EWallPart part = cb->battleHexToWallPart(hex);
+		EWallPart part = cb->battleHexToWallPart(hex);
 
-		if(part == EWallPart::INVALID || part == EWallPart::INDESTRUCTIBLE_PART_OF_GATE)
+		if(part == EWallPart::INVALID)
 			return true;//no fortification here
-		else if(static_cast<int>(part) < 0)
+		else if(part == EWallPart::INDESTRUCTIBLE_PART_OF_GATE)
+			return true; // location accessible
+		else if(part == EWallPart::INDESTRUCTIBLE_PART)
 			return false;//indestructible part (cant be checked by battleGetWallState)
 		else if(part == EWallPart::BOTTOM_TOWER || part == EWallPart::UPPER_TOWER)
 			return false;//destructible, but should not be available

server/CGameHandler.cpp

@@ -4440,6 +4440,26 @@ static EndAction end_action;
 
 void CGameHandler::updateGateState()
 {
+	// GATE_BRIDGE - leftmost tile, located over moat
+	// GATE_OUTER - central tile, mostly covered by gate image
+	// GATE_INNER - rightmost tile, inside the walls
+
+	// GATE_OUTER or GATE_INNER:
+	// - if defender moves unit on these tiles, bridge will open
+	// - if there is a creature (dead or alive) on these tiles, bridge will always remain open
+	// - blocked to attacker if bridge is closed
+
+	// GATE_BRIDGE
+	// - if there is a unit or corpse here, bridge can't open (and can't close in fortress)
+	// - if Force Field is cast here, bridge can't open (but can close, in any town)
+	// - deals moat damage to attacker if bridge is closed (fortress only)
+
+	bool hasForceFieldOnBridge = !battleGetAllObstaclesOnPos(BattleHex(ESiegeHex::GATE_BRIDGE), true).empty();
+	bool hasStackAtGateInner   = gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_INNER), false) != nullptr;
+	bool hasStackAtGateOuter   = gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_OUTER), false) != nullptr;
+	bool hasStackAtGateBridge  = gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_OUTER), false) != nullptr;
+	bool hasLongBridge         = gs->curB->town->subID == ETownType::FORTRESS;
+
 	BattleUpdateGateState db;
 	db.state = gs->curB->si.gateState;
 	if (gs->curB->si.wallState[EWallPart::GATE] == EWallState::DESTROYED)
@@ -4448,24 +4468,23 @@ void CGameHandler::updateGateState()
 	}
 	else if (db.state == EGateState::OPENED)
 	{
-		if (!gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_OUTER), false) &&
-			!gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_INNER), false))
-		{
-			if (gs->curB->town->subID == ETownType::FORTRESS)
-			{
-				if (!gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_BRIDGE), false))
-					db.state = EGateState::CLOSED;
-			}
-			else if (gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_BRIDGE)))
-				db.state = EGateState::BLOCKED;
-			else
-				db.state = EGateState::CLOSED;
-		}
+		bool hasStackOnLongBridge = hasStackAtGateBridge && hasLongBridge;
+		bool gateCanClose = !hasStackAtGateInner && !hasStackAtGateOuter && !hasStackOnLongBridge;
+
+		if (gateCanClose)
+			db.state = EGateState::CLOSED;
+		else
+			db.state = EGateState::OPENED;
+	}
+	else // CLOSED or BLOCKED
+	{
+		bool gateBlocked = hasForceFieldOnBridge || hasStackAtGateBridge;
+
+		if (gateBlocked)
+			db.state = EGateState::BLOCKED;
+		else
+			db.state = EGateState::CLOSED;
 	}
-	else if (gs->curB->battleGetStackByPos(BattleHex(ESiegeHex::GATE_BRIDGE), false))
-		db.state = EGateState::BLOCKED;
-	else
-		db.state = EGateState::CLOSED;
 
 	if (db.state != gs->curB->si.gateState)
 		sendAndApply(&db);
@@ -4804,7 +4823,7 @@ bool CGameHandler::makeBattleAction(BattleAction &ba)
 	case EActionType::CATAPULT:
 		{
 			//TODO: unify with spells::effects:Catapult
-			auto getCatapultHitChance = [&](EWallPart::EWallPart part, const CHeroHandler::SBallisticsLevelInfo & sbi) -> int
+			auto getCatapultHitChance = [](EWallPart part, const CHeroHandler::SBallisticsLevelInfo & sbi) -> int
 			{
 				switch(part)
 				{
@@ -4825,115 +4844,105 @@ bool CGameHandler::makeBattleAction(BattleAction &ba)
 				}
 			};
 
-			auto wrapper = wrapAction(ba);
-
-			if(target.size() < 1)
+			auto getBallisticsInfo = [this, &ba] (const CStack * actor)
 			{
-				complain("Destination required for catapult action.");
-				ok = false;
-				break;
-			}
-			auto destination = target.at(0).hexValue;
-
-			const CGHeroInstance * attackingHero = gs->curB->battleGetFightingHero(ba.side);
+				const CGHeroInstance * attackingHero = gs->curB->battleGetFightingHero(ba.side);
 
-			CHeroHandler::SBallisticsLevelInfo stackBallisticsParameters;
-			if(stack->getCreature()->idNumber == CreatureID::CATAPULT)
-				stackBallisticsParameters = VLC->heroh->ballistics.at(attackingHero->valOfBonuses(Bonus::SECONDARY_SKILL_PREMY, SecondarySkill::BALLISTICS));
-			else
-			{
-				if(stack->hasBonusOfType(Bonus::CATAPULT_EXTRA_SHOTS)) //by design use advanced ballistics parameters with this bonus present, upg. cyclops use advanced ballistics, nonupg. use basic in OH3
-				{
-					stackBallisticsParameters = VLC->heroh->ballistics.at(2);
-					stackBallisticsParameters.shots = 1; //skip default "2 shots" from adv. ballistics
-				}
+				if(actor->getCreature()->idNumber == CreatureID::CATAPULT)
+					return VLC->heroh->ballistics.at(attackingHero->valOfBonuses(Bonus::SECONDARY_SKILL_PREMY, SecondarySkill::BALLISTICS));
 				else
-					stackBallisticsParameters = VLC->heroh->ballistics.at(1);
+				{
+					//by design use advanced ballistics parameters with this bonus present, upg. cyclops use advanced ballistics, nonupg. use basic in OH3
+					int ballisticsLevel = actor->hasBonusOfType(Bonus::CATAPULT_EXTRA_SHOTS) ? 2 : 1;
 
-				stackBallisticsParameters.shots += std::max(stack->valOfBonuses(Bonus::CATAPULT_EXTRA_SHOTS), 0); //0 is allowed minimum to let modders force advanced ballistics for "oneshotting creatures"
-			}
+					auto parameters = VLC->heroh->ballistics.at(ballisticsLevel);
+					parameters.shots = 1 + std::max(actor->valOfBonuses(Bonus::CATAPULT_EXTRA_SHOTS), 0);
+
+					return parameters;
+				}
+			};
 
-			auto wallPart = gs->curB->battleHexToWallPart(destination);
-			if (!gs->curB->isWallPartPotentiallyAttackable(wallPart))
+			auto isWallPartAttackable = [this] (EWallPart part)
 			{
-				complain("catapult tried to attack non-catapultable hex!");
-				break;
-			}
+				return (gs->curB->si.wallState[part] == EWallState::REINFORCED || gs->curB->si.wallState[part] == EWallState::INTACT || gs->curB->si.wallState[part] == EWallState::DAMAGED);
+			};
 
-			//in successive iterations damage is dealt but not yet subtracted from wall's HPs
-			auto &currentHP = gs->curB->si.wallState;
+			CHeroHandler::SBallisticsLevelInfo stackBallisticsParameters = getBallisticsInfo(stack);
 
-			if (currentHP.at(wallPart) == EWallState::DESTROYED  ||  currentHP.at(wallPart) == EWallState::NONE)
-			{
-				complain("catapult tried to attack already destroyed wall part!");
-				break;
-			}
+			auto wrapper = wrapAction(ba);
+			auto destination = target.empty() ? BattleHex(BattleHex::INVALID) : target.at(0).hexValue;
+			auto desiredTarget = gs->curB->battleHexToWallPart(destination);
 
-			for (int g=0; g<stackBallisticsParameters.shots; ++g)
+			for (int shotNumber=0; shotNumber<stackBallisticsParameters.shots; ++shotNumber)
 			{
-				bool hitSuccessfull = false;
-				auto attackedPart = wallPart;
+				auto actualTarget = EWallPart::INVALID;
 
-				do // catapult has chance to attack desired target. Otherwise - attacks randomly
+				if ( isWallPartAttackable(desiredTarget) &&
+					 getRandomGenerator().nextInt(99) < getCatapultHitChance(desiredTarget, stackBallisticsParameters))
 				{
-					if (currentHP.at(attackedPart) != EWallState::DESTROYED && // this part can be hit
-					   currentHP.at(attackedPart) != EWallState::NONE &&
-					   getRandomGenerator().nextInt(99) < getCatapultHitChance(attackedPart, stackBallisticsParameters))//hit is successful
-					{
-						hitSuccessfull = true;
-					}
-					else // select new target
-					{
-						std::vector<EWallPart::EWallPart> allowedTargets;
-						for (size_t i=0; i< currentHP.size(); i++)
-						{
-							if(currentHP.at(i) != EWallState::DESTROYED &&
-								currentHP.at(i) != EWallState::NONE)
-								allowedTargets.push_back(EWallPart::EWallPart(i));
-						}
-						if (allowedTargets.empty())
-							break;
-						attackedPart = *RandomGeneratorUtil::nextItem(allowedTargets, getRandomGenerator());
-					}
+					actualTarget = desiredTarget;
 				}
-				while (!hitSuccessfull);
+				else
+				{
+					static const std::array<EWallPart, 4> walls = { EWallPart::BOTTOM_WALL, EWallPart::BELOW_GATE, EWallPart::OVER_GATE, EWallPart::UPPER_WALL };
+					static const std::array<EWallPart, 3> towers= { EWallPart::BOTTOM_TOWER, EWallPart::KEEP, EWallPart::UPPER_TOWER };
+					static const EWallPart gates = EWallPart::GATE;
 
-				if (!hitSuccessfull) // break triggered - no target to shoot at
-					break;
+					// in H3, catapult under automatic control will attack objects in following order:
+					// walls, gates, towers
+					std::vector<EWallPart> potentialTargets;
+					for (auto & part : walls )
+						if (isWallPartAttackable(part))
+							potentialTargets.push_back(part);
 
-				CatapultAttack ca; //package for clients
-				CatapultAttack::AttackInfo attack;
-				attack.attackedPart = attackedPart;
-				attack.destinationTile = destination;
-				attack.damageDealt = 0;
-				BattleUnitsChanged removeUnits;
+					if (potentialTargets.empty() && isWallPartAttackable(gates))
+							potentialTargets.push_back(gates);
 
-				int dmgChance[] = { stackBallisticsParameters.noDmg, stackBallisticsParameters.oneDmg, stackBallisticsParameters.twoDmg }; //dmgChance[i] - chance for doing i dmg when hit is successful
+					if (potentialTargets.empty())
+						for (auto & part : towers )
+							if (isWallPartAttackable(part))
+								potentialTargets.push_back(part);
+
+					if (potentialTargets.empty())
+						break; // everything is gone, can't attack anymore
+
+					actualTarget = *RandomGeneratorUtil::nextItem(potentialTargets, getRandomGenerator());
+				}
+				assert(actualTarget != EWallPart::INVALID);
+
+				std::array<int, 3> damageChances = { stackBallisticsParameters.noDmg, stackBallisticsParameters.oneDmg, stackBallisticsParameters.twoDmg }; //dmgChance[i] - chance for doing i dmg when hit is successful
+				int totalChance = std::accumulate(damageChances.begin(), damageChances.end(), 0);
+				int damageRandom = getRandomGenerator().nextInt(totalChance - 1);
+				int dealtDamage = 0;
 
-				int dmgRand = getRandomGenerator().nextInt(99);
-				//accumulating dmgChance
-				dmgChance[1] += dmgChance[0];
-				dmgChance[2] += dmgChance[1];
 				//calculating dealt damage
-				for (int damage = 0; damage < ARRAY_COUNT(dmgChance); ++damage)
+				for (int damage = 0; damage < damageChances.size(); ++damage)
 				{
-					if (dmgRand <= dmgChance[damage])
+					if (damageRandom <= damageChances[damage])
 					{
-						attack.damageDealt = damage;
+						dealtDamage = damage;
 						break;
 					}
+					damageRandom -= damageChances[damage];
 				}
-				// attacked tile may have changed - update destination
-				attack.destinationTile = gs->curB->wallPartToBattleHex(EWallPart::EWallPart(attack.attackedPart));
+
+				CatapultAttack::AttackInfo attack;
+				attack.attackedPart = actualTarget;
+				attack.destinationTile = gs->curB->wallPartToBattleHex(actualTarget);
+				attack.damageDealt = dealtDamage;
+
+				CatapultAttack ca; //package for clients
+				ca.attacker = ba.stackNumber;
+				ca.attackedParts.push_back(attack);
+				sendAndApply(&ca);
 
 				logGlobal->trace("Catapult attacks %d dealing %d damage", (int)attack.attackedPart, (int)attack.damageDealt);
 
 				//removing creatures in turrets / keep if one is destroyed
-				if (currentHP.at(attackedPart) - attack.damageDealt <= 0 && (attackedPart == EWallPart::KEEP || //HP enum subtraction not intuitive, consider using SiegeInfo::applyDamage
-					attackedPart == EWallPart::BOTTOM_TOWER || attackedPart == EWallPart::UPPER_TOWER))
+				if (gs->curB->si.wallState[actualTarget] == EWallState::DESTROYED && (actualTarget == EWallPart::KEEP || actualTarget == EWallPart::BOTTOM_TOWER || actualTarget == EWallPart::UPPER_TOWER))
 				{
 					int posRemove = -1;
-					switch(attackedPart)
+					switch(actualTarget)
 					{
 					case EWallPart::KEEP:
 						posRemove = BattleHex::CASTLE_CENTRAL_TOWER;
@@ -4950,18 +4959,13 @@ bool CGameHandler::makeBattleAction(BattleAction &ba)
 					{
 						if(elem->initialPosition == posRemove)
 						{
+							BattleUnitsChanged removeUnits;
 							removeUnits.changedStacks.emplace_back(elem->unitId(), UnitChanges::EOperation::REMOVE);
+							sendAndApply(&removeUnits);
 							break;
 						}
 					}
 				}
-				ca.attacker = ba.stackNumber;
-				ca.attackedParts.push_back(attack);
-
-				sendAndApply(&ca);
-
-				if(!removeUnits.changedStacks.empty())
-					sendAndApply(&removeUnits);
 			}
 			//finish by scope guard
 			break;
@@ -6783,8 +6787,6 @@ void CGameHandler::runBattle()
 				if (!curOwner || getRandomGenerator().nextInt(99) >= curOwner->valOfBonuses(Bonus::MANUAL_CONTROL, CreatureID::CATAPULT))
 				{
 					BattleAction attack;
-					auto destination = *RandomGeneratorUtil::nextItem(attackableBattleHexes, getRandomGenerator());
-					attack.aimToHex(destination);
 					attack.actionType = EActionType::CATAPULT;
 					attack.side = next->side;
 					attack.stackNumber = next->ID;