From bac8a9d0c8e8f298150ace297be422126db60738 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Sun, 4 Jan 2026 12:36:25 +0100 Subject: [PATCH 1/7] Translate some comments --- libs/s25main/buildings/nobHarborBuilding.cpp | 2 +- libs/s25main/figures/nofCarrier.cpp | 40 +++++++--------- libs/s25main/figures/nofWarehouseWorker.cpp | 50 ++++++++------------ 3 files changed, 38 insertions(+), 54 deletions(-) diff --git a/libs/s25main/buildings/nobHarborBuilding.cpp b/libs/s25main/buildings/nobHarborBuilding.cpp index c7ad44a99f..b9027bc1d1 100644 --- a/libs/s25main/buildings/nobHarborBuilding.cpp +++ b/libs/s25main/buildings/nobHarborBuilding.cpp @@ -628,7 +628,6 @@ void nobHarborBuilding::ShipArrived(noShip& ship) } } -/// Legt eine Ware im Lagerhaus ab void nobHarborBuilding::AddWare(std::unique_ptr ware) { if(ware->GetGoal() && ware->GetGoal() != this) @@ -655,6 +654,7 @@ void nobHarborBuilding::AddWare(std::unique_ptr ware) // Regular handling below } } + // When ware should be transported to any other goal we returned above, so now we need to take it // Brauchen wir die Ware? if(expedition.active) diff --git a/libs/s25main/figures/nofCarrier.cpp b/libs/s25main/figures/nofCarrier.cpp index 7f70ef8b8c..655fb2b424 100644 --- a/libs/s25main/figures/nofCarrier.cpp +++ b/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(((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(((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)); } diff --git a/libs/s25main/figures/nofWarehouseWorker.cpp b/libs/s25main/figures/nofWarehouseWorker.cpp index af0a1e044a..bd039286b1 100644 --- a/libs/s25main/figures/nofWarehouseWorker.cpp +++ b/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(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(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 @@ -62,6 +60,7 @@ void nofWarehouseWorker::Draw(DrawPoint drawPt) void nofWarehouseWorker::GoalReached() { const nobBaseWarehouse* wh = world->GetSpecObj(world->GetNeighbour(pos, Direction::NorthWest)); + auto* flag = world->GetSpecObj(pos); if(!shouldBringWareIn) { // Ware an der Fahne ablegen ( wenn noch genug Platz ist, 8 max pro Flagge!) @@ -74,36 +73,35 @@ void nofWarehouseWorker::GoalReached() // Ware soll ihren weiteren Weg berechnen carried_ware->RecalcRoute(); - - // Ware ablegen - world->GetSpecObj(pos)->AddWare(std::move(carried_ware)); + flag->AddWare(std::move(carried_ware)); } else - // ansonsten Ware wieder mit reinnehmen + { + // Bring back in carried_ware->Carry(world->GetSpecObj(world->GetNeighbour(pos, Direction::NorthWest))); + } } else { - // Ware aufnehmen - carried_ware = world->GetSpecObj(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(world->GetNeighbour(pos, Direction::NorthWest))); } - // 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(pos); @@ -112,34 +110,24 @@ void nofWarehouseWorker::Walked() 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(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 +139,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); From 121687bd8f1fd846f47810500257f68ed56c7df5 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Sun, 4 Jan 2026 12:37:45 +0100 Subject: [PATCH 2/7] Simplify condtions for putting down ware at warehouse The check `GetGoal() != GetLocation()` is always met because the location is the flag and the goal is NULL if it was destroyed while the ware is being carried out. --- libs/s25main/figures/nofWarehouseWorker.cpp | 22 ++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/libs/s25main/figures/nofWarehouseWorker.cpp b/libs/s25main/figures/nofWarehouseWorker.cpp index bd039286b1..9fcca1be67 100644 --- a/libs/s25main/figures/nofWarehouseWorker.cpp +++ b/libs/s25main/figures/nofWarehouseWorker.cpp @@ -63,15 +63,14 @@ void nofWarehouseWorker::GoalReached() auto* flag = world->GetSpecObj(pos); 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(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(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(); flag->AddWare(std::move(carried_ware)); } else @@ -105,9 +104,14 @@ void nofWarehouseWorker::Walked() if(world->GetNO(pos)->GetType() == NodalObjectType::Building) { auto* wh = world->GetSpecObj(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 LooseWare(); // Warehouse is missing --> destroy ware From 53b128b82b0dab15bb3a2ff3cb43b86fce3db008 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Sun, 4 Jan 2026 12:42:41 +0100 Subject: [PATCH 3/7] Factor out BuildRoadForBlds to test GCExecutor --- tests/s25Main/CMakeLists.txt | 1 + tests/s25Main/integration/testAttacking.cpp | 11 ----------- tests/s25Main/integration/testSeaAttacking.cpp | 11 ----------- tests/s25Main/worldFixtures/GCExecutor.cpp | 15 +++++++++++++++ tests/s25Main/worldFixtures/GCExecutor.h | 3 +++ 5 files changed, 19 insertions(+), 22 deletions(-) create mode 100644 tests/s25Main/worldFixtures/GCExecutor.cpp diff --git a/tests/s25Main/CMakeLists.txt b/tests/s25Main/CMakeLists.txt index 2edb3e1696..49217a0c0c 100644 --- a/tests/s25Main/CMakeLists.txt +++ b/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 diff --git a/tests/s25Main/integration/testAttacking.cpp b/tests/s25Main/integration/testAttacking.cpp index 55061ecf2d..bafa22e0c8 100644 --- a/tests/s25Main/integration/testAttacking.cpp +++ b/tests/s25Main/integration/testAttacking.cpp @@ -99,17 +99,6 @@ struct AttackFixtureBase : public WorldWithGCExecution road = FindPathForRoad(world, start, end, false); - BOOST_TEST_REQUIRE(!road.empty()); - this->BuildRoad(start, false, road); - BOOST_TEST_REQUIRE(world.GetPointRoad(start, road.front()) == PointRoad::Normal); - } - void AddSoldiersWithRank(MapPoint bldPos, unsigned numSoldiers, unsigned rank) { BOOST_TEST_REQUIRE(rank <= world.GetGGS().GetMaxMilitaryRank()); diff --git a/tests/s25Main/integration/testSeaAttacking.cpp b/tests/s25Main/integration/testSeaAttacking.cpp index fbe2d46ab4..34f8e57650 100644 --- a/tests/s25Main/integration/testSeaAttacking.cpp +++ b/tests/s25Main/integration/testSeaAttacking.cpp @@ -162,17 +162,6 @@ struct SeaAttackFixture : public SeaWorldWithGCExecution<3, 62, 64> return pts.at(0); } - /// Constructs a road connecting 2 buildings and checks for success - void BuildRoadForBlds(const MapPoint bldPosFrom, const MapPoint bldPosTo) - { - const MapPoint start = world.GetNeighbour(bldPosFrom, Direction::SouthEast); - const MapPoint end = world.GetNeighbour(bldPosTo, Direction::SouthEast); - std::vector road = FindPathForRoad(world, start, end, false); - BOOST_TEST_REQUIRE(!road.empty()); - this->BuildRoad(start, false, road); - BOOST_TEST_REQUIRE(world.GetPointRoad(start, road.front()) == PointRoad::Normal); - } - void SetCurPlayer(unsigned playerIdx) { curPlayer = playerIdx; diff --git a/tests/s25Main/worldFixtures/GCExecutor.cpp b/tests/s25Main/worldFixtures/GCExecutor.cpp new file mode 100644 index 0000000000..334194f637 --- /dev/null +++ b/tests/s25Main/worldFixtures/GCExecutor.cpp @@ -0,0 +1,15 @@ +#include "GCExecutor.h" +#include "pathfinding/FindPathForRoad.h" +#include "world/GameWorld.h" +#include "gameTypes/GameTypesOutput.h" + +void GCExecutor::BuildRoadForBlds(const MapPoint bldPosFrom, const MapPoint bldPosTo) +{ + auto& world = GetWorld(); + const MapPoint start = world.GetNeighbour(bldPosFrom, Direction::SouthEast); + const MapPoint end = world.GetNeighbour(bldPosTo, Direction::SouthEast); + std::vector road = FindPathForRoad(world, start, end, false); + BOOST_TEST_REQUIRE(!road.empty()); + this->BuildRoad(start, false, road); + BOOST_TEST_REQUIRE(world.GetPointRoad(start, road.front()) == PointRoad::Normal); +} diff --git a/tests/s25Main/worldFixtures/GCExecutor.h b/tests/s25Main/worldFixtures/GCExecutor.h index 75534ef0bd..b949e8b6ed 100644 --- a/tests/s25Main/worldFixtures/GCExecutor.h +++ b/tests/s25Main/worldFixtures/GCExecutor.h @@ -31,5 +31,8 @@ class GCExecutor : public GameCommandFactory return true; } + /// Constructs a road connecting 2 buildings and checks for success + void BuildRoadForBlds(MapPoint bldPosFrom, MapPoint bldPosTo); + virtual GameWorld& GetWorld() = 0; }; From 378cc0449183d9190ca82635be19046a0d35454a Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Sun, 4 Jan 2026 12:52:26 +0100 Subject: [PATCH 4/7] 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 --- libs/s25main/Ware.cpp | 1 + libs/s25main/Ware.h | 1 + libs/s25main/buildings/nobHarborBuilding.cpp | 33 ++++++-- libs/s25main/gameTypes/GameTypesOutput.h | 4 +- tests/s25Main/integration/testSeafaring.cpp | 86 ++++++++++++++++++++ 5 files changed, 117 insertions(+), 8 deletions(-) diff --git a/libs/s25main/Ware.cpp b/libs/s25main/Ware.cpp index 160da9c6d4..b88c979b0c 100644 --- a/libs/s25main/Ware.cpp +++ b/libs/s25main/Ware.cpp @@ -112,6 +112,7 @@ void Ware::RecalcRoute() static_cast(goal)->WareDontWantToTravelByShip(this); } else { + // TODO(Replay) This should calculate the next dir even when carried FindRouteToWarehouse(); } } else diff --git a/libs/s25main/Ware.h b/libs/s25main/Ware.h index d70b300f73..4f20fa1ea9 100644 --- a/libs/s25main/Ware.h +++ b/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 diff --git a/libs/s25main/buildings/nobHarborBuilding.cpp b/libs/s25main/buildings/nobHarborBuilding.cpp index b9027bc1d1..98695a490c 100644 --- a/libs/s25main/buildings/nobHarborBuilding.cpp +++ b/libs/s25main/buildings/nobHarborBuilding.cpp @@ -635,23 +635,42 @@ void nobHarborBuilding::AddWare(std::unique_ptr 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 diff --git a/libs/s25main/gameTypes/GameTypesOutput.h b/libs/s25main/gameTypes/GameTypesOutput.h index 598b549fc2..410d399134 100644 --- a/libs/s25main/gameTypes/GameTypesOutput.h +++ b/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 @@ -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 diff --git a/tests/s25Main/integration/testSeafaring.cpp b/tests/s25Main/integration/testSeafaring.cpp index a0a1a7afee..33a26eae7a 100644 --- a/tests/s25Main/integration/testSeafaring.cpp +++ b/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(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(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()) + { + if(d != Direction::NorthWest) + DestroyRoad(harbor2.GetFlagPos(), d); + } + ware = std::make_unique(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(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(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() From d7e98ebdaf8567a72fb1909f121daf7367e9ca54 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Sun, 4 Jan 2026 12:53:42 +0100 Subject: [PATCH 5/7] Translate comments in nobHarborBuilding --- libs/s25main/buildings/nobHarborBuilding.cpp | 16 ++++++++-------- tests/s25Main/integration/testSeafaring.cpp | 3 +-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/libs/s25main/buildings/nobHarborBuilding.cpp b/libs/s25main/buildings/nobHarborBuilding.cpp index 98695a490c..2d4a667978 100644 --- a/libs/s25main/buildings/nobHarborBuilding.cpp +++ b/libs/s25main/buildings/nobHarborBuilding.cpp @@ -675,25 +675,25 @@ void nobHarborBuilding::AddWare(std::unique_ptr ware) } // 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; } diff --git a/tests/s25Main/integration/testSeafaring.cpp b/tests/s25Main/integration/testSeafaring.cpp index 33a26eae7a..e2d6e507f3 100644 --- a/tests/s25Main/integration/testSeafaring.cpp +++ b/tests/s25Main/integration/testSeafaring.cpp @@ -653,11 +653,10 @@ BOOST_FIXTURE_TEST_CASE(AddWareWithUnreachableGoalToHarbor, ShipAndHarborsReadyF // 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(); + const 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(BuildingFactory::CreateBuilding( world, BuildingType::Storehouse, harbor2.GetPos() + MapPoint(2, 0), curPlayer, Nation::Romans)); BOOST_TEST_REQUIRE(wh); From db9c7c0911e21b4596d98292a31f3a02943fe41a Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 12 Jan 2026 15:53:35 +0100 Subject: [PATCH 6/7] Assert wh and flag exists for warehouse worker --- libs/s25main/figures/nofWarehouseWorker.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/libs/s25main/figures/nofWarehouseWorker.cpp b/libs/s25main/figures/nofWarehouseWorker.cpp index 9fcca1be67..668cf0763b 100644 --- a/libs/s25main/figures/nofWarehouseWorker.cpp +++ b/libs/s25main/figures/nofWarehouseWorker.cpp @@ -59,8 +59,10 @@ void nofWarehouseWorker::Draw(DrawPoint drawPt) void nofWarehouseWorker::GoalReached() { - const nobBaseWarehouse* wh = world->GetSpecObj(world->GetNeighbour(pos, Direction::NorthWest)); - auto* flag = world->GetSpecObj(pos); + nobBaseWarehouse* wh = world->GetSpecObj(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) { // Put ware down at flag if enough space. @@ -76,14 +78,14 @@ void nofWarehouseWorker::GoalReached() } else { // Bring back in - carried_ware->Carry(world->GetSpecObj(world->GetNeighbour(pos, Direction::NorthWest))); + carried_ware->Carry(wh); } } else { // Take ware if any carried_ware = flag->SelectWare(Direction::NorthWest, false, this); if(carried_ware) - carried_ware->Carry(world->GetSpecObj(world->GetNeighbour(pos, Direction::NorthWest))); + carried_ware->Carry(wh); } // Start walking back From 5080b0cbd39509457090c043b2068c58f9a9269d Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 13 Jan 2026 08:57:30 +0100 Subject: [PATCH 7/7] Fix clang-tidy warning --- libs/s25main/figures/nofWarehouseWorker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/s25main/figures/nofWarehouseWorker.cpp b/libs/s25main/figures/nofWarehouseWorker.cpp index 668cf0763b..94380d1fe5 100644 --- a/libs/s25main/figures/nofWarehouseWorker.cpp +++ b/libs/s25main/figures/nofWarehouseWorker.cpp @@ -59,7 +59,7 @@ void nofWarehouseWorker::Draw(DrawPoint drawPt) void nofWarehouseWorker::GoalReached() { - nobBaseWarehouse* wh = world->GetSpecObj(world->GetNeighbour(pos, Direction::NorthWest)); + auto* wh = world->GetSpecObj(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);