Skip to content

Terrain generation - #49

Open
Enn3Developer wants to merge 13 commits into
masterfrom
n/terrain
Open

Terrain generation#49
Enn3Developer wants to merge 13 commits into
masterfrom
n/terrain

Conversation

@Enn3Developer

Copy link
Copy Markdown
Owner

Closes #18

@Enn3Developer Enn3Developer added the enhancement New feature or request label Mar 25, 2025
@Enn3Developer Enn3Developer added this to the 0.1.0 milestone Mar 25, 2025
Enn3Developer and others added 6 commits October 15, 2025 16:45
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
@Enn3Developer
Enn3Developer marked this pull request as ready for review August 12, 2026 14:30
claude added 2 commits August 12, 2026 14:54
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 Enn3Developer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 unvalidated PlayerTribes through the unmasked SetBits, and missing player-count validation in the Common-library TerrainGeneration API (crash/hang/bit-overflow when called directly).
  • 1 concurrency issue: GenerateOnReady racing a user-initiated GenerateMapAsync.
  • 1 perf issue: per-candidate Task.Yield() in village placement.
  • 1 doc regression in the Grid.cs tile bit-layout comment, and 3 missing .uid companion 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) => {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the CollectCapitalCandidates fallback. For capital 2 no Field tile exists (capital 1 is now Village), so the fallback picks a random non-Village tile — it only checks Kind == Village, not Owner or the zone map — which can be adjacent to capital 1. Capital 2's claim then sets Owner = player2, City = 2 on capital 1's city tile, while its CityData still 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];

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = []: GenerateInitialCitiesAsync places no capitals, then NearestCapitalTribe reads players[0].Tribe and throws IndexOutOfRangeException — after the land pass has already mutated the grid.
  • 16+ players: player.Id = 16 overflows the 4-bit Owner field through the unmasked SetBits, corrupting the adjacent Building bits.
  • More players than grid cells (e.g. 17 players on a 4x4 grid): the fallback loop while (grid[index].Kind == TileKind.Village) in CollectCapitalCandidates never 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() {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread OpenPolytopia.Common/Grid.cs Outdated
/// <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>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

All 7 review findings addressed in e66dc1f:

  • Player-count validation: the 1–15 check now lives in TerrainGeneration.GenerateMapAsync itself (kept in the node too), so direct consumers like StdbModule get it as well.
  • PlayerTribes corruption: the node now rejects values not defined in TribeType, and SetBits masks its data in all three extension classes (ulong/uint/int) so an overflow can never bleed into neighboring bit fields.
  • Overlapping capital claims: the 3x3 territory claim skips tiles that are already owned or a village, and the ocean fallback only picks unclaimed tiles (preferring free zones) — throwing a clear InvalidOperationException instead of looping forever when nothing is left. Verified across 50 seeds with InitialLand = 0 and 2 players: capital tiles keep their own owner and city id.
  • GenerateOnReady race: GenerateMapAsync stores the running task and awaits it on re-entry, so a user call can't race the ready-time run and MapGenerated fires once per generation.
  • Village-loop perf: Task.Yield() moved after the zone check, so it only yields once per placed village.
  • Grid.cs doc regression: reverted to 1 -> 1.
  • .uid files: added for the three new scripts.

Also added two regression tests: TerrainGenerationTest.TestInvalidPlayers (Common-level empty players throws) and TerrainGenerationNodeTest.TestInvalidTribe (out-of-range tribe value throws).


Generated by Claude Code

@Enn3Developer Enn3Developer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. TerrainGeneration.GenerateMapAsync is not re-runnable — internal state (_capitals, _zoneMap, _citiesCount) never resets, second call crashes in NearestCapitalTribe (players[i] out of range).
  2. Player.Id unvalidated — id 0 breaks the territory/water-protection invariants; id ≥ 16 silently truncates in the 4-bit Tile.Owner field.
  3. Ruins can spawn on border-ring tiles that already have a resource modifier.
  4. TerrainGenerationNode publishes Grid/CityManager/Players before generation completes (partial state observable during regeneration/failure).
  5. Validation split/duplicated between node and TerrainGeneration (tribe validity only checked in the node).
  6. Nit: (index - x) / size is just index / 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() {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) in GenerateInitialCitiesAsync doesn't protect that territory,
  • the "city territory never converts to water" guard (grid[i].Owner == 0 at line 344) fails and the capital's territory can be flooded,
  • CollectCapitalCandidates treats 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) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread OpenPolytopia.Common/Grid.cs Outdated
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public Vector2I IndexToGridPosition(uint index) {
var x = index % size;
var y = (index - x) / size;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

All 6 findings from the second review addressed in bbcb625:

  • Re-runnability: TerrainGeneration.GenerateMapAsync now throws a clear InvalidOperationException on a second call. A full internal reset wasn't viable because CityManager has no way to unregister cities, so the instance is explicitly single-use and documented as such; added TestSingleUse covering it.
  • Player.Id validation: ids must be 1–15 and unique, validated next to the player-count check (bitmask for duplicates); undefined tribes are rejected in the same loop.
  • Ruins on resources: the ruin rejection condition now also skips tiles with Modifier != 0; TestRuins asserts no ruin carries a modifier, and I verified 30 seeded 20x20 maps produce zero overlaps.
  • Partial state exposure: the node builds Grid/CityManager/Players into locals and publishes all three only after the generation succeeds, right before MapGenerated — a failed or in-flight regeneration leaves the previous complete map observable.
  • Split validation: all player validation (count, ids, tribes) now lives in TerrainGeneration.GenerateMapAsync; the node's duplicated count check and node-only Enum.IsDefined check are gone, so direct consumers get identical guarantees.
  • Nit: (index - x) / size simplified to index / size in both IndexToGridPosition overloads.

Generated by Claude Code

@Enn3Developer Enn3Developer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Village spacing enforces Chebyshev distance >= 3 while docs/tests say 2 (GenerateCitiesAsync + MarkCityZone)
  2. CityManager.Cities leaks the internal mutable list
  3. TerrainGenerationNode._jsonOptions is the third copy of the same JsonSerializerOptions block
  4. Per-tile capturing-lambda allocations in the generation loops where the Grid indexer 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 &&

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread OpenPolytopia.Common/City.cs Outdated
public class CityManager(Grid grid) {
private readonly List<uint> _cities = [];

public IEnumerable<uint> Cities => _cities;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
};

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

All 4 findings from the third review addressed in 65f1319:

  1. Village spacing — the documented 2-tile distance was the intended design, so the candidate filter (and the re-check in the placement loop) now allows ZONE_BORDER tiles instead of requiring ZONE_FREE. Villages can now spawn exactly 2 tiles away from another city/village, producing noticeably denser maps (e.g. 11 → 16 villages on the 16x16 seed-42 map). The pairwise-distance ≥ 2 invariant still holds across all test configurations.
  2. CityManager.Cities leak — now exposed as IReadOnlyList<uint> with a remark documenting that city ids are positions in the list. All existing consumers only iterate, so no call sites changed.
  3. Duplicated JsonSerializerOptions — hoisted into EmbeddedResources as a single private options instance (with JsonTypeInfoResolver.Combine over both source-gen contexts, since the node used TribeGenerationContext while the troop tests used TroopGenerationContext), exposed through LoadTribes()/LoadTroops() helpers. All three copies are gone; the node and both troop tests now call the helpers.
  4. Per-tile closure allocations — the three flagged sites (smoothing pass, biome assignment, shallow-water pass) now use the Grid indexer read-modify-write instead of ModifyTile with a capturing lambda. Note the shallow-water lambda captured nothing (so the compiler cached it), but it's converted anyway for consistency; the capture-free ModifyTile lambdas elsewhere are left as-is since they're allocation-free and match the repo idiom.

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 Enn3Developer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &&

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. At Relief = 7 an 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. At Relief = 0 a tile survives only if all 9 cells are land, which is impossible on the border and rare inside, so after Smoothing passes 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

Copy link
Copy Markdown
Owner Author

All 4 findings from the fourth review addressed in 0c1ae56:

  1. Maximal village packing — kept the 2-tile spacing but added a village budget: about one village every 9 eligible tiles (candidates.Count / 9, min 1), so the loop stops at the target instead of draining the candidate list. On the 16x16 seed-42 map this lands at 5 villages + 2 capitals ≈ one city per 10 land tiles, in line with Polytopia's density, and the 64x64 case stays far away from the MAX_CITIES implementation cap.
  2. Ruin starvation — resolved by the budget above rather than by relaxing the zone filter, so ruins still keep their distance from cities and villages. The seed-42 map now places the full tiles / 40 = 6 ruins (it was 2, all on water, before). TestRuins now asserts the lower bound (total - total / 3 <= ruins <= total) and that water ruins stay within total / 3, so a regression can't hide behind ruins > 0 again.
  3. Stale-parameter generationGenerateMapAsync now queues a fresh generation after the in-flight one (awaits it, observing but not rethrowing its failure since that run reports to its own caller, then runs GenerateInternalAsync), so an explicit call always generates with the parameters set before the call. Added TestQueuedGeneration asserting two overlapping calls emit MapGenerated twice.
  4. Inverted Relief docs + degenerate range — remarks on both the Common property and the node export now state the actual behavior (lower erodes land into ocean, higher expands it), and the exported range is narrowed to 3,6 so the all-ocean/all-land extremes aren't reachable from the inspector; the Common remark warns that values below 3 can erode the whole map.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Terrain generation

2 participants