Earthquake spell: do not target already destroyed sections

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> possibleTargets =
+	static const std::set<EWallPart::EWallPart> potentialTargets =
 	{
 		EWallPart::KEEP,
 		EWallPart::BOTTOM_TOWER,
@@ -80,7 +80,20 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 		EWallPart::GATE
 	};
 
-	assert(possibleTargets.size() == EWallPart::PARTS_COUNT);
+	assert(potentialTargets.size() == EWallPart::PARTS_COUNT);
+
+	std::set<EWallPart::EWallPart> allowedTargets;
+
+	for (auto const & target : potentialTargets)
+	{
+		auto state = m->battle()->battleGetWallState(target);
+
+		if(state != EWallState::DESTROYED && state != EWallState::NONE)
+			allowedTargets.insert(target);
+	}
+	assert(!allowedTargets.empty());
+	if (allowedTargets.empty())
+		return;
 
 	CatapultAttack ca;
 	ca.attacker = -1;
@@ -89,21 +102,31 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 
 	for(int i = 0; i < targetsToAttack; i++)
 	{
-		//Any destructible part can be hit regardless of its HP. Multiple hit on same target is allowed.
-		EWallPart::EWallPart target = *RandomGeneratorUtil::nextItem(possibleTargets, *server->getRNG());
+		// 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());
 
 		auto state = m->battle()->battleGetWallState(target);
 
-		if(state == EWallState::DESTROYED || state == EWallState::NONE)
-			continue;
-
-		CatapultAttack::AttackInfo attackInfo;
-
-		attackInfo.damageDealt = 1;
-		attackInfo.attackedPart = target;
-		attackInfo.destinationTile = m->battle()->wallPartToBattleHex(target);
+		auto attackInfo = ca.attackedParts.begin();
+		for ( ; attackInfo != ca.attackedParts.end(); ++attackInfo)
+			if ( attackInfo->attackedPart == target )
+				break;
 
-		ca.attackedParts.push_back(attackInfo);
+		if (attackInfo == ca.attackedParts.end()) // new part
+		{
+			CatapultAttack::AttackInfo newInfo;
+			newInfo.damageDealt = 1;
+			newInfo.attackedPart = target;
+			newInfo.destinationTile = m->battle()->wallPartToBattleHex(target);
+			ca.attackedParts.push_back(newInfo);
+			attackInfo = ca.attackedParts.end() - 1;
+		}
+		else // already damaged before, update damage
+		{
+			attackInfo->damageDealt += 1;
+		}
 
 		//removing creatures in turrets / keep if one is destroyed
 		BattleHex posRemove;
@@ -111,17 +134,17 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 		switch(target)
 		{
 		case EWallPart::KEEP:
-			posRemove = -2;
+			posRemove = BattleHex::CASTLE_CENTRAL_TOWER;
 			break;
 		case EWallPart::BOTTOM_TOWER:
-			posRemove = -3;
+			posRemove = BattleHex::CASTLE_BOTTOM_TOWER;
 			break;
 		case EWallPart::UPPER_TOWER:
-			posRemove = -4;
+			posRemove = BattleHex::CASTLE_UPPER_TOWER;
 			break;
 		}
 
-		if(posRemove != BattleHex::INVALID && state - attackInfo.damageDealt <= 0) //HP enum subtraction not intuitive, consider using SiegeInfo::applyDamage
+		if(posRemove != BattleHex::INVALID && state - attackInfo->damageDealt <= 0) //HP enum subtraction not intuitive, consider using SiegeInfo::applyDamage
 		{
 			auto all = m->battle()->battleGetUnitsIf([=](const battle::Unit * unit)
 			{
@@ -130,11 +153,20 @@ void Catapult::apply(ServerCallback * server, const Mechanics * m, const EffectT
 
 			for(auto & elem : all)
 			{
-				if(elem->getPosition() == posRemove)
+				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)
 				{
-					removeUnits.changedStacks.emplace_back(elem->unitId(), UnitChanges::EOperation::REMOVE);
-					break;
+					if (removed.id == elem->unitId())
+						stackWasRemovedBefore = true;
 				}
+
+				if (!stackWasRemovedBefore)
+					removeUnits.changedStacks.emplace_back(elem->unitId(), UnitChanges::EOperation::REMOVE);
+				break;
 			}
 		}
 	}

server/CGameHandler.cpp

@@ -4920,13 +4920,13 @@ bool CGameHandler::makeBattleAction(BattleAction &ba)
 					switch(attackedPart)
 					{
 					case EWallPart::KEEP:
-						posRemove = -2;
+						posRemove = BattleHex::CASTLE_CENTRAL_TOWER;
 						break;
 					case EWallPart::BOTTOM_TOWER:
-						posRemove = -3;
+						posRemove = BattleHex::CASTLE_BOTTOM_TOWER;
 						break;
 					case EWallPart::UPPER_TOWER:
-						posRemove = -4;
+						posRemove = BattleHex::CASTLE_UPPER_TOWER;
 						break;
 					}