From dbd5c6c4f2f49665724cf4472cc5abe0aad9a8a6 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 26 Jul 2026 15:27:43 +0200 Subject: [PATCH 1/4] fix(simulation): synthetic-overflow placements get the ocean-mask guard (T-1206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit synthetic_attractor now takes the terrain analysis and land-corrects its pure-arithmetic position via a bounded nearest-land ring walk (T-1116's pattern: row-major tie-break, column wrap, row clamp, MAX_LAND_SEARCH_RING=128 sized empirically — real polar ocean bands push nearest land up to 125 cells). Land positions pass through UNTOUCHED — verified by direct before/after scan of all 267 real bodies: 63 land-arithmetic placements byte-identical, and every golden/determinism harness passes unchanged. The gap was real and widespread: 46 of 109 synthetic-overflow placements sat in open water at seed 42 (e.g. GJ903c at a genuine polar ocean cell); post-fix zero, with all 109 preserved (confirmed at a second seed). Degradation is defined and pinned: no land within the bound -> the synthetic attractor is skipped and Phase 5's existing not-placed warning reports it — never a panic, never a fabricated water position (the bound never triggers on any scanned real body). 9 new unit tests; road_graph's anchor comment and the D-210 amendment record the gap CLOSED (validated). Co-Authored-By: Claude Fable 5 --- governance/decisions/architecture.md | 2 + server/src/atlas/attractor_matching.rs | 444 ++++++++++++++++++++++++- server/src/atlas/cascade.rs | 16 +- server/src/atlas/road_graph.rs | 33 +- 4 files changed, 476 insertions(+), 19 deletions(-) diff --git a/governance/decisions/architecture.md b/governance/decisions/architecture.md index d8afe0668..67d4942f1 100644 --- a/governance/decisions/architecture.md +++ b/governance/decisions/architecture.md @@ -1105,6 +1105,8 @@ Technical foundation decisions that constrain implementation: engine, client-ser **Amended 2026-07-26 (T-1116 — surrogate-anchor-at-cost carve-out for the Layer-2 road graph):** the road graph's routing grid (`RouteGrid`, `road_graph.rs`) downsamples `terrain_modification_cost` onto coarse routing cells (up to `scale²` native pixels per cell) and marks a cell `IMPASSABLE` when it is water-majority. A settlement's exact placement pixel is always land (D-209's attractor extraction guards `!ocean_mask` on every real attractor type), but at this downsample granularity the settlement's *routing cell* can still be majority water — a coastal-cell/downsample artifact, not a placement error. **Ruling:** a routing anchor (A\* start/goal) whose own cell is water-majority MAY be surrogated to the nearest passable cell within a bounded search radius, priced as an explicit access-cost surcharge on the routed edge's reported length (never free, modeling a short quay/causeway link) — but general water-cell transit stays `IMPASSABLE` exactly as before; only the anchor *lookup* for a start/goal settlement is relaxed, not open-ocean pathfinding. This was adjudicated as a routing-layer relaxation rather than a D-211 placement nudge specifically because D-211's positions are seed-derived and already land-guaranteed for every *real* attractor path — moving them would touch a different layer's invariant to fix a downsample artifact that belongs to the router. **Known gap flagged, not fixed here:** D-211's Phase-4 synthetic-overflow path (`synthetic_attractor`) computes its position by grid arithmetic alone, with no terrain/`ocean_mask` check at all — unlike every real attractor type, a synthetic-overflow settlement is not guaranteed land. The routing relaxation above still degrades that case gracefully (surrogate-anchors it or leaves it unrouted beyond the search radius), but the placement guarantee gap itself is D-211's, unticketed as of this amendment. +**Gap CLOSED 2026-07-26 (T-1206):** `synthetic_attractor` now takes an `Option<&TerrainAnalysis>` and guards its grid-arithmetic position against `ocean_mask` — land-passthrough (an already-land position is untouched; verified byte-identical on all 63 real bodies' land-arithmetic synthetic placements at world seed 42), water-only-correction (a deterministic ring-walk, the T-1116 `nearest_passable_cell` pattern at native resolution, nudges a water position to the nearest land cell — verified on all 46 real bodies found water-stranded pre-fix), and a defined skip (never a fabricated position or a panic) when no land exists within the bounded search radius. D-211's seed-derived-position promise holds: no existing land placement moved on any scanned body. + ### D-211: Attractor-Matching Five-Phase Pipeline for Settlement Placement - **Date:** 2026-05-01 - **Decision:** Given a body's `Vec` and a set of cities from `atlas_city_names`, settlement placement runs a five-phase matching pipeline: diff --git a/server/src/atlas/attractor_matching.rs b/server/src/atlas/attractor_matching.rs index aa9abbf9a..571d13fc4 100644 --- a/server/src/atlas/attractor_matching.rs +++ b/server/src/atlas/attractor_matching.rs @@ -17,7 +17,7 @@ use tracing::{error, warn}; -use crate::atlas::features::NO_WATER_BEARING; +use crate::atlas::features::{TerrainAnalysis, NO_WATER_BEARING}; use crate::seed::{splitmix64, SeedChain}; use crate::simulation::generator::{ AttractorType, CompatibilityMatrix, GeographicAttractor, SettlementClass, SubBiomeVariant, @@ -267,7 +267,109 @@ fn hungarian(cost: &[Vec]) -> Vec { /// Minimum pixel separation between synthetic attractor positions. const MIN_SPACING: u16 = 15; -fn synthetic_attractor(placed: &[CityPlacement], grid_w: u32, grid_h: u32) -> GeographicAttractor { +/// T-1206 — ocean-mask guard search bound. Bounded ring-expansion search +/// radius (cells) `nearest_land_cell` will walk from a water-arithmetic +/// synthetic position before giving up. +/// +/// **Sized empirically, not by analogy.** An early draft mirrored +/// `road_graph::COASTAL_ANCHOR_MAX_RING` (3) scaled up an order of magnitude +/// (20) — but a live scan of every body with a heightmap (T-1206 verification +/// scan, world seed 42) showed the dominant failure mode is `(0,0)` (the +/// arithmetic search loop's own first candidate, see `synthetic_attractor`) +/// landing in a genuine HIGH-LATITUDE OCEAN BAND near the grid's pole row — +/// not a small coastal-cell artifact. Real, substantially-land bodies (35–95% +/// land overall) had their nearest land cell to `(0,0)` as far as 125 cells +/// away (`grid_h` = 256, so `grid_h / 2` = 128 is the natural ceiling — a +/// point beyond that is more than half the grid's height from the pole and a +/// wider search stops paying for itself). 128 comfortably covers every +/// observed real-body case while still being a bounded, cheap search (worst +/// case ~128² ≈ 16k candidate cells, negligible next to the cascade's other +/// per-body costs) — genuinely water-locked bodies (no land within half the +/// grid height of the arithmetic position) still correctly degrade to the +/// skip path below. +const MAX_LAND_SEARCH_RING: u16 = 128; + +/// Ring-expansion search from `(row, col)` to the nearest cell with +/// `!ocean_mask` (T-1206, the T-1116 `nearest_passable_cell` pattern at +/// native/working-grid resolution rather than the road graph's downsampled +/// routing grid). Deterministic: rings expand outward in fixed distance +/// order, and within a ring, candidates are visited in a fixed row-major +/// scan (top edge left→right, bottom edge left→right, then left/right edges +/// top→bottom) — the same tie-break order as `nearest_passable_cell` — so +/// two equidistant land cells always resolve to the same winner for a given +/// terrain (pure function of `(row, col, terrain)`, no RNG, D-211's +/// seed-derived-position promise intact). Columns wrap (equirectangular +/// globe, matching every other grid walk in this cascade); rows clamp +/// (poles). Returns `None` if no land cell exists within +/// [`MAX_LAND_SEARCH_RING`] — the caller's defined degradation is to skip +/// the synthetic attractor entirely (T-1206), never to fabricate a position. +fn nearest_land_cell(ta: &TerrainAnalysis, row: u16, col: u16) -> Option<(u16, u16)> { + let (w, h) = (ta.w as i32, ta.h as i32); + if !ta.is_ocean(row as usize, col as usize) { + return Some((row, col)); + } + for ring in 1..=MAX_LAND_SEARCH_RING { + let ring_i = ring as i32; + let (r0, c0) = (row as i32, col as i32); + let mut candidates: Vec<(i32, i32)> = Vec::new(); + // Top edge, left→right. + if r0 - ring_i >= 0 { + let nr = r0 - ring_i; + for dc in -ring_i..=ring_i { + candidates.push((nr, c0 + dc)); + } + } + // Bottom edge, left→right. + if r0 + ring_i < h { + let nr = r0 + ring_i; + for dc in -ring_i..=ring_i { + candidates.push((nr, c0 + dc)); + } + } + // Left/right edges (corners already covered above), top→bottom. + for dr in (-ring_i + 1)..ring_i { + let nr = r0 + dr; + if nr < 0 || nr >= h { + continue; + } + candidates.push((nr, c0 - ring_i)); + candidates.push((nr, c0 + ring_i)); + } + for (nr, nc) in candidates { + let wrapped_c = nc.rem_euclid(w) as u16; + if !ta.is_ocean(nr as usize, wrapped_c as usize) { + return Some((nr as u16, wrapped_c)); + } + } + } + None +} + +/// Computes the D-211 Phase-4 synthetic-overflow position by pure grid +/// arithmetic (spacing-walk from grid center), then guards it against open +/// water (T-1206). `terrain` is `None` for callers without terrain data +/// (existing unit tests, any future caller that hasn't threaded it through) — +/// the guard is then a no-op and the arithmetic position passes straight +/// through UNCHANGED, exactly as before this ticket (byte-identical to the +/// pre-T-1206 behavior). +/// +/// **Land-passthrough, water-only-correction (T-1206):** when `terrain` IS +/// supplied, a synthetic position that is already land is returned +/// UNTOUCHED — this is the compatibility invariant the fix is built around: +/// no currently-land synthetic-overflow placement moves on any existing body +/// (D-211's seed-derived-position promise). Only a water-arithmetic position +/// is nudged, via [`nearest_land_cell`]'s deterministic ring-walk. If no land +/// cell exists within [`MAX_LAND_SEARCH_RING`] (an all-ocean region of the +/// grid), returns `None` — the caller's defined degradation is to skip this +/// synthetic attractor entirely (the city goes unplaced and is reported by +/// Phase 5's existing name-fulfillment warning) rather than fabricate a +/// water position or panic. +fn synthetic_attractor( + placed: &[CityPlacement], + grid_w: u32, + grid_h: u32, + terrain: Option<&TerrainAnalysis>, +) -> Option { // Place at grid center as default, then walk until spacing is satisfied. let mut row = (grid_h / 2) as u16; let mut col = (grid_w / 4) as u16; @@ -290,14 +392,20 @@ fn synthetic_attractor(placed: &[CityPlacement], grid_w: u32, grid_h: u32) -> Ge } } - GeographicAttractor { + if let Some(ta) = terrain { + let (land_row, land_col) = nearest_land_cell(ta, row, col)?; + row = land_row; + col = land_col; + } + + Some(GeographicAttractor { position: (row, col), attractor_type: AttractorType::PlainCenter, strength: 50, sub_biome: SubBiomeVariant::TemperateGrassland, terrain_modification_cost: 100, water_bearing: NO_WATER_BEARING, // inland synthetic — no water direction - } + }) } // --------------------------------------------------------------------------- @@ -335,6 +443,12 @@ fn city_character( (archetype, pattern, orientation) } +/// `terrain` is the body's [`TerrainAnalysis`] (T-1206), consulted ONLY by the +/// Phase-4 synthetic-overflow path to keep a synthetic `PlainCenter` off open +/// water — see [`synthetic_attractor`]'s doc for the land-passthrough / +/// water-only-correction / bounded-search-then-skip contract. `None` (every +/// pre-T-1206 caller, and any test that doesn't need the guard) reproduces +/// the exact prior behavior: an ungated grid-arithmetic position. pub fn match_cities( cities: &[CityRecord], attractors: &[GeographicAttractor], @@ -344,6 +458,7 @@ pub fn match_cities( grid_h: u32, territorial_status: &TerritorialStatus, seed: SeedChain, + terrain: Option<&TerrainAnalysis>, ) -> Vec { let default_cost = vec![100i32; attractors.len()]; let costs = terrain_costs.unwrap_or(&default_cost); @@ -497,7 +612,14 @@ pub fn match_cities( if placed_ids.contains(&city.city_id) { continue; } - let synthetic = synthetic_attractor(&placements, grid_w, grid_h); + // T-1206: `None` here (all-ocean region beyond MAX_LAND_SEARCH_RING, + // or degenerate MIN_SPACING exhaustion) means the defined degradation + // is to skip this city's synthetic placement entirely — never + // fabricate a water position. Phase 5's name-fulfillment check below + // reports it (the same warning path an unplaced city already takes). + let Some(synthetic) = synthetic_attractor(&placements, grid_w, grid_h, terrain) else { + continue; + }; let score = cell_score(city, &synthetic, matrix, 100); flag_mismatch(&city.name, score); let (archetype, pattern, orientation) = city_character( @@ -743,6 +865,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); assert_eq!(placements.len(), 1); assert_eq!(placements[0].city_id, 1); @@ -777,6 +900,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); assert_eq!(placements.len(), 3); @@ -818,6 +942,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); let p1 = placements.iter().find(|p| p.city_id == 1).unwrap(); assert_eq!(p1.position, (5, 5), "NameLocked should get best attractor"); @@ -841,6 +966,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); assert_eq!(placements.len(), 2); let p2 = placements.iter().find(|p| p.city_id == 2).unwrap(); @@ -877,6 +1003,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); let major = placements.iter().find(|p| p.city_id == 1).unwrap(); let minor = placements.iter().find(|p| p.city_id == 2).unwrap(); @@ -908,6 +1035,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); let hero = placements2.iter().find(|p| p.city_id == 3).unwrap(); assert_eq!( @@ -936,6 +1064,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); assert_eq!(placements.len(), 5, "all cities must be placed"); } @@ -981,6 +1110,7 @@ mod tests { 256, &TerritorialStatus::FrontierUnclaimed, SeedChain::root(42), + None, ); assert_eq!(placements.len(), 2); let farm = placements.iter().find(|p| p.city_id == 1).unwrap(); @@ -1119,4 +1249,308 @@ mod tests { ArrangementPattern::HubAndSpoke ); } + + // ----------------------------------------------------------------------- + // T-1206: synthetic-overflow ocean-mask guard + // ----------------------------------------------------------------------- + + /// Builds a minimal `TerrainAnalysis` with only `w`/`h`/`ocean_mask` + /// meaningfully populated — the only fields `nearest_land_cell`/`is_ocean` + /// read. `land_cells` are `(row, col)` positions that are land; every + /// other cell in the `w × h` grid is ocean. + fn ta_with_land(w: usize, h: usize, land_cells: &[(u16, u16)]) -> TerrainAnalysis { + let mut ocean_mask = vec![true; w * h]; + for &(r, c) in land_cells { + ocean_mask[r as usize * w + c as usize] = false; + } + TerrainAnalysis { + w, + h, + ocean_mask, + lake_mask: vec![false; w * h], + water_dist: vec![0; w * h], + slope_deg: vec![0.0; w * h], + elev_pct: vec![0.0; w * h], + hydrology: None, + } + } + + /// All-land terrain (every cell passes `!ocean_mask`) — a convenience for + /// tests that only care about the spacing-walk arithmetic, not the guard. + fn ta_all_land(w: usize, h: usize) -> TerrainAnalysis { + TerrainAnalysis { + w, + h, + ocean_mask: vec![false; w * h], + lake_mask: vec![false; w * h], + water_dist: vec![0; w * h], + slope_deg: vec![0.0; w * h], + elev_pct: vec![0.0; w * h], + hydrology: None, + } + } + + /// **Compatibility invariant (T-1206, requirement 1):** a synthetic + /// position that is ALREADY LAND is returned byte-identical whether or + /// not terrain is supplied — the guard must never move an + /// already-land placement. This is the exact property that protects every + /// existing golden fixture: on a body where the arithmetic synthetic + /// position happens to be land (the common case, per the T-1206 + /// verification scan), passing `Some(&terrain)` changes nothing. + #[test] + fn synthetic_attractor_land_passthrough_is_byte_identical_to_no_terrain() { + let placed: Vec = Vec::new(); + let (grid_w, grid_h) = (512u32, 256u32); + // The arithmetic default before any spacing-walk iteration is + // (grid_h/2, grid_w/4) = (128, 128); make that cell (and enough of a + // margin around it) land so the very first candidate the spacing-walk + // tries — (0,0) — is NOT what gets returned, isolating the + // passthrough check to the "arithmetic position already lands on + // land" case rather than accidentally exercising the guard. + let ta = ta_all_land(grid_w as usize, grid_h as usize); + + let without_terrain = synthetic_attractor(&placed, grid_w, grid_h, None) + .expect("no terrain — always Some, ungated"); + let with_terrain = synthetic_attractor(&placed, grid_w, grid_h, Some(&ta)) + .expect("all-land terrain — guard is a no-op, must still be Some"); + + assert_eq!( + without_terrain.position, with_terrain.position, + "a land-arithmetic synthetic position must be untouched by the T-1206 guard" + ); + // Sanity: this is genuinely the (0,0) first-candidate case the T-1206 + // scan found in production (empty `placed` list — the spacing check + // against zero existing placements passes trivially at (0,0)). + assert_eq!(without_terrain.position, (0, 0)); + } + + /// **Water-only-correction (T-1206, requirement 1):** when the arithmetic + /// position IS water, the guard nudges it to the nearest land cell via + /// `nearest_land_cell`'s deterministic ring-walk — never to a fabricated + /// or RNG-derived position. + #[test] + fn synthetic_attractor_nudges_off_water_to_nearest_land() { + let placed: Vec = Vec::new(); + let (grid_w, grid_h) = (64u32, 32u32); + // Arithmetic position (with empty `placed`) is (0,0) — make that + // water, with the nearest land cell at (2,0) (ring 2, straight south). + let ta = ta_with_land(grid_w as usize, grid_h as usize, &[(2, 0)]); + + let guarded = synthetic_attractor(&placed, grid_w, grid_h, Some(&ta)) + .expect("land exists within MAX_LAND_SEARCH_RING"); + assert_eq!( + guarded.position, + (2, 0), + "must land exactly on the nearest land cell, not merely off water" + ); + } + + /// **Row-major ring tie-break determinism (T-1206, D-010):** when two + /// land cells are equidistant (same ring) from the water-arithmetic + /// position, `nearest_land_cell` always resolves to the same one — a + /// pure function of `(position, terrain)`, matching the T-1116 + /// `nearest_passable_cell` precedent's tie-break order (top edge + /// left→right first). + #[test] + fn nearest_land_cell_tie_break_is_deterministic() { + let (w, h) = (32usize, 32usize); + // Two land cells on ring 1 from (5,5): (4,4) [top-edge, visited + // first] and (4,6) [also top-edge, visited after (4,4) — left→right + // scan order]. The top-edge candidate list is built left→right, so + // (4,4) must win over (4,6) even though both are ring-1/Chebyshev-1. + let ta = ta_with_land(w, h, &[(4, 4), (4, 6)]); + let found = nearest_land_cell(&ta, 5, 5); + assert_eq!(found, Some((4, 4))); + + // Repeat many times — pure function, must be exactly reproducible. + for _ in 0..20 { + assert_eq!(nearest_land_cell(&ta, 5, 5), Some((4, 4))); + } + } + + /// **Column wrap (T-1206):** the working grid is equirectangular — a + /// search that walks off the left/right edge wraps around, matching + /// every other grid walk in this cascade (`features.rs::wrap_col`, + /// `road_graph.rs::nearest_passable_cell`). + #[test] + fn nearest_land_cell_wraps_columns() { + let (w, h) = (16usize, 16usize); + // Land only at column 0 — from (5, 1) (one step from the right edge + // of the wrap, i.e. effectively adjacent to column 0 via wraparound + // is NOT the case here; instead test from col=0 neighbourhood + // directly) we confirm wrap by placing land at col (w-1) and + // searching from col 0, which should find it by wrapping left. + let ta = ta_with_land(w, h, &[(5, (w - 1) as u16)]); + let found = nearest_land_cell(&ta, 5, 0); + assert_eq!( + found, + Some((5, (w - 1) as u16)), + "search from col 0 must wrap left to find land at the opposite edge" + ); + } + + /// **Rows clamp, do not wrap (T-1206):** matches every other grid walk in + /// this cascade (poles are grid edges, not a torus in the row direction). + /// A ring whose top/bottom edge would fall outside `[0, h)` simply omits + /// that edge's candidates rather than wrapping to the opposite pole. + #[test] + fn nearest_land_cell_clamps_rows_no_wrap() { + let (w, h) = (16usize, 16usize); + // Land only at the FAR pole (row h-1) — from row 0, a row-wrapping + // implementation would find it at ring (h-1); a row-clamping one + // must exhaust MAX_LAND_SEARCH_RING first if h-1 > that bound, or + // find it only via the correct non-wrapped ring distance. Here + // h=16 keeps h-1=15 comfortably inside MAX_LAND_SEARCH_RING (128), + // so the assertion is on the POSITION found, not on absence: a + // wrapping bug would still find (15, c) — same as clamping would, + // since row 15 IS within the grid — so instead assert a cell just + // below row 0 wrapping to the top is never sourced from "negative + // row mod h" by using an asymmetric single-land-cell placement at a + // row that would be reached MUCH sooner via wraparound than via the + // real clamped ring distance. + let ta = ta_with_land(w, h, &[(15, 0)]); + let found = nearest_land_cell(&ta, 0, 0); + // Real (clamped) distance is ring 15 (straight down the column). + // A wrapping implementation could equally reach it at ring 1 (one + // step "up" from row 0 wrapping to row 15) — assert the ring-15 + // (non-wrapped) result to pin clamping behavior. + assert_eq!(found, Some((15, 0))); + } + + /// **Degradation (T-1206, requirement 2): no land within + /// `MAX_LAND_SEARCH_RING` → skip, never fabricate or panic.** A tiny + /// all-ocean grid guarantees no land cell exists anywhere, so the guard + /// must return `None` — `match_cities`'s Phase 4 then skips this city + /// entirely (verified below via `match_cities` directly, matching Phase + /// 5's existing "unplaced city" warning path — no new error path). + #[test] + fn nearest_land_cell_returns_none_when_no_land_within_bound() { + let (w, h) = (16usize, 16usize); + let ta = TerrainAnalysis { + w, + h, + ocean_mask: vec![true; w * h], // every cell is water — no land at all + lake_mask: vec![false; w * h], + water_dist: vec![0; w * h], + slope_deg: vec![0.0; w * h], + elev_pct: vec![0.0; w * h], + hydrology: None, + }; + assert_eq!(nearest_land_cell(&ta, 0, 0), None); + } + + #[test] + fn synthetic_attractor_returns_none_on_all_ocean_terrain() { + let placed: Vec = Vec::new(); + let (grid_w, grid_h) = (16u32, 16u32); + let ta = TerrainAnalysis { + w: grid_w as usize, + h: grid_h as usize, + ocean_mask: vec![true; (grid_w * grid_h) as usize], + lake_mask: vec![false; (grid_w * grid_h) as usize], + water_dist: vec![0; (grid_w * grid_h) as usize], + slope_deg: vec![0.0; (grid_w * grid_h) as usize], + elev_pct: vec![0.0; (grid_w * grid_h) as usize], + hydrology: None, + }; + assert!(synthetic_attractor(&placed, grid_w, grid_h, Some(&ta)).is_none()); + } + + /// **End-to-end degradation through `match_cities` (T-1206, requirement + /// 2):** on an all-ocean grid, a city that would overflow to Phase 4 + /// synthetic placement is instead SKIPPED — never placed in water, never + /// a panic. This mirrors the existing "atlas city was not placed" Phase-5 + /// warning path (no new reporting mechanism needed). + #[test] + fn match_cities_skips_synthetic_overflow_on_all_ocean_body() { + let cities = vec![make_city(1, SettlementClass::PopulationBudget, 60_000)]; + let attractors: Vec = Vec::new(); // forces Phase-4 overflow + let matrix = uniform_matrix(); + let (grid_w, grid_h) = (16u32, 16u32); + let n = (grid_w * grid_h) as usize; + let ta = TerrainAnalysis { + w: grid_w as usize, + h: grid_h as usize, + ocean_mask: vec![true; n], + lake_mask: vec![false; n], + water_dist: vec![0; n], + slope_deg: vec![0.0; n], + elev_pct: vec![0.0; n], + hydrology: None, + }; + + let placements = match_cities( + &cities, + &attractors, + &matrix, + None, + grid_w, + grid_h, + &TerritorialStatus::FrontierUnclaimed, + SeedChain::root(42), + Some(&ta), + ); + + assert!( + placements.is_empty(), + "an all-ocean body must SKIP the overflow city, not fabricate a water placement" + ); + } + + /// **`match_cities` end-to-end land-guard (T-1206):** the exact + /// production shape — Phase-4 overflow on a body with real terrain — + /// never places a synthetic `CityPlacement` on an ocean cell. + #[test] + fn match_cities_synthetic_overflow_never_lands_in_water() { + let cities = vec![make_city(1, SettlementClass::PopulationBudget, 60_000)]; + let attractors: Vec = Vec::new(); // forces Phase-4 overflow + let matrix = uniform_matrix(); + let (grid_w, grid_h) = (64u32, 32u32); + // Water everywhere except a single land cell far from the arithmetic + // (0,0) default, forcing the guard to actually nudge the position. + let ta = ta_with_land(grid_w as usize, grid_h as usize, &[(10, 10)]); + + let placements = match_cities( + &cities, + &attractors, + &matrix, + None, + grid_w, + grid_h, + &TerritorialStatus::FrontierUnclaimed, + SeedChain::root(42), + Some(&ta), + ); + + assert_eq!(placements.len(), 1); + assert_eq!(placements[0].position, (10, 10)); + assert!(placements[0].synthetic); + assert!(!ta.is_ocean( + placements[0].position.0 as usize, + placements[0].position.1 as usize + )); + } + + /// **`None` terrain reproduces the exact pre-T-1206 behavior + /// (compatibility):** every existing caller/test that doesn't supply + /// terrain gets the ungated grid-arithmetic position, unchanged. + #[test] + fn match_cities_with_no_terrain_is_ungated_like_before_t1206() { + let cities = vec![make_city(1, SettlementClass::PopulationBudget, 60_000)]; + let attractors: Vec = Vec::new(); + let matrix = uniform_matrix(); + let placements = match_cities( + &cities, + &attractors, + &matrix, + None, + 512, + 256, + &TerritorialStatus::FrontierUnclaimed, + SeedChain::root(42), + None, + ); + assert_eq!(placements.len(), 1); + assert_eq!(placements[0].position, (0, 0)); + } } diff --git a/server/src/atlas/cascade.rs b/server/src/atlas/cascade.rs index bc54553bc..6b7372556 100644 --- a/server/src/atlas/cascade.rs +++ b/server/src/atlas/cascade.rs @@ -196,6 +196,8 @@ impl CascadeSnapshot { /// /// `territorial_status` (D-212, from the body's `dominant_faction`) and `seed` /// drive the per-settlement spatial-character enrichment (#956, D-213/214/215). +/// `terrain` (T-1206) gates the Phase-4 synthetic-overflow path against open +/// water — see [`match_cities`]'s doc. fn run_layer3( attractors: &[GeographicAttractor], cities: &[CityRecord], @@ -203,6 +205,7 @@ fn run_layer3( seed: SeedChain, grid_w: u32, grid_h: u32, + terrain: Option<&TerrainAnalysis>, ) -> Layer3Output { let matrix = CompatibilityMatrix::d195(); let placements = match_cities( @@ -214,6 +217,7 @@ fn run_layer3( grid_h, territorial_status, seed, + terrain, ); Layer3Output { placements } } @@ -321,8 +325,15 @@ pub fn run_cascade_from_heightmap( None => &[], }; // cache seam: run_layer3 is a pure, deterministic function of - // (attractors, cities, territorial_status, seed) — wrap a persistent - // cache here when we add one (build-time bake or local cache; see #1021). + // (attractors, cities, territorial_status, seed, terrain) — wrap a + // persistent cache here when we add one (build-time bake or local + // cache; see #1021). + // + // T-1206: `snapshot.terrain_analysis` was just populated by the + // Topography block above (guaranteed `Some` here — Settlement > + // Topography in CascadeLayer's Ord, so the guard above always ran + // first) — passed by reference so the DistrictProfile/RoadGraph pass + // below still gets to consume (and drop) the same transient value. let l3 = run_layer3( attractors, cities, @@ -330,6 +341,7 @@ pub fn run_cascade_from_heightmap( body_seed, snapshot.heightmap.width, snapshot.heightmap.height, + snapshot.terrain_analysis.as_ref(), ); snapshot.layer3 = Some(l3); } diff --git a/server/src/atlas/road_graph.rs b/server/src/atlas/road_graph.rs index 7d742f3f7..ffe1e2bd0 100644 --- a/server/src/atlas/road_graph.rs +++ b/server/src/atlas/road_graph.rs @@ -95,18 +95,27 @@ const MIN_CELL_COST: u32 = RIVER_COST; /// edge touching it (the documented T-1116 bug: GJ251c land 0.55 / GJ380c land /// 0.588 — land-majority BODIES with 0 routable edges). /// -/// **Known gap this relaxation also happens to cover, but does not fix at the -/// source:** D-211's Phase-4 **synthetic overflow** path -/// (`attractor_matching.rs::synthetic_attractor`, used when a body has more -/// cities than real attractors) picks a position by pure grid arithmetic -/// (`grid_h/2, grid_w/4` then a fixed spacing-walk) with **no terrain check at -/// all** — it takes no heightmap/`TerrainAnalysis` argument and never reads -/// `ocean_mask`. A synthetic-overflow settlement CAN land in open ocean. This -/// routing relaxation still degrades that case gracefully (anchors to the -/// nearest passable cell within `COASTAL_ANCHOR_MAX_RING`, or leaves it -/// unrouted beyond that), but the placement itself is not guaranteed land — -/// that gap belongs to D-211/Layer 3, not this file, and is reported rather -/// than silently patched over here (flagged in PR #215 review, not yet ticketed). +/// **Gap this relaxation also happened to cover, now CLOSED at the source +/// (T-1206, 2026-07-26):** D-211's Phase-4 **synthetic overflow** path +/// (`attractor_matching.rs::synthetic_attractor`) used to pick a position by +/// pure grid arithmetic (`grid_h/2, grid_w/4` then a fixed spacing-walk) with +/// **no terrain check at all** — a synthetic-overflow settlement could land +/// in open ocean (confirmed on 46 real bodies at world seed 42/"yolo" before +/// the fix). `synthetic_attractor` now takes an `Option<&TerrainAnalysis>` +/// and, when supplied, guards the arithmetic position: already-land stays +/// byte-identical (verified unmoved on all 63 real land-arithmetic +/// placements), water gets nudged to the nearest land cell via a +/// deterministic ring-walk (`nearest_land_cell`, the same tie-break pattern +/// as this file's own `nearest_passable_cell` below, at native/working-grid +/// resolution rather than the routing grid's downsample), and a city with no +/// land within the bounded search radius is SKIPPED entirely (never +/// fabricated, never a panic — reported via the existing Phase-5 +/// name-fulfillment warning). This routing relaxation still degrades +/// gracefully for the coastal-cell/downsample case it was built for (anchors +/// to the nearest passable cell within `COASTAL_ANCHOR_MAX_RING`, or leaves +/// it unrouted beyond that) — the two fixes are independent and both apply +/// (T-1206 guarantees the placement pixel is land; this relaxation still +/// covers the routing CELL being water-majority at downsample granularity). /// /// The fix anchors routing to the NEAREST passable routing cell (ring-expansion /// search, deterministic tie-break) rather than the settlement's own impassable From 4b75be5975a604b9d2abf42a12c82316934661e5 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Thu, 6 Aug 2026 17:43:31 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(simulation):=20PR=20#218=20review=20rou?= =?UTF-8?q?nd=20=E2=80=94=20real-body=20evidence,=20D-211,=20derived=20bou?= =?UTF-8?q?nd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from Hoshe (QA) and Tyre (architecture), plus a bug the first of them uncovered. HOSHE — the headline evidence lived only in a deleted scratch scan. All nine tests used synthetic fixtures, so nothing committed held the "46 of 109 synthetic placements in water" claim on real data. Two tests now do. The T-1206 verification scan itself is committed as an #[ignore]d full-corpus test (267 bodies, run with --ignored), which also makes the recalibration instruction on the search bound executable rather than aspirational; a fast test pins the bodies it identifies. That scan promptly caught a bug in its own first draft, and it is the reason this commit is worth reading. `CascadeSnapshot::terrain_analysis` is transient — the cascade nulls it the moment DistrictProfile and RoadGraph are done (D-203/T-1048, ~2 MB a body) — so it is ALWAYS None on a returned snapshot, whatever cascade_snapshot_for_body's doc-comment implies. Reading the ocean mask off the snapshot and skipping when absent therefore skipped every body while reporting success: 267 bodies "scanned", 0 findings, a green assert over an empty set, in 697 seconds. Terrain is now re-derived through the same run_layer1_with_moisture call the cascade used, reproducing the grid the placements were computed against. Two habits caught it, both prompted by Hoshe's finding: a vacuity guard that refuses to pass when no synthetic placement was seen, and counters that stop "none found" and "never got that far" from looking identical. Corrected figures at seed 42: 267 bodies, all reaching Layer 3, 344 placements, 109 synthetic, 0 in water — the synthetic count matching the original scan, so the claim is reproducible now rather than anecdotal. TYRE 1 — MAX_LAND_SEARCH_RING was justified as grid_h/2 but written as a literal 128, leaving the 512x256 coupling implicit. It is now derived from the grid in scope, so the value cannot drift from its own rationale. On the current working grid it evaluates to exactly 128: no behaviour change, and the byte-identical-placement guarantee is untouched. Recalibration owner recorded. That derivation does change one test. nearest_land_cell_clamps_rows_no_wrap uses a 16x16 fixture, so its bound drops 128 -> 8, which now sits BETWEEN the clamped distance to the far pole (15) and the wrapped one (1). The assertion moves from position to absence and gets sharper for it: previously both implementations returned Some((15,0)) and only the position could be pinned; now any Some at all proves rows wrapped. TYRE 2 — D-211 carried no note though its behaviour changed. Dated amendment added: step 4's outcome set is no longer total (synthetic overflow may now resolve to a defined SKIP), and step 5's warning fires for a new legitimate reason. No re-decision needed — position remains a pure function of seed and terrain — and the dead-end cross-reference to D-210's closure is now a live anchor. Full cargo test green (30 binaries). Co-Authored-By: Claude Opus 5 (1M context) --- governance/decisions/architecture.md | 5 +- server/src/atlas/attractor_matching.rs | 315 ++++++++++++++++++++++--- 2 files changed, 287 insertions(+), 33 deletions(-) diff --git a/governance/decisions/architecture.md b/governance/decisions/architecture.md index 67d4942f1..5d9de3f8c 100644 --- a/governance/decisions/architecture.md +++ b/governance/decisions/architecture.md @@ -1118,9 +1118,10 @@ Technical foundation decisions that constrain implementation: engine, client-ser - **Two-tier mismatch flagging:** If a matched city-attractor pair has score < 0.35, log a `WARNING` (below expected quality). If score < 0.15, log an `ERROR` and flag for manual review. Generation proceeds in both cases; the flags are for content auditing, not hard blockers. - **Output:** `Vec` written to `atlas_city_positions` at build time. - **Rationale:** Greedy-first for large/locked cities ensures anchor cities (capitals, corp HQs, wiki-named cities) are placed at terrain features that match their lore role. Hungarian for medium cities finds the globally optimal assignment, not just locally optimal. Synthetic overflow prevents the algorithm from failing on bodies where city count exceeds natural attractor count (dense, flat worlds). The two-tier warning system enables content QA without blocking generation. -- **Ticket:** T-919, T-925 +- **Amendment (T-1206, 2026-08-06 — step 4 may now legitimately place nothing):** phase 4's synthetic position was derived by pure grid arithmetic with **no terrain input at all**, so it could and did land in open water — a scan of every heightmap body at world seed 42 found **46 of 109 synthetic placements sitting in ocean** (e.g. GJ903c at a genuine polar ocean cell). Synthetic overflow now takes the terrain analysis and land-corrects via a bounded nearest-land ring walk; land positions pass through untouched, so **no existing land placement moves** and this record's seed-derived-position promise is intact (position remains a pure function of seed + terrain — this fulfils the placement intent rather than deviating from it, so no re-decision is required). What **does** change is the record's outcome set: step 4's "cities that cannot be matched receive a synthetic `PlainCenter`" is no longer total — where no land exists within the search bound, the synthetic attractor is **skipped**, defined and deliberate, never a fabricated water position and never a panic. Step 5's name-fulfillment warning consequently fires for a new legitimate reason (a genuinely water-locked body), not only for a pipeline failure. +- **Ticket:** T-919, T-925, T-1206 (ocean-mask guard amendment) - **Raised by:** Generation cascade workshop (T-897) -- **Cross-reference:** D-195 (CompatibilityMatrix), D-207 (atlas_city_names), D-209 (GeographicAttractor — input), D-210 (terrain_modification_cost — input) +- **Cross-reference:** D-195 (CompatibilityMatrix), D-207 (atlas_city_names), D-209 (GeographicAttractor — input), [D-210](#d-210) (terrain_modification_cost — input; its 2026-07-26 amendment records the *matched-attractor* half of this same placement-in-water gap, closed and validated under T-1116/T-1206) ### D-212: TerritorialStatus Priority-Ordered Derivation Algorithm - **Date:** 2026-05-01 diff --git a/server/src/atlas/attractor_matching.rs b/server/src/atlas/attractor_matching.rs index 571d13fc4..e5508389a 100644 --- a/server/src/atlas/attractor_matching.rs +++ b/server/src/atlas/attractor_matching.rs @@ -279,15 +279,32 @@ const MIN_SPACING: u16 = 15; /// landing in a genuine HIGH-LATITUDE OCEAN BAND near the grid's pole row — /// not a small coastal-cell artifact. Real, substantially-land bodies (35–95% /// land overall) had their nearest land cell to `(0,0)` as far as 125 cells -/// away (`grid_h` = 256, so `grid_h / 2` = 128 is the natural ceiling — a -/// point beyond that is more than half the grid's height from the pole and a -/// wider search stops paying for itself). 128 comfortably covers every -/// observed real-body case while still being a bounded, cheap search (worst -/// case ~128² ≈ 16k candidate cells, negligible next to the cascade's other -/// per-body costs) — genuinely water-locked bodies (no land within half the +/// away. Half the grid's height is the natural ceiling: a point beyond that +/// is more than half the grid from the pole, and a wider search stops paying +/// for itself. It is also a bounded, cheap search — worst case ~(h/2)² ≈ 16k +/// candidate cells on the current grid, negligible beside the cascade's other +/// per-body costs. Genuinely water-locked bodies (no land within half the /// grid height of the arithmetic position) still correctly degrade to the -/// skip path below. -const MAX_LAND_SEARCH_RING: u16 = 128; +/// skip path in the caller. +/// +/// **Derived, not hardcoded** (PR #218 review, Tyre). The bound is `h / 2` +/// and it is computed from the grid actually in scope, because the whole +/// justification above is expressed in grid heights: a literal `128` silently +/// carried the 512×256 working-grid assumption, so a change in grid +/// resolution would have left the constant behind while its own doc still +/// claimed to be half the height. On the current 512×256 working grid this +/// evaluates to exactly the 128 it replaces — no behaviour change, and the +/// PR's byte-identical-placement guarantee is untouched. +/// +/// **Recalibration owner:** whoever changes the body corpus or the working- +/// grid resolution. Re-run the T-1206 verification scan (every heightmap +/// body at world seed 42, recording each synthetic placement's distance to +/// nearest land) and confirm the observed maximum still sits comfortably +/// under `h / 2`. If it does not, the ceiling — not just the literal — is +/// what needs revisiting. +fn max_land_search_ring(ta: &TerrainAnalysis) -> u16 { + (ta.h / 2) as u16 +} /// Ring-expansion search from `(row, col)` to the nearest cell with /// `!ocean_mask` (T-1206, the T-1116 `nearest_passable_cell` pattern at @@ -301,14 +318,14 @@ const MAX_LAND_SEARCH_RING: u16 = 128; /// seed-derived-position promise intact). Columns wrap (equirectangular /// globe, matching every other grid walk in this cascade); rows clamp /// (poles). Returns `None` if no land cell exists within -/// [`MAX_LAND_SEARCH_RING`] — the caller's defined degradation is to skip +/// [`max_land_search_ring`] — the caller's defined degradation is to skip /// the synthetic attractor entirely (T-1206), never to fabricate a position. fn nearest_land_cell(ta: &TerrainAnalysis, row: u16, col: u16) -> Option<(u16, u16)> { let (w, h) = (ta.w as i32, ta.h as i32); if !ta.is_ocean(row as usize, col as usize) { return Some((row, col)); } - for ring in 1..=MAX_LAND_SEARCH_RING { + for ring in 1..=max_land_search_ring(ta) { let ring_i = ring as i32; let (r0, c0) = (row as i32, col as i32); let mut candidates: Vec<(i32, i32)> = Vec::new(); @@ -359,7 +376,7 @@ fn nearest_land_cell(ta: &TerrainAnalysis, row: u16, col: u16) -> Option<(u16, u /// no currently-land synthetic-overflow placement moves on any existing body /// (D-211's seed-derived-position promise). Only a water-arithmetic position /// is nudged, via [`nearest_land_cell`]'s deterministic ring-walk. If no land -/// cell exists within [`MAX_LAND_SEARCH_RING`] (an all-ocean region of the +/// cell exists within [`max_land_search_ring`] (an all-ocean region of the /// grid), returns `None` — the caller's defined degradation is to skip this /// synthetic attractor entirely (the city goes unplaced and is reported by /// Phase 5's existing name-fulfillment warning) rather than fabricate a @@ -612,7 +629,7 @@ pub fn match_cities( if placed_ids.contains(&city.city_id) { continue; } - // T-1206: `None` here (all-ocean region beyond MAX_LAND_SEARCH_RING, + // T-1206: `None` here (all-ocean region beyond max_land_search_ring, // or degenerate MIN_SPACING exhaustion) means the defined degradation // is to skip this city's synthetic placement entirely — never // fabricate a water position. Phase 5's name-fulfillment check below @@ -1337,7 +1354,7 @@ mod tests { let ta = ta_with_land(grid_w as usize, grid_h as usize, &[(2, 0)]); let guarded = synthetic_attractor(&placed, grid_w, grid_h, Some(&ta)) - .expect("land exists within MAX_LAND_SEARCH_RING"); + .expect("land exists within max_land_search_ring"); assert_eq!( guarded.position, (2, 0), @@ -1396,29 +1413,34 @@ mod tests { #[test] fn nearest_land_cell_clamps_rows_no_wrap() { let (w, h) = (16usize, 16usize); - // Land only at the FAR pole (row h-1) — from row 0, a row-wrapping - // implementation would find it at ring (h-1); a row-clamping one - // must exhaust MAX_LAND_SEARCH_RING first if h-1 > that bound, or - // find it only via the correct non-wrapped ring distance. Here - // h=16 keeps h-1=15 comfortably inside MAX_LAND_SEARCH_RING (128), - // so the assertion is on the POSITION found, not on absence: a - // wrapping bug would still find (15, c) — same as clamping would, - // since row 15 IS within the grid — so instead assert a cell just - // below row 0 wrapping to the top is never sourced from "negative - // row mod h" by using an asymmetric single-land-cell placement at a - // row that would be reached MUCH sooner via wraparound than via the - // real clamped ring distance. + // Land ONLY at the far pole (row h-1), searching from row 0. + // + // The two implementations disagree by an order of magnitude here. + // Clamped, the only route is straight down the column: ring 15. + // Wrapping, row 0 steps "up" to row 15 immediately: ring 1. + // + // Since the search bound became h/2 (PR #218 review — see + // `max_land_search_ring`), this grid bounds the walk at ring 8, which + // sits BETWEEN those two distances. That turns a position assertion + // into an absence assertion, and a sharper one: ring 15 is now out of + // reach by construction, so a correct clamping walk must give up and + // return None, while ANY `Some` result proves rows wrapped — there is + // no other way to reach row 15 within 8 rings. + // + // (Before the bound was derived it was a flat 128, far beyond this + // 16-row fixture, so both implementations returned Some((15, 0)) and + // the test could only pin the position. It now discriminates.) let ta = ta_with_land(w, h, &[(15, 0)]); let found = nearest_land_cell(&ta, 0, 0); - // Real (clamped) distance is ring 15 (straight down the column). - // A wrapping implementation could equally reach it at ring 1 (one - // step "up" from row 0 wrapping to row 15) — assert the ring-15 - // (non-wrapped) result to pin clamping behavior. - assert_eq!(found, Some((15, 0))); + assert_eq!( + found, None, + "rows must clamp: the far pole is 15 rings away and the bound is \ + h/2 = 8, so any Some(..) here means the walk wrapped rows" + ); } /// **Degradation (T-1206, requirement 2): no land within - /// `MAX_LAND_SEARCH_RING` → skip, never fabricate or panic.** A tiny + /// `max_land_search_ring` → skip, never fabricate or panic.** A tiny /// all-ocean grid guarantees no land cell exists anywhere, so the guard /// must return `None` — `match_cities`'s Phase 4 then skips this city /// entirely (verified below via `match_cities` directly, matching Phase @@ -1553,4 +1575,235 @@ mod tests { assert_eq!(placements.len(), 1); assert_eq!(placements[0].position, (0, 0)); } + + /// Run the real cascade for `body` and hand back its placements together + /// with the `TerrainAnalysis` they must be checked against. + /// + /// The re-derivation is not incidental — it is the whole reason this + /// helper exists. `CascadeSnapshot::terrain_analysis` is transient: the + /// cascade sets it to `None` the moment DistrictProfile and RoadGraph are + /// done with it (D-203/T-1048, ~2 MB a body), so by the time a completed + /// snapshot is handed back it is ALWAYS `None`, whatever + /// `cascade_snapshot_for_body`'s doc-comment implies. A caller that reads + /// it off the snapshot and skips when absent skips every body, silently, + /// and reports a clean run — which is exactly what the first draft of the + /// scan below did across all 267 bodies. + /// + /// So re-derive it from the surviving heightmap through the SAME call the + /// cascade itself used (`run_layer1_with_moisture` with the body's own + /// moisture ceiling), which reproduces the identical grid the placements + /// were computed against. + #[cfg(test)] + fn cascade_placements_and_terrain( + body: &str, + seed: u64, + ) -> Option<(Vec, TerrainAnalysis)> { + let (snapshot, params) = + crate::atlas::believability::cascade_snapshot_for_body(seed, body).ok()?; + let placements = snapshot.layer3.as_ref()?.placements.clone(); + let (_l1, ta) = crate::atlas::layer1::run_layer1_with_moisture( + &snapshot.heightmap, + crate::atlas::district_profile::derive_moisture_ceiling_q(¶ms), + ); + Some((placements, ta)) + } + + /// Directory of bodies carrying a committed heightmap, read off the wiki + /// tree rather than the DB (the tree IS the heightmap corpus, and this + /// avoids a second source of truth). Tries repo-root and `server/`-relative + /// paths, matching `believability::find_heightmap`'s own CWD tolerance. + #[cfg(test)] + fn bodies_with_heightmaps() -> Vec { + let root = ["wiki/star-systems", "../wiki/star-systems"] + .iter() + .map(std::path::PathBuf::from) + .find(|p| p.is_dir()); + let Some(root) = root else { + return Vec::new(); + }; + let mut out = Vec::new(); + let Ok(systems) = std::fs::read_dir(&root) else { + return out; + }; + for system in systems.flatten() { + let bodies_dir = system.path().join("bodies"); + let Ok(bodies) = std::fs::read_dir(&bodies_dir) else { + continue; + }; + for body in bodies.flatten() { + if body.path().join("heightmap.png").is_file() { + if let Some(name) = body.file_name().to_str() { + out.push(name.to_string()); + } + } + } + } + out.sort(); + out + } + + /// **The T-1206 verification scan, committed (PR #218 review, Hoshe).** + /// + /// The scan that motivated this fix — every heightmap body at world seed + /// 42, counting synthetic placements that land in water — was run once and + /// thrown away, which is precisely why its headline number (46 of 109 in + /// water) had nothing holding it. This is that scan, re-runnable: + /// + /// ```text + /// cargo test --lib t1206_verification_scan -- --ignored --nocapture + /// ``` + /// + /// `#[ignore]` because it derives the full cascade for ~267 bodies — far + /// too slow for the default suite. The fast invariant lives in + /// `real_body_synthetic_placements_never_land_in_ocean` below, which pins + /// the bodies this scan identifies. + /// + /// Re-run it when the body corpus or the working-grid resolution changes: + /// it reports both the ocean-guard invariant AND the distance-to-land + /// distribution that sizes `max_land_search_ring`. + #[test] + #[ignore = "full-corpus scan (~267 bodies) — run explicitly"] + fn t1206_verification_scan() { + let bodies = bodies_with_heightmaps(); + assert!(!bodies.is_empty(), "no heightmap bodies found"); + + let (mut loaded, mut synthetic, mut in_water) = (0usize, 0usize, 0usize); + // Counted separately so a zero cannot be ambiguous: "no synthetic + // placements" and "Layer 3 never ran" would otherwise look identical, + // and the difference is the whole meaning of the result. + let (mut with_layer3, mut total_placements) = (0usize, 0usize); + let mut offenders: Vec = Vec::new(); + let mut with_synthetic: Vec = Vec::new(); + + for body in &bodies { + let Some((placements, ta)) = cascade_placements_and_terrain(body, 42) else { + continue; + }; + loaded += 1; + with_layer3 += 1; + total_placements += placements.len(); + let mut body_synthetic = 0usize; + for p in placements.iter().filter(|p| p.synthetic) { + synthetic += 1; + body_synthetic += 1; + let (row, col) = p.position; + if ta.is_ocean(row as usize, col as usize) { + in_water += 1; + offenders.push(format!("{body} city {} @ {:?}", p.city_id, p.position)); + } + } + if body_synthetic > 0 { + with_synthetic.push(format!("{body}({body_synthetic})")); + } + } + + println!( + "T-1206 scan @ seed 42: {loaded} bodies loaded, {with_layer3} reached Layer 3, \ + {total_placements} placements total, {synthetic} synthetic, {in_water} in water" + ); + println!( + "bodies with synthetic placements: {}", + with_synthetic.join(" ") + ); + for o in &offenders { + println!(" IN WATER: {o}"); + } + assert_eq!( + in_water, 0, + "the ocean-mask guard has regressed on real data: {offenders:?}" + ); + } + + /// **The claim this PR was built on, pinned to real data (PR #218 review, + /// Hoshe).** + /// + /// Every other test above proves the guard on synthetic fixtures. But the + /// evidence that motivated T-1206 was a scan of production data — 46 of + /// 109 synthetic placements across the 267 real bodies sat in open water + /// at world seed 42 — and that scan was a throwaway. Nothing committed + /// held the line on the bodies that actually exhibited the bug, so a + /// regression would have been caught only by fixtures, never by the data + /// that revealed it. + /// + /// This walks the real cascade over real bodies (the T-1116 precedent in + /// `road_graph.rs::real_body_coastal_settlements_produce_routable_edges` + /// — committed `systems.db` + `wiki/star-systems` heightmaps, entered + /// through the same loader production uses) and asserts the invariant + /// directly: NO synthetic placement may sit on an ocean cell. + /// + /// `cascade_snapshot_for_body`, not `cascade_for_body`, because the + /// `TerrainAnalysis` carrying `ocean_mask` is transient and is dropped by + /// `into_body_world_state()` (D-203/T-1048 size budget) — the mask has to + /// be read off the snapshot before conversion. + /// + /// Guarded against passing vacuously: the run must produce at least one + /// synthetic placement somewhere in the corpus, otherwise the assertion + /// below is over an empty set and proves nothing. If the body list ever + /// stops exercising overflow, that guard fails loudly rather than going + /// quietly green. It has already earned that — it caught both a body list + /// that exercised nothing and, worse, a terrain-source bug that made the + /// whole-corpus scan report a clean 267 bodies while checking none of them + /// (see `cascade_placements_and_terrain`). + /// + /// Full-corpus figures at seed 42, from `t1206_verification_scan`: + /// **267 bodies, all reaching Layer 3, 344 placements, 109 of them + /// synthetic, 0 in water.** The synthetic count matches the number the + /// original T-1206 scan reported, so the fix's headline evidence is now + /// reproducible rather than anecdotal. + #[test] + fn real_body_synthetic_placements_never_land_in_ocean() { + // Every one of these DOES reach Phase-4 synthetic overflow at seed 42 + // — taken from `t1206_verification_scan`'s own output, not guessed. + // That matters: the first draft of this list paired GJ903c with four + // headline bodies (GJ251c/GJ380c/GJ820Bc/GJ338Bd) that turn out to + // have no synthetic placements at all, so the assertion rested on a + // single body. GJ903c is the case the T-1206 scan named (a genuine + // polar ocean cell); GJ3737e-m2 is the only body carrying two. + let bodies = [ + "GJ903c", + "GJ3737e-m2", + "GJ1245Bb", + "GJ667Ce", + "GJ581c", + "GJ892f", + ]; + let mut synthetic_seen = 0usize; + let mut bodies_loaded = 0usize; + let mut total_placements = 0usize; + + for body in bodies { + // A body may legitimately be absent from this checkout's data; + // skip rather than fail, matching the believability harness's own + // contract for the `*_for_body` loaders. + let Some((placements, ta)) = cascade_placements_and_terrain(body, 42) else { + continue; + }; + bodies_loaded += 1; + total_placements += placements.len(); + + for p in placements.iter().filter(|p| p.synthetic) { + synthetic_seen += 1; + let (row, col) = p.position; + assert!( + !ta.is_ocean(row as usize, col as usize), + "{body}: synthetic placement for city {} sits in open water at \ + {:?} — the T-1206 ocean-mask guard has regressed", + p.city_id, + p.position + ); + } + } + + assert!( + bodies_loaded > 0, + "no test body could be loaded — systems.db or the heightmaps are \ + missing, so this test verified nothing" + ); + assert!( + synthetic_seen > 0, + "{bodies_loaded} bodies loaded, {total_placements} placements among them, \ + but not one synthetic — this test would pass vacuously; pick bodies that \ + still reach Phase-4 overflow" + ); + } } From fc55bd897f9496b4a1bec5c2b1c29ed268b2f74e Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Thu, 6 Aug 2026 19:06:57 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(simulation):=20PR=20#218=20round=202=20?= =?UTF-8?q?=E2=80=94=20the=20guard=20was=20spending=20D-211's=20spacing=20?= =?UTF-8?q?promise?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoshe and Tyre independently found the same defect, neither having seen the other's review. They were right. THE SPACING REGRESSION. synthetic_attractor's walk picks a candidate that satisfies MIN_SPACING against every already-placed city, and the ocean correction then moves that candidate up to h/2 = 128 cells with no re-validation. D-211 step 4 promises the synthetic attractor is placed "at a position that respects minimum city spacing" — the fix was buying the land half of that promise with the spacing half. road_graph::collapse_colocated is no safety net either: it dedupes by name, not position. The spacing predicate now goes INTO the ring walk (nearest_cell_matching) rather than running before it, so the search returns the nearest cell satisfying land AND spacing, with the same tie-break order and the same degrade-to-skip. A predicate on the existing walk, not a second walk. Unmeasured, and stated rather than implied: whether this was ever a LIVE violation or only a latent one. The old behaviour was replaced before it was measured. What the corpus does say is that 12 of the 13 bodies showing any sub-MIN_SPACING pair carry no synthetic placement at all. AND THE FIRST VERSION OF THAT FIX OVER-ASSERTED. Checking MIN_SPACING across ALL placement pairs found 13 violations corpus-wide, none of them the guard's doing: the promise is step 4's alone, and matched placements (Tier A greedy, Tier B/C Hungarian) sit on their terrain attractor, never subject to it. Two real river mouths 12 cells apart is geography. Shipping that assertion would have failed the gate and blamed this guard for pre-existing placements. Both checks are now scoped to pairs involving a synthetic placement, with the 13 matched-pair proximities recorded in-code so they are not re-litigated. Corpus, both invariants: 267 bodies, 267 reaching Layer 3, 344 placements, 109 synthetic, 0 in water, 0 spacing violations. ALSO FIXED: - t1206_verification_scan could still pass vacuously (Hoshe). The fast test got bodies_loaded>0 / synthetic_seen>0 guards last round; the scan — the one test whose entire purpose is being the re-runnable evidence — did not, and !bodies.is_empty() only proves the directory listing worked. Both added. - cascade_snapshot_for_body's doc-comment claimed the snapshot "still carries the transient TerrainAnalysis" (Tyre). It is always None for a full-cascade call. Corrected in place, with the re-derivation recipe and a note that this sentence cost a false-clean 267-body scan. - The passthrough test's comment described a land-island fixture and claimed (0,0) is not returned; the fixture is ta_all_land and the test asserts (0,0) IS returned (both reviewers). Rewritten to match reality. - max_land_search_ring's cost note said ~(h/2)^2 = 16k candidates (Hoshe). That is one quadrant's area, not cumulative ring cost: sum of 8r over 1..=128 is 66,048. Conclusion unchanged; the arithmetic is the executable recalibration rationale, so it has to be right. - The fast test ran 25.8s, close to the long pole of the whole --lib suite (Hoshe). Trimmed 6 bodies to 3: 13.0s. Not to one — synthetic_seen>0 needs only one body, but resting on one is how the first draft ended up depending on GJ903c alone. RETRACTED: the shared-ring-search-helper finding. Round 1 ruled duplicate- over-share CORRECT for this walk and said so "stated so it isn't re-litigated"; round 2 asks to factor it with no new evidence and no change to either implementation. Fixing the spacing gap by predicate rather than by a second walk moves that direction anyway. Full cargo test green (40 binaries). Co-Authored-By: Claude Opus 5 (1M context) --- server/src/atlas/attractor_matching.rs | 212 ++++++++++++++++++++++--- server/src/atlas/believability.rs | 30 +++- 2 files changed, 208 insertions(+), 34 deletions(-) diff --git a/server/src/atlas/attractor_matching.rs b/server/src/atlas/attractor_matching.rs index e5508389a..06f22e95c 100644 --- a/server/src/atlas/attractor_matching.rs +++ b/server/src/atlas/attractor_matching.rs @@ -281,11 +281,15 @@ const MIN_SPACING: u16 = 15; /// land overall) had their nearest land cell to `(0,0)` as far as 125 cells /// away. Half the grid's height is the natural ceiling: a point beyond that /// is more than half the grid from the pole, and a wider search stops paying -/// for itself. It is also a bounded, cheap search — worst case ~(h/2)² ≈ 16k -/// candidate cells on the current grid, negligible beside the cascade's other -/// per-body costs. Genuinely water-locked bodies (no land within half the -/// grid height of the arithmetic position) still correctly degrade to the -/// skip path in the caller. +/// for itself. It is also a bounded, cheap search: each ring contributes about +/// `8 · ring` candidates, so exhausting the bound costs `Σ 8r` for r in +/// `1..=h/2` ≈ **66k candidate cells** on the current grid — negligible beside +/// the cascade's other per-body costs, and reached only by a body with no land +/// at all within half a grid of the arithmetic position. (An earlier version of +/// this paragraph said `~(h/2)² ≈ 16k`, which is the area of one quadrant +/// rather than the cumulative ring cost — off by ~4×, though the "negligible" +/// conclusion is unchanged. PR #218 review, Hoshe.) Genuinely water-locked +/// bodies still correctly degrade to the skip path in the caller. /// /// **Derived, not hardcoded** (PR #218 review, Tyre). The bound is `h / 2` /// and it is computed from the grid actually in scope, because the whole @@ -320,9 +324,41 @@ fn max_land_search_ring(ta: &TerrainAnalysis) -> u16 { /// (poles). Returns `None` if no land cell exists within /// [`max_land_search_ring`] — the caller's defined degradation is to skip /// the synthetic attractor entirely (T-1206), never to fabricate a position. +/// Test-only since the spacing predicate landed (PR #218 round 2): production +/// always has a spacing constraint to apply, so it calls +/// [`nearest_cell_matching`] directly and nothing outside `cfg(test)` wants the +/// unconstrained form. Kept because the land-only behaviour is a contract worth +/// pinning on its own — the wrap/clamp/tie-break tests read far more clearly +/// against it than against a call with a `|_, _| true` predicate bolted on. +#[cfg(test)] fn nearest_land_cell(ta: &TerrainAnalysis, row: u16, col: u16) -> Option<(u16, u16)> { + nearest_cell_matching(ta, row, col, |_, _| true) +} + +/// Ring-expansion search for the nearest cell that is land AND satisfies a +/// caller-supplied acceptance test. The production entry point; `nearest_land_cell` +/// (test-only) is this with a trivial predicate. +/// +/// Exists because land is not the only thing D-211 promises about a synthetic +/// position. Step 4 says the overflow attractor is placed "at a position that +/// respects minimum city spacing", and the spacing walk in +/// [`synthetic_attractor`] runs BEFORE this correction — so nudging an accepted +/// candidate up to `h/2` cells to reach land could quietly land it on top of a +/// city the walk had just cleared it against, trading one guarantee for the +/// other (PR #218 review, Tyre). Threading the caller's own predicate into the +/// same ring walk keeps both promises without a second copy of this geometry. +/// +/// `accept` is consulted only for cells that are already land, and the walk +/// continues outward when it returns false — so the result is the nearest cell +/// satisfying BOTH conditions, with the identical tie-break order. +fn nearest_cell_matching( + ta: &TerrainAnalysis, + row: u16, + col: u16, + accept: impl Fn(u16, u16) -> bool, +) -> Option<(u16, u16)> { let (w, h) = (ta.w as i32, ta.h as i32); - if !ta.is_ocean(row as usize, col as usize) { + if !ta.is_ocean(row as usize, col as usize) && accept(row, col) { return Some((row, col)); } for ring in 1..=max_land_search_ring(ta) { @@ -354,7 +390,7 @@ fn nearest_land_cell(ta: &TerrainAnalysis, row: u16, col: u16) -> Option<(u16, u } for (nr, nc) in candidates { let wrapped_c = nc.rem_euclid(w) as u16; - if !ta.is_ocean(nr as usize, wrapped_c as usize) { + if !ta.is_ocean(nr as usize, wrapped_c as usize) && accept(nr as u16, wrapped_c) { return Some((nr as u16, wrapped_c)); } } @@ -375,7 +411,7 @@ fn nearest_land_cell(ta: &TerrainAnalysis, row: u16, col: u16) -> Option<(u16, u /// UNTOUCHED — this is the compatibility invariant the fix is built around: /// no currently-land synthetic-overflow placement moves on any existing body /// (D-211's seed-derived-position promise). Only a water-arithmetic position -/// is nudged, via [`nearest_land_cell`]'s deterministic ring-walk. If no land +/// is nudged, via [`nearest_cell_matching`]'s deterministic ring-walk. If no land /// cell exists within [`max_land_search_ring`] (an all-ocean region of the /// grid), returns `None` — the caller's defined degradation is to skip this /// synthetic attractor entirely (the city goes unplaced and is reported by @@ -410,7 +446,25 @@ fn synthetic_attractor( } if let Some(ta) = terrain { - let (land_row, land_col) = nearest_land_cell(ta, row, col)?; + // Carry the spacing constraint INTO the land search rather than + // applying it only to the arithmetic candidate above. The walk can move + // the position by up to h/2 cells, which is far enough to cross a + // neighbour it was just cleared against — D-211 step 4 promises land + // AND minimum spacing, and correcting one must not silently spend the + // other (PR #218 review, Tyre). + // + // Degradation is unchanged: if no cell satisfies both within the bound, + // this is `None` and the caller skips the synthetic attractor, exactly + // as it already did for the land-only case. A skipped city is reported + // by Phase 5's existing name-fulfillment warning; it is never placed in + // water, and now never placed on top of a neighbour either. + let (land_row, land_col) = nearest_cell_matching(ta, row, col, |r, c| { + placed.iter().all(|p| { + let dr = (p.position.0 as i32 - r as i32).unsigned_abs() as u16; + let dc = (p.position.1 as i32 - c as i32).unsigned_abs() as u16; + dr.max(dc) >= MIN_SPACING + }) + })?; row = land_row; col = land_col; } @@ -1318,12 +1372,16 @@ mod tests { fn synthetic_attractor_land_passthrough_is_byte_identical_to_no_terrain() { let placed: Vec = Vec::new(); let (grid_w, grid_h) = (512u32, 256u32); - // The arithmetic default before any spacing-walk iteration is - // (grid_h/2, grid_w/4) = (128, 128); make that cell (and enough of a - // margin around it) land so the very first candidate the spacing-walk - // tries — (0,0) — is NOT what gets returned, isolating the - // passthrough check to the "arithmetic position already lands on - // land" case rather than accidentally exercising the guard. + // Every cell is land, so the guard is a guaranteed no-op and the only + // thing under test is passthrough. + // + // The returned position is (0, 0), not the (grid_h/2, grid_w/4) + // arithmetic default: with `placed` empty the spacing-walk's `.all()` + // is vacuously true on its very first candidate (dr=0, dc=0), so it + // breaks there and the default is never reached. That is exactly the + // production case the T-1206 scan found — an empty overflow list + // putting the first synthetic city at the grid origin, which on a real + // body is frequently open water. let ta = ta_all_land(grid_w as usize, grid_h as usize); let without_terrain = synthetic_attractor(&placed, grid_w, grid_h, None) @@ -1335,9 +1393,6 @@ mod tests { without_terrain.position, with_terrain.position, "a land-arithmetic synthetic position must be untouched by the T-1206 guard" ); - // Sanity: this is genuinely the (0,0) first-candidate case the T-1206 - // scan found in production (empty `placed` list — the spacing check - // against zero existing placements passes trivially at (0,0)). assert_eq!(without_terrain.position, (0, 0)); } @@ -1668,6 +1723,8 @@ mod tests { assert!(!bodies.is_empty(), "no heightmap bodies found"); let (mut loaded, mut synthetic, mut in_water) = (0usize, 0usize, 0usize); + let mut too_close = 0usize; + let mut crowded: Vec = Vec::new(); // Counted separately so a zero cannot be ambiguous: "no synthetic // placements" and "Layer 3 never ran" would otherwise look identical, // and the difference is the whole meaning of the result. @@ -1692,6 +1749,42 @@ mod tests { offenders.push(format!("{body} city {} @ {:?}", p.city_id, p.position)); } } + // D-211 step 4's OTHER promise: minimum spacing. + // + // Scoped to pairs involving a SYNTHETIC placement, because that is + // the extent of the promise. Step 4 says the synthetic overflow + // attractor is generated "at a position that respects minimum city + // spacing"; matched placements (Tier A greedy, Tier B/C Hungarian) + // sit whereever their terrain attractor is and were never subject + // to it — two real river mouths 12 cells apart is legitimate + // geography, not a defect. An earlier draft of this check asserted + // across ALL pairs and duly found 13 such pairs corpus-wide, none + // of which the guard had anything to do with. + let synthetic_ids: std::collections::BTreeSet = placements + .iter() + .filter(|p| p.synthetic) + .map(|p| p.city_id) + .collect(); + for (i, a) in placements.iter().enumerate() { + for b in placements.iter().skip(i + 1) { + if !synthetic_ids.contains(&a.city_id) && !synthetic_ids.contains(&b.city_id) { + continue; + } + let dr = (a.position.0 as i32 - b.position.0 as i32).unsigned_abs() as u16; + let dc = (a.position.1 as i32 - b.position.1 as i32).unsigned_abs() as u16; + if dr.max(dc) < MIN_SPACING { + too_close += 1; + crowded.push(format!( + "{body} cities {}/{} @ {:?}/{:?} (Chebyshev {})", + a.city_id, + b.city_id, + a.position, + b.position, + dr.max(dc) + )); + } + } + } if body_synthetic > 0 { with_synthetic.push(format!("{body}({body_synthetic})")); } @@ -1705,13 +1798,40 @@ mod tests { "bodies with synthetic placements: {}", with_synthetic.join(" ") ); + println!("spacing violations (< {MIN_SPACING} cells Chebyshev): {too_close}"); for o in &offenders { println!(" IN WATER: {o}"); } + for c in &crowded { + println!(" TOO CLOSE: {c}"); + } + // Same vacuity guards as the fast test, and for the same reason: this + // is the one test whose whole point is being the re-runnable evidence, + // so it must not be the one that can go green over an empty set. Its + // first draft did exactly that across all 267 bodies (PR #218 review, + // Hoshe) — `!bodies.is_empty()` above only proves the directory listing + // worked, not that a single body loaded through the cascade. + assert!( + loaded > 0, + "{} bodies on disk but none loaded through the cascade — check CWD \ + and systems.db; this test verified nothing", + bodies.len() + ); + assert!( + synthetic > 0, + "{loaded} bodies loaded, {with_layer3} reached Layer 3, {total_placements} \ + placements — but zero synthetic, so both assertions below are over an \ + empty set and prove nothing" + ); assert_eq!( in_water, 0, "the ocean-mask guard has regressed on real data: {offenders:?}" ); + assert_eq!( + too_close, 0, + "D-211's minimum-spacing promise is violated on real data — the land \ + correction may be nudging placements onto their neighbours: {crowded:?}" + ); } /// **The claim this PR was built on, pinned to real data (PR #218 review, @@ -1759,14 +1879,18 @@ mod tests { // have no synthetic placements at all, so the assertion rested on a // single body. GJ903c is the case the T-1206 scan named (a genuine // polar ocean cell); GJ3737e-m2 is the only body carrying two. - let bodies = [ - "GJ903c", - "GJ3737e-m2", - "GJ1245Bb", - "GJ667Ce", - "GJ581c", - "GJ892f", - ]; + // Three, not six. The helper re-derives layer 1 per body (the only way + // to recover the dropped TerrainAnalysis), so each body costs ~4.2s — + // double the `real_body_coastal_settlements_produce_routable_edges` + // precedent, and at six bodies this became the long pole of the whole + // --lib suite's wall clock (PR #218 review, Hoshe). Trimmed to ~13s. + // + // Not trimmed to one, though: `synthetic_seen > 0` needs only a single + // body, but a single body is how the first draft of this test ended up + // resting on GJ903c alone. Three keeps the invariant checked across + // independent terrain while paying a third less. The 267-body sweep + // remains available behind `--ignored`. + let bodies = ["GJ903c", "GJ3737e-m2", "GJ1245Bb"]; let mut synthetic_seen = 0usize; let mut bodies_loaded = 0usize; let mut total_placements = 0usize; @@ -1792,6 +1916,42 @@ mod tests { p.position ); } + + // D-211 step 4 promises minimum spacing as well as land, and the + // land correction runs after the spacing walk — so a nudge toward + // shore could otherwise undo it (PR #218 review, Tyre + Hoshe, + // found independently by both). + // + // Only pairs involving a SYNTHETIC placement are asserted: the + // spacing promise is step 4's alone. Matched placements land on + // their terrain attractor and are not subject to it — corpus-wide + // there are 13 matched-pair proximities under 15 cells, all + // legitimate geography. + let synthetic_ids: std::collections::BTreeSet = placements + .iter() + .filter(|p| p.synthetic) + .map(|p| p.city_id) + .collect(); + for (i, a) in placements.iter().enumerate() { + for b in placements.iter().skip(i + 1) { + if !synthetic_ids.contains(&a.city_id) && !synthetic_ids.contains(&b.city_id) { + continue; + } + let dr = (a.position.0 as i32 - b.position.0 as i32).unsigned_abs() as u16; + let dc = (a.position.1 as i32 - b.position.1 as i32).unsigned_abs() as u16; + assert!( + dr.max(dc) >= MIN_SPACING, + "{body}: cities {} and {} are {} cells apart at {:?}/{:?}, \ + under the {MIN_SPACING}-cell minimum — the land correction \ + has spent D-211's spacing guarantee to buy its land one", + a.city_id, + b.city_id, + dr.max(dc), + a.position, + b.position + ); + } + } } assert!( diff --git a/server/src/atlas/believability.rs b/server/src/atlas/believability.rs index de470178a..d6da1e736 100644 --- a/server/src/atlas/believability.rs +++ b/server/src/atlas/believability.rs @@ -613,15 +613,29 @@ pub fn cascade_for_body(world_seed: u64, body_id: &str) -> Result Date: Sat, 8 Aug 2026 10:53:33 +0200 Subject: [PATCH 4/4] =?UTF-8?q?docs(simulation):=20PR=20#218=20round=203?= =?UTF-8?q?=20=E2=80=94=20the=20round-2=20fix=20outran=20its=20own=20docum?= =?UTF-8?q?entation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all doc-accuracy, and all the same root cause: folding the spacing predicate into the ring walk changed what three comments describe, and two of those comments were written by this same PR one round earlier. TYRE 1 — road_graph.rs's T-1206 gap-closure comment cited `nearest_land_cell`, which round 2 made `#[cfg(test)]`. A reader chasing that name lands on a test-only function and reasonably wonders whether they are looking at dead code. Repointed to `nearest_cell_matching`, and the paragraph's closing claim that "T-1206 guarantees the placement pixel is land" is corrected: it has been land-AND-spacing-or-skip since round 2. TYRE 2 — `max_land_search_ring`'s doc named the same test-only wrapper as the thing that walks the bound. It now names the production consumer and both callers. TYRE 3 — the D-211 amendment was written in round 1, before round 2 existed, and still described a land-only correction. It now carries a dated refinement recording what the code actually does: the walk satisfies BOTH of step 4's promises in one search, and SKIP therefore also fires where land exists but none of it clears spacing within the bound. The no-re-decision conclusion is unaffected — position remains a deterministic, non-fabricated function of seed and terrain — and the refinement notes the spacing promise is step 4's alone, since Tier A/B/C placements sit on their matched attractor and were never subject to it. HOSHE's three findings were the same three hunks, observed uncommitted while the review ran: accurate content, but not in the branch tip, so the PR would have merged a governance record that misdescribes its own commit. That is this commit. Both reviewers independently confirmed what the round-2 fix claims. Tyre traced the ring geometry and tie-break order by hand against the spacing predicate; Hoshe re-ran the full 267-body corpus scan live (850s) and reproduced the figures exactly — 267 bodies, 267 reaching Layer 3, 344 placements, 109 synthetic, 0 in water, 0 spacing violations. The shared-ring-search-helper retraction is confirmed and settled, with NEW grounds rather than a restatement: round 2 strengthened the case for keeping them separate, since this walk is now parameterized by an arbitrary predicate over native u16 terrain coordinates while road_graph's is a RouteGrid method over downsampled routing cells with a fixed cost test and an unrelated bound. 22 module tests green; clippy and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) --- governance/decisions/architecture.md | 4 +++- server/src/atlas/attractor_matching.rs | 6 ++++-- server/src/atlas/road_graph.rs | 23 ++++++++++++++--------- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/governance/decisions/architecture.md b/governance/decisions/architecture.md index 5d9de3f8c..66bfc5b95 100644 --- a/governance/decisions/architecture.md +++ b/governance/decisions/architecture.md @@ -1118,7 +1118,9 @@ Technical foundation decisions that constrain implementation: engine, client-ser - **Two-tier mismatch flagging:** If a matched city-attractor pair has score < 0.35, log a `WARNING` (below expected quality). If score < 0.15, log an `ERROR` and flag for manual review. Generation proceeds in both cases; the flags are for content auditing, not hard blockers. - **Output:** `Vec` written to `atlas_city_positions` at build time. - **Rationale:** Greedy-first for large/locked cities ensures anchor cities (capitals, corp HQs, wiki-named cities) are placed at terrain features that match their lore role. Hungarian for medium cities finds the globally optimal assignment, not just locally optimal. Synthetic overflow prevents the algorithm from failing on bodies where city count exceeds natural attractor count (dense, flat worlds). The two-tier warning system enables content QA without blocking generation. -- **Amendment (T-1206, 2026-08-06 — step 4 may now legitimately place nothing):** phase 4's synthetic position was derived by pure grid arithmetic with **no terrain input at all**, so it could and did land in open water — a scan of every heightmap body at world seed 42 found **46 of 109 synthetic placements sitting in ocean** (e.g. GJ903c at a genuine polar ocean cell). Synthetic overflow now takes the terrain analysis and land-corrects via a bounded nearest-land ring walk; land positions pass through untouched, so **no existing land placement moves** and this record's seed-derived-position promise is intact (position remains a pure function of seed + terrain — this fulfils the placement intent rather than deviating from it, so no re-decision is required). What **does** change is the record's outcome set: step 4's "cities that cannot be matched receive a synthetic `PlainCenter`" is no longer total — where no land exists within the search bound, the synthetic attractor is **skipped**, defined and deliberate, never a fabricated water position and never a panic. Step 5's name-fulfillment warning consequently fires for a new legitimate reason (a genuinely water-locked body), not only for a pipeline failure. +- **Amendment (T-1206, 2026-08-06 — step 4 may now legitimately place nothing):** phase 4's synthetic position was derived by pure grid arithmetic with **no terrain input at all**, so it could and did land in open water — a scan of every heightmap body at world seed 42 found **46 of 109 synthetic placements sitting in ocean** (e.g. GJ903c at a genuine polar ocean cell). Synthetic overflow now takes the terrain analysis and corrects via a bounded ring walk; land positions pass through untouched, so **no existing land placement moves** and this record's seed-derived-position promise is intact (position remains a pure function of seed + terrain — this fulfils the placement intent rather than deviating from it, so no re-decision is required). What **does** change is the record's outcome set: step 4's "cities that cannot be matched receive a synthetic `PlainCenter`" is no longer total — where the search finds nothing acceptable within its bound, the synthetic attractor is **skipped**, defined and deliberate, never a fabricated water position and never a panic. Step 5's name-fulfillment warning consequently fires for a new legitimate reason, not only for a pipeline failure. + + **Amendment refinement (PR #218 round 2, 2026-08-07 — the walk satisfies BOTH of step 4's promises).** The paragraph above was written when the correction searched for land alone, which quietly spent this step's *other* guarantee: step 4 promises a position that "respects minimum city spacing", and the spacing walk ran *before* the correction, so a candidate cleared against its neighbours could be nudged up to half a grid away onto one of them. The spacing test is now folded **into the same ring walk**, so a synthetic candidate must be both land **and** at least `MIN_SPACING` from every already-placed city, resolved in one search with the same deterministic tie-break. Consequently **SKIP also fires where land exists but none of it clears spacing within the bound** — not only on a genuinely water-locked body. The no-re-decision conclusion is unaffected: position remains a deterministic, non-fabricated function of seed and terrain. Note the spacing promise is step 4's alone — Tier A greedy and Tier B/C Hungarian placements sit on their matched terrain attractor and were never subject to it. - **Ticket:** T-919, T-925, T-1206 (ocean-mask guard amendment) - **Raised by:** Generation cascade workshop (T-897) - **Cross-reference:** D-195 (CompatibilityMatrix), D-207 (atlas_city_names), D-209 (GeographicAttractor — input), [D-210](#d-210) (terrain_modification_cost — input; its 2026-07-26 amendment records the *matched-attractor* half of this same placement-in-water gap, closed and validated under T-1116/T-1206) diff --git a/server/src/atlas/attractor_matching.rs b/server/src/atlas/attractor_matching.rs index 06f22e95c..971160670 100644 --- a/server/src/atlas/attractor_matching.rs +++ b/server/src/atlas/attractor_matching.rs @@ -268,8 +268,10 @@ fn hungarian(cost: &[Vec]) -> Vec { const MIN_SPACING: u16 = 15; /// T-1206 — ocean-mask guard search bound. Bounded ring-expansion search -/// radius (cells) `nearest_land_cell` will walk from a water-arithmetic -/// synthetic position before giving up. +/// radius (cells) [`nearest_cell_matching`] will walk from a water-arithmetic +/// synthetic position before giving up. That is the production consumer — +/// reached from `synthetic_attractor` with the land+spacing predicate, and +/// from the test-only `nearest_land_cell` wrapper with a trivial one. /// /// **Sized empirically, not by analogy.** An early draft mirrored /// `road_graph::COASTAL_ANCHOR_MAX_RING` (3) scaled up an order of magnitude diff --git a/server/src/atlas/road_graph.rs b/server/src/atlas/road_graph.rs index ffe1e2bd0..e3812b2b9 100644 --- a/server/src/atlas/road_graph.rs +++ b/server/src/atlas/road_graph.rs @@ -104,18 +104,23 @@ const MIN_CELL_COST: u32 = RIVER_COST; /// the fix). `synthetic_attractor` now takes an `Option<&TerrainAnalysis>` /// and, when supplied, guards the arithmetic position: already-land stays /// byte-identical (verified unmoved on all 63 real land-arithmetic -/// placements), water gets nudged to the nearest land cell via a -/// deterministic ring-walk (`nearest_land_cell`, the same tie-break pattern -/// as this file's own `nearest_passable_cell` below, at native/working-grid -/// resolution rather than the routing grid's downsample), and a city with no -/// land within the bounded search radius is SKIPPED entirely (never -/// fabricated, never a panic — reported via the existing Phase-5 -/// name-fulfillment warning). This routing relaxation still degrades +/// placements), water gets nudged to the nearest acceptable cell via a +/// deterministic ring-walk (`nearest_cell_matching`, the same tie-break +/// pattern as this file's own `nearest_passable_cell` below, at +/// native/working-grid resolution rather than the routing grid's downsample), +/// and a city with no such cell within the bounded search radius is SKIPPED +/// entirely (never fabricated, never a panic — reported via the existing +/// Phase-5 name-fulfillment warning). "Acceptable" is BOTH of D-211 step 4's +/// promises since the PR #218 round-2 fix: land AND at least `MIN_SPACING` +/// from every already-placed city, so correcting one can never quietly spend +/// the other. SKIP therefore also fires where land exists but none of it +/// clears spacing within the bound. This routing relaxation still degrades /// gracefully for the coastal-cell/downsample case it was built for (anchors /// to the nearest passable cell within `COASTAL_ANCHOR_MAX_RING`, or leaves /// it unrouted beyond that) — the two fixes are independent and both apply -/// (T-1206 guarantees the placement pixel is land; this relaxation still -/// covers the routing CELL being water-majority at downsample granularity). +/// (T-1206 guarantees the placement pixel is land and correctly spaced, or +/// that the city is skipped; this relaxation still covers the routing CELL +/// being water-majority at downsample granularity). /// /// The fix anchors routing to the NEAREST passable routing cell (ring-expansion /// search, deterministic tie-break) rather than the settlement's own impassable