Correctly handle wares carried into harbors with no path to goal

When the goal for a ware becomes unreachable while it is carried into a harbor
`RecalcRoute` will call `FindRouteToWarehouse`.
Usually the selected warehouse will be the harbor as it is trivially the closest.
However when the harbor does not accept this ware type another one might be chosen
and the ware needs to be moved out instead of stored.

But `FindRouteToWarehouse` does NOT set `next_dir` while the ware is carried even when
called from `RecalcRoute` where the caller assumes it is set.
Changing this causes replays to go async, so work around it by calling `RecalcRoute` again
which now succeeds as it reached the goal in the previous call.

This bug also affects `WantInBuilding` which also calls `RecalcRoute` assuming it does so in all cases.

Fixes #1843

Author: Flamefire

libs/s25main/Ware.cpp

@@ -112,6 +112,7 @@ void Ware::RecalcRoute()
             static_cast<nobHarborBuilding*>(goal)->WareDontWantToTravelByShip(this);
         } else
         {
+            // TODO(Replay) This should calculate the next dir even when carried
             FindRouteToWarehouse();
         }
     } else

libs/s25main/Ware.h

@@ -83,6 +83,7 @@ class Ware : public GameObject
     bool IsWaitingAtFlag() const { return (state == State::WaitAtFlag); }
     bool IsWaitingInWarehouse() const { return (state == State::WaitInWarehouse); }
     bool IsWaitingForShip() const { return (state == State::WaitForShip); }
+    bool IsCarried() const { return (state == State::Carried); }
     /// Sagt dem Träger Bescheid, dass sie in die aktuelle (next_dir) Richtung nicht mehr getragen werden will
     void RemoveWareJobForDir(RoadPathDirection last_next_dir);
     /// Überprüft, ob es noch ein Weg zum Ziel gibt

libs/s25main/buildings/nobHarborBuilding.cpp

@@ -635,23 +635,42 @@ void nobHarborBuilding::AddWare(std::unique_ptr<Ware> ware)
         // This is not the goal but we have one -> Get new route
         ware->RecalcRoute();
 
-        // Will diese Ware mit dem Schiff irgendwo hin fahren?
+        // Go by ship next?
         if(ware->GetNextDir() == RoadPathDirection::Ship)
         {
-            // Dann fügen wir die mal bei uns hinzu
             AddWareForShip(std::move(ware));
             return;
-        } else if(ware->GetNextDir() != RoadPathDirection::None)
+        }
+        if(ware->GetNextDir() != RoadPathDirection::None)
         {
             // Travel on roads -> Carry out
             RTTR_Assert(ware->GetGoal() != this);
             AddWaitingWare(std::move(ware));
             return;
-        } else
+        }
+        // No next dir means the ware reached its goal, i.e. us,
+        // or there is no valid, reachable goal
+        // In both cases we take it as we initially would have.
+        if(ware->GetGoal() && ware->GetGoal() != this)
         {
-            // Pathfinding failed -> Ware would want to go here
-            RTTR_Assert(ware->GetGoal() == this);
-            // Regular handling below
+            // We have a goal but it isn't this and there is no next dir
+            // This can only happen when the ware was redirected to a warehouse while being carried,
+            // see Ware::FindRouteToWarehouse called by Ware::RecalcRoute
+
+            // TODO(Replay) When Ware::FindRouteToWarehouse recalculates the route when called from RecalcRoute
+            // and next_dir is None then goal will be NULL or us.
+            // So the condition of this branch can never be true and the branch can be replaced by:
+            // RTTR_Assert(!ware->GetGoal() || ware->GetGoal() == this)
+            RTTR_Assert(ware->IsCarried());
+            // Explicitly calculate the route which now should set it.
+            ware->RecalcRoute();
+            RTTR_Assert(ware->GetGoal());
+            RTTR_Assert(ware->GetNextDir() != RoadPathDirection::None);
+            if(ware->GetNextDir() == RoadPathDirection::Ship)
+                AddWareForShip(std::move(ware));
+            else
+                AddWaitingWare(std::move(ware));
+            return;
         }
     }
     // When ware should be transported to any other goal we returned above, so now we need to take it

libs/s25main/gameTypes/GameTypesOutput.h

@@ -22,6 +22,7 @@
 #include "gameTypes/Direction.h"
 #include "gameTypes/FoWNode.h"
 #include "gameTypes/MapTypes.h"
+#include "gameTypes/RoadPathDirection.h"
 #include "gameTypes/TeamTypes.h"
 #include "gameData/DescIdx.h"
 #include <boost/preprocessor/seq/for_each.hpp>
@@ -84,9 +85,10 @@ RTTR_ENUM_OUTPUT(Level, Easy, Medium, Hard)
     }
 
 RTTR_ENUM_OUTPUT(BuildingType)
+RTTR_ENUM_OUTPUT(GO_Type)
 RTTR_ENUM_OUTPUT(Job)
 RTTR_ENUM_OUTPUT(Nation)
-RTTR_ENUM_OUTPUT(GO_Type)
+RTTR_ENUM_OUTPUT(RoadPathDirection)
 
 #undef RTTR_ENUM_OUTPUT
 

tests/s25Main/integration/testSeafaring.cpp

@@ -4,6 +4,7 @@
 
 #include "GamePlayer.h"
 #include "PointOutput.h"
+#include "Ware.h"
 #include "buildings/noBuildingSite.h"
 #include "buildings/nobHarborBuilding.h"
 #include "buildings/nobShipYard.h"
