jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(simulation): PR #218 round 2 — the guard was spending D-211's spacing promise

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-08-07 09:49:39 +02:00
co-authored by Claude Opus 5
parent 4b75be5975
commit fc55bd897f
2 changed files with 208 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/atlas/attractor_matching.rs
+186 -26
View File
@@ -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<CityPlacement> = 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<String> = 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<u64> = 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<u64> = 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!(
server/src/atlas/believability.rs
+22 -8
View File
@@ -613,15 +613,29 @@ pub fn cascade_for_body(world_seed: u64, body_id: &str) -> Result<BodyWorldState
.map(|(snapshot, _params)| snapshot.into_body_world_state())
}
/// Like [`cascade_for_body`], but returns the raw [`CascadeSnapshot`] — which still
/// carries the transient `TerrainAnalysis` — together with the body's [`BodyParams`].
/// Like [`cascade_for_body`], but returns the raw [`CascadeSnapshot`] together with
/// the body's [`BodyParams`], instead of converting to a `BodyWorldState`.
///
/// This is the entry point for callers that need the *on-demand* district-derivation
/// inputs (`district_profile::derive_district` wants `&TerrainAnalysis` +
/// `&BodyParams`), e.g. the aliveness probe's `--render` district-window mode
/// (T-1123). [`cascade_for_body`] is this plus the `BodyWorldState` conversion,
/// which drops the TerrainAnalysis per the D-203/T-1048 size budget — take it off
/// the snapshot *before* converting.
/// **The returned snapshot's `terrain_analysis` is `None`.** This function always runs
/// the cascade to `CascadeLayer::Region`, and `run_cascade_from_heightmap` nulls the
/// transient `TerrainAnalysis` as soon as DistrictProfile and RoadGraph have consumed
/// it (`cascade.rs`, D-203/T-1048 — it is ~2 MB a body and is deliberately not
/// persisted). An earlier version of this comment claimed the snapshot "still carries
/// the transient TerrainAnalysis"; it does not, and the claim was not harmless — a
/// caller that read the mask off the snapshot and skipped bodies where it was absent
/// skipped **every** body while reporting a clean run over all 267 of them (T-1206,
/// PR #218).
///
/// So a caller needing the terrain must **re-derive** it from `snapshot.heightmap`,
/// which does survive, via the same call the cascade itself used:
/// `layer1::run_layer1_with_moisture(&snapshot.heightmap,
/// district_profile::derive_moisture_ceiling_q(&params))`. That reproduces the exact
/// grid the cascade's own outputs were computed against.
///
/// This is still the entry point for callers that need the *on-demand*
/// district-derivation inputs (`district_profile::derive_district` wants
/// `&TerrainAnalysis` + `&BodyParams`), e.g. the aliveness probe's `--render`
/// district-window mode (T-1123) — they just have to re-derive rather than read.
pub fn cascade_snapshot_for_body(
world_seed: u64,
body_id: &str,