Fix waiting attackers becoming stuck when target flag becomes unreachable.

libs/s25main/buildings/nobBaseMilitary.cpp

@@ -165,29 +165,32 @@ nofAttacker* nobBaseMilitary::FindAggressor(nofAggressiveDefender& defender)
 
 MapPoint nobBaseMilitary::FindAnAttackerPlace(unsigned short& ret_radius, const nofAttacker& soldier)
 {
-    const MapPoint flagPos = world->GetNeighbour(pos, Direction::SouthEast);
+    const MapPoint flagPos = GetFlagPos();
+    const MapPoint soldierPos = soldier.GetPos();
 
-    // Diesen Flaggenplatz nur nehmen, wenn es auch nich gerade eingenommen wird, sonst gibts Deserteure!
-    // Eigenommen werden können natürlich nur richtige Militärgebäude
-    bool capturing =
-      (BuildingProperties::IsMilitary(bldType_)) ? (static_cast<nobMilitary*>(this)->IsBeingCaptured()) : false;
+    // Only consider  the flag position for fighting if the building isn't currently being captured.
+    // Only "real" military buildings can be captured
+    const bool capturing =
+      BuildingProperties::IsMilitary(bldType_) && static_cast<nobMilitary*>(this)->IsBeingCaptured();
 
     if(!capturing && world->IsValidPointForFighting(flagPos, soldier, false))
     {
-        ret_radius = 0;
-        return flagPos;
+        // Also check if we can reach this.
+        // If not, still consider the other points as the flag could become reachable by then.
+        // TODO(Replay) Limit distance by MAX_ATTACKING_RUN_DISTANCE
+        if(soldierPos == flagPos || world->FindHumanPath(soldierPos, flagPos) != boost::none)
+        {
+            ret_radius = 0;
+            return flagPos;
+        }
     }
 
-    const MapPoint soldierPos = soldier.GetPos();
-    // Get points AROUND the flag. Never AT the flag
-    const auto nodes = world->GetPointsInRadius(flagPos, 3, ReturnMapPointWithRadius{});
-
-    // Weg zu allen möglichen Punkten berechnen und den mit den kürzesten Weg nehmen
-    // Die bisher kürzeste gefundene Länge
+    // Check all points around the flag and take shortest
     unsigned min_length = std::numeric_limits<unsigned>::max();
     MapPoint minPt = MapPoint::Invalid();
-    ret_radius = 100;
-    for(const auto& node : nodes)
+    constexpr auto MAX_RADIUS = 3;
+    ret_radius = MAX_RADIUS;
+    for(const auto& node : world->GetPointsInRadius(flagPos, MAX_RADIUS, ReturnMapPointWithRadius{}))
     {
         // We found a point with a better radius
         if(node.second > ret_radius)
@@ -196,18 +199,19 @@ MapPoint nobBaseMilitary::FindAnAttackerPlace(unsigned short& ret_radius, const
         if(!world->ValidWaitingAroundBuildingPoint(node.first, pos))
             continue;
 
-        // Derselbe Punkt? Dann können wir gleich abbrechen, finden ja sowieso keinen kürzeren Weg mehr
+        // If this is where the soldier currently is, it is already the shortest possible distance
         if(soldierPos == node.first)
         {
             ret_radius = node.second;
             return node.first;
         }
 
         unsigned length = 0;
-        // Gültiger Weg gefunden
+        // Is there a path at all?
+        // TODO(Replay) Limit distance by MAX_ATTACKING_RUN_DISTANCE instead of 100
         if(world->FindHumanPath(soldierPos, node.first, 100, false, &length))
         {
-            // Kürzer als bisher kürzester Weg? --> Dann nehmen wir diesen Punkt (vorerst)
+            // Take if shorter
             if(length < min_length)
             {
                 minPt = node.first;
@@ -252,30 +256,30 @@ bool nobBaseMilitary::CallDefender(nofAttacker& attacker)
 
 nofAttacker* nobBaseMilitary::FindAttackerNearBuilding()
 {
-    // Alle angreifenden Soldaten durchgehen
-    // Den Soldaten, der am nächsten dran steht, nehmen
     nofAttacker* best_attacker = nullptr;
-    unsigned best_radius = 0xFFFFFFFF;
-
-    for(auto* aggressor : aggressors)
+    do
     {
-        // Ist der Soldat überhaupt bereit zum Kämpfen (also wartet er um die Flagge herum oder rückt er nach)?
-        if(aggressor->IsAttackerReady())
+        // Call the closest attacker to fight the defender
+        best_attacker = nullptr;
+        unsigned best_radius = 0xFFFFFFFF;
+        for(auto* aggressor : aggressors)
         {
-            // Besser als bisher bester?
-            if(aggressor->GetRadius() < best_radius || !best_attacker)
+            // Only consider those waiting around the flag, otherwise the defender could go/stay inside
+            if(aggressor->IsAttackerReady())
             {
-                best_attacker = aggressor;
-                best_radius = best_attacker->GetRadius();
+                if(!best_attacker || aggressor->GetRadius() < best_radius)
+                {
+                    best_attacker = aggressor;
+                    best_radius = aggressor->GetRadius();
+                }
             }
         }
-    }
-
-    if(best_attacker)
-        best_attacker->AttackDefenderAtFlag();
-
-    // und ihn zurückgeben, wenns keine gibt, natürlich 0
-    return best_attacker;
+        // Return the attacker if found and he starts attacking, else try again
+        // The current one will be removed from aggressors or not be ready again
+        if(best_attacker && best_attacker->AttackDefenderAtFlag())
+            return best_attacker;
+    } while(best_attacker); // Stop if we didn't found any attacker at all
+    return nullptr;
 }
 
 void nobBaseMilitary::CheckArrestedAttackers()

libs/s25main/figures/nofAggressiveDefender.cpp

@@ -248,6 +248,7 @@ void nofAggressiveDefender::AttackerLost()
 {
     RTTR_Assert(attacker);
     attacker = nullptr;
+    // TODO(Replay) When still in leave queue abort to avoid going out and right back
 }
 
 void nofAggressiveDefender::NeedForHomeDefence()

libs/s25main/figures/nofAttacker.cpp

@@ -605,18 +605,23 @@ bool nofAttacker::AttackDefenderAtFlag()
 {
     // Walk to flag if possible
     const auto dir = world->FindHumanPath(pos, attacked_goal->GetFlagPos(), 3, true);
-    if(!dir)
-        return false;
-
-    const bool waiting_around_building = (state == SoldierState::AttackingWaitingAroundBuilding);
-    state = SoldierState::AttackingAttackingFlag;
+    if(dir)
+    {
+        const bool waiting_around_building = (state == SoldierState::AttackingWaitingAroundBuilding);
+        state = SoldierState::AttackingAttackingFlag;
 
-    if(waiting_around_building)
+        if(waiting_around_building)
+        {
+            StartWalking(*dir);
+            attacked_goal->SendSuccessor(pos, radius);
+        }
+        return true;
+    } else
     {
-        StartWalking(*dir);
-        attacked_goal->SendSuccessor(pos, radius);
+        state = nofActiveSoldier::SoldierState::AttackingWalkingToGoal;
+        MissAttackingWalk();
+        return false;
     }
-    return true;
 }
 
 void nofAttacker::AttackFlag()

tests/s25Main/integration/testAttacking.cpp

@@ -19,6 +19,7 @@
 #include "pathfinding/FindPathForRoad.h"
 #include "worldFixtures/WorldWithGCExecution.h"
 #include "worldFixtures/initGameRNG.hpp"
+#include "worldFixtures/terrainHelpers.h"
 #include "world/GameWorldViewer.h"
 #include "nodeObjs/noFlag.h"
 #include "gameTypes/GameTypesOutput.h"
@@ -27,9 +28,19 @@
 #include <boost/test/unit_test.hpp>
 #include <iostream>
 
+using SoldierState = nofActiveSoldier::SoldierState;
+
+// LCOV_EXCL_START
+static std::ostream& operator<<(std::ostream& out, const SoldierState s)
+{
+    return out << static_cast<unsigned>(rttr::enum_cast(s));
+}
+// LCOV_EXCL_STOP
+
 BOOST_AUTO_TEST_SUITE(AttackSuite)
 
 namespace {
+
 struct AttackDefaults
 {
     static constexpr unsigned width = 20;
@@ -768,6 +779,113 @@ BOOST_FIXTURE_TEST_CASE(ConquerWithCarriersWalkingIn, AttackFixture<2>)
     RTTR_EXEC_TILL(40, !milBld1->IsDoorOpen());
 }
 
+BOOST_FIXTURE_TEST_CASE(FlagBecomesUnreachableForWaitingAttacker, AttackFixture<2>)
+{
+    // Setup:
+    //   - Fight at building flag which is only reachable via road over water
+    //   - 2nd attacker waits on other side of water, 3rd on same side
+    //   - Road gets removed -> 2nd attacker cannot reach flag anymore
+    //   - After fight ended 2nd attacker should go home and 3rd should be chosen to fight
+    // Using 3 attackers to verify the 2nd one is skipped but remaining ones are considered
+    auto* attackerBld = milBld1;
+    auto* defenderBld = milBld0;
+    auto plAttacker = 1;
+    auto plDefender = 0;
+    // Attack from right to left
+    if(milBld0Pos.x < milBld1Pos.x)
+    {
+        using std::swap;
+        swap(attackerBld, defenderBld);
+        swap(plAttacker, plDefender);
+    }
+    const auto attackerBldPos = attackerBld->GetPos();
+    const auto defenderBldPos = defenderBld->GetPos();
+    AddSoldiers(defenderBldPos, 0, 1);
+    AddSoldiersWithRank(attackerBldPos, 6, 0);
+    // Make defender building flag unreachable
+    const auto terrain = this->world.GetDescription().terrain.find(
+      [](const TerrainDesc& t) { return !t.Is(ETerrain::Walkable) && t.GetBQ() != TerrainBQ::Danger; });
+    const auto flagPos = defenderBld->GetFlagPos();
+    // At right there is 1 node of water, so not passable but soldiers can stand on other side
+    /* "B"=Attacked building, "F"=Flag, "L"=Land terrain, Rest is water
+
+      /\  /\  /\LL/
+     /__\/__\/LL\/
+        B
+     \  /\  /\LL/\
+      \/LL\/__\/LL\
+           F       F
+      /\  /\  /\LL/
+     /__\/__\/LL\/
+     --> 1 triangle right of left flag is not walkable and hence F<->F not passable without road
+    */
+    for(const auto dx : helpers::range(-2, 1))
+    {
+        for(const auto dy : helpers::range(-2, 3))
+        {
+            MapPoint pt = world.MakeMapPoint(flagPos + Position(dx, dy));
+            auto& node = world.GetNodeWriteable(pt);
+            node.t1 = node.t2 = terrain;
+        }
+    }
+    {
+        // RSU triangle of node right of B so flag is not reachable from above
+        MapPoint pt = world.GetNeighbour(defenderBldPos, Direction::East);
+        auto& node = world.GetNodeWriteable(pt);
+        node.t1 = terrain;
+    }
+    {
+        // RSU triangle of building so it is reachable from flag
+        auto& node = world.GetNodeWriteable(defenderBldPos);
+        node.t1 = GetLandTerrain(world.GetDescription()); // RSU triangle of node right of B
+    }
+    GameWorldViewer gwv(plAttacker, world);
+    BOOST_TEST_REQUIRE(gwv.GetNumSoldiersForAttack(defenderBldPos) == 0u);
+    // Road over water makes it reachable
+    curPlayer = plDefender;
+    const MapPoint roadFlagPos = world.MakeMapPoint(flagPos + Position(2, 0));
+    this->BuildRoad(flagPos, false, std::vector<Direction>(2, Direction::East));
+    auto* flag = world.GetSpecObj<noFlag>(roadFlagPos);
+    BOOST_TEST_REQUIRE(flag);
+
+    BOOST_TEST_REQUIRE(gwv.GetNumSoldiersForAttack(defenderBldPos) >= 3u);
+
+    curPlayer = plAttacker;
+    this->Attack(defenderBldPos, 3, true);
+    BOOST_TEST_REQUIRE(attackerBld->GetLeavingFigures().size() == 3);
+    auto& attacker1 = dynamic_cast<nofAttacker&>(attackerBld->GetLeavingFigures().front());
+    auto& attacker2 = dynamic_cast<nofAttacker&>(*++attackerBld->GetLeavingFigures().begin());
+    auto& attacker3 = dynamic_cast<nofAttacker&>(*++(++attackerBld->GetLeavingFigures().begin()));
+    // Let all exit and move almost to goal
+    RTTR_EXEC_TILL(200, attackerBld->GetLeavingFigures().empty());
+    moveObjTo(world, attacker1, world.GetNeighbour(flagPos, Direction::East));
+    moveObjTo(world, attacker2, world.GetNeighbour(roadFlagPos, Direction::East));
+    moveObjTo(world, attacker3, world.GetNeighbour(flagPos, Direction::West));
+    RTTR_EXEC_TILL(200, !defenderBld->GetLeavingFigures().empty());
+    auto& defender = dynamic_cast<nofDefender&>(defenderBld->GetLeavingFigures().front());
+    RTTR_EXEC_TILL(200, defender.IsFightingAtFlag());
+    RTTR_EXEC_TILL(10, attacker2.IsAttackerReady() && attacker3.IsAttackerReady());
+    BOOST_TEST_REQUIRE(attacker1.GetState() == SoldierState::AttackingFightingVsDefender);
+
+    // Setup done, now remove the road and verify the goal is no longer attackable
+    curPlayer = plDefender;
+    DestroyRoad(flagPos, Direction::East);
+    BOOST_TEST_REQUIRE(gwv.GetNumSoldiersForAttack(defenderBldPos) == 0u);
+    // Fight ended
+    RTTR_EXEC_TILL(200, !defender.IsFightingAtFlag());
+    // Attacker2 should go home
+    BOOST_TEST(attacker2.GetState() == SoldierState::WalkingHome);
+    BOOST_TEST(!attacker2.GetAttackedGoal());
+    BOOST_TEST(!helpers::contains(defenderBld->GetAggressors(), &attacker2));
+    BOOST_TEST(attacker2.IsMoving()); // Not stuck
+    // Attacker3 should go and fight now
+    BOOST_TEST(!attacker3.IsAttackerReady());
+    BOOST_TEST(attacker3.GetAttackedGoal());
+    BOOST_TEST(attacker3.IsMoving()); // Not stuck
+    RTTR_EXEC_TILL(100, defender.IsFightingAtFlag());
+    BOOST_TEST(attacker3.GetState() == SoldierState::AttackingFightingVsDefender);
+}
+
 using DestroyRoadsOnConquerFixture = AttackFixture<2, 24>;
 BOOST_FIXTURE_TEST_CASE(DestroyRoadsOnConquer, DestroyRoadsOnConquerFixture)
 {
@@ -849,8 +967,8 @@ struct FreeFightFixture : AttackFixture<2>
         moveObjTo(world, defender, fightSpot + MapPoint(1, 0));
         rescheduleWalkEvent(em, attacker, 1);
         RTTR_SKIP_GFS(1);
-        BOOST_TEST_REQUIRE((defender.GetState() == nofActiveSoldier::SoldierState::MeetEnemy));
-        BOOST_TEST_REQUIRE((attacker.GetState() == nofActiveSoldier::SoldierState::MeetEnemy));
+        BOOST_TEST_REQUIRE(defender.GetState() == SoldierState::MeetEnemy);
+        BOOST_TEST_REQUIRE(attacker.GetState() == SoldierState::MeetEnemy);
     }
 
     /// Add blocking terrain around building of defender starting left of the specified position
@@ -889,8 +1007,8 @@ BOOST_FIXTURE_TEST_CASE(Attacker_Returns_When_AgressiveDefender_Aborts, FreeFigh
     moveObjTo(world, attacker, fightSpot);
     rescheduleWalkEvent(em, attacker, 1);
     RTTR_SKIP_GFS(1);
-    BOOST_TEST_REQUIRE((defender.GetState() == nofActiveSoldier::SoldierState::MeetEnemy));
-    BOOST_TEST_REQUIRE((attacker.GetState() == nofActiveSoldier::SoldierState::WaitingForFight));
+    BOOST_TEST_REQUIRE(defender.GetState() == SoldierState::MeetEnemy);
+    BOOST_TEST_REQUIRE(attacker.GetState() == SoldierState::WaitingForFight);
     // The next part of the test assumes they use this fight spot.
     // This doesn't need to stay true which only needs adjustments to the test.
     BOOST_TEST_REQUIRE(attacker.GetPos() == fightSpot);
@@ -903,8 +1021,8 @@ BOOST_FIXTURE_TEST_CASE(Attacker_Returns_When_AgressiveDefender_Aborts, FreeFigh
     RTTR_SKIP_GFS(1);
     BOOST_TEST(defender.IsMoving());
     BOOST_TEST(attacker.IsMoving());
-    BOOST_TEST((defender.GetState() != nofActiveSoldier::SoldierState::MeetEnemy));
-    BOOST_TEST((attacker.GetState() != nofActiveSoldier::SoldierState::WaitingForFight));
+    BOOST_TEST(defender.GetState() != SoldierState::MeetEnemy);
+    BOOST_TEST(attacker.GetState() != SoldierState::WaitingForFight);
 
     BOOST_TEST_REQUIRE(attackedBld.GetNumTroops() == 5u); // Sanity check
     RTTR_EXEC_TILL(100, attackedBld.GetNumTroops() == 6u);
@@ -925,8 +1043,8 @@ BOOST_FIXTURE_TEST_CASE(AgressiveDefender_Returns_When_Attacker_Aborts, FreeFigh
     moveObjTo(world, defender, fightSpot);
     rescheduleWalkEvent(em, defender, 1);
     RTTR_SKIP_GFS(1);
-    BOOST_TEST_REQUIRE((defender.GetState() == nofActiveSoldier::SoldierState::WaitingForFight));
-    BOOST_TEST_REQUIRE((attacker.GetState() == nofActiveSoldier::SoldierState::MeetEnemy));
+    BOOST_TEST_REQUIRE(defender.GetState() == SoldierState::WaitingForFight);
+    BOOST_TEST_REQUIRE(attacker.GetState() == SoldierState::MeetEnemy);
     // The next part of the test assumes they use this fight spot.
     // This doesn't need to stay true which only needs adjustments to the test.
     BOOST_TEST_REQUIRE(defender.GetPos() == fightSpot);
@@ -939,8 +1057,8 @@ BOOST_FIXTURE_TEST_CASE(AgressiveDefender_Returns_When_Attacker_Aborts, FreeFigh
     RTTR_SKIP_GFS(1);
     BOOST_TEST(defender.IsMoving());
     BOOST_TEST(attacker.IsMoving());
-    BOOST_TEST((defender.GetState() != nofActiveSoldier::SoldierState::WaitingForFight));
-    BOOST_TEST((attacker.GetState() != nofActiveSoldier::SoldierState::MeetEnemy));
+    BOOST_TEST(defender.GetState() != SoldierState::WaitingForFight);
+    BOOST_TEST(attacker.GetState() != SoldierState::MeetEnemy);
 
     BOOST_TEST_REQUIRE(attackedBld.GetNumTroops() == 5u); // Sanity check
     RTTR_EXEC_TILL(100, attackedBld.GetNumTroops() == 6u);