From eb3937e73ebb54224cd72a175851811ef6dfae1d Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 2 Feb 2026 11:11:05 +0100 Subject: [PATCH 1/6] Run `DestroyCurrentObjs` in Test-EventManager This is run for every gameframe in "real mode" and required for some tests simulating e.g. a single step. --- .../worldFixtures/TestEventManager.cpp | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/tests/s25Main/worldFixtures/TestEventManager.cpp b/tests/s25Main/worldFixtures/TestEventManager.cpp index 49a02e6766..e12213abb5 100644 --- a/tests/s25Main/worldFixtures/TestEventManager.cpp +++ b/tests/s25Main/worldFixtures/TestEventManager.cpp @@ -1,4 +1,4 @@ -// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org) +// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org) // // SPDX-License-Identifier: GPL-2.0-or-later @@ -9,22 +9,18 @@ unsigned TestEventManager::ExecuteNextEvent(unsigned maxGF) { if(GetCurrentGF() >= maxGF) return 0; - if(events.empty()) + unsigned numGFs; + if(events.empty() || events.begin()->first > maxGF) { - unsigned numGFs = maxGF - GetCurrentGF(); + numGFs = maxGF - GetCurrentGF(); currentGF = maxGF; - return numGFs; - } - auto itEvents = events.begin(); - if(itEvents->first > maxGF) + } else { - unsigned numGFs = maxGF - GetCurrentGF(); - currentGF = maxGF; - return numGFs; + auto itEvents = events.begin(); + numGFs = itEvents->first - GetCurrentGF(); + currentGF = itEvents->first; + ExecuteEvents(itEvents); } - unsigned numGFs = itEvents->first - GetCurrentGF(); - currentGF = itEvents->first; - ExecuteEvents(itEvents); DestroyCurrentObjects(); return numGFs; } From 4e3c03bc57ceefde975fbe78a780544aca2d7201 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 2 Feb 2026 11:15:27 +0100 Subject: [PATCH 2/6] Const-correct `GamePlayer::GetFirstWH` --- libs/s25main/GamePlayer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/s25main/GamePlayer.h b/libs/s25main/GamePlayer.h index 922c62ba8d..7c54b3accd 100644 --- a/libs/s25main/GamePlayer.h +++ b/libs/s25main/GamePlayer.h @@ -114,7 +114,7 @@ class GamePlayer : public GamePlayerInfo /// Returns true if the given wh does still exist and hence the ptr is valid bool IsWarehouseValid(nobBaseWarehouse* wh) const; /// Gibt erstes Lagerhaus zurück - nobBaseWarehouse* GetFirstWH() + nobBaseWarehouse* GetFirstWH() const { return buildings.GetStorehouses().empty() ? nullptr : buildings.GetStorehouses().front(); } From 7dc76a28a31634477daa6b6f311256a1a8a8bd7d Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 2 Feb 2026 11:16:12 +0100 Subject: [PATCH 3/6] Take non-NULL ship param by reference --- libs/s25main/GamePlayer.cpp | 13 +++---------- libs/s25main/GamePlayer.h | 2 +- libs/s25main/nodeObjs/noShip.cpp | 4 ++-- 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/libs/s25main/GamePlayer.cpp b/libs/s25main/GamePlayer.cpp index 17d9b099dd..87ff3d44ae 100644 --- a/libs/s25main/GamePlayer.cpp +++ b/libs/s25main/GamePlayer.cpp @@ -1897,17 +1897,10 @@ bool GamePlayer::OrderShip(nobHarborBuilding& hb) return (false); } -/// Meldet das Schiff wieder ab -void GamePlayer::RemoveShip(noShip* ship) +void GamePlayer::RemoveShip(noShip& ship) { - for(unsigned i = 0; i < ships.size(); ++i) - { - if(ships[i] == ship) - { - ships.erase(ships.begin() + i); - return; - } - } + RTTR_Assert(helpers::contains(ships, &ship)); + helpers::erase(ships, &ship); } /// Versucht, für ein untätiges Schiff eine Arbeit zu suchen diff --git a/libs/s25main/GamePlayer.h b/libs/s25main/GamePlayer.h index 7c54b3accd..a2804e389b 100644 --- a/libs/s25main/GamePlayer.h +++ b/libs/s25main/GamePlayer.h @@ -234,7 +234,7 @@ class GamePlayer : public GamePlayerInfo /// Registriert ein Schiff beim Einwohnermeldeamt void RegisterShip(noShip& ship); /// Meldet das Schiff wieder ab - void RemoveShip(noShip* ship); + void RemoveShip(noShip& ship); /// Versucht, für ein untätiges Schiff eine Arbeit zu suchen void GetJobForShip(noShip& ship); /// Schiff für Hafen bestellen. Wenn ein Schiff kommt, true. diff --git a/libs/s25main/nodeObjs/noShip.cpp b/libs/s25main/nodeObjs/noShip.cpp index 5a3fec80c4..abee78d1a4 100644 --- a/libs/s25main/nodeObjs/noShip.cpp +++ b/libs/s25main/nodeObjs/noShip.cpp @@ -1,4 +1,4 @@ -// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org) +// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org) // // SPDX-License-Identifier: GPL-2.0-or-later @@ -104,7 +104,7 @@ void noShip::Destroy() RTTR_Assert(wares.empty()); world->GetNotifications().publish(ShipNote(ShipNote::Destroyed, ownerId_, pos)); // Schiff wieder abmelden - world->GetPlayer(ownerId_).RemoveShip(this); + world->GetPlayer(ownerId_).RemoveShip(*this); } void noShip::Draw(DrawPoint drawPt) From 92c0d0147a4a1b6b87e574d28a17b9ce0a6d39a7 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 2 Feb 2026 11:18:32 +0100 Subject: [PATCH 4/6] Add `noShip::Sink` --- libs/s25main/nodeObjs/noShip.cpp | 22 ++++++++++++ libs/s25main/nodeObjs/noShip.h | 5 ++- tests/s25Main/integration/testSeafaring.cpp | 37 ++++++++++++++++++++- 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/libs/s25main/nodeObjs/noShip.cpp b/libs/s25main/nodeObjs/noShip.cpp index abee78d1a4..0fe4c9f003 100644 --- a/libs/s25main/nodeObjs/noShip.cpp +++ b/libs/s25main/nodeObjs/noShip.cpp @@ -1230,3 +1230,25 @@ void noShip::NewHarborBuilt(nobHarborBuilding* hb) break; } } + +void noShip::Sink() +{ + for(auto& figure : figures) + { + figure->Abrogate(); + figure->SetGoalTonullptr(); + figure->RemoveFromInventory(); + } + figures.clear(); + + for(auto& ware : wares) + { + ware->WareLost(ownerId_); + ware->Destroy(); + } + wares.clear(); + + GetEvMgr().RemoveEvent(current_ev); + GetEvMgr().AddToKillList(world->RemoveFigure(pos, *this)); + world->RecalcVisibilitiesAroundPoint(pos, GetVisualRange(), ownerId_, nullptr); +} diff --git a/libs/s25main/nodeObjs/noShip.h b/libs/s25main/nodeObjs/noShip.h index bf154146f3..5f8999e7f4 100644 --- a/libs/s25main/nodeObjs/noShip.h +++ b/libs/s25main/nodeObjs/noShip.h @@ -1,4 +1,4 @@ -// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org) +// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org) // // SPDX-License-Identifier: GPL-2.0-or-later @@ -223,4 +223,7 @@ class noShip : public noMovable void HarborDestroyed(nobHarborBuilding* hb); /// Sagt dem Schiff, dass ein neuer Hafen erbaut wurde void NewHarborBuilt(nobHarborBuilding* hb); + + /// Destroy the ship immediately + void Sink(); }; diff --git a/tests/s25Main/integration/testSeafaring.cpp b/tests/s25Main/integration/testSeafaring.cpp index e2d6e507f3..09ad7c973b 100644 --- a/tests/s25Main/integration/testSeafaring.cpp +++ b/tests/s25Main/integration/testSeafaring.cpp @@ -1,4 +1,4 @@ -// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org) +// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org) // // SPDX-License-Identifier: GPL-2.0-or-later @@ -718,4 +718,39 @@ BOOST_FIXTURE_TEST_CASE(AddWareWithUnreachableGoalToHarbor, ShipAndHarborsReadyF BOOST_TEST(MockWare::destroyed); } +BOOST_FIXTURE_TEST_CASE(SinkShipLoosesCargo, ShipAndHarborsReadyFixture<1>) +{ + const GamePlayer& player = world.GetPlayer(curPlayer); + noShip& ship = *player.GetShipByID(0); + + const auto& harbors = player.GetBuildingRegister().GetHarbors(); + BOOST_TEST_REQUIRE(harbors.size() >= 2u); + nobHarborBuilding& harbor1 = *harbors.front(); + nobHarborBuilding& harbor2 = **(++harbors.begin()); + + // Transport something + BOOST_TEST_REQUIRE(harbor1.OrderJob(Job::Woodcutter, harbor2, false)); + BOOST_TEST_REQUIRE(harbor1.OrderWare(GoodType::Wood, harbor2)); + BOOST_TEST_REQUIRE(harbor1.OrderWare(GoodType::Wood, harbor2)); + + RTTR_EXEC_TILL(90, ship.IsLoading()); + RTTR_EXEC_TILL(200, ship.IsMoving()); + BOOST_TEST(ship.GetWares().size() == 2u); + BOOST_TEST(ship.GetFigures().size() == 1u); + + const auto shipPos = ship.GetPos(); + BOOST_TEST_REQUIRE(player.GetNumShips() == 1u); + BOOST_TEST_REQUIRE(world.GetFigures(shipPos).size() == 1u); + BOOST_TEST_REQUIRE(dynamic_cast(&world.GetFigures(shipPos).front())); + + const auto numWood = player.GetInventory()[GoodType::Wood]; + const auto numWoodcutters = player.GetInventory()[Job::Woodcutter]; + ship.Sink(); + RTTR_SKIP_GFS(1); // Handle delayed destruction + BOOST_TEST(player.GetNumShips() == 0u); + BOOST_TEST(world.GetFigures(shipPos).size() == 0u); + BOOST_TEST(player.GetInventory()[GoodType::Wood] == numWood - 2); + BOOST_TEST(player.GetInventory()[Job::Woodcutter] == numWoodcutters - 1); +} + BOOST_AUTO_TEST_SUITE_END() From 5847d0fce6a46e46442408333805fc39cb9f4990 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 2 Feb 2026 11:19:00 +0100 Subject: [PATCH 5/6] Destroy ships when player is defeated See #183 --- libs/s25main/GamePlayer.cpp | 4 +++ tests/s25Main/integration/testSeafaring.cpp | 34 +++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/libs/s25main/GamePlayer.cpp b/libs/s25main/GamePlayer.cpp index 87ff3d44ae..4276bf727e 100644 --- a/libs/s25main/GamePlayer.cpp +++ b/libs/s25main/GamePlayer.cpp @@ -1426,6 +1426,10 @@ void GamePlayer::Surrender() if(isDefeated) return; + const auto shipsCopy = ships; // copy to avoid modification during iteration + for(auto* ship : shipsCopy) + ship->Sink(); + isDefeated = true; // GUI Bescheid sagen diff --git a/tests/s25Main/integration/testSeafaring.cpp b/tests/s25Main/integration/testSeafaring.cpp index 09ad7c973b..d801f3f0d8 100644 --- a/tests/s25Main/integration/testSeafaring.cpp +++ b/tests/s25Main/integration/testSeafaring.cpp @@ -753,4 +753,38 @@ BOOST_FIXTURE_TEST_CASE(SinkShipLoosesCargo, ShipAndHarborsReadyFixture<1>) BOOST_TEST(player.GetInventory()[Job::Woodcutter] == numWoodcutters - 1); } +BOOST_FIXTURE_TEST_CASE(RemoveShipsOnDefeat, ShipAndHarborsReadyFixture<2>) +{ + const GamePlayer& player = world.GetPlayer(curPlayer); + const noShip& ship = *player.GetShipByID(0); + + const auto& harbors = player.GetBuildingRegister().GetHarbors(); + BOOST_TEST_REQUIRE(harbors.size() >= 2u); + nobHarborBuilding& harbor1 = *harbors.front(); + nobHarborBuilding& harbor2 = **(++harbors.begin()); + + // Transport something + BOOST_TEST_REQUIRE(harbor1.OrderJob(Job::Woodcutter, harbor2, false)); + BOOST_TEST_REQUIRE(harbor1.OrderWare(GoodType::Wood, harbor2)); + + RTTR_EXEC_TILL(90, ship.IsLoading()); + RTTR_EXEC_TILL(200, ship.IsMoving()); + const auto numWood = ship.GetWares().size(); + const auto numWoodcutters = ship.GetFigures().size(); + BOOST_TEST(numWood == 1u); + BOOST_TEST(numWoodcutters == 1u); + + const auto shipPos = ship.GetPos(); + BOOST_TEST_REQUIRE(player.GetNumShips() == 1u); + BOOST_TEST_REQUIRE(world.GetFigures(shipPos).size() == 1u); + BOOST_TEST_REQUIRE(dynamic_cast(&world.GetFigures(shipPos).front())); + const auto warehouses = player.GetBuildingRegister().GetStorehouses(); // Copy for iteration + for(const auto* bld : warehouses) + world.DestroyBuilding(bld->GetPos(), curPlayer); + RTTR_SKIP_GFS(1); // Handle delayed destruction + BOOST_TEST(player.IsDefeated()); + BOOST_TEST(player.GetNumShips() == 0u); + BOOST_TEST(world.GetFigures(shipPos).size() == 0u); +} + BOOST_AUTO_TEST_SUITE_END() From 9ce412ba902619991003fcbf105741c2f1c1cf26 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 2 Feb 2026 11:20:55 +0100 Subject: [PATCH 6/6] Allow `nobHarborBuilding::WareLost` during destruction This is a corner case when destroying the harbor leads to defeat which leads to sinking ships potentially with wares to this building. --- libs/s25main/buildings/nobHarborBuilding.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/libs/s25main/buildings/nobHarborBuilding.cpp b/libs/s25main/buildings/nobHarborBuilding.cpp index 7f829e1fe6..11f5e81c07 100644 --- a/libs/s25main/buildings/nobHarborBuilding.cpp +++ b/libs/s25main/buildings/nobHarborBuilding.cpp @@ -441,8 +441,7 @@ void nobHarborBuilding::StopExplorationExpedition() /// Bestellt die zusätzlichen erforderlichen Waren für eine Expedition void nobHarborBuilding::OrderExpeditionWares() { - RTTR_Assert(!IsBeingDestroyedNow()); // Wares should already be canceled! - if(this->IsBeingDestroyedNow()) // don't order new stuff if we are about to be destroyed + if(IsBeingDestroyedNow()) // don't order new stuff if we are about to be destroyed return; if(!expedition.active) // expedition no longer active? @@ -496,11 +495,8 @@ void nobHarborBuilding::OrderExpeditionWares() orderware_ev = GetEvMgr().AddEvent(this, 210, 10); } -/// Eine bestellte Ware konnte doch nicht kommen void nobHarborBuilding::WareLost(Ware& ware) { - RTTR_Assert(!IsBeingDestroyedNow()); - // ggf. neue Waren für Expedition bestellen if(expedition.active && (ware.type == GoodType::Boards || ware.type == GoodType::Stones)) OrderExpeditionWares(); nobBaseWarehouse::WareLost(ware);