Add limited extra harbor spots addon#1921
Conversation
|
Thanks, addressed the review points.
Semantics are unchanged: generated harbor spots are still created only during sea/harbor initialization, and runtime BQ recalculation does not invent additional harbor spots. Validation:
|
|
Isn't there quite a potential for breaking the game? Not sure if we correctly check if harbors are very close or not, but I think this is a dangerous option for people who do not know how the seafaring and harbors actually work. Although I do like the idea, I'd still not use such addon and rather go for a modified version of a map. |
That is a fair concern. The addon is disabled by default and it only adds spots during harbor initialization, but I agree that this can still change map design quite heavily for players who do not understand the seafaring rules. I can either make the addon text more explicit that this is an advanced/map-altering option, or close this PR if the preferred direction is to solve these cases with modified maps instead. |
|
Best would be showing a hint when enabling it I think. But we don't have that yet, not sure how hard it is to implement this. Also people sadly do not read the description in most cases. If we can have a popup, I'd go for that. If not, we maybe can prefix it with "Dangerous: ..."? Maybe this makes people question what this is about and they read the hint, |
Done. I prefixed the addon with I’ll keep the popup idea separate from this PR. A small generic addon-warning popup could be worth exploring as a follow-up, then we can review whether that direction makes sense before tying it to this addon. |
Very valid concern I'd say. E.g. exploration expeditions go to the "next" unexplored harbor spot, and (founders) expedition are steered towards the next (free) harbor spot. If now suddenly there are so many of them this could become unusable. I'm not sure if or for which maps this addon would be useful. Any example? |
That is a fair concern, and I agree we should not add this only because it is technically possible. I ran a local audit with Codex to get concrete data instead of guessing. The temporary audit loaded the available The scan found real examples where maps have no map-defined harbor spots but would get generated candidates:
So there are existing island/coastal maps where this addon has a concrete use case. However, the audit also confirms the risk: some maps would receive a very large number of generated harbor spots, which could indeed make expedition behavior worse or change the map too much. Maybe the better direction is not “allow every suitable coastal castle site”, but to add stricter generation rules. For example, generate only a small number of harbor spots per land component, enforce a minimum distance between generated harbors, and maybe limit small islands to one harbor at most while larger landmasses get only a few well-spaced candidates. I attached the audit report here so we can discuss whether the current broad version is acceptable, whether it should be constrained further, or whether this should stay out of upstream. |
Those are very few maps aren't they? Also those "Insel" Maps are rarely played. Thinking about this, I now also fear that people to expect this to work on most maps (e.g. they may think they can make "The green island" a seafaring map) but it fails most of the time. 5 / 444 maps are only some rare edge cases I think. If those maps are truly meant for seafaring, we should modify them I think. |
|
I feel similar: If there is a seafaring map then harbors will be placed by the author. Very likely in places where it makes sense and on purpose. Was something like this ever requested? If so in which situation? |
I’ve noticed this on a few maps myself, so the idea came from my own observation rather than a specific request. In some of those cases, I think additional harbor spots could have been useful. That said, I agree with your overall point: on a seafaring map, harbor placement is usually intentional and adding more indiscriminately would likely reduce map quality. It also probably wouldn’t have as much positive impact as one might expect. Given that this only affects a small number of maps, adjusting the system to avoid “harbor spam” doesn’t seem worth the effort. |
|
Ok, so it was basically your own request. Well, if we add this we should at least be sensible, i.e. this makes sense:
However this might be significant effort to identify landmasses/islands. There is already a quite elaborate algorithm for the map generator which works on "coastlines" but that can't be used here directly. Not sure if this is worth the effort.
That wouldn't work? Why?
That's the maps that had no harbors at all. So on others there were harbors, and now are more. That would also be an option: Limit it to maps with original harbors. Either way the add-on could then be described as "(add) extra harbors" Just some thoughts. |
So this is a map that feels quite coastal (but has no large building spots). I just wanted to say, that players might expect such maps to have seafaring with this addon enabled.
I like this idea to spread them somewhat evenly on a landmass. Also this should have some validation, if a harbor can connect any roads at all - maybe the harbor would block itself or trees are blocking buildings, so you'd have a harbor that can not be used. That even spread is important so you may not have most harbors on one side. But implementing this could be hard in some cases (e.g. if one side of the island is a mountain area and the other is flat for harbors). Could we preview the amount of generated harbors? Or is it smarted to only generate them after the game actually starts so disabling/enabling the addon does not cause that generation? As long as things are an addon, I'm fine with any addition - just want to make sure those addons don't confuse people or cause false positive bug reports because results don't meet expectation :) |
So it just doesn't make (much) sense as there is only a single island. But you could still use harbors as means of fast(er?) transport, can't you?
That might then be too much already. "spread even on a landmass" requires determining what a "landmass" is first. Possible but not trivial. Hence the idea to use randomness and distance thresholds to get (likely) something close to that.
Preview where? But yes as addons are part of a game they will be generated when loading the map for the game.
True. But we could make that clearer in the description. If it has something like "randomly converts some coastal spots to harbor spots" it would convey that results might not be perfect. Or even have that in the tooltip that not all such harbors might be useful or even usable. |
Honestly, on a map like the green island seafaring makes more sense in multiplayer than on some other maps. Simply because it is not necessary, but adds a second front with multiple players (if sea attacks are enabled. So normally you may not attack from the upper left to the lower right - but with seafaring you could.
I think we should, simpyl because even our random maps sometimes generated such spots and having a harbor you can not attack is pretty annyoing - or if you can not build anything next to it.
Like a message saying "X - Harbor spots generated on Y islands". So at least players know that something happened.
👍 maybe I'm to fearful, that people don't read those^^ |
That could happen when starting the game as a message printed to the log. Or do you want a message box? Place harbors on all spots but keep distance X to harbors on the same island and distance Y to any other harbor in "lexical" order (L->R, T->B)
Not our fault if they don't ;-) |
|
Thanks for the feedback and suggestions. I have my final exams this week, so I won’t be able to properly continue the discussion or rework this PR until after that. My current feeling is that the addon should probably be constrained more than the current broad version. The audit shows real markerless island/coastal map cases where generated harbor spots can help, but it also shows that allowing every suitable coastal castle site can create too many harbor spots on some maps. So I think the better direction might be stricter generation rules, for example limiting generated harbor spots per landmass and enforcing some spacing, instead of simply accepting every suitable candidate. I’ll take another look after my exams. |
|
After rereading the discussion, I agree that the current broad version is probably too aggressive. The audit shows real candidate maps, but it also shows the main problem: generating every suitable coastal castle site can create far too many harbor spots and may hurt expedition behavior. If this PR should continue, I would rework it toward a much more limited “extra harbor spots” addon instead of “free harbor spots”: collect candidate coastal castle sites, keep existing map harbors preferred, reject candidates too close to existing/generated harbors, and cap the number of generated spots. I would also keep the selection deterministic, not random per game, to avoid replay/network surprises. If that direction is still considered too niche for upstream, I’m also fine with closing this PR and leaving such cases to modified maps. |
DevOpsOfChaos
changed the title
|
Updated the PR to address the harbor-spam/design risk. The addon no longer generates every suitable coastal castle site as a harbor spot. Instead it now performs a deterministic limited selection:
This keeps the feature optional and still dangerous/advanced, but avoids the previous “turn every suitable coastal castle site into a harbor” behavior. The implementation intentionally does not add popup UI or expedition/ship logic changes in this PR. Validation:
|
Flamefire
left a comment
Flamefire
left a comment
There was a problem hiding this comment.
There is one big issue: This will basically create 4 harbors in the top of the map which doesn't sound like much of an improvement. I'd suggest to pick all viable spots and choose the "best" ones. That could be done fully randomly seeded by something map-specific for reproducibility or some "smart" algorithm. E.g.: Pick a random initial point and then add one-by-one points that have the largest min-distance to all existing ones >= MIN_GENERATED_HARBOR_DISTANCE
Or maybe someone has a better idea, but currently it just leads to a (very small) cluster of harbors
Additionally: If this is an addon already, why not let the user choose how many harbors to add? E.g. "None", "few", "many" (and 1-3 more options?) which then limit to some (internal) number which we could adjust in the future instead of letting users select e.g. "5 extra harbors". That could even scale with the map size and/or number/size of seas, possibly adjusted if there are many harbors already.
| // 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) |
There was a problem hiding this comment.
This should not be in here as it slows down processing during the game. This can be fully done at the place where this information is required and it makes the "keep in sync" part easier as both will be in the same file
| namespace { | ||
| bool hasHarborAt(const World& world, const MapPoint pt) | ||
| { | ||
| for(const auto harborId : helpers::idRange<HarborId>(world.GetNumHarborPoints())) |
There was a problem hiding this comment.
This might be unnecessarily slow: You can simply collect all harbor positions in a vector and use helpers::contains, can't you?
|
|
||
| bool isFarEnoughFromHarbors(const World& world, const MapPoint pt, const std::vector<MapPoint>& generatedHarbors) | ||
| { | ||
| for(const auto harborId : helpers::idRange<HarborId>(world.GetNumHarborPoints())) |
There was a problem hiding this comment.
Similar as above: If you have a single vector of all harbor points this is much faster and shorter
|
I tried to open the Jenkins job, but I revalidated the current head locally and could not reproduce a failure:
Could someone with Jenkins/GitLab access please paste the first failing console-log section or rerun the job? |
3 participants
Summary
FREE_HARBOR_SPOTSaddonMotivation
Some maps contain coastal locations that are physically suitable for harbor buildings, but do not define explicit harbor markers. On such maps, players may be unable to build a harbor even though the terrain would otherwise support one.
The original version of this PR generated harbor spots for all suitable coastal castle sites. Review feedback correctly pointed out that this was too aggressive: it could create harbor spam, change map design too heavily, confuse players, and make expedition behavior worse.
This version keeps the addon optional and disabled by default, but constrains the generated spots to a small deterministic set.
Behavior
When the addon is disabled:
When the addon is enabled:
Scope / safety notes
This PR intentionally keeps the behavior limited:
harborDataduring sea/harbor initializationharborId,seaId, coastal-point and neighbor data through the existing harbor initialization pathThe intent is not to turn arbitrary coastal maps into seafaring maps. It is only a constrained optional fallback for maps that already have suitable coastal castle sites but no corresponding harbor markers.
Validation
Test_integrationlocally with Visual Studio 2022 / DebugTest_integration.exe --run_test=SeaWorldCreationSuite --report_level=shortctest --test-dir .\build-vs-x64-debug-local -C Debug -R "^Test_integration$" --output-on-failuregit diff --check upstream/master...HEAD