Add reproducer for crash due to too far distance to harbor

Author: Flamefire

libs/s25main/world/World.cpp

@@ -425,7 +425,7 @@ unsigned World::CalcHarborDistance(HarborId haborId1, HarborId harborId2) const
         }
     }
 
-    return 0xffffffff;
+    return std::numeric_limits<unsigned>::max();
 }
 
 SeaId World::GetSeaFromCoastalPoint(const MapPoint pt) const

tests/s25Main/integration/testSeafaring.cpp

@@ -712,6 +712,105 @@ BOOST_FIXTURE_TEST_CASE(HarborDestroyed, ShipAndHarborsReadyFixture<1>)
     BOOST_TEST_REQUIRE(ship.GetTargetHarbor() == hb1Id);
 }
 
+// Dimensions must be large enough so path tolerances get too small
+using ShipReadyFixtureLarger = ShipReadyFixture<1, 2, 90, 260>;
+BOOST_FIXTURE_TEST_CASE(IncreasedPathLengthWhenHarborDestroyed, ShipReadyFixtureLarger)
+{
+    // Edge case: Path to harbor longer than any direct connection
+    /*
+     *     2
+     *     W
+     *    WLW
+     *    WLW
+     *   WWLWW
+     * 4WLLLLLW5
+     *   WWLWW
+     *   WWLWW
+     *     7
+     */
+    // Shortest path 4->5 is over south, ship might be between 2 & 4 => still finds path to 5
+    constexpr HarborId hbIdTop(2);
+    constexpr HarborId hbIdLeft(4);
+    constexpr HarborId hbIdRight(5);
+    constexpr HarborId hbIdBottom(7);
+    const MapPoint hbTopPos = world.GetHarborPoint(hbIdTop);
+    const MapPoint hbLeftPos = world.GetHarborPoint(hbIdLeft);
+    const MapPoint hbRightPos = world.GetHarborPoint(hbIdRight);
+    const MapPoint hbBottomPos = world.GetHarborPoint(hbIdBottom);
+    // Sanity check expected geometry, ensured by fixture
+    BOOST_TEST_REQUIRE(hbLeftPos.x < hbRightPos.x);
+    BOOST_TEST_REQUIRE(hbLeftPos.y == hbRightPos.y);
+    BOOST_TEST_REQUIRE(hbTopPos.y < hbLeftPos.y);
+    BOOST_TEST_REQUIRE(hbLeftPos.y < hbBottomPos.y);
+    const unsigned yDiff = hbLeftPos.y - hbTopPos.y;
+    BOOST_TEST_REQUIRE(yDiff >= 4u * 3u); // So divisions below yield noticeable difference
+    const auto t = GetLandTerrain(world.GetDescription(), ETerrain::Buildable);
+    for(const auto y : helpers::range(hbLeftPos.y - yDiff * 3u / 4u, hbLeftPos.y + yDiff / 2u))
+    {
+        auto& node = world.GetNodeWriteable(MapPoint(hbTopPos.x, y));
+        node.t1 = node.t2 = t;
+    }
+    constexpr auto horStripHeight = 2u;
+    // Horizontal land strip to avoid short cuts
+    for(const auto y : helpers::range(hbLeftPos.y - horStripHeight, hbLeftPos.y + horStripHeight))
+    {
+        for(const auto x : helpers::range(hbLeftPos.x + 4, hbRightPos.x - 4))
+        {
+            auto& node = world.GetNodeWriteable(MapPoint(x, y));
+            node.t1 = node.t2 = t;
+        }
+    }
+    MapLoader::InitSeasAndHarbors(world);
+    // Harbors unchanged
+    BOOST_TEST_REQUIRE(world.GetHarborPointID(hbTopPos) == hbIdTop);
+    BOOST_TEST_REQUIRE(world.GetHarborPointID(hbLeftPos) == hbIdLeft);
+    BOOST_TEST_REQUIRE(world.GetHarborPointID(hbRightPos) == hbIdRight);
+    BOOST_TEST_REQUIRE(world.GetHarborPointID(hbBottomPos) == hbIdBottom);
+
+    const GamePlayer& player = world.GetPlayer(curPlayer);
+    const noShip& ship = ensureNonNull(player.GetShipByID(0));
+    const SeaId seaId = ship.GetSeaID();
+
+    // Start shipment to left harbor
+    nobHarborBuilding& topHarbor = *player.GetBuildingRegister().GetHarbors().front();
+    BuildingFactory::CreateBuilding(world, BuildingType::HarborBuilding, hbLeftPos, curPlayer, Nation::Romans);
+
+    topHarbor.AddToInventory(GoodCounts::make(GoodType::Wood, 10), true);
+    SetInventorySetting(hbLeftPos, GoodType::Wood, EInventorySetting::Collect);
+
+    RTTR_EXEC_TILL(90, ship.IsLoading());
+    BOOST_TEST_REQUIRE(ship.GetPos() == world.GetCoastalPoint(hbIdTop, seaId));
+    RTTR_EXEC_TILL(400, ship.IsMoving());
+    BOOST_TEST_REQUIRE(ship.GetTargetHarbor() == hbIdLeft);
+    const auto leftCoastPt = world.GetCoastalPoint(hbIdLeft, seaId);
+    const auto dist = world.CalcDistance(ship.GetPos(), leftCoastPt);
+
+    // Wait till ship is close to left harbor
+    constexpr auto extraDist = 16u; // Choosen to be larger than a magic number used as tolerance
+    RTTR_EXEC_TILL(2000, dist < extraDist * 2);
+
+    // Destroy home so it cannot go back, then target harbor
+    destroyBldAndFire(world, hbTopPos);
+    destroyBldAndFire(world, hbLeftPos);
+    RTTR_EXEC_TILL(10, ship.IsLost());
+    BOOST_TEST_REQUIRE(!ship.GetHomeHarbor());
+    BOOST_TEST_REQUIRE(!ship.GetTargetHarbor());
+
+    // Building harbor at right makes ship go there even though distance is (too) long
+    unsigned len;
+    BOOST_TEST_REQUIRE(
+      world.FindShipPath(ship.GetPos(), world.GetCoastalPoint(hbIdRight, seaId), unsigned(-1), nullptr, &len));
+    const auto hbDistance = world.CalcHarborDistance(hbIdLeft, hbIdRight);
+    // Reproducer requires the path to be longer than the direct distance and other connections
+    BOOST_TEST_REQUIRE(len > hbDistance + extraDist);
+    BOOST_TEST_REQUIRE(hbDistance > world.CalcHarborDistance(hbIdRight, hbIdTop));
+    BOOST_TEST_REQUIRE(hbDistance > world.CalcHarborDistance(hbIdRight, hbIdBottom));
+    BuildingFactory::CreateBuilding(world, BuildingType::HarborBuilding, hbRightPos, curPlayer, Nation::Romans);
+    BOOST_TEST_REQUIRE(ship.IsMoving());
+    BOOST_TEST_REQUIRE(ship.GetHomeHarbor() == hbIdRight);
+    BOOST_TEST_REQUIRE(ship.GetTargetHarbor() == hbIdRight);
+}
+
 struct MockWare : Ware
 {
     static bool destroyed;