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