@@ -633,4 +634,89 @@ BOOST_FIXTURE_TEST_CASE(HarborDestroyed, ShipAndHarborsReadyFixture<1>)
     BOOST_TEST_REQUIRE(ship.GetTargetHarbor() == 1u);
 }
 
+struct MockWare : Ware
+{
+    static bool destroyed;
+    MockWare(GoodType type, noBaseBuilding* goal, noRoadNode* location) : Ware(type, goal, location)
+    {
+        destroyed = false;
+    }
+    ~MockWare() { destroyed = true; }
+};
+bool MockWare::destroyed = false;
+
+BOOST_FIXTURE_TEST_CASE(AddWareWithUnreachableGoalToHarbor, ShipAndHarborsReadyFixture<1>)
+{
+    // Recreate issue sensitive to timing:
+    // 1. Ware almost at harbor flag and should be shipped -> will be carried into building
+    // 2. Goal becomes unreachable (e.g. road removed)
+    // 3. Ware reaches harbor and needs to be carried into a storehouse
+    // 4. Harbor itself does not accept the ware so another storehouse must be found
+    GamePlayer& player = world.GetPlayer(curPlayer);
+    auto& harbors = player.GetBuildingRegister().GetHarbors();
+    BOOST_TEST_REQUIRE(harbors.size() >= 2u);
+    nobHarborBuilding& harbor1 = *harbors.front();
+    nobHarborBuilding& harbor2 = **(++harbors.begin());
+    auto& hq = *player.GetFirstWH();
+    auto* wh = static_cast<nobBaseWarehouse*>(BuildingFactory::CreateBuilding(
+      world, BuildingType::Storehouse, harbor2.GetPos() + MapPoint(2, 0), curPlayer, Nation::Romans));
+    BOOST_TEST_REQUIRE(wh);
+    BuildRoadForBlds(harbor2.GetPos(), wh->GetPos());
+    constexpr auto goodType = GoodType::Wood;
+
+    // Sanity check: Move from harbor1 to wh (over harbor2)
+    auto ware = std::make_unique<MockWare>(goodType, wh, &harbor1);
+    auto* warePtr = ware.get();
+    ware->Carry(&harbor1);
+    auto numVisWaresBefore = harbor1.GetNumVisualWares(goodType);
+    auto numRealWaresBefore = harbor1.GetNumRealWares(goodType);
+    harbor1.AddWare(std::move(ware));
+    BOOST_TEST(harbor1.GetNumVisualWares(goodType) == numVisWaresBefore + 1u);
+    // Wares that will be moved out are only in visual count
+    BOOST_TEST(harbor1.GetNumRealWares(goodType) == numRealWaresBefore);
+    BOOST_TEST_REQUIRE(!MockWare::destroyed);
+    BOOST_TEST(warePtr->GetGoal() == wh);
+    BOOST_TEST(warePtr->GetNextDir() == RoadPathDirection::Ship);
+
+    // Add ware with unreachable goal -> Added to harbor
+    // First fully disconnect 2nd harbor -> Only reachable via ship
+    for(const auto d : helpers::enumRange<Direction>())
+    {
+        if(d != Direction::NorthWest)
+            DestroyRoad(harbor2.GetFlagPos(), d);
+    }
+    ware = std::make_unique<MockWare>(goodType, wh, &harbor1);
+    ware->Carry(&harbor1);
+    numVisWaresBefore = harbor1.GetNumVisualWares(goodType);
+    numRealWaresBefore = harbor1.GetNumRealWares(goodType);
+    harbor1.AddWare(std::move(ware));
+    BOOST_TEST(harbor1.GetNumVisualWares(goodType) == numVisWaresBefore + 1u);
+    BOOST_TEST(harbor1.GetNumRealWares(goodType) == numRealWaresBefore + 1u);
+    BOOST_TEST(MockWare::destroyed);
+
+    // Same but disallow harbor as goal -> Find another storehouse
+    SetInventorySetting(harbor1.GetPos(), goodType, EInventorySetting::Stop);
+    ware = std::make_unique<MockWare>(goodType, wh, &harbor1);
+    ware->Carry(&harbor1);
+    warePtr = ware.get();
+    numVisWaresBefore = harbor1.GetNumVisualWares(goodType);
+    numRealWaresBefore = harbor1.GetNumRealWares(goodType);
+    harbor1.AddWare(std::move(ware));
+    BOOST_TEST(harbor1.GetNumVisualWares(goodType) == numVisWaresBefore + 1u);
+    BOOST_TEST(harbor1.GetNumRealWares(goodType) == numRealWaresBefore);
+    BOOST_TEST_REQUIRE(!MockWare::destroyed);
+    BOOST_TEST(warePtr->GetGoal() == &harbor2);
+
+    // If no other storehouse is available take it anyway
+    SetInventorySetting(harbor2.GetPos(), goodType, EInventorySetting::Stop);
+    ware = std::make_unique<MockWare>(goodType, wh, &harbor1);
+    ware->Carry(&harbor1);
+    numVisWaresBefore = harbor1.GetNumVisualWares(goodType);
+    numRealWaresBefore = harbor1.GetNumRealWares(goodType);
+    harbor1.AddWare(std::move(ware));
+    BOOST_TEST(harbor1.GetNumVisualWares(goodType) == numVisWaresBefore + 1u);
+    BOOST_TEST(harbor1.GetNumRealWares(goodType) == numRealWaresBefore + 1u);
+    BOOST_TEST(MockWare::destroyed);
+}
+
 BOOST_AUTO_TEST_SUITE_END()