Correctly handle wares carried into harbors with no path to goal

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

@@ -628,53 +628,72 @@ void nobHarborBuilding::ShipArrived(noShip& ship)
     }
 }
 
-/// Legt eine Ware im Lagerhaus ab
 void nobHarborBuilding::AddWare(std::unique_ptr<Ware> ware)
 {
     if(ware->GetGoal() && ware->GetGoal() != this)
     {
         // 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
 
-    // Brauchen wir die Ware?
+    // Do we need the ware for an expedition?
     if(expedition.active)
     {
         if((ware->type == GoodType::Boards && expedition.boards < BUILDING_COSTS[BuildingType::HarborBuilding].boards)
            || (ware->type == GoodType::Stones
                && expedition.stones < BUILDING_COSTS[BuildingType::HarborBuilding].stones))
         {
+            // Don't wait for it any longer if it had a goal, i.e. us.
+            // Without a goal it was a "lost" ware.
+            if(ware->GetGoal())
+                RemoveDependentWare(*ware);
+            // Add to expedition
+            world->GetPlayer(player).RemoveWare(*ware);
             if(ware->type == GoodType::Boards)
                 ++expedition.boards;
             else
                 ++expedition.stones;
 
-            // Ware nicht mehr abhängig
-            if(ware->GetGoal())
-                RemoveDependentWare(*ware);
-            // Dann zweigen wir die einfach mal für die Expedition ab
-            world->GetPlayer(player).RemoveWare(*ware);
-
-            // Ggf. ist jetzt alles benötigte da
+            // Could be ready now
             CheckExpeditionReady();
             return;
         }

libs/s25main/figures/nofCarrier.cpp

@@ -375,60 +375,56 @@ void nofCarrier::Walked()
         break;
         case CarrierState::CarryWare:
         {
-            // Sind wir schon da?
+            // Reached target flag?
             if(rs_pos == cur_rs->GetLength())
             {
-                // Flagge, an der wir gerade stehen
+                // Flag just reached
                 auto* this_flag = static_cast<noFlag*>(((rs_dir) ? workplace->GetF1() : workplace->GetF2()));
 
                 bool calculated = false;
 
-                // Will die Waren jetzt gleich zur Baustelle neben der Flagge?
+                // Check if the ware should go into the building connected to the flag
                 if(WantInBuilding(&calculated))
                 {
-                    // Erst noch zur Baustelle bzw Gebäude laufen
+                    // Walk to building or building site
                     state = CarrierState::CarryWareToBuilding;
                     StartWalking(Direction::NorthWest);
                     cur_rs = this_flag->GetRoute(Direction::NorthWest);
-                    // location wird immer auf nächste Flagge gesetzt --> in dem Fall aktualisieren
+                    // Set location to next road node, i.e. building
                     carried_ware->Carry((cur_rs->GetF1() == this_flag) ? cur_rs->GetF2() : cur_rs->GetF1());
                 } else
                 {
-                    // Ist an der Flagge noch genügend Platz (wenn wir wieder eine Ware mitnehmen, kann sie auch voll
-                    // sein)
+                    // Put ware at flag if there is space.
+                    // If not try swapping it with another ware at that flag
                     if(this_flag->HasSpaceForWare())
                     {
                         carried_ware->WaitAtFlag(this_flag);
 
-                        // Ware soll ihren weiteren Weg berechnen
+                        // Calc next route if not done in the WantInBuilding call
                         if(!calculated)
                             carried_ware->RecalcRoute();
 
-                        // Ware ablegen
+                        // Put down ware
                         this_flag->AddWare(std::move(carried_ware));
                         RTTR_Assert(carried_ware == nullptr);
-                        // Gibts an den Flaggen etwas, was ich tragen muss, ansonsten wieder in die Mitte gehen und
-                        // warten
+                        // Check if we can pick up another ware at this flag, else go back to middle of road
                         LookForWares();
                     } else if(workplace->AreWareJobs(!rs_dir, ct, true))
                     {
-                        // die Flagge ist voll, aber wir können eine Ware mitnehmen, daher erst Ware nehmen und dann
-                        // erst ablegen
+                        // We can swap the ware with another one at this flag
 
-                        // Ware "merken"
+                        // Temporary store carried ware, so we can fetch another one before putting down the old one
+                        // which triggers a search for an available carrier which must not be us
                         auto tmp_ware = std::move(carried_ware);
-                        // neue Ware aufnehmen
                         FetchWare(true);
 
-                        // alte Ware ablegen
                         tmp_ware->WaitAtFlag(this_flag);
-
                         if(!calculated)
                             tmp_ware->RecalcRoute();
                         this_flag->AddWare(std::move(tmp_ware));
                     } else
                     {
-                        // wenn kein Platz mehr ist --> wieder umdrehen und zurückgehen
+                        // No space at flag, go back and try again later
                         state = CarrierState::GoBackFromFlag;
                         rs_dir = !rs_dir;
                         rs_pos = cur_rs->GetLength() - rs_pos;
@@ -437,16 +433,16 @@ void nofCarrier::Walked()
                 }
             } else if(rs_pos == cur_rs->GetLength() - 1)
             {
-                // Wenn wir fast da sind, gucken, ob an der Flagge noch ein freier Platz ist
+                // If we are one step before the flag, check if we have to wait for space
                 auto* this_flag = static_cast<noFlag*>(((rs_dir) ? workplace->GetF1() : workplace->GetF2()));
-
+                // If there is space at the flag, or we can carry it directly to the building or swap it with another
+                // ware continue to the flag
                 if(this_flag->HasSpaceForWare() || WantInBuilding(nullptr) || cur_rs->AreWareJobs(!rs_dir, ct, true))
                 {
-                    // Es ist Platz, dann zur Flagge laufen
                     StartWalking(cur_rs->GetDir(rs_dir, rs_pos));
                 } else
                 {
-                    // Wenn kein Platz ist, stehenbleiben und warten!
+                    // No space at flag, wait here
                     state = CarrierState::WaitForWareSpace;
                     FaceDir(cur_rs->GetDir(rs_dir, rs_pos));
                 }

libs/s25main/figures/nofWarehouseWorker.cpp

@@ -18,10 +18,10 @@ nofWarehouseWorker::nofWarehouseWorker(const MapPoint pos, const unsigned char p
     : noFigure(Job::Helper, pos, player, world->GetSpecObj<noFlag>(world->GetNeighbour(pos, Direction::SouthEast))),
       carried_ware(std::move(ware)), shouldBringWareIn(task), fat((RANDOM_RAND(2)) != 0)
 {
-    // Zur Inventur hinzufügen, sind ja sonst nicht registriert
+    // New figure
     world->GetPlayer(player).IncreaseInventoryJob(Job::Helper, 1);
 
-    /// Straße (also die 1-er-Straße vor dem Lagerhaus) setzen
+    /// Set the road to building flag (1-piece)
     cur_rs = static_cast<noFlag*>(GetGoal())->GetRoute(Direction::NorthWest);
     RTTR_Assert(cur_rs->GetLength() == 1);
     rs_dir = true;
@@ -31,7 +31,6 @@ nofWarehouseWorker::~nofWarehouseWorker() = default;
 
 void nofWarehouseWorker::Destroy()
 {
-    // Ware vernichten (abmelden)
     RTTR_Assert(!carried_ware); // TODO Check if this holds true and remove the LooseWare below
     LooseWare();
 }
@@ -52,7 +51,6 @@ nofWarehouseWorker::nofWarehouseWorker(SerializedGameData& sgd, const unsigned o
 
 void nofWarehouseWorker::Draw(DrawPoint drawPt)
 {
-    // Trage ich ne Ware oder nicht?
     if(carried_ware)
         DrawWalkingCarrier(drawPt, carried_ware->type, fat);
     else
@@ -61,85 +59,81 @@ void nofWarehouseWorker::Draw(DrawPoint drawPt)
 
 void nofWarehouseWorker::GoalReached()
 {
-    const nobBaseWarehouse* wh = world->GetSpecObj<nobBaseWarehouse>(world->GetNeighbour(pos, Direction::NorthWest));
+    auto* wh = world->GetSpecObj<nobBaseWarehouse>(world->GetNeighbour(pos, Direction::NorthWest));
+    RTTR_Assert(wh); // When worker is still working, the warehouse (and its flag) exists
+    auto* flag = wh->GetFlag();
+    RTTR_Assert(flag);
     if(!shouldBringWareIn)
     {
-        // Ware an der Fahne ablegen ( wenn noch genug Platz ist, 8 max pro Flagge!)
-        // außerdem ggf. Waren wieder mit reinnehmen, deren Zi­el zerstört wurde
-        // ( dann ist goal = location )
-        if(world->GetSpecObj<noFlag>(pos)->HasSpaceForWare() && carried_ware->GetGoal() != carried_ware->GetLocation()
-           && carried_ware->GetGoal() != wh)
+        // Put ware down at flag if enough space.
+        // Might need to take it back in if goal was destroyed or changed to the warehouse
+        if(flag->HasSpaceForWare() && carried_ware->GetGoal() && carried_ware->GetGoal() != wh)
         {
-            carried_ware->WaitAtFlag(world->GetSpecObj<noFlag>(pos));
+            // TODO: Remove assert. Was added to verify prior condition
+            RTTR_Assert(carried_ware->GetGoal() != carried_ware->GetLocation());
 
-            // Ware soll ihren weiteren Weg berechnen
+            carried_ware->WaitAtFlag(flag);
             carried_ware->RecalcRoute();
-
-            // Ware ablegen
-            world->GetSpecObj<noFlag>(pos)->AddWare(std::move(carried_ware));
+            flag->AddWare(std::move(carried_ware));
         } else
-            // ansonsten Ware wieder mit reinnehmen
-            carried_ware->Carry(world->GetSpecObj<noRoadNode>(world->GetNeighbour(pos, Direction::NorthWest)));
+        {
+            // Bring back in
+            carried_ware->Carry(wh);
+        }
     } else
     {
-        // Ware aufnehmen
-        carried_ware = world->GetSpecObj<noFlag>(pos)->SelectWare(Direction::NorthWest, false, this);
-
+        // Take ware if any
+        carried_ware = flag->SelectWare(Direction::NorthWest, false, this);
         if(carried_ware)
-            carried_ware->Carry(world->GetSpecObj<noRoadNode>(world->GetNeighbour(pos, Direction::NorthWest)));
+            carried_ware->Carry(wh);
     }
 
-    // Wieder ins Schloss gehen
+    // Start walking back
     StartWalking(Direction::NorthWest);
     InitializeRoadWalking(wh->GetRoute(Direction::SouthEast), 0, false);
 }
 
 void nofWarehouseWorker::Walked()
 {
-    // Wieder im Schloss angekommen
+    // Arrived back. Check if we were supposed to bring a ware or carry on out
     if(!shouldBringWareIn)
     {
-        // If I still cary a ware than either the flag was full or I should not bring it there (goal=warehouse or goal
-        // destroyed -> goal=location) So re-add it to waiting wares or to inventory
+        // If I still carry a ware than either the flag was full or I should not bring it there.
+        // So re-add it to waiting wares or to inventory
         if(carried_ware)
         {
-            // Ware ins Lagerhaus einlagern (falls es noch existiert und nicht abgebrannt wurde)
+            // Add to warehouse inventory (if it still exists and was not burnt down)
             if(world->GetNO(pos)->GetType() == NodalObjectType::Building)
             {
                 auto* wh = world->GetSpecObj<nobBaseWarehouse>(pos);
-                if(carried_ware->GetGoal() == carried_ware->GetLocation() || carried_ware->GetGoal() == wh)
+                // Store the ware if its goal is this warehouse or it has no goal (anymore)
+                // Else it wants to go somewhere else, so add to waiting wares
+                if(!carried_ware->GetGoal() || carried_ware->GetGoal() == wh)
+                {
+                    // TODO: Remove assert. Was added to verify prior condition
+                    RTTR_Assert(!carried_ware->GetGoal() || carried_ware->GetGoal() == carried_ware->GetLocation());
                     wh->AddWare(std::move(carried_ware));
-                else
+                } else
                     wh->AddWaitingWare(std::move(carried_ware));
             } else
-            {
-                // Lagerhaus abgebrannt --> Ware vernichten
-                LooseWare();
-            }
-            // Ich trage keine Ware mehr
+                LooseWare(); // Warehouse is missing --> destroy ware
             RTTR_Assert(carried_ware == nullptr);
         }
     } else
     {
         if(carried_ware)
         {
-            // Ware ins Lagerhaus einlagern (falls es noch existiert und nicht abgebrannt wurde)
+            // Add ware to warehouse if it still exists
             if(world->GetNO(pos)->GetType() == NodalObjectType::Building)
                 world->GetSpecObj<nobBaseWarehouse>(pos)->AddWare(std::move(carried_ware));
             else
-            {
-                // Lagerhaus abgebrannt --> Ware vernichten
-                LooseWare();
-            }
-            // Ich trage keine Ware mehr
+                LooseWare(); // Warehouse is missing --> destroy ware
             RTTR_Assert(carried_ware == nullptr);
         }
     }
 
-    // dann mich killen
+    // Remove myself
     GetEvMgr().AddToKillList(world->RemoveFigure(pos, *this));
-
-    // Von der Inventur wieder abziehen
     world->GetPlayer(player).DecreaseInventoryJob(Job::Helper, 1);
 }
 
@@ -151,7 +145,7 @@ void nofWarehouseWorker::AbrogateWorkplace()
 
 void nofWarehouseWorker::LooseWare()
 {
-    // Wenn ich noch ne Ware in der Hand habe, muss die gelöscht werden
+    // Destroy carried ware if any
     if(carried_ware)
     {
         carried_ware->WareLost(player);

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/CMakeLists.txt

@@ -18,6 +18,7 @@ add_library(testWorldFixtures STATIC
     worldFixtures/CreateEmptyWorld.h
     worldFixtures/CreateSeaWorld.cpp
     worldFixtures/CreateSeaWorld.h
+    worldFixtures/GCExecutor.cpp
     worldFixtures/GCExecutor.h
     worldFixtures/initGameRNG.cpp
     worldFixtures/initGameRNG.hpp