Terrain generation - #49
Conversation
Implements the full map generation pipeline in TerrainGeneration, following the algorithm used by The Battle of Polytopia: - land generation: random land seeding followed by cellular-automata smoothing passes, tunable via InitialLand, Smoothing and Relief - capital placement: capitals spawn on land away from the map border, maximizing the distance between each other, with progressively relaxed requirements so placement never fails on tiny or watery maps - terrain assignment: every tile gets the biome of the nearest capital and rolls mountain/forest/water using the tribe terrain rates, with ocean adjacent to land becoming shallow water - village placement: villages spawn on free land at least 2 tiles away from any other city or village until no free tile remains - resource spawning: resources spawn only within 2 tiles of a city or village, at a third of the rate on the outer ring, using the tribe spawn rates as multipliers - ruins: one ancient ruin every 40 tiles, away from villages, never adjacent to each other and with at most a third of them on water Generation is fully deterministic for a given seed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
Use the American spelling "neighbor" to match the rest of the codebase, replace Mathf with System.Math in the terrain generation test and add "Chebyshev" to the cspell dictionary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
Adds a GlobalClass node wrapping TerrainGeneration so a map can be generated directly from a scene: all generation parameters (grid size, seed, initial land, smoothing, relief, water rate, player tribes) are exported to the inspector, the map can generate automatically when the node is ready or on demand via GenerateMapAsync, and the MapGenerated signal notifies when the grid and city manager are available. The node registers the tribes from the embedded tribes.json automatically; the file was updated to match the shape expected by TribeGenerationContext (tribe_type key and starting_stars inside the tribe object) since it previously failed to deserialize. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
Move detailed explanations from multi-paragraph summaries into remarks blocks and drop trailing periods from summaries, matching the style of the existing doc comments Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
Enn3Developer
left a comment
There was a problem hiding this comment.
Full review (including an adversarial pass) of the terrain generation PR — 7 inline comments below, ranked roughly by severity:
- 3 correctness bugs: overlapping capital territory claims in the fallback/tier-3 placement paths, map-wide
Owner-bit corruption from unvalidatedPlayerTribesthrough the unmaskedSetBits, and missing player-count validation in the Common-libraryTerrainGenerationAPI (crash/hang/bit-overflow when called directly). - 1 concurrency issue:
GenerateOnReadyracing a user-initiatedGenerateMapAsync. - 1 perf issue: per-candidate
Task.Yield()in village placement. - 1 doc regression in the
Grid.cstile bit-layout comment, and 3 missing.uidcompanion files.
Also checked and found fine: the tribes.json restructure matches TribeSerializedData/Tribe with no stale consumers, _capitals/players index alignment in NearestCapitalTribe, seed determinism, and the village-spacing/resource-zone logic.
Generated by Claude Code
| }); | ||
|
|
||
| // claim the starting 3x3 territory | ||
| ForEachInRadius(index, 1, neighbor => grid.ModifyTile(neighbor, (ref Tile tile) => { |
There was a problem hiding this comment.
Bug: a later capital's 3x3 claim can overwrite an earlier capital's territory — including its city tile.
This ForEachInRadius claim sets tile.Owner and tile.City unconditionally. In the normal tier-1/tier-2 placement paths capitals are far apart so the rings never overlap, but two paths break that guarantee:
- Ocean fallback: with
InitialLand = 0(a legal inspector value) and 2 players, capital 1 is raised from ocean by theCollectCapitalCandidatesfallback. For capital 2 noFieldtile exists (capital 1 is nowVillage), so the fallback picks a random non-Village tile — it only checksKind == Village, notOwneror the zone map — which can be adjacent to capital 1. Capital 2's claim then setsOwner = player2,City = 2on capital 1's city tile, while itsCityDatastill says player 1: a corrupted map. - Tier-3 (
ZONE_BORDER) candidates: capitals can end up at Chebyshev distance 2, so their 3x3 rings overlap and the later capital silently steals the shared tiles.
Suggest skipping tiles where Owner != 0 (or Kind == Village) in this claim, and/or making the fallback also reject owned/zoned tiles.
Generated by Claude Code
| /// The player at index <c>i</c> gets id <c>i + 1</c> | ||
| /// </remarks> | ||
| [Export] | ||
| public int[] PlayerTribes { get; set; } = [(int)TribeType.Imperius, (int)TribeType.Bardur]; |
There was a problem hiding this comment.
Bug: out-of-range PlayerTribes values silently corrupt the Owner bits of every tile.
Any int is accepted here from the inspector, but TribeType only defines values 0–5 and Biome is a 5-bit field. Extensions.SetBits doesn't mask its input (value.ClearBits(bits, position) | (data << position)), so e.g. PlayerTribes = [40] writes (TribeType)40 (0b101000, 6 bits) at TILE_BIOME_POSITION = 45, which sets bit 50 — the low bit of the adjacent 4-bit Owner field — without clearing it. Since GenerateTerrainAsync assigns a biome to every tile, unowned tiles map-wide end up with Owner = 1 and player 2's tiles with Owner = 3. No exception, just a silently broken map.
Suggest validating each entry against Enum.IsDefined(typeof(TribeType), ...) next to the existing player-count check (and ideally masking data in SetBits so overflow can never bleed into neighboring fields).
Generated by Claude Code
| /// <param name="tribeManager">the tribe manager to use</param> | ||
| /// <param name="players">all the players in the game</param> | ||
| /// <param name="seed">the optional random seed</param> | ||
| public class TerrainGeneration( |
There was a problem hiding this comment.
The player-count validation lives only in the Godot node wrapper — the Common-library API itself accepts anything.
TerrainGenerationNode.GenerateMapAsync guards 1–15 players, but this class (the API server-side code like StdbModule would call directly) doesn't:
players = []:GenerateInitialCitiesAsyncplaces no capitals, thenNearestCapitalTribereadsplayers[0].Tribeand throwsIndexOutOfRangeException— after the land pass has already mutated the grid.- 16+ players:
player.Id = 16overflows the 4-bitOwnerfield through the unmaskedSetBits, corrupting the adjacentBuildingbits. - More players than grid cells (e.g. 17 players on a 4x4 grid): the fallback loop
while (grid[index].Kind == TileKind.Village)inCollectCapitalCandidatesnever terminates once every tile is a village.
Suggest moving (or duplicating) the 1–15 check into this constructor or GenerateMapAsync so every consumer gets it.
Generated by Claude Code
| /// </remarks> | ||
| public TribeManager TribeManager { get; set; } = new(); | ||
|
|
||
| public override void _Ready() { |
There was a problem hiding this comment.
Race: the fire-and-forget GenerateOnReady run can overlap a user-initiated GenerateMapAsync.
GenerateOnReady defaults to true, and the class docs' example tells users to call node.GenerateMapAsync() themselves without mentioning turning it off — so two generations can easily run at once. They interleave at the many await Task.Yield() points; the second call overwrites Grid/CityManager/Players mid-flight, so when the first run's MapGenerated fires, a handler reading node.Grid sees the second, still-incomplete grid (missing cities/resources) — and MapGenerated is emitted twice.
Suggest an in-progress guard (e.g. a flag or storing the running Task and awaiting/returning it on re-entry).
Generated by Claude Code
| } | ||
| } | ||
|
|
||
| while (candidates.Count > 0 && _citiesCount < MAX_CITIES) { |
There was a problem hiding this comment.
Perf: this loop awaits Task.Yield() once per candidate — including every rejected one.
On a 64x64 map the candidate list holds up to ~3800 interior tiles and the loop drains it one element per iteration, yielding before each. Every other phase yields once per pass or per row; this loop alone posts thousands of continuations. If the synchronization context defers continuations to the next frame pump, village placement alone stretches across thousands of frames (tens of seconds); even when drained same-frame it's pure scheduling overhead.
Suggest yielding only after actually placing a village (i.e. after the _zoneMap check passes), matching the per-row yields elsewhere.
Generated by Claude Code
| /// <list type="bullet"> | ||
| /// <item>0 -> 1: has road; 0 doesn't have any road; (bridge if on water)</item> | ||
| /// <item>1 -> 1: has ancient ruin; 0 doesn't have any ruin</item> | ||
| /// <item>1 -> 2: has ancient ruin; 0 doesn't have any ruin</item> |
There was a problem hiding this comment.
Doc regression: 1 -> 2 is wrong here — the ruin flag is a single bit.
In this list's bit -> value format the second number is the value (the sibling entry 0 -> 1: has road means bit 0, value 1 = set), so the original 1 -> 1 was correct: bit 1 (RUIN_POSITION = 62), value 1. 1 -> 2 reads as if ruin spanned bits 1–2, but bit 2 is the first bit of the 3-bit Kind field ([2, 4] -> Kind, TILEKIND_POSITION = 59) — a developer trusting this comment and treating ruin as a 2-bit field would clobber the high bit of every tile's Kind. This change should be reverted.
Generated by Claude Code
There was a problem hiding this comment.
Missing .uid companion files for the three new Godot-project scripts.
TerrainGenerationNode.cs, TerrainGenerationNodeTest.cs and TerrainGenerationTest.cs don't have the .uid companions that every other tracked script in OpenPolytopia/src and OpenPolytopia/test/src has (e.g. Game.cs.uid, GridTest.cs.uid). Godot 4.4+ generates one per script on import, so the first time anyone (or the visual-tests CI) imports the project, three untracked *.cs.uid files appear as working-tree noise — and the [GlobalClass] TerrainGenerationNode gets a machine-generated UID that isn't shared through version control, so scenes referencing it by UID can diverge between checkouts. Please import once locally and commit the three .uid files.
Generated by Claude Code
- validate the player count in TerrainGeneration itself so every consumer gets it, not just the Godot node - validate PlayerTribes values in the node so an out-of-range tribe can't corrupt the biome bits - mask the data in SetBits so overflows can't bleed into neighboring bit fields - never steal already claimed tiles when a capital claims its starting territory and make the ocean fallback reject claimed tiles too, throwing instead of looping forever when no tile is left - guard GenerateMapAsync against racing the GenerateOnReady run - yield once per placed village instead of once per candidate - revert the wrong ruin bit change in the Grid docs - add the missing .uid files for the new scripts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
|
All 7 review findings addressed in e66dc1f:
Also added two regression tests: Generated by Claude Code |
Enn3Developer
left a comment
There was a problem hiding this comment.
Automated code review (high effort). Note: this was a single-pass inline review without the multi-agent fan-out/verify pass (Agent tool unavailable in this context), so findings were self-checked against the diff only.
6 findings, ranked most severe first:
TerrainGeneration.GenerateMapAsyncis not re-runnable — internal state (_capitals,_zoneMap,_citiesCount) never resets, second call crashes inNearestCapitalTribe(players[i]out of range).Player.Idunvalidated — id 0 breaks the territory/water-protection invariants; id ≥ 16 silently truncates in the 4-bitTile.Ownerfield.- Ruins can spawn on border-ring tiles that already have a resource modifier.
TerrainGenerationNodepublishesGrid/CityManager/Playersbefore generation completes (partial state observable during regeneration/failure).- Validation split/duplicated between node and
TerrainGeneration(tribe validity only checked in the node). - Nit:
(index - x) / sizeis justindex / size.
Also verified during review (no issues found): the SetBits masking change is safe for all existing callers; the tribes.json restructure now matches TribeSerializedData/Tribe (the old shape would have failed to deserialize); tile bit-field layouts don't overlap; capital/village/resource/ruin ordering and zone logic are internally consistent.
Generated by Claude Code
| /// await terrainGeneration.GenerateMapAsync(); | ||
| /// </code> | ||
| /// </example> | ||
| public async Task GenerateMapAsync() { |
There was a problem hiding this comment.
Correctness: calling GenerateMapAsync twice on the same instance crashes (and a reused grid is never fully reset).
_capitals, _zoneMap and _citiesCount are never cleared, so a second call appends another players.Length capitals to _capitals. NearestCapitalTribe then iterates for (i = 0; i < _capitals.Count; i++) and indexes players[i], which throws IndexOutOfRangeException as soon as i >= players.Length. Even before that, the stale zone map and leftover Owner/City/Modifier/Ruin bits (land generation only resets Kind to Ocean) corrupt capital candidate selection.
TerrainGenerationNode happens to build a fresh TerrainGeneration + Grid each run, but nothing in this public API documents or enforces single-use. Either reset the internal state (and clear the tiles fully) at the top of GenerateMapAsync, or guard against re-entry with a clear exception.
Generated by Claude Code
| /// </example> | ||
| public async Task GenerateMapAsync() { | ||
| // Tile.Owner is 4 bits, so there can't be more than 15 players | ||
| if (players.Length is 0 or > 15) { |
There was a problem hiding this comment.
Correctness: player count is validated but Player.Id is not (must be 1..15 and unique).
With new Player(tribe, 0), the capital tile and its 3x3 territory get Owner = 0, so:
- the "never steal tiles already claimed" check (
tile.Owner != 0) inGenerateInitialCitiesAsyncdoesn't protect that territory, - the "city territory never converts to water" guard (
grid[i].Owner == 0at line 344) fails and the capital's territory can be flooded, CollectCapitalCandidatestreats the claimed territory as free land for the next capital.
With Id >= 16, the 4-bit Tile.Owner field silently truncates it (e.g. 16 becomes owner 0, 17 becomes owner 1), producing wrong ownership with no error. Duplicate ids silently merge two players' territories. Since you already validate the count here, also validate that every id is in 1..15 and distinct.
Generated by Claude Code
| var tile = grid[index]; | ||
|
|
||
| // keep ruins away from cities, villages and their territory | ||
| if (tile.Ruin || tile.Kind == TileKind.Village || _zoneMap[index] >= ZONE_TERRITORY) { |
There was a problem hiding this comment.
Correctness: ruins can land on tiles that already carry a resource modifier.
Resources spawn on ZONE_BORDER tiles (at a third of the rate), and ruins are only excluded from _zoneMap[index] >= ZONE_TERRITORY, so a border-ring tile can first roll e.g. FieldTileModifier.Fruit in GenerateResourcesAsync and then get Ruin = true here. The result is a tile with both a fruit and an ancient ruin, which Polytopia doesn't produce and the renderer will have to display somehow. Adding tile.Modifier != 0 (or checking the modifier per kind) to this rejection condition fixes it.
Generated by Claude Code
| players[i] = new Player((TribeType)PlayerTribes[i], i + 1); | ||
| } | ||
|
|
||
| Players = players; |
There was a problem hiding this comment.
Correctness (minor): Players/Grid/CityManager are published before generation completes.
These properties are assigned before await generation.GenerateMapAsync(), so during a regeneration any consumer polling node.Grid (e.g. a renderer updating per frame) sees a half-generated map instead of the previous complete one, and if generation throws, the node is left permanently exposing a partial Grid even though the docs say the properties are valid once MapGenerated is emitted. Building into locals and assigning the three properties only after the await succeeds keeps the observable state always consistent.
Generated by Claude Code
|
|
||
| private async Task GenerateInternalAsync() { | ||
| // Tile.Owner is 4 bits, so there can't be more than 15 players | ||
| if (PlayerTribes.Length is 0 or > 15) { |
There was a problem hiding this comment.
Altitude/duplication: validation is split between the node and TerrainGeneration.
This player-count check duplicates the identical check (same message) at the top of TerrainGeneration.GenerateMapAsync, while the tribe-validity check below exists only here — anyone using TerrainGeneration directly (server code, tests) can still pass an undefined TribeType and silently get base-rate terrain via the tribeManager[biome]? null fallback. Consider moving the Enum.IsDefined validation into TerrainGeneration.GenerateMapAsync next to the player-count check, and dropping the duplicated count check here.
Generated by Claude Code
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public Vector2I IndexToGridPosition(uint index) { | ||
| var x = index % size; | ||
| var y = (index - x) / size; |
There was a problem hiding this comment.
Simplification (nit): (index - x) / size is just integer division — var y = index / size; gives the same result in both overloads without depending on x being computed first.
Generated by Claude Code
- guard GenerateMapAsync against being called twice on the same instance, since the grid and city manager keep the generated data - validate player ids (1-15, unique) and tribes in TerrainGeneration, dropping the duplicated checks from the node so every consumer gets the same validation - never spawn a ruin on a tile that already has a resource - publish Grid/CityManager/Players from the node only after the generation completes, so consumers never see a partial map - simplify IndexToGridPosition: (index - x) / size is just index / size Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
|
All 6 findings from the second review addressed in bbcb625:
Generated by Claude Code |
Enn3Developer
left a comment
There was a problem hiding this comment.
Code review of PR #49 (single-pass inline review at high effort; the multi-agent fan-out and separate verify pass were unavailable in this session, so all findings come from one manual pass over the diff and surrounding code).
4 findings posted inline, ranked most severe first:
- Village spacing enforces Chebyshev distance >= 3 while docs/tests say 2 (
GenerateCitiesAsync+MarkCityZone) CityManager.Citiesleaks the internal mutable listTerrainGenerationNode._jsonOptionsis the third copy of the same JsonSerializerOptions block- Per-tile capturing-lambda allocations in the generation loops where the
Gridindexer setter would allocate nothing
No crash-level or data-corruption bugs found. Things explicitly verified as correct: _capitals/players index alignment in NearestCapitalTribe; the SetBits masking change against all 27 call sites (all pass masks matching field widths — the change is a safe hardening fix); tribes.json restructure matches TribeSerializedData/Tribe (master's json was un-deserializable, this PR fixes it, and nothing on master consumed it); bit-field layouts (city id 8 bits vs MAX_CITIES=255, owner 4 bits vs 15-player cap, biome 5 bits vs Enum.IsDefined validation); zone-map/ruin/resource interactions; the _generation guard and exception paths in TerrainGenerationNode; and uint/int mixed comparisons in the margin checks (promoted to long, no wraparound).
Generated by Claude Code
| var candidates = new List<uint>(); | ||
| for (var i = 0u; i < cells; i++) { | ||
| grid.IndexToGridPosition(i, out var x, out var y); | ||
| if (_zoneMap[i] == ZONE_FREE && grid[i].Kind is TileKind.Field or TileKind.Forest && |
There was a problem hiding this comment.
Village spacing is stricter than documented. MarkCityZone marks every tile within Chebyshev distance 2 of a city as ZONE_BORDER, and this candidate filter requires ZONE_FREE, so a village can never spawn closer than distance 3 to another city/village. The XML remarks (here and on the class) say "2 tiles away from any other city or village", and TestVillages only asserts distance >= 2, so the test can't detect the discrepancy. Either the docs/test should say 3, or (if distance 2 was intended) the filter should allow ZONE_BORDER tiles, e.g. _zoneMap[i] <= ZONE_BORDER here and in the re-check inside the placement loop — the current behavior produces noticeably sparser villages than the stated design.
Generated by Claude Code
| public class CityManager(Grid grid) { | ||
| private readonly List<uint> _cities = []; | ||
|
|
||
| public IEnumerable<uint> Cities => _cities; |
There was a problem hiding this comment.
Internal list leaked through IEnumerable. This returns the live _cities list; a caller can do (List<uint>)cityManager.Cities and add/remove/reorder entries, silently corrupting the city-id-to-index mapping used by this[uint id], GetIndex and ModifyCity (ids are positions in this list). Consider public IReadOnlyList<uint> Cities => _cities; (still cast-able but conveys intent) or _cities.AsReadOnly() for a hard guarantee.
Generated by Claude Code
| Encoder = System.Text.Encodings.Web.JavaScriptEncoder.Create(UnicodeRanges.All), | ||
| TypeInfoResolver = TribeGenerationContext.Default, | ||
| PropertyNamingPolicy = JsonNamingPolicy.SnakeCaseLower | ||
| }; |
There was a problem hiding this comment.
Third copy of these JsonSerializerOptions. The identical options block (JavaScriptEncoder + context resolver + SnakeCaseLower) already exists in OpenPolytopia/test/src/TroopManagerTest.cs and TroopMovementTest.cs. Since this is now needed in production code, consider hoisting a shared options instance (or a LoadTribes() helper) into OpenPolytopia.Common next to EmbeddedResources/TribeGenerationContext, and reusing it from the tests, so the deserialization conventions can't drift between the three copies.
Generated by Claude Code
| } | ||
|
|
||
| var biome = NearestCapitalTribe(i); | ||
| grid.ModifyTile(i, (ref Tile tile) => tile.Biome = biome); |
There was a problem hiding this comment.
Per-tile closure allocations in the generation loops. This lambda captures biome (a fresh local each iteration), so every tile allocates a closure object plus a delegate; the same pattern occurs with kind in the smoothing pass of GenerateLandAsync (line 218) and the shallow-water pass (line 412) — roughly cells * (2 + passes) short-lived allocations. Grid already exposes an indexer setter designed for this read-modify-write case: var tile = grid[i]; tile.Biome = biome; grid[i] = tile; allocates nothing. Minor for a one-shot 64x64 generation, but cheap to fix.
Generated by Claude Code
- allow villages on border expansion tiles so the spacing matches the documented 2-tile minimum distance - expose CityManager.Cities as IReadOnlyList so the internal list can't be modified from outside - hoist the json serialization options into EmbeddedResources with LoadTribes/LoadTroops helpers, removing the three duplicated copies - replace the capturing lambdas in the generation loops with the grid indexer to avoid per-tile closure allocations Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
|
All 4 findings from the third review addressed in 65f1319:
Verified locally: Common builds with no new warnings, the invariant harness passes on all 5 map configurations plus determinism, the edge-case harness passes (ocean fallback, player validation, single-use guard, ruin/resource overlap), and cspell reports 0 issues. Generated by Claude Code |
Enn3Developer
left a comment
There was a problem hiding this comment.
Reviewed the terrain-generation pipeline end to end (4th round). The previously-raised issues all look properly addressed: the capital territory claim now guards Owner/Village, player id/tribe validation moved into TerrainGeneration.GenerateMapAsync, SetBits masks its input so an out-of-range enum can no longer bleed into neighbouring bit fields, single-use is enforced, the ruin check excludes tiles that already carry a resource, and the node's concurrency guard plus the json-options hoist are in.
Four remaining findings inline. The first two share a root cause: relaxing village spacing from 3 to 2 in the last commit turned village placement into a maximal packing, which saturates the land with ZONE_TERRITORY and starves ruin placement.
Generated by Claude Code
| var candidates = new List<uint>(); | ||
| for (var i = 0u; i < cells; i++) { | ||
| grid.IndexToGridPosition(i, out var x, out var y); | ||
| if (_zoneMap[i] <= ZONE_BORDER && grid[i].Kind is TileKind.Field or TileKind.Forest && |
There was a problem hiding this comment.
Village placement is a maximal packing, so ~1 in 4-5 eligible land tiles becomes a village.
Every Field/Forest interior tile is put in candidates and each one is popped exactly once (RemoveAt happens before the _zoneMap re-check). So when the loop ends, every candidate has either become a village or is within Chebyshev distance 1 of one — a maximal independent set in the king graph, whose density is ~20-25%.
Concretely, on the default 16x16 map (~128 land tiles, ~86% of them Field/Forest after the terrain rolls) that is roughly 20+ villages; Polytopia ships closer to 8-10 at that size. On a 64x64 map the packing wants ~350 villages, so _citiesCount < MAX_CITIES silently truncates placement at 255 and the resulting density depends on an implementation cap rather than on the design.
The knock-on effect is worse than the density itself: because the set is maximal, every Field/Forest interior tile ends up ZONE_TERRITORY, which is exactly what GenerateRuinsAsync rejects (see the separate comment there).
Before the last commit this filter was == ZONE_FREE (spacing 3, density ~1/9), which matches Polytopia much better. If spacing 2 is really the target, the loop probably also needs a village budget (e.g. a land / 9 target count) rather than draining the whole candidate list.
Generated by Claude Code
| var tile = grid[index]; | ||
|
|
||
| // keep ruins away from cities, villages, their territory and tiles with a resource | ||
| if (tile.Ruin || tile.Modifier != 0 || tile.Kind == TileKind.Village || _zoneMap[index] >= ZONE_TERRITORY) { |
There was a problem hiding this comment.
Ruins can almost never land on land, so the map gets at most a third of the documented tiles / 40 ruins — and all of them on water.
GenerateCitiesAsync produces a maximal distance-2 village packing, which means every Field/Forest interior tile ends the phase at ZONE_TERRITORY (it is either a village or adjacent to one). Mountains and border-row tiles are mostly adjacent to a village too. This rejection therefore filters out essentially the whole landmass, leaving only ZONE_FREE ocean as a viable ruin site.
Ocean sites then hit the very next guard: onWater && placedOnWater >= maxOnWater with maxOnWater = total / 3. Concretely on the default 16x16 map: total = 6, maxOnWater = 2 — 2 ruins get placed on ocean, the remaining ~178 attempts all fail, and the map ends with 2 water ruins and zero land ruins instead of the "tiles / 40 ruins, at most a third of them on water" in the XML remarks.
TestRuins only asserts ruins > 0 and ruins <= 6, so it passes and hides this. Worth asserting a lower bound (e.g. >= total - maxOnWater) and/or relaxing the zone filter to > ZONE_TERRITORY / excluding only ZONE_CITY.
Generated by Claude Code
| /// </example> | ||
| public async Task GenerateMapAsync() { | ||
| // if a generation is already running, wait for it instead of racing it | ||
| if (_generation != null) { |
There was a problem hiding this comment.
With GenerateOnReady = true (the default), the first user-initiated GenerateMapAsync() silently returns a map built from the inspector parameters, not the ones the caller just set.
Godot calls _Ready on children before their parent, so this is the normal ordering for the documented usage:
// parent's _Ready — the child node's _Ready already ran
var node = GetNode<TerrainGenerationNode>("TerrainGenerationNode");
node.GridSize = 32;
node.Seed = 1234;
node.PlayerTribes = [ (int)TribeType.Imperius, (int)TribeType.Bardur, (int)TribeType.Kickoo ];
await node.GenerateMapAsync();
var grid = node.Grid!; // 16x16, random seed, 2 default tribes_Ready already started GenerateAndReportAsync, so _generation != null here and the call just awaits that in-flight run and returns. The caller gets a completed, non-null Grid — no exception, no warning — that ignores every parameter it set, and MapGenerated has already fired for it. It also contradicts the summary right above ("A fresh Grid and CityManager are created on every call").
The remarks do mention the waiting behaviour, but silently discarding the caller's parameters is a sharp edge. Consider queueing a follow-up generation instead of returning the in-flight one (await it, then start a fresh one), or defaulting GenerateOnReady to false.
Generated by Claude Code
| /// Land/water balance of the smoothing passes | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// A cell stays land when at most <c>Relief</c> of the 9 cells around it are water, |
There was a problem hiding this comment.
The Relief remark describes the opposite of what the rule does, and the low end of the exported range produces a degenerate all-ocean map.
The rule is land iff waterCount <= Relief over the 3x3 block (centre included, out-of-bounds counted as water), so Relief is a land-growth threshold:
- higher
Relief-> more permissive -> land floods outward. AtRelief = 7an ocean tile with a single land neighbour becomes land, so successive passes fill the map with land — not "rougher coasts". - lower
Relief-> more restrictive -> land erodes. AtRelief = 0a tile survives only if all 9 cells are land, which is impossible on the border and rare inside, so afterSmoothingpasses the map is (near) all ocean — not "more compact continents".
That low end is reachable from the inspector: TerrainGenerationNode.Relief is exported as Range "0,8,1" and its tooltip repeats the same inverted claim. Setting Relief to 0-2 leaves no Field tiles at all, so every capital falls through all three tiers of CollectCapitalCandidates into the "raise a random tile from the ocean" fallback and the player gets a map of isolated one-tile islands with no error or warning.
Please fix the remark on both the property and the [Export] in TerrainGenerationNode, and consider narrowing the exported range (e.g. 3,6) so a degenerate map isn't one inspector drag away.
Generated by Claude Code
- bound village placement to about one village every 9 eligible tiles instead of maximally packing the land, which also frees up land for ruins to reach the documented tiles / 40 count - queue a new generation after the running one in TerrainGenerationNode so an explicit call never returns a map built from stale parameters - fix the inverted Relief remarks and narrow its exported range to 3-6 so a degenerate all-ocean map can't be selected from the inspector - assert the ruin count lower bound and the water ruins cap in TestRuins and add a queued generation test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JpezEueE6SxbBAWziamygM
|
All 4 findings from the fourth review addressed in 0c1ae56:
Verified locally: both projects build with 0 errors and no new warnings, the invariant harness passes all 5 configurations plus determinism (ruins now at the full count on every config), the edge-case harness passes all 7 scenarios, and cspell reports 0 issues. Generated by Claude Code |
Closes #18