From 31aec23f20775431c6188632c7b3957c0721ca98 Mon Sep 17 00:00:00 2001 From: Manu Date: Fri, 1 May 2026 15:25:53 +0200 Subject: [PATCH 01/10] Add optional free harbor spots addon --- libs/s25main/GlobalGameSettings.cpp | 1 + libs/s25main/addons/AddonFreeHarborSpots.h | 17 +++++ libs/s25main/addons/Addons.h | 1 + libs/s25main/addons/const_addons.h | 2 +- libs/s25main/world/BQCalculator.h | 25 ++++++- libs/s25main/world/GameWorldBase.cpp | 2 +- libs/s25main/world/GameWorldViewer.cpp | 3 +- libs/s25main/world/MapLoader.cpp | 43 +++++++++++- libs/s25main/world/MapLoader.h | 3 +- .../integration/testSeaWorldCreation.cpp | 66 +++++++++++++++++++ 10 files changed, 153 insertions(+), 10 deletions(-) create mode 100644 libs/s25main/addons/AddonFreeHarborSpots.h diff --git a/libs/s25main/GlobalGameSettings.cpp b/libs/s25main/GlobalGameSettings.cpp index 63f240221e..a092dfe9b5 100644 --- a/libs/s25main/GlobalGameSettings.cpp +++ b/libs/s25main/GlobalGameSettings.cpp @@ -75,6 +75,7 @@ void GlobalGameSettings::registerAllAddons() AddonDurableGeologistSigns, AddonEconomyModeGameLength, AddonExhaustibleWater, + AddonFreeHarborSpots, AddonFrontierDistanceReachable, AddonHalfCostMilEquip, AddonInexhaustibleFish, diff --git a/libs/s25main/addons/AddonFreeHarborSpots.h b/libs/s25main/addons/AddonFreeHarborSpots.h new file mode 100644 index 0000000000..5d42cbff9e --- /dev/null +++ b/libs/s25main/addons/AddonFreeHarborSpots.h @@ -0,0 +1,17 @@ +// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org) +// +// SPDX-License-Identifier: GPL-2.0-or-later + +#pragma once + +#include "AddonBool.h" +#include "mygettext/mygettext.h" + +class AddonFreeHarborSpots : public AddonBool +{ +public: + AddonFreeHarborSpots() + : AddonBool(AddonId::FREE_HARBOR_SPOTS, AddonGroup::GamePlay, _("Build harbors without map markers"), + _("Allows harbors on suitable coastal castle sites even if the map does not define harbor spots.")) + {} +}; diff --git a/libs/s25main/addons/Addons.h b/libs/s25main/addons/Addons.h index 4b773587d0..2d01258e60 100644 --- a/libs/s25main/addons/Addons.h +++ b/libs/s25main/addons/Addons.h @@ -53,6 +53,7 @@ #include "addons/AddonCoinsCapturedBld.h" #include "addons/AddonDemolishBldWORes.h" +#include "addons/AddonFreeHarborSpots.h" #include "addons/AddonFrontierDistanceReachable.h" #include "addons/AddonDurableGeologistSigns.h" diff --git a/libs/s25main/addons/const_addons.h b/libs/s25main/addons/const_addons.h index 4ae5da8c88..f066b0c423 100644 --- a/libs/s25main/addons/const_addons.h +++ b/libs/s25main/addons/const_addons.h @@ -75,7 +75,7 @@ ENUM_WITH_STRING(AddonId, LIMIT_CATAPULTS = 0x00000000, INEXHAUSTIBLE_MINES = 0x AUTOFLAGS = 0x00F00000, WINE = 0x01000000, LEATHER = 0x01000001, NO_ARMOR_DEFAULT = 0x01000002, - ARMOR_CAPTURED_BLD = 0x01000003) + ARMOR_CAPTURED_BLD = 0x01000003, FREE_HARBOR_SPOTS = 0x01000004) //-V:AddonId:801 enum class AddonGroup : unsigned diff --git a/libs/s25main/world/BQCalculator.h b/libs/s25main/world/BQCalculator.h index 88db722d3c..6452b6fa15 100644 --- a/libs/s25main/world/BQCalculator.h +++ b/libs/s25main/world/BQCalculator.h @@ -10,13 +10,16 @@ struct BQCalculator { - BQCalculator(const World& world) : world(world) {} + BQCalculator(const World& world, const bool allowFreeHarborSpots = false) + : world(world), allowFreeHarborSpots(allowFreeHarborSpots) + {} template BuildingQuality operator()(MapPoint pt, T_IsOnRoad isOnRoad, bool flagOnly = false) const; private: const World& world; + bool allowFreeHarborSpots; }; template @@ -219,8 +222,24 @@ BuildingQuality BQCalculator::operator()(const MapPoint pt, T_IsOnRoad isOnRoad, } // If we can build a castle and this is a harbor point -> Allow harbor - if(curBQ == BuildingQuality::Castle && world.GetNode(pt).harborId) - curBQ = BuildingQuality::Harbor; + if(curBQ == BuildingQuality::Castle) + { + bool isHarborPoint = world.GetNode(pt).harborId.isValid(); + if(!isHarborPoint && allowFreeHarborSpots) + { + for(const auto dir : helpers::EnumRange{}) + { + // Keep this in sync with harbor initialization: NW-only coasts are rejected there. + if(dir != Direction::NorthWest && world.GetSeaFromCoastalPoint(neighbours[dir])) + { + isHarborPoint = true; + break; + } + } + } + if(isHarborPoint) + curBQ = BuildingQuality::Harbor; + } ////////////////////////////////////////////////////////////////////////// // At this point we can still build a building/mine diff --git a/libs/s25main/world/GameWorldBase.cpp b/libs/s25main/world/GameWorldBase.cpp index 9e7c1d1971..4158fdd602 100644 --- a/libs/s25main/world/GameWorldBase.cpp +++ b/libs/s25main/world/GameWorldBase.cpp @@ -696,7 +696,7 @@ GameWorldBase::GetSoldiersForSeaAttack(const unsigned char player_attacker, cons void GameWorldBase::RecalcBQ(const MapPoint pt) { - BQCalculator calcBQ(*this); + BQCalculator calcBQ(*this, GetGGS().isEnabled(AddonId::FREE_HARBOR_SPOTS)); if(SetBQ(pt, calcBQ(pt, [this](auto pt) { return this->IsOnRoad(pt); }))) { GetNotifications().publish(NodeNote(NodeNote::BQ, pt)); diff --git a/libs/s25main/world/GameWorldViewer.cpp b/libs/s25main/world/GameWorldViewer.cpp index 9230026b2f..47aee3e13c 100644 --- a/libs/s25main/world/GameWorldViewer.cpp +++ b/libs/s25main/world/GameWorldViewer.cpp @@ -7,6 +7,7 @@ #include "GameInterface.h" #include "GamePlayer.h" #include "GlobalGameSettings.h" +#include "addons/const_addons.h" #include "RttrForeachPt.h" #include "buildings/nobMilitary.h" #include "network/GameClient.h" @@ -278,7 +279,7 @@ void GameWorldViewer::RoadConstructionEnded(const RoadNote& note) void GameWorldViewer::RecalcBQ(const MapPoint& pt) { - BQCalculator calcBQ(GetWorld()); + BQCalculator calcBQ(GetWorld(), GetWorld().GetGGS().isEnabled(AddonId::FREE_HARBOR_SPOTS)); visualNodes[pt].bq = calcBQ(pt, [this](const MapPoint& pos) { return IsOnRoad(pos); }); } diff --git a/libs/s25main/world/MapLoader.cpp b/libs/s25main/world/MapLoader.cpp index da4d04a4a3..deaf6809c7 100644 --- a/libs/s25main/world/MapLoader.cpp +++ b/libs/s25main/world/MapLoader.cpp @@ -3,12 +3,14 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include "world/MapLoader.h" +#include "BQCalculator.h" #include "Game.h" #include "GamePlayer.h" #include "GameWorldBase.h" #include "GlobalGameSettings.h" #include "PointOutput.h" #include "RttrForeachPt.h" +#include "addons/const_addons.h" #include "buildings/nobHQ.h" #include "factories/BuildingFactory.h" #include "helpers/IdRange.h" @@ -53,7 +55,7 @@ bool MapLoader::Load(const libsiedler2::ArchivItem_Map& map, Exploration explora return false; PlaceObjects(map); PlaceAnimals(map); - if(!InitSeasAndHarbors(world_)) + if(!InitSeasAndHarbors(world_, std::vector(), world_.GetGGS().isEnabled(AddonId::FREE_HARBOR_SPOTS))) return false; /// Schatten @@ -420,10 +422,39 @@ bool MapLoader::PlaceHQs(GameWorldBase& world, const std::vector& hqPo return true; } -bool MapLoader::InitSeasAndHarbors(World& world, const std::vector& additionalHarbors) +namespace { +bool hasHarborAt(const World& world, const MapPoint pt) +{ + for(const auto harborId : helpers::idRange(world.GetNumHarborPoints())) + { + if(world.GetHarborPoint(harborId) == pt) + return true; + } + return false; +} + +std::vector getGeneratedHarbors(const World& world) +{ + std::vector generatedHarbors; + BQCalculator calcBQ(world, true); + RTTR_FOREACH_PT(MapPoint, world.GetSize()) + { + if(!hasHarborAt(world, pt) + && calcBQ(pt, [](const MapPoint&) { return false; }) == BuildingQuality::Harbor) + generatedHarbors.push_back(pt); + } + return generatedHarbors; +} +} // namespace + +bool MapLoader::InitSeasAndHarbors(World& world, const std::vector& additionalHarbors, + const bool generateHarborSpots) { for(MapPoint pt : additionalHarbors) - world.harborData.push_back(HarborPos(pt)); + { + if(!hasHarborAt(world, pt)) + world.harborData.push_back(HarborPos(pt)); + } // Clear current harbors and seas RTTR_FOREACH_PT(MapPoint, world.GetSize()) //-V807 { @@ -446,6 +477,12 @@ bool MapLoader::InitSeasAndHarbors(World& world, const std::vector& ad } } + if(generateHarborSpots) + { + for(MapPoint pt : getGeneratedHarbors(world)) + world.harborData.push_back(HarborPos(pt)); + } + /// Determine seas adjacent to the harbor places HarborId curHarborId(1); for(auto it = world.harborData.begin(); it != world.harborData.end();) diff --git a/libs/s25main/world/MapLoader.h b/libs/s25main/world/MapLoader.h index 4dd35e9a36..3415d42682 100644 --- a/libs/s25main/world/MapLoader.h +++ b/libs/s25main/world/MapLoader.h @@ -57,7 +57,8 @@ class MapLoader static void InitShadows(World& world); static void SetMapExplored(World& world); static bool InitSeasAndHarbors(World& world, - const std::vector& additionalHarbors = std::vector()); + const std::vector& additionalHarbors = std::vector(), + bool generateHarborSpots = false); /// Place the HQs on a loaded map and add starting wares if desired. /// Return false if there was an error. static bool PlaceHQs(GameWorldBase& world, const std::vector& hqPositions, bool addStartWares = true); diff --git a/tests/s25Main/integration/testSeaWorldCreation.cpp b/tests/s25Main/integration/testSeaWorldCreation.cpp index 499a480b10..76072b42c6 100644 --- a/tests/s25Main/integration/testSeaWorldCreation.cpp +++ b/tests/s25Main/integration/testSeaWorldCreation.cpp @@ -3,10 +3,15 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include "RTTR_AssertError.h" +#include "RttrForeachPt.h" #include "helpers/IdRange.h" +#include "addons/const_addons.h" #include "worldFixtures/SeaWorldWithGCExecution.h" +#include "worldFixtures/terrainHelpers.h" #include "gameTypes/GameTypesOutput.h" #include "gameTypes/ShipDirection.h" +#include "lua/GameDataLoader.h" +#include "world/MapLoader.h" #include #include @@ -61,6 +66,30 @@ void testShipDir(const MapBase& world, const MapPoint fromPt) BOOST_TEST_REQUIRE(getShipDir(world, fromPt, DiffPt(100, -173)) == ShipDirection::NorthEast); BOOST_TEST_REQUIRE(getShipDir(world, fromPt, DiffPt(100, -174)) == ShipDirection::North); } + +void createMarkerlessIslandWorld(GameWorld& world) +{ + world.Unload(); + loadGameData(world.GetDescriptionWriteable()); + world.Init(MapExtent(30, 30)); + + const auto water = GetWaterTerrain(world.GetDescription()); + RTTR_FOREACH_PT(MapPoint, world.GetSize()) + { + MapNode& node = world.GetNodeWriteable(pt); + node.t1 = node.t2 = water; + } + + const auto land = GetLandTerrain(world.GetDescription(), ETerrain::Buildable); + for(MapPoint pt(8, 8); pt.y < 22; ++pt.y) + { + for(pt.x = 8; pt.x < 22; ++pt.x) + { + MapNode& node = world.GetNodeWriteable(pt); + node.t1 = node.t2 = land; + } + } +} } // namespace BOOST_AUTO_TEST_CASE(GetShipDir) @@ -128,6 +157,43 @@ BOOST_FIXTURE_TEST_CASE(HarborSpotCreation, SeaWorldWithGCExecution<>) } } +BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonAddsCoastalHarbors, SeaWorldWithGCExecution<>) +{ + const unsigned initialHarbors = world.GetNumHarborPoints(); + + ggs.setSelection(AddonId::FREE_HARBOR_SPOTS, 1); + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); + world.InitAfterLoad(); + + BOOST_TEST_REQUIRE(world.GetNumHarborPoints() > initialHarbors); + for(unsigned harborIdx = initialHarbors + 1; harborIdx <= world.GetNumHarborPoints(); ++harborIdx) + { + const MapPoint harborPt = world.GetHarborPoint(HarborId(harborIdx)); + BOOST_TEST_REQUIRE(harborPt.isValid()); + BOOST_TEST_REQUIRE(world.GetHarborPointID(harborPt) == HarborId(harborIdx)); + BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); + } +} + +BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, SeaWorldWithGCExecution<>) +{ + createMarkerlessIslandWorld(world); + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); + BOOST_TEST_REQUIRE(world.GetNumHarborPoints() == 0u); + + ggs.setSelection(AddonId::FREE_HARBOR_SPOTS, 1); + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); + world.InitAfterLoad(); + + BOOST_TEST_REQUIRE(world.GetNumHarborPoints() > 0u); + for(const auto harborId : helpers::idRange(world.GetNumHarborPoints())) + { + const MapPoint harborPt = world.GetHarborPoint(harborId); + BOOST_TEST_REQUIRE(world.GetHarborPointID(harborPt) == harborId); + BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); + } +} + BOOST_FIXTURE_TEST_CASE(HarborNeighbors, SeaWorldWithGCExecution<>) { // Now just test some assumptions: 2 harbor spots per possible HQ. From 7eb77282b725159efab74ae1aab380867cb388ee Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 2 May 2026 13:04:02 +0200 Subject: [PATCH 02/10] Generate free harbor spots only during harbor init --- libs/s25main/world/GameWorldBase.cpp | 2 +- libs/s25main/world/GameWorldViewer.cpp | 3 +- .../integration/testSeaWorldCreation.cpp | 49 ++++++++++++++++--- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/libs/s25main/world/GameWorldBase.cpp b/libs/s25main/world/GameWorldBase.cpp index 4158fdd602..9e7c1d1971 100644 --- a/libs/s25main/world/GameWorldBase.cpp +++ b/libs/s25main/world/GameWorldBase.cpp @@ -696,7 +696,7 @@ GameWorldBase::GetSoldiersForSeaAttack(const unsigned char player_attacker, cons void GameWorldBase::RecalcBQ(const MapPoint pt) { - BQCalculator calcBQ(*this, GetGGS().isEnabled(AddonId::FREE_HARBOR_SPOTS)); + BQCalculator calcBQ(*this); if(SetBQ(pt, calcBQ(pt, [this](auto pt) { return this->IsOnRoad(pt); }))) { GetNotifications().publish(NodeNote(NodeNote::BQ, pt)); diff --git a/libs/s25main/world/GameWorldViewer.cpp b/libs/s25main/world/GameWorldViewer.cpp index 47aee3e13c..9230026b2f 100644 --- a/libs/s25main/world/GameWorldViewer.cpp +++ b/libs/s25main/world/GameWorldViewer.cpp @@ -7,7 +7,6 @@ #include "GameInterface.h" #include "GamePlayer.h" #include "GlobalGameSettings.h" -#include "addons/const_addons.h" #include "RttrForeachPt.h" #include "buildings/nobMilitary.h" #include "network/GameClient.h" @@ -279,7 +278,7 @@ void GameWorldViewer::RoadConstructionEnded(const RoadNote& note) void GameWorldViewer::RecalcBQ(const MapPoint& pt) { - BQCalculator calcBQ(GetWorld(), GetWorld().GetGGS().isEnabled(AddonId::FREE_HARBOR_SPOTS)); + BQCalculator calcBQ(GetWorld()); visualNodes[pt].bq = calcBQ(pt, [this](const MapPoint& pos) { return IsOnRoad(pos); }); } diff --git a/tests/s25Main/integration/testSeaWorldCreation.cpp b/tests/s25Main/integration/testSeaWorldCreation.cpp index 76072b42c6..a25d326096 100644 --- a/tests/s25Main/integration/testSeaWorldCreation.cpp +++ b/tests/s25Main/integration/testSeaWorldCreation.cpp @@ -90,6 +90,39 @@ void createMarkerlessIslandWorld(GameWorld& world) } } } + +unsigned countHarborBQ(const GameWorld& world) +{ + unsigned result = 0; + RTTR_FOREACH_PT(MapPoint, world.GetSize()) + { + if(world.GetNode(pt).bq == BuildingQuality::Harbor) + ++result; + } + return result; +} + +void testHarborPoint(const GameWorld& world, const HarborId harborId) +{ + const MapPoint harborPt = world.GetHarborPoint(harborId); + BOOST_TEST_REQUIRE(harborPt.isValid()); + BOOST_TEST_REQUIRE(world.GetHarborPointID(harborPt) == harborId); + + bool hasSea = false; + for(const auto dir : helpers::EnumRange{}) + { + const SeaId seaId = world.GetSeaId(harborId, dir); + if(!seaId) + continue; + + hasSea = true; + const MapPoint coastalPt = world.GetCoastalPoint(harborId, seaId); + BOOST_TEST_REQUIRE(coastalPt.isValid()); + BOOST_TEST_REQUIRE(world.GetSeaFromCoastalPoint(coastalPt) == seaId); + } + BOOST_TEST_REQUIRE(hasSea); + BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); +} } // namespace BOOST_AUTO_TEST_CASE(GetShipDir) @@ -168,10 +201,7 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonAddsCoastalHarbors, SeaWorldWithGCEx BOOST_TEST_REQUIRE(world.GetNumHarborPoints() > initialHarbors); for(unsigned harborIdx = initialHarbors + 1; harborIdx <= world.GetNumHarborPoints(); ++harborIdx) { - const MapPoint harborPt = world.GetHarborPoint(HarborId(harborIdx)); - BOOST_TEST_REQUIRE(harborPt.isValid()); - BOOST_TEST_REQUIRE(world.GetHarborPointID(harborPt) == HarborId(harborIdx)); - BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); + testHarborPoint(world, HarborId(harborIdx)); } } @@ -179,18 +209,23 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, SeaWorldWith { createMarkerlessIslandWorld(world); BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); + world.InitAfterLoad(); BOOST_TEST_REQUIRE(world.GetNumHarborPoints() == 0u); + BOOST_TEST_REQUIRE(countHarborBQ(world) == 0u); ggs.setSelection(AddonId::FREE_HARBOR_SPOTS, 1); + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); + world.InitAfterLoad(); + BOOST_TEST_REQUIRE(world.GetNumHarborPoints() == 0u); + BOOST_TEST_REQUIRE(countHarborBQ(world) == 0u); + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); world.InitAfterLoad(); BOOST_TEST_REQUIRE(world.GetNumHarborPoints() > 0u); for(const auto harborId : helpers::idRange(world.GetNumHarborPoints())) { - const MapPoint harborPt = world.GetHarborPoint(harborId); - BOOST_TEST_REQUIRE(world.GetHarborPointID(harborPt) == harborId); - BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); + testHarborPoint(world, harborId); } } From 676ed66278ae5172db3bd1a43988b332f74b827e Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 2 May 2026 13:37:53 +0200 Subject: [PATCH 03/10] Clarify free harbor spot review changes --- libs/s25main/addons/AddonFreeHarborSpots.h | 2 +- libs/s25main/world/BQCalculator.h | 15 +++++++-------- .../s25Main/integration/testSeaWorldCreation.cpp | 14 +++++++++++--- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/libs/s25main/addons/AddonFreeHarborSpots.h b/libs/s25main/addons/AddonFreeHarborSpots.h index 5d42cbff9e..a06f4f044c 100644 --- a/libs/s25main/addons/AddonFreeHarborSpots.h +++ b/libs/s25main/addons/AddonFreeHarborSpots.h @@ -12,6 +12,6 @@ class AddonFreeHarborSpots : public AddonBool public: AddonFreeHarborSpots() : AddonBool(AddonId::FREE_HARBOR_SPOTS, AddonGroup::GamePlay, _("Build harbors without map markers"), - _("Allows harbors on suitable coastal castle sites even if the map does not define harbor spots.")) + _("Allows harbors on all suitable coastal castle sites even if the map does not define harbor spots.")) {} }; diff --git a/libs/s25main/world/BQCalculator.h b/libs/s25main/world/BQCalculator.h index 6452b6fa15..750d77cf81 100644 --- a/libs/s25main/world/BQCalculator.h +++ b/libs/s25main/world/BQCalculator.h @@ -10,8 +10,8 @@ struct BQCalculator { - BQCalculator(const World& world, const bool allowFreeHarborSpots = false) - : world(world), allowFreeHarborSpots(allowFreeHarborSpots) + BQCalculator(const World& world, const bool allowHarborsWithoutMapMarkers = false) + : world(world), allowHarborsWithoutMapMarkers(allowHarborsWithoutMapMarkers) {} template @@ -19,7 +19,7 @@ struct BQCalculator private: const World& world; - bool allowFreeHarborSpots; + bool allowHarborsWithoutMapMarkers; }; template @@ -224,21 +224,20 @@ BuildingQuality BQCalculator::operator()(const MapPoint pt, T_IsOnRoad isOnRoad, // If we can build a castle and this is a harbor point -> Allow harbor if(curBQ == BuildingQuality::Castle) { - bool isHarborPoint = world.GetNode(pt).harborId.isValid(); - if(!isHarborPoint && allowFreeHarborSpots) + if(world.GetNode(pt).harborId.isValid()) + curBQ = BuildingQuality::Harbor; + else if(allowHarborsWithoutMapMarkers) { for(const auto dir : helpers::EnumRange{}) { // Keep this in sync with harbor initialization: NW-only coasts are rejected there. if(dir != Direction::NorthWest && world.GetSeaFromCoastalPoint(neighbours[dir])) { - isHarborPoint = true; + curBQ = BuildingQuality::Harbor; break; } } } - if(isHarborPoint) - curBQ = BuildingQuality::Harbor; } ////////////////////////////////////////////////////////////////////////// diff --git a/tests/s25Main/integration/testSeaWorldCreation.cpp b/tests/s25Main/integration/testSeaWorldCreation.cpp index a25d326096..58133bcae6 100644 --- a/tests/s25Main/integration/testSeaWorldCreation.cpp +++ b/tests/s25Main/integration/testSeaWorldCreation.cpp @@ -6,7 +6,9 @@ #include "RttrForeachPt.h" #include "helpers/IdRange.h" #include "addons/const_addons.h" +#include "worldFixtures/CreateSeaWorld.h" #include "worldFixtures/SeaWorldWithGCExecution.h" +#include "worldFixtures/WorldFixture.h" #include "worldFixtures/terrainHelpers.h" #include "gameTypes/GameTypesOutput.h" #include "gameTypes/ShipDirection.h" @@ -123,6 +125,13 @@ void testHarborPoint(const GameWorld& world, const HarborId harborId) BOOST_TEST_REQUIRE(hasSea); BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); } + +using SeaWorldFixture = WorldFixture; + +struct MarkerlessIslandFixture : WorldFixtureBase +{ + MarkerlessIslandFixture() : WorldFixtureBase(3) { createMarkerlessIslandWorld(world); } +}; } // namespace BOOST_AUTO_TEST_CASE(GetShipDir) @@ -190,7 +199,7 @@ BOOST_FIXTURE_TEST_CASE(HarborSpotCreation, SeaWorldWithGCExecution<>) } } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonAddsCoastalHarbors, SeaWorldWithGCExecution<>) +BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonAddsCoastalHarbors, SeaWorldFixture) { const unsigned initialHarbors = world.GetNumHarborPoints(); @@ -205,9 +214,8 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonAddsCoastalHarbors, SeaWorldWithGCEx } } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, SeaWorldWithGCExecution<>) +BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, MarkerlessIslandFixture) { - createMarkerlessIslandWorld(world); BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); world.InitAfterLoad(); BOOST_TEST_REQUIRE(world.GetNumHarborPoints() == 0u); From 49cd2274fb6b2c4dff9ac87854601ae9581f21a1 Mon Sep 17 00:00:00 2001 From: Manu Date: Sun, 3 May 2026 10:08:12 +0200 Subject: [PATCH 04/10] gameplay: warn about free harbor spots addon risk --- libs/s25main/addons/AddonFreeHarborSpots.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libs/s25main/addons/AddonFreeHarborSpots.h b/libs/s25main/addons/AddonFreeHarborSpots.h index a06f4f044c..86c409dae2 100644 --- a/libs/s25main/addons/AddonFreeHarborSpots.h +++ b/libs/s25main/addons/AddonFreeHarborSpots.h @@ -11,7 +11,8 @@ class AddonFreeHarborSpots : public AddonBool { public: AddonFreeHarborSpots() - : AddonBool(AddonId::FREE_HARBOR_SPOTS, AddonGroup::GamePlay, _("Build harbors without map markers"), - _("Allows harbors on all suitable coastal castle sites even if the map does not define harbor spots.")) + : AddonBool(AddonId::FREE_HARBOR_SPOTS, AddonGroup::GamePlay, _("Dangerous: Build harbors without map markers"), + _("Advanced option. Allows harbors on all suitable coastal castle sites even if the map does not " + "define harbor spots. May heavily alter intended map seafaring design.")) {} }; From 5694b357f8a8679654928654e97dbc37cf5edcc5 Mon Sep 17 00:00:00 2001 From: Manu Date: Mon, 22 Jun 2026 13:24:50 +0200 Subject: [PATCH 05/10] Move extra harbor spot generation to map setup --- libs/s25main/world/BQCalculator.h | 24 ++-------- libs/s25main/world/MapLoader.cpp | 45 ++++++++++-------- .../integration/testSeaWorldCreation.cpp | 47 +++++++++++++++++-- 3 files changed, 73 insertions(+), 43 deletions(-) diff --git a/libs/s25main/world/BQCalculator.h b/libs/s25main/world/BQCalculator.h index 750d77cf81..88db722d3c 100644 --- a/libs/s25main/world/BQCalculator.h +++ b/libs/s25main/world/BQCalculator.h @@ -10,16 +10,13 @@ struct BQCalculator { - BQCalculator(const World& world, const bool allowHarborsWithoutMapMarkers = false) - : world(world), allowHarborsWithoutMapMarkers(allowHarborsWithoutMapMarkers) - {} + BQCalculator(const World& world) : world(world) {} template BuildingQuality operator()(MapPoint pt, T_IsOnRoad isOnRoad, bool flagOnly = false) const; private: const World& world; - bool allowHarborsWithoutMapMarkers; }; template @@ -222,23 +219,8 @@ BuildingQuality BQCalculator::operator()(const MapPoint pt, T_IsOnRoad isOnRoad, } // If we can build a castle and this is a harbor point -> Allow harbor - if(curBQ == BuildingQuality::Castle) - { - if(world.GetNode(pt).harborId.isValid()) - curBQ = BuildingQuality::Harbor; - else if(allowHarborsWithoutMapMarkers) - { - for(const auto dir : helpers::EnumRange{}) - { - // Keep this in sync with harbor initialization: NW-only coasts are rejected there. - if(dir != Direction::NorthWest && world.GetSeaFromCoastalPoint(neighbours[dir])) - { - curBQ = BuildingQuality::Harbor; - break; - } - } - } - } + if(curBQ == BuildingQuality::Castle && world.GetNode(pt).harborId) + curBQ = BuildingQuality::Harbor; ////////////////////////////////////////////////////////////////////////// // At this point we can still build a building/mine diff --git a/libs/s25main/world/MapLoader.cpp b/libs/s25main/world/MapLoader.cpp index 7f8735ee31..52beeeecf2 100644 --- a/libs/s25main/world/MapLoader.cpp +++ b/libs/s25main/world/MapLoader.cpp @@ -14,6 +14,7 @@ #include "buildings/nobHQ.h" #include "factories/BuildingFactory.h" #include "helpers/IdRange.h" +#include "helpers/containerUtils.h" #include "lua/GameDataLoader.h" #include "pathfinding/PathConditionShip.h" #include "random/Random.h" @@ -423,26 +424,21 @@ bool MapLoader::PlaceHQs(GameWorldBase& world, const std::vector& hqPo } namespace { -bool hasHarborAt(const World& world, const MapPoint pt) +bool hasEligibleHarborCoast(const World& world, const MapPoint pt) { - for(const auto harborId : helpers::idRange(world.GetNumHarborPoints())) + for(const auto dir : helpers::EnumRange{}) { - if(world.GetHarborPoint(harborId) == pt) + if(dir != Direction::NorthWest && world.GetSeaFromCoastalPoint(world.GetNeighbour(pt, dir))) return true; } return false; } -bool isFarEnoughFromHarbors(const World& world, const MapPoint pt, const std::vector& generatedHarbors) +bool isFarEnoughFromHarbors(const World& world, const MapPoint pt, const std::vector& harborPositions) { - for(const auto harborId : helpers::idRange(world.GetNumHarborPoints())) + for(const MapPoint harborPt : harborPositions) { - if(world.CalcDistance(pt, world.GetHarborPoint(harborId)) < MapLoader::MIN_GENERATED_HARBOR_DISTANCE) - return false; - } - for(const MapPoint generatedHarbor : generatedHarbors) - { - if(world.CalcDistance(pt, generatedHarbor) < MapLoader::MIN_GENERATED_HARBOR_DISTANCE) + if(world.CalcDistance(pt, harborPt) < MapLoader::MIN_GENERATED_HARBOR_DISTANCE) return false; } return true; @@ -451,16 +447,27 @@ bool isFarEnoughFromHarbors(const World& world, const MapPoint pt, const std::ve std::vector getGeneratedHarbors(const World& world) { std::vector generatedHarbors; - BQCalculator calcBQ(world, true); + std::vector harborPositions; + harborPositions.reserve(world.GetNumHarborPoints() + MapLoader::MAX_GENERATED_HARBOR_SPOTS); + for(const auto harborId : helpers::idRange(world.GetNumHarborPoints())) + harborPositions.push_back(world.GetHarborPoint(harborId)); + + BQCalculator calcBQ(world); RTTR_FOREACH_PT(MapPoint, world.GetSize()) { - if(!hasHarborAt(world, pt) && calcBQ(pt, [](const MapPoint&) { return false; }) == BuildingQuality::Harbor - && isFarEnoughFromHarbors(world, pt, generatedHarbors)) - { - generatedHarbors.push_back(pt); - if(generatedHarbors.size() == MapLoader::MAX_GENERATED_HARBOR_SPOTS) - return generatedHarbors; - } + if(helpers::contains(harborPositions, pt)) + continue; + if(calcBQ(pt, [](const MapPoint&) { return false; }) != BuildingQuality::Castle) + continue; + if(!hasEligibleHarborCoast(world, pt)) + continue; + if(!isFarEnoughFromHarbors(world, pt, harborPositions)) + continue; + + generatedHarbors.push_back(pt); + harborPositions.push_back(pt); + if(generatedHarbors.size() == MapLoader::MAX_GENERATED_HARBOR_SPOTS) + return generatedHarbors; } return generatedHarbors; } diff --git a/tests/s25Main/integration/testSeaWorldCreation.cpp b/tests/s25Main/integration/testSeaWorldCreation.cpp index e0b50c93c3..804e1b1bda 100644 --- a/tests/s25Main/integration/testSeaWorldCreation.cpp +++ b/tests/s25Main/integration/testSeaWorldCreation.cpp @@ -108,11 +108,20 @@ unsigned countHarborBQ(const GameWorld& world) std::vector getMarkerlessHarborCandidates(const GameWorld& world) { std::vector result; - BQCalculator calcBQ(world, true); + BQCalculator calcBQ(world); RTTR_FOREACH_PT(MapPoint, world.GetSize()) { - if(calcBQ(pt, [](const MapPoint&) { return false; }) == BuildingQuality::Harbor) - result.push_back(pt); + if(calcBQ(pt, [](const MapPoint&) { return false; }) != BuildingQuality::Castle) + continue; + + for(const auto dir : helpers::EnumRange{}) + { + if(dir != Direction::NorthWest && world.GetSeaFromCoastalPoint(world.GetNeighbour(pt, dir))) + { + result.push_back(pt); + break; + } + } } return result; } @@ -284,6 +293,25 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, MarkerlessIs } } +BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonIsDeterministic, MarkerlessIslandFixture) +{ + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); + world.InitAfterLoad(); + const std::vector generatedHarbors = getHarborPointsFrom(world, 1); + BOOST_TEST_REQUIRE(!generatedHarbors.empty()); + + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); + world.InitAfterLoad(); + const std::vector generatedHarborsAgain = getHarborPointsFrom(world, 1); + + BOOST_TEST_REQUIRE(generatedHarborsAgain.size() == generatedHarbors.size()); + for(unsigned i = 0; i < generatedHarbors.size(); ++i) + { + BOOST_TEST_REQUIRE(generatedHarborsAgain[i].x == generatedHarbors[i].x); + BOOST_TEST_REQUIRE(generatedHarborsAgain[i].y == generatedHarbors[i].y); + } +} + BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonKeepsGeneratedHarborsAwayFromExistingOnes, MarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); @@ -317,6 +345,19 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonDoesNotAffectRuntimeBQRecalculation, BOOST_TEST_REQUIRE(world.GetNode(candidates.front()).bq != BuildingQuality::Harbor); } +BOOST_FIXTURE_TEST_CASE(RuntimeBQRecalculationKeepsExistingHarborBQ, SeaWorldFixture) +{ + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); + world.InitAfterLoad(); + + const MapPoint harborPt = world.GetHarborPoint(HarborId(1)); + BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); + + world.RecalcBQ(harborPt); + + BOOST_TEST_REQUIRE(world.GetNode(harborPt).bq == BuildingQuality::Harbor); +} + BOOST_FIXTURE_TEST_CASE(HarborNeighbors, SeaWorldWithGCExecution<>) { // Now just test some assumptions: 2 harbor spots per possible HQ. From 9baf098d461b200330845e165704167f38c24ce5 Mon Sep 17 00:00:00 2001 From: Manu Date: Wed, 24 Jun 2026 15:55:59 +0200 Subject: [PATCH 06/10] Spread generated harbor spots deterministically --- libs/s25main/world/MapLoader.cpp | 40 ++++++++++++++--- .../integration/testSeaWorldCreation.cpp | 45 ++++++++++++++++--- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/libs/s25main/world/MapLoader.cpp b/libs/s25main/world/MapLoader.cpp index c4d9a6434e..e6d0fbb9dc 100644 --- a/libs/s25main/world/MapLoader.cpp +++ b/libs/s25main/world/MapLoader.cpp @@ -36,6 +36,8 @@ #include "s25util/Log.h" #include #include +#include +#include #include #include @@ -545,14 +547,22 @@ bool isFarEnoughFromHarbors(const World& world, const MapPoint pt, const std::ve return true; } +unsigned getMinimumHarborDistance(const World& world, const MapPoint pt, const std::vector& harborPositions) +{ + unsigned minDistance = std::numeric_limits::max(); + for(const MapPoint harborPt : harborPositions) + minDistance = std::min(minDistance, world.CalcDistance(pt, harborPt)); + return minDistance; +} + std::vector getGeneratedHarbors(const World& world) { - std::vector generatedHarbors; std::vector harborPositions; harborPositions.reserve(world.GetNumHarborPoints() + MapLoader::MAX_GENERATED_HARBOR_SPOTS); for(const auto harborId : helpers::idRange(world.GetNumHarborPoints())) harborPositions.push_back(world.GetHarborPoint(harborId)); + std::vector candidates; BQCalculator calcBQ(world); RTTR_FOREACH_PT(MapPoint, world.GetSize()) { @@ -565,10 +575,30 @@ std::vector getGeneratedHarbors(const World& world) if(!isFarEnoughFromHarbors(world, pt, harborPositions)) continue; - generatedHarbors.push_back(pt); - harborPositions.push_back(pt); - if(generatedHarbors.size() == MapLoader::MAX_GENERATED_HARBOR_SPOTS) - return generatedHarbors; + candidates.push_back(pt); + } + + std::vector generatedHarbors; + while(!candidates.empty() && generatedHarbors.size() < MapLoader::MAX_GENERATED_HARBOR_SPOTS) + { + auto bestCandidate = candidates.begin(); + unsigned bestDistance = getMinimumHarborDistance(world, *bestCandidate, harborPositions); + for(auto it = std::next(candidates.begin()); it != candidates.end(); ++it) + { + const unsigned distance = getMinimumHarborDistance(world, *it, harborPositions); + if(distance > bestDistance) + { + bestDistance = distance; + bestCandidate = it; + } + } + + if(bestDistance < MapLoader::MIN_GENERATED_HARBOR_DISTANCE) + break; + + generatedHarbors.push_back(*bestCandidate); + harborPositions.push_back(*bestCandidate); + candidates.erase(bestCandidate); } return generatedHarbors; } diff --git a/tests/s25Main/integration/testSeaWorldCreation.cpp b/tests/s25Main/integration/testSeaWorldCreation.cpp index 804e1b1bda..9cee927809 100644 --- a/tests/s25Main/integration/testSeaWorldCreation.cpp +++ b/tests/s25Main/integration/testSeaWorldCreation.cpp @@ -17,6 +17,7 @@ #include "gameTypes/ShipDirection.h" #include #include +#include // LCOV_EXCL_START static std::ostream& operator<<(std::ostream& out, const ShipDirection& dir) @@ -70,11 +71,12 @@ void testShipDir(const MapBase& world, const MapPoint fromPt) BOOST_TEST_REQUIRE(getShipDir(world, fromPt, DiffPt(100, -174)) == ShipDirection::North); } -void createMarkerlessIslandWorld(GameWorld& world) +void createMarkerlessRectangularIslandWorld(GameWorld& world, const MapExtent size, const MapPoint topLeft, + const MapPoint bottomRight) { world.Unload(); loadGameData(world.GetDescriptionWriteable()); - world.Init(MapExtent(30, 30)); + world.Init(size); const auto water = GetWaterTerrain(world.GetDescription()); RTTR_FOREACH_PT(MapPoint, world.GetSize()) @@ -84,9 +86,9 @@ void createMarkerlessIslandWorld(GameWorld& world) } const auto land = GetLandTerrain(world.GetDescription(), ETerrain::Buildable); - for(MapPoint pt(8, 8); pt.y < 22; ++pt.y) + for(MapPoint pt(topLeft); pt.y < bottomRight.y; ++pt.y) { - for(pt.x = 8; pt.x < 22; ++pt.x) + for(pt.x = topLeft.x; pt.x < bottomRight.x; ++pt.x) { MapNode& node = world.GetNodeWriteable(pt); node.t1 = node.t2 = land; @@ -94,6 +96,11 @@ void createMarkerlessIslandWorld(GameWorld& world) } } +void createMarkerlessIslandWorld(GameWorld& world) +{ + createMarkerlessRectangularIslandWorld(world, MapExtent(30, 30), MapPoint(8, 8), MapPoint(22, 22)); +} + unsigned countHarborBQ(const GameWorld& world) { unsigned result = 0; @@ -174,6 +181,14 @@ struct MarkerlessIslandFixture : WorldFixtureBase { MarkerlessIslandFixture() : WorldFixtureBase(3) { createMarkerlessIslandWorld(world); } }; + +struct LargeMarkerlessIslandFixture : WorldFixtureBase +{ + LargeMarkerlessIslandFixture() : WorldFixtureBase(3) + { + createMarkerlessRectangularIslandWorld(world, MapExtent(96, 96), MapPoint(16, 16), MapPoint(80, 80)); + } +}; } // namespace BOOST_AUTO_TEST_CASE(GetShipDir) @@ -300,9 +315,10 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonIsDeterministic, MarkerlessIslandFix const std::vector generatedHarbors = getHarborPointsFrom(world, 1); BOOST_TEST_REQUIRE(!generatedHarbors.empty()); - BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); - world.InitAfterLoad(); - const std::vector generatedHarborsAgain = getHarborPointsFrom(world, 1); + MarkerlessIslandFixture repeatedWorld; + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(repeatedWorld.world, std::vector(), true)); + repeatedWorld.world.InitAfterLoad(); + const std::vector generatedHarborsAgain = getHarborPointsFrom(repeatedWorld.world, 1); BOOST_TEST_REQUIRE(generatedHarborsAgain.size() == generatedHarbors.size()); for(unsigned i = 0; i < generatedHarbors.size(); ++i) @@ -312,6 +328,21 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonIsDeterministic, MarkerlessIslandFix } } +BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonSpreadsGeneratedHarborsBeyondEarlyScanCluster, LargeMarkerlessIslandFixture) +{ + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); + world.InitAfterLoad(); + + const std::vector generatedHarbors = getHarborPointsFrom(world, 1); + BOOST_TEST_REQUIRE(generatedHarbors.size() == MapLoader::MAX_GENERATED_HARBOR_SPOTS); + testMinimumHarborDistance(world, generatedHarbors); + + BOOST_TEST_REQUIRE( + std::any_of(generatedHarbors.begin(), generatedHarbors.end(), [](const MapPoint pt) { return pt.x >= 70; })); + BOOST_TEST_REQUIRE( + std::any_of(generatedHarbors.begin(), generatedHarbors.end(), [](const MapPoint pt) { return pt.y >= 70; })); +} + BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonKeepsGeneratedHarborsAwayFromExistingOnes, MarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); From 83fda19135be147ce844161083b8976d100669eb Mon Sep 17 00:00:00 2001 From: Manu Date: Wed, 24 Jun 2026 16:16:52 +0200 Subject: [PATCH 07/10] Enforce minimum distance for generated harbor spots --- .../integration/testSeaWorldCreation.cpp | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/s25Main/integration/testSeaWorldCreation.cpp b/tests/s25Main/integration/testSeaWorldCreation.cpp index 9cee927809..5424194ac7 100644 --- a/tests/s25Main/integration/testSeaWorldCreation.cpp +++ b/tests/s25Main/integration/testSeaWorldCreation.cpp @@ -182,6 +182,14 @@ struct MarkerlessIslandFixture : WorldFixtureBase MarkerlessIslandFixture() : WorldFixtureBase(3) { createMarkerlessIslandWorld(world); } }; +struct SmallMarkerlessIslandFixture : WorldFixtureBase +{ + SmallMarkerlessIslandFixture() : WorldFixtureBase(3) + { + createMarkerlessRectangularIslandWorld(world, MapExtent(24, 24), MapPoint(8, 8), MapPoint(16, 16)); + } +}; + struct LargeMarkerlessIslandFixture : WorldFixtureBase { LargeMarkerlessIslandFixture() : WorldFixtureBase(3) @@ -308,6 +316,29 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, MarkerlessIs } } +BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonStopsWhenRemainingCandidatesAreTooClose, SmallMarkerlessIslandFixture) +{ + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); + const std::vector candidates = getMarkerlessHarborCandidates(world); + BOOST_TEST_REQUIRE(candidates.size() > MapLoader::MAX_GENERATED_HARBOR_SPOTS); + + for(const MapPoint candidate : candidates) + { + BOOST_TEST_REQUIRE(world.CalcDistance(candidates.front(), candidate) + < MapLoader::MIN_GENERATED_HARBOR_DISTANCE); + } + + BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); + world.InitAfterLoad(); + + const std::vector generatedHarbors = getHarborPointsFrom(world, 1); + BOOST_TEST_REQUIRE(generatedHarbors.size() == 1u); + BOOST_TEST_REQUIRE(generatedHarbors.front().x == candidates.front().x); + BOOST_TEST_REQUIRE(generatedHarbors.front().y == candidates.front().y); + testMinimumHarborDistance(world, generatedHarbors); + testHarborPoint(world, HarborId(1)); +} + BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonIsDeterministic, MarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); From 03e780339983d2e0c0ce844307a6d41749bfe402 Mon Sep 17 00:00:00 2001 From: Manu Date: Mon, 20 Jul 2026 10:43:20 +0200 Subject: [PATCH 08/10] Address remaining harbor addon review comments --- libs/s25main/GlobalGameSettings.cpp | 2 +- .../addons/AddonAdditionalHarborSpots.h | 19 ++++++++++ libs/s25main/addons/AddonFreeHarborSpots.h | 18 ---------- libs/s25main/addons/Addons.h | 2 +- libs/s25main/addons/const_addons.h | 2 +- libs/s25main/world/MapLoader.cpp | 36 ++++++++----------- .../integration/testSeaWorldCreation.cpp | 21 +++++------ 7 files changed, 47 insertions(+), 53 deletions(-) create mode 100644 libs/s25main/addons/AddonAdditionalHarborSpots.h delete mode 100644 libs/s25main/addons/AddonFreeHarborSpots.h diff --git a/libs/s25main/GlobalGameSettings.cpp b/libs/s25main/GlobalGameSettings.cpp index 1ab4a794a3..e88dd126cd 100644 --- a/libs/s25main/GlobalGameSettings.cpp +++ b/libs/s25main/GlobalGameSettings.cpp @@ -75,7 +75,7 @@ void GlobalGameSettings::registerAllAddons() AddonDurableGeologistSigns, AddonEconomyModeGameLength, AddonExhaustibleWater, - AddonFreeHarborSpots, + AddonAdditionalHarborSpots, AddonFrontierDistanceReachable, AddonHalfCostMilEquip, AddonInexhaustibleFish, diff --git a/libs/s25main/addons/AddonAdditionalHarborSpots.h b/libs/s25main/addons/AddonAdditionalHarborSpots.h new file mode 100644 index 0000000000..bb59104fdc --- /dev/null +++ b/libs/s25main/addons/AddonAdditionalHarborSpots.h @@ -0,0 +1,19 @@ +// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org) +// +// SPDX-License-Identifier: GPL-2.0-or-later + +#pragma once + +#include "AddonBool.h" +#include "mygettext/mygettext.h" + +class AddonAdditionalHarborSpots : public AddonBool +{ +public: + AddonAdditionalHarborSpots() + : AddonBool(AddonId::ADDITIONAL_HARBOR_SPOTS, AddonGroup::GamePlay, + _("Dangerous: Add extra harbor spots"), + _("Advanced option. Converts a small set of suitable coastal castle sites to harbor spots. " + "Caution: May alter intended map seafaring design.")) + {} +}; diff --git a/libs/s25main/addons/AddonFreeHarborSpots.h b/libs/s25main/addons/AddonFreeHarborSpots.h deleted file mode 100644 index 345877e973..0000000000 --- a/libs/s25main/addons/AddonFreeHarborSpots.h +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org) -// -// SPDX-License-Identifier: GPL-2.0-or-later - -#pragma once - -#include "AddonBool.h" -#include "mygettext/mygettext.h" - -class AddonFreeHarborSpots : public AddonBool -{ -public: - AddonFreeHarborSpots() - : AddonBool(AddonId::FREE_HARBOR_SPOTS, AddonGroup::GamePlay, _("Dangerous: Add limited extra harbor spots"), - _("Advanced option. Adds only a small deterministic set of suitable coastal castle sites as extra " - "harbor spots. May alter intended map seafaring design.")) - {} -}; diff --git a/libs/s25main/addons/Addons.h b/libs/s25main/addons/Addons.h index 27a00039b0..96ce073dd6 100644 --- a/libs/s25main/addons/Addons.h +++ b/libs/s25main/addons/Addons.h @@ -53,7 +53,7 @@ #include "addons/AddonCoinsCapturedBld.h" #include "addons/AddonDemolishBldWORes.h" -#include "addons/AddonFreeHarborSpots.h" +#include "addons/AddonAdditionalHarborSpots.h" #include "addons/AddonFrontierDistanceReachable.h" #include "addons/AddonDurableGeologistSigns.h" diff --git a/libs/s25main/addons/const_addons.h b/libs/s25main/addons/const_addons.h index bd7cedfa4c..2ff320f424 100644 --- a/libs/s25main/addons/const_addons.h +++ b/libs/s25main/addons/const_addons.h @@ -77,7 +77,7 @@ ENUM_WITH_STRING(AddonId, LIMIT_CATAPULTS = 0x00000000, INEXHAUSTIBLE_MINES = 0x AUTOFLAGS = 0x00F00000, WINE = 0x01000000, LEATHER = 0x01000001, NO_ARMOR_DEFAULT = 0x01000002, - ARMOR_CAPTURED_BLD = 0x01000003, FREE_HARBOR_SPOTS = 0x01000004, + ARMOR_CAPTURED_BLD = 0x01000003, ADDITIONAL_HARBOR_SPOTS = 0x01000004, FORESTER_FARM_FIELD_AVOIDANCE = 0x01100000, diff --git a/libs/s25main/world/MapLoader.cpp b/libs/s25main/world/MapLoader.cpp index e6d0fbb9dc..569c9b493c 100644 --- a/libs/s25main/world/MapLoader.cpp +++ b/libs/s25main/world/MapLoader.cpp @@ -36,7 +36,6 @@ #include "s25util/Log.h" #include #include -#include #include #include #include @@ -60,7 +59,8 @@ bool MapLoader::Load(const libsiedler2::ArchivItem_Map& map, Exploration explora return false; PlaceObjects(map); PlaceAnimals(map); - if(!InitSeasAndHarbors(world_, std::vector(), world_.GetGGS().isEnabled(AddonId::FREE_HARBOR_SPOTS))) + if(!InitSeasAndHarbors(world_, std::vector(), + world_.GetGGS().isEnabled(AddonId::ADDITIONAL_HARBOR_SPOTS))) return false; /// Schatten @@ -531,22 +531,13 @@ bool hasEligibleHarborCoast(const World& world, const MapPoint pt) { for(const auto dir : helpers::EnumRange{}) { + // Skip the NW point because a harbor north of an island often has no usable path from that coastal point. if(dir != Direction::NorthWest && world.GetSeaFromCoastalPoint(world.GetNeighbour(pt, dir))) return true; } return false; } -bool isFarEnoughFromHarbors(const World& world, const MapPoint pt, const std::vector& harborPositions) -{ - for(const MapPoint harborPt : harborPositions) - { - if(world.CalcDistance(pt, harborPt) < MapLoader::MIN_GENERATED_HARBOR_DISTANCE) - return false; - } - return true; -} - unsigned getMinimumHarborDistance(const World& world, const MapPoint pt, const std::vector& harborPositions) { unsigned minDistance = std::numeric_limits::max(); @@ -572,33 +563,34 @@ std::vector getGeneratedHarbors(const World& world) continue; if(!hasEligibleHarborCoast(world, pt)) continue; - if(!isFarEnoughFromHarbors(world, pt, harborPositions)) + if(helpers::contains_if(harborPositions, [&](const MapPoint harborPt) { + return world.CalcDistance(pt, harborPt) < MapLoader::MIN_GENERATED_HARBOR_DISTANCE; + })) continue; candidates.push_back(pt); } std::vector generatedHarbors; - while(!candidates.empty() && generatedHarbors.size() < MapLoader::MAX_GENERATED_HARBOR_SPOTS) + while(generatedHarbors.size() < MapLoader::MAX_GENERATED_HARBOR_SPOTS) { - auto bestCandidate = candidates.begin(); - unsigned bestDistance = getMinimumHarborDistance(world, *bestCandidate, harborPositions); - for(auto it = std::next(candidates.begin()); it != candidates.end(); ++it) + MapPoint bestCandidate = MapPoint::Invalid(); + unsigned bestDistance = 0; + for(const MapPoint candidate : candidates) { - const unsigned distance = getMinimumHarborDistance(world, *it, harborPositions); + const unsigned distance = getMinimumHarborDistance(world, candidate, harborPositions); if(distance > bestDistance) { bestDistance = distance; - bestCandidate = it; + bestCandidate = candidate; } } if(bestDistance < MapLoader::MIN_GENERATED_HARBOR_DISTANCE) break; - generatedHarbors.push_back(*bestCandidate); - harborPositions.push_back(*bestCandidate); - candidates.erase(bestCandidate); + generatedHarbors.push_back(bestCandidate); + harborPositions.push_back(bestCandidate); } return generatedHarbors; } diff --git a/tests/s25Main/integration/testSeaWorldCreation.cpp b/tests/s25Main/integration/testSeaWorldCreation.cpp index 5424194ac7..105b80d35b 100644 --- a/tests/s25Main/integration/testSeaWorldCreation.cpp +++ b/tests/s25Main/integration/testSeaWorldCreation.cpp @@ -264,11 +264,11 @@ BOOST_FIXTURE_TEST_CASE(HarborSpotCreation, SeaWorldWithGCExecution<>) } } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonAddsCoastalHarbors, SeaWorldFixture) +BOOST_FIXTURE_TEST_CASE(AdditionalHarborSpotsAddonAddsCoastalHarbors, SeaWorldFixture) { const unsigned initialHarbors = world.GetNumHarborPoints(); - ggs.setSelection(AddonId::FREE_HARBOR_SPOTS, 1); + ggs.setSelection(AddonId::ADDITIONAL_HARBOR_SPOTS, 1); BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); world.InitAfterLoad(); @@ -288,7 +288,7 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonAddsCoastalHarbors, SeaWorldFixture) } } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, MarkerlessIslandFixture) +BOOST_FIXTURE_TEST_CASE(AdditionalHarborSpotsAddonWorksWithoutMapMarkers, MarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); world.InitAfterLoad(); @@ -297,7 +297,7 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, MarkerlessIs const std::vector candidates = getMarkerlessHarborCandidates(world); BOOST_TEST_REQUIRE(candidates.size() > MapLoader::MAX_GENERATED_HARBOR_SPOTS); - ggs.setSelection(AddonId::FREE_HARBOR_SPOTS, 1); + ggs.setSelection(AddonId::ADDITIONAL_HARBOR_SPOTS, 1); BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); world.InitAfterLoad(); BOOST_TEST_REQUIRE(world.GetNumHarborPoints() == 0u); @@ -316,7 +316,7 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonWorksWithoutMapMarkers, MarkerlessIs } } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonStopsWhenRemainingCandidatesAreTooClose, SmallMarkerlessIslandFixture) +BOOST_FIXTURE_TEST_CASE(AdditionalHarborSpotsAddonStopsWhenRemainingCandidatesAreTooClose, SmallMarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); const std::vector candidates = getMarkerlessHarborCandidates(world); @@ -339,7 +339,7 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonStopsWhenRemainingCandidatesAreTooCl testHarborPoint(world, HarborId(1)); } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonIsDeterministic, MarkerlessIslandFixture) +BOOST_FIXTURE_TEST_CASE(AdditionalHarborSpotsAddonIsDeterministic, MarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); world.InitAfterLoad(); @@ -359,7 +359,8 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonIsDeterministic, MarkerlessIslandFix } } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonSpreadsGeneratedHarborsBeyondEarlyScanCluster, LargeMarkerlessIslandFixture) +BOOST_FIXTURE_TEST_CASE(AdditionalHarborSpotsAddonSpreadsGeneratedHarborsBeyondEarlyScanCluster, + LargeMarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world, std::vector(), true)); world.InitAfterLoad(); @@ -374,7 +375,7 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonSpreadsGeneratedHarborsBeyondEarlySc std::any_of(generatedHarbors.begin(), generatedHarbors.end(), [](const MapPoint pt) { return pt.y >= 70; })); } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonKeepsGeneratedHarborsAwayFromExistingOnes, MarkerlessIslandFixture) +BOOST_FIXTURE_TEST_CASE(AdditionalHarborSpotsAddonKeepsGeneratedHarborsAwayFromExistingOnes, MarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); const std::vector candidates = getMarkerlessHarborCandidates(world); @@ -394,13 +395,13 @@ BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonKeepsGeneratedHarborsAwayFromExistin } } -BOOST_FIXTURE_TEST_CASE(FreeHarborSpotsAddonDoesNotAffectRuntimeBQRecalculation, MarkerlessIslandFixture) +BOOST_FIXTURE_TEST_CASE(AdditionalHarborSpotsAddonDoesNotAffectRuntimeBQRecalculation, MarkerlessIslandFixture) { BOOST_TEST_REQUIRE(MapLoader::InitSeasAndHarbors(world)); const std::vector candidates = getMarkerlessHarborCandidates(world); BOOST_TEST_REQUIRE(!candidates.empty()); - ggs.setSelection(AddonId::FREE_HARBOR_SPOTS, 1); + ggs.setSelection(AddonId::ADDITIONAL_HARBOR_SPOTS, 1); world.RecalcBQ(candidates.front()); BOOST_TEST_REQUIRE(world.GetNumHarborPoints() == 0u); From 24c2560a65803bf42a401edbd05b66943e337d66 Mon Sep 17 00:00:00 2001 From: Manuel Ries Date: Mon, 20 Jul 2026 11:29:51 +0200 Subject: [PATCH 09/10] Fix additional harbor spots addon build and ordering issues - MapLoader.cpp: include world/BQCalculator.h after GameWorldBase.h. BQCalculator.h is not self-contained (it needs noBase, BlockingManner and absDiff), so including it as the first header broke the GCC and Clang builds while MSVC still accepted it. - Apply clang-format 10 to AddonAdditionalHarborSpots.h, which the CI formatting job rejected. - Move the addon include in Addons.h and the registration entry in GlobalGameSettings.cpp to the positions matching the existing ordering conventions after the rename from "Free" to "Additional". - Rename getGeneratedHarbors to selectAdditionalHarborSpots: the function selects spots, it is not a getter. Co-Authored-By: Claude Opus 4.8 (1M context) --- libs/s25main/GlobalGameSettings.cpp | 2 +- libs/s25main/addons/AddonAdditionalHarborSpots.h | 3 +-- libs/s25main/addons/Addons.h | 2 +- libs/s25main/world/MapLoader.cpp | 7 ++++--- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/libs/s25main/GlobalGameSettings.cpp b/libs/s25main/GlobalGameSettings.cpp index e88dd126cd..d070375afd 100644 --- a/libs/s25main/GlobalGameSettings.cpp +++ b/libs/s25main/GlobalGameSettings.cpp @@ -75,7 +75,6 @@ void GlobalGameSettings::registerAllAddons() AddonDurableGeologistSigns, AddonEconomyModeGameLength, AddonExhaustibleWater, - AddonAdditionalHarborSpots, AddonFrontierDistanceReachable, AddonHalfCostMilEquip, AddonInexhaustibleFish, @@ -106,6 +105,7 @@ void GlobalGameSettings::registerAllAddons() AddonLeather, AddonNoArmorDefault, AddonArmorCapturedBld, + AddonAdditionalHarborSpots, AddonForesterFarmFieldAvoidance, AddonForesterReachRadius, AddonWoodcutterReachRadius, diff --git a/libs/s25main/addons/AddonAdditionalHarborSpots.h b/libs/s25main/addons/AddonAdditionalHarborSpots.h index bb59104fdc..fa008ad4e8 100644 --- a/libs/s25main/addons/AddonAdditionalHarborSpots.h +++ b/libs/s25main/addons/AddonAdditionalHarborSpots.h @@ -11,8 +11,7 @@ class AddonAdditionalHarborSpots : public AddonBool { public: AddonAdditionalHarborSpots() - : AddonBool(AddonId::ADDITIONAL_HARBOR_SPOTS, AddonGroup::GamePlay, - _("Dangerous: Add extra harbor spots"), + : AddonBool(AddonId::ADDITIONAL_HARBOR_SPOTS, AddonGroup::GamePlay, _("Dangerous: Add extra harbor spots"), _("Advanced option. Converts a small set of suitable coastal castle sites to harbor spots. " "Caution: May alter intended map seafaring design.")) {} diff --git a/libs/s25main/addons/Addons.h b/libs/s25main/addons/Addons.h index 96ce073dd6..44a80556f0 100644 --- a/libs/s25main/addons/Addons.h +++ b/libs/s25main/addons/Addons.h @@ -53,7 +53,6 @@ #include "addons/AddonCoinsCapturedBld.h" #include "addons/AddonDemolishBldWORes.h" -#include "addons/AddonAdditionalHarborSpots.h" #include "addons/AddonFrontierDistanceReachable.h" #include "addons/AddonDurableGeologistSigns.h" @@ -62,6 +61,7 @@ #include "addons/AddonAutoFlags.h" +#include "addons/AddonAdditionalHarborSpots.h" #include "addons/AddonArmorCapturedBld.h" #include "addons/AddonForesterFarmFieldAvoidance.h" #include "addons/AddonLeather.h" diff --git a/libs/s25main/world/MapLoader.cpp b/libs/s25main/world/MapLoader.cpp index 569c9b493c..d3b688d1f7 100644 --- a/libs/s25main/world/MapLoader.cpp +++ b/libs/s25main/world/MapLoader.cpp @@ -3,7 +3,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include "world/MapLoader.h" -#include "BQCalculator.h" #include "Game.h" #include "GamePlayer.h" #include "GameWorldBase.h" @@ -20,6 +19,8 @@ #include "lua/GameDataLoader.h" #include "pathfinding/PathConditionShip.h" #include "random/Random.h" +// Note: BQCalculator.h is not self-contained and must be included after GameWorldBase.h +#include "world/BQCalculator.h" #include "world/World.h" #include "nodeObjs/noAnimal.h" #include "nodeObjs/noEnvObject.h" @@ -546,7 +547,7 @@ unsigned getMinimumHarborDistance(const World& world, const MapPoint pt, const s return minDistance; } -std::vector getGeneratedHarbors(const World& world) +std::vector selectAdditionalHarborSpots(const World& world) { std::vector harborPositions; harborPositions.reserve(world.GetNumHarborPoints() + MapLoader::MAX_GENERATED_HARBOR_SPOTS); @@ -625,7 +626,7 @@ bool MapLoader::InitSeasAndHarbors(World& world, const std::vector& ad if(generateHarborSpots) { - for(MapPoint pt : getGeneratedHarbors(world)) + for(MapPoint pt : selectAdditionalHarborSpots(world)) world.harborData.push_back(HarborPos(pt)); } From ecd316034db42ff9d2c5553903aa204f41939470 Mon Sep 17 00:00:00 2001 From: Manuel Ries Date: Mon, 20 Jul 2026 13:20:41 +0200 Subject: [PATCH 10/10] Make BQCalculator.h self-contained It uses noBase, BlockingManner and absDiff without including their headers, so it only compiled when included after a header that pulls them in. MapLoader.cpp is the first user where the include sort order puts BQCalculator.h first, so include the two missing headers instead of relying on include order. Co-Authored-By: Claude Opus 4.8 (1M context) --- libs/s25main/world/BQCalculator.h | 2 ++ libs/s25main/world/MapLoader.cpp | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/s25main/world/BQCalculator.h b/libs/s25main/world/BQCalculator.h index 88db722d3c..02f57d2e92 100644 --- a/libs/s25main/world/BQCalculator.h +++ b/libs/s25main/world/BQCalculator.h @@ -5,7 +5,9 @@ #pragma once #include "World.h" +#include "commonDefines.h" // absDiff #include "helpers/containerUtils.h" +#include "nodeObjs/noBase.h" // noBase (used via World::GetNO) and BlockingManner #include "gameData/TerrainDesc.h" struct BQCalculator diff --git a/libs/s25main/world/MapLoader.cpp b/libs/s25main/world/MapLoader.cpp index d3b688d1f7..d90d31556d 100644 --- a/libs/s25main/world/MapLoader.cpp +++ b/libs/s25main/world/MapLoader.cpp @@ -19,7 +19,6 @@ #include "lua/GameDataLoader.h" #include "pathfinding/PathConditionShip.h" #include "random/Random.h" -// Note: BQCalculator.h is not self-contained and must be included after GameWorldBase.h #include "world/BQCalculator.h" #include "world/World.h" #include "nodeObjs/noAnimal.h"