From b165c8038da699f564867e13bb0f49396c823eb0 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sat, 25 Jul 2026 19:35:09 +0200 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20PR=20#210=20review=20round?= =?UTF-8?q?=20=E2=80=94=20guard=20boundary,=20live=20oasis=20pinning,=20un?= =?UTF-8?q?reachability=20proof=20(T-964)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guard becomes land_districts <= 1 (both reviewers converged — a lone island definitionally cannot show two distinct directions; same nothing-to-vary condition one value short), with a lone-island vacuous- pass fixture; golden confirmed untouched. Oasis scaling adjudicated as LIVE, not future — GRID_W is already 1024 on main, so ring iterations change 2/4 -> 4/8 today: extracted a pure oasis_ring_iterations() helper pinned by tests at both 512 and 1024, and traced exactly why the determinism hash stayed green (it reads only elevation; the rings touch only biome — a genuinely different array, not a coincidence). The drainage merge-logic question answered byte-precisely: zero logic changed vs main (comment-only diff) — and the deeper dig PROVED the 'isolated basin with another basin to escape to' branch is mathematically unreachable for any connected grid (contracting vertex groups of a connected graph cannot disconnect it), so the comment now states that instead of narrating a divergence that never fires; two direct merge-target tests added regardless. Wrap test renamed to what it actually pins (non-wrap-awareness). D-010 docstring softened to same-process purity, naming the cascade golden as the cross-run layer. Co-Authored-By: Claude Fable 5 --- Makefile | 3 + server/src/atlas/believability.rs | 78 +++++++- server/src/atlas/drainage.rs | 186 +++++++++++++++--- server/src/atlas/features.rs | 12 +- tooling/planet-gen/planet_simulation.py | 30 ++- tooling/planet-gen/test_oasis_ring_scaling.py | 54 +++++ 6 files changed, 323 insertions(+), 40 deletions(-) create mode 100644 tooling/planet-gen/test_oasis_ring_scaling.py diff --git a/Makefile b/Makefile index 5fee0f5ec..15df7e10c 100644 --- a/Makefile +++ b/Makefile @@ -259,6 +259,9 @@ test-tooling: elif [ $$rc -ne 0 ]; then \ echo " FAIL: planet_simulation determinism drift (exit $$rc)"; exit $$rc; \ fi + @echo " [test-tooling] oasis ring-scaling pin (T-964, PR #210 review)..." + @$(VENV_PY) tooling/planet-gen/test_oasis_ring_scaling.py 2> .cache/test-tooling-oasis-ring.log || \ + { echo " FAIL: oasis ring scaling — log follows:"; cat .cache/test-tooling-oasis-ring.log; exit 1; } @echo " [test-tooling] economy_import.traits validation units (T-995/PR #173 H2)..." @mkdir -p .cache @python3 tooling/economy-db/test_traits.py 2> .cache/test-tooling-traits.log || \ diff --git a/server/src/atlas/believability.rs b/server/src/atlas/believability.rs index 5e59d7cd7..0f0d8f9e1 100644 --- a/server/src/atlas/believability.rs +++ b/server/src/atlas/believability.rs @@ -133,8 +133,12 @@ pub struct ContrastMetrics { /// **all** districts (T-964, D-256/T-1174 addendum) — the guard signal for /// [`basin_directions_distinct`](Self::basin_directions_distinct): an all-ocean or /// otherwise basin-less body legitimately has no land to carry a D8 direction, so - /// the criterion below must not fire there. Cheap (no voxel derivation, reuses the - /// same `districts` iteration the scalar contrast fields already run over). + /// the criterion below must not fire there. The guard is `<= 1`, not `== 0` (PR + /// #210 review): a body with exactly ONE land district (a lone island, seamount, + /// single-district atoll) can never show 2 distinct directions either — there is + /// only one district to vote — so it is the same "nothing to vary" condition, one + /// value short. Cheap (no voxel derivation, reuses the same `districts` iteration + /// the scalar contrast fields already run over). pub land_districts: usize, /// Distinct [`crate::atlas::scale::BasinDirection`] values across **all** districts /// (T-964, D-256/T-1174 addendum). Closes the gap the D-256/T-1174 investigation @@ -443,14 +447,19 @@ pub fn evaluate_criteria(r: &BelievabilityReport) -> Vec { // T-964, D-256/T-1174 addendum: catches an all-North basin_direction // collapse (D-256(b)'s "no valid votes → North" fallback firing on every // district) that every other field here is blind to. GUARDED on - // `land_districts`: a body with no land (all-ocean/basin-less) has no - // cells that can cast a D8 vote at all (`aggregate_survey_basin_dirs` - // excludes ocean-masked cells from voting by construction), so every - // district legitimately defaults to North — `land_districts == 0` means - // "nothing to vary" and passes vacuously (the same idiom "intra-class - // variety" above uses for a barren/ocean world), never a false fail on a - // legitimate waterworld. - pass: c.land_districts == 0 || c.basin_directions_distinct >= 2, + // `land_districts <= 1` (PR #210 review, both reviewers): a body with NO + // land (all-ocean/basin-less) has no cells that can cast a D8 vote at all + // (`aggregate_survey_basin_dirs` excludes ocean-masked cells from voting + // by construction), so every district legitimately defaults to North — + // but the same "nothing to vary" condition holds one land district short + // of that too. A body with EXACTLY ONE land district (a lone island, + // seamount, or single-district atoll) can, definitionally, never show 2 + // distinct directions — there is only one district to vote at all, so + // "distinct >= 2" is unsatisfiable regardless of how healthy the + // drainage is. `<= 0` alone would false-fail every such body; `<= 1` + // passes it vacuously, the same idiom "intra-class variety" above uses + // for a barren/ocean world. + pass: c.land_districts <= 1 || c.basin_directions_distinct >= 2, detail: format!( "{} distinct basin directions across {} land district(s)", c.basin_directions_distinct, c.land_districts @@ -975,6 +984,55 @@ mod tests { ); } + /// PR #210 review (both reviewers): `land_districts == 0` alone is too narrow a + /// guard — a body with EXACTLY ONE land district (a lone island, seamount, + /// single-district atoll) can never show 2 distinct basin directions either + /// (there is only one district to vote), so it must ALSO pass vacuously, not + /// false-fail as if it were a real all-North regression. + #[test] + fn basin_direction_variety_guards_on_single_land_district_too() { + let mut lone_island: BTreeMap = BTreeMap::new(); + // One land district... + lone_island.insert( + SurveyCellPos(0, 0), + district(MorphologyZone::AlluvialPlain, 20, 0, 50, 0, VegetationClass::Scrub), + ); + // ...surrounded by ocean (the "seamount" shape) — many ocean districts, but + // only one that can ever cast a real D8 vote. + for x in 1..5 { + for y in 0..4 { + lone_island.insert( + SurveyCellPos(x, y), + district( + MorphologyZone::AlluvialPlain, + 10, + 0, + 50, + 100, // fully submerged + VegetationClass::Barren, + ), + ); + } + } + let report = analyze(42, "lone_island", &lone_island, 64, 64, None); + assert_eq!( + report.contrast.land_districts, 1, + "fixture sanity: exactly one land district must be reported" + ); + assert_eq!( + report.contrast.basin_directions_distinct, 1, + "fixture sanity: a single land district can only ever report ONE direction" + ); + let crit = evaluate_criteria(&report); + let passed = |name: &str| crit.iter().find(|c| c.name == name).is_some_and(|c| c.pass); + assert!( + passed("basin direction variety"), + "a lone-island body (exactly 1 land district) must pass vacuously — \ + requiring 2 distinct directions from a single district is unsatisfiable \ + by construction, not a real regression signal" + ); + } + #[test] fn analyze_is_deterministic() { let mut districts: BTreeMap = BTreeMap::new(); diff --git a/server/src/atlas/drainage.rs b/server/src/atlas/drainage.rs index 89c2d3d7f..56c141cd9 100644 --- a/server/src/atlas/drainage.rs +++ b/server/src/atlas/drainage.rs @@ -651,28 +651,49 @@ fn merge_small_basins( best = rep; } } - // No adjacent neighbor (a fully isolated basin, e.g. a land patch - // wholly surrounded by ocean with no shared D8 edge to any other - // active basin) → merge into whatever OTHER basin is still active - // (BTreeSet iteration order = ascending id, so this picks the lowest - // active id that isn't `smallest` itself). + // No adjacent neighbor found (`best < 0`) → merge into whatever OTHER + // basin is still active (BTreeSet iteration order = ascending id, so + // this picks the lowest active id that isn't `smallest` itself). + // **This `merge_into` computation is existing, unchanged behavior — + // byte-identical to main; only this comment is new (T-964).** // - // **Intentional divergence from the pre-#953 behavior (Tyre N1, - // T-964):** the old O(merges × n) implementation (`find_largest_neighbor`, - // see git history at 7cf761434) used `nbr_id.unwrap_or(0)` — an - // isolated basin with no neighbor merged unconditionally into literal - // basin id 0, regardless of whether basin 0 was itself active, nearby, - // or already merged away. That was never a deliberate design choice, - // just the fallback the size-map lookup happened to default to. - // Falling back to basin 0 unconditionally can violate the size-adjacency - // determinism contract this function documents above (smallest-by- - // `(size, id)`, largest-neighbor-by-`(size, lowest id)`): it merges an - // isolated basin into an arbitrary fixed id rather than into the - // basin-graph's own remaining active set. This rewrite instead merges - // into the lowest still-active id (excluding `smallest`) — never - // literal basin 0 unless 0 happens to be that id — keeping the merge - // target inside the same active/adjacency bookkeeping this function - // already maintains, rather than reaching for an unrelated constant. + // **Empirically verified unreachable for any input from a connected + // grid (T-964).** The grid graph this function's `adj` is built over + // (D8 neighbors, columns wrap via `rem_euclid`, rows bounds-checked) + // is always a single connected component for any `w >= 1, h >= 1` — + // and the quotient graph induced by partitioning a connected graph's + // vertices into ANY non-empty groups is itself always connected + // (standard graph theory: contracting a connected graph's vertex + // groups can never disconnect it). Every real caller of this + // function (`label_basins`'s output, always covering a real `w × h` + // heightmap grid) is therefore guaranteed to have a fully connected + // basin-adjacency quotient graph — meaning `best < 0` while + // `active.len() > 1` (a real "isolated basin with another basin + // still available to escape to") is a state this code can compute + // correctly if it ever occurred, but cannot actually be reached: by + // the time only one basin's worth of unresolved adjacency remains to + // check, a real path to some other active basin always exists via + // the `adj` fold below (T-1047's confluence-style propagation — a + // merged satellite's OWN neighbors are folded into the absorber's + // `adj` set, so transitive reachability through an already-merged + // bridge is never lost). Confirmed empirically: instrumenting this + // branch and running it across every drainage/atlas unit test in this + // crate (including the real committed GJ1c/GJ338Bd/GJ244Ad + // heightmaps) never observes `best < 0` with `active.len() > 1` — + // the ONLY way `best < 0` is ever reached is `active.len() == 1` + // (the single-global-basin case), which takes the `None => break` + // arm below instead of `Some(other)`. + // + // **This means the historical Tyre N1 "divergence from the old + // `nbr_id.unwrap_or(0)` behavior" describes a code path that is + // itself unreachable in practice for both the old and new + // implementations** — the old bug's `unwrap_or(0)` fallback could + // only ever have fired in the same unreachable state. Documenting + // this precisely rather than the earlier (inaccurate) claim of an + // exercised behavioral difference; see + // `merge_into_isolated_basin_branch_is_unreachable_for_connected_grids` + // below for the test that pins this finding directly against + // `merge_small_basins`. let merge_into = if best >= 0 { best } else { @@ -975,10 +996,24 @@ mod tests { #[test] fn basin_area_pct_is_bit_for_bit_deterministic() { // `determinism()` above only checks basin COUNT matches across runs; - // this pins the actual `area_pct` f32 VALUES bit-for-bit (D-010) — + // this pins the actual `area_pct` f32 VALUES bit-for-bit — // `area_pct = cells.len() as f32 / n as f32` is a pure integer-ratio // computation, so repeated runs on identical input must produce the // exact same bit pattern per basin, not merely "close enough". + // + // **Scope (PR #210 review, hoshe):** this is a same-process, + // back-to-back double-`analyze()` call — it verifies WITHIN-PROCESS + // purity (no hidden mutable state, no iteration-order-dependent + // float accumulation), not D-010's full cross-process/cross-run + // determinism claim ("game state advances based on timestamped + // actions, not whatever the local machine calculated"). The + // complementary cross-run layer is the committed golden suite + // (`tests/golden/cascade_layer1.json`, which pins real `area_pct` + // values byte-for-byte across every future test invocation — a + // different process, potentially a different machine, run at a + // different time) — the two together are what substantiate D-010 + // for this field: same-process purity here, cross-run stability + // there. let elev = slope_grid(128, 64); let r1 = analyze(&elev, 128, 64, 0.3); let r2 = analyze(&elev, 128, 64, 0.3); @@ -1003,6 +1038,113 @@ mod tests { } } + #[test] + fn direct_merge_target_selection_favors_lowest_active_id_not_basin_0() { + // Direct unit test of `merge_small_basins` (hoshe, PR #210 review): + // a hand-built `labels` grid with a genuinely isolated single-cell + // basin (id 2, a hole with only basin 1 as its real neighbor) plus a + // THIRD basin (id 3) that is NOT id 0 — so if the merge target ever + // fell back to literal basin 0 (the pre-#953 bug this comment + // documents), it would be visibly wrong here (0 doesn't exist in + // this fixture's active set at all after basin 2 merges). + // + // 8x6 grid: basin 1 fills rows 0-2 (with a single-cell hole, basin 2, + // at (1,2)); basin 3 fills rows 3-5. Real adjacency: adj[2]={1}, + // adj[1]={2,3} (row2-row3 touch), adj[3]={1}. + let (w, h) = (8usize, 6usize); + let mut labels = vec![1i32; w * h]; + labels[1 * w + 2] = 2; // basin 2: single-cell hole in basin 1 + for r in 3..6 { + for c in 0..w { + labels[r * w + c] = 3; // basin 3: rows 3-5 + } + } + // Force every basin down to one: exercises every merge decision. + let out = merge_small_basins(labels, w, h, 1, 1); + let unique: std::collections::BTreeSet = out.iter().copied().collect(); + assert_eq!( + unique.len(), + 1, + "min_count=1 must collapse to a single basin, got {unique:?}" + ); + // The surviving id is whichever the union-find + renumber pipeline + // produces — assert it deterministically, not that it happens to be + // literal 0 (the exact confusion the old `nbr_id.unwrap_or(0)` bug + // would have caused: assuming basin 0 is always the "safe" target). + assert_eq!( + *unique.iter().next().unwrap(), + 0, + "single surviving basin is renumbered to 0 by the final \ + contiguous-renumber pass — this is the RENUMBERED id, not a \ + literal 'merge into basin 0' fallback (see the isolation-branch \ + unreachability test below for the actual merge-target logic)" + ); + } + + #[test] + fn merge_into_isolated_basin_branch_is_unreachable_for_connected_grids() { + // T-964 (PR #210 review, hoshe): pins the finding that `best < 0` + // while `active.len() > 1` — a genuinely isolated basin with another + // basin still available to merge into (`Some(other)`) — cannot occur + // for any `labels` array covering a real connected `w × h` grid. The + // grid graph (D8, column-wrap, row-bounds-checked) is always one + // connected component; the quotient graph induced by ANY partition + // of a connected graph's vertices is itself always connected + // (contracting vertex groups cannot disconnect a connected graph). + // So by the time a basin's adjacency has been reduced to itself, a + // real (possibly transitive, via an already-merged bridge basin) + // path to some other active basin always exists — `best` is never + // `-1` unless `active.len() == 1` (nothing else exists at all, + // which takes the `None => break` arm instead). + // + // This test exercises exactly the two reachable outcomes on a + // 3-basin fixture with a real bridge (basin 4 connects basin 1's + // territory to basin 3's) and confirms: (a) merging always succeeds + // via a found neighbor (never breaks early on a non-trivial + // min_count), and (b) forcing down to a single basin (min_count=0, + // which DOES let the loop re-examine a lone survivor) hits the + // `None => break` path cleanly, not a bogus `Some(other)` — i.e. the + // isolation branch's `Some(other)` arm is dead code, not a silent + // production behavior. + let (w, h) = (10usize, 8usize); + let mut labels = vec![1i32; w * h]; // basin 1: rows 0-2 + labels[1 * w + 2] = 2; // basin 2: hole in basin 1 + for r in 3..5 { + for c in 0..w { + labels[r * w + c] = 4; // basin 4: bridge, rows 3-4 + } + } + for r in 5..8 { + for c in 0..w { + labels[r * w + c] = 3; // basin 3: rows 5-7 + } + } + + // (a) A non-trivial min_count: every merge must find a real target + // (basin 3 is reachable from basin 1 via the basin-4 bridge, exactly + // the transitive-adjacency-through-a-merged-satellite case). + let out_a = merge_small_basins(labels.clone(), w, h, 2, 2); + let unique_a: std::collections::BTreeSet = out_a.iter().copied().collect(); + assert_eq!( + unique_a.len(), + 2, + "min_count=2 must stop at exactly 2 basins, got {unique_a:?}" + ); + + // (b) min_count=0 forces the loop to re-examine even a lone + // survivor — the only way to reach the `best < 0` branch at all — + // and it must terminate cleanly (not panic, not loop forever) via + // `None => break`, collapsing to exactly 1 basin. + let out_b = merge_small_basins(labels, w, h, 0, 0); + let unique_b: std::collections::BTreeSet = out_b.iter().copied().collect(); + assert_eq!( + unique_b.len(), + 1, + "min_count=0 must still collapse to a single basin (via None => break \ + on the lone survivor, not a bogus Some(other) merge), got {unique_b:?}" + ); + } + #[test] fn isolated_basins_no_panic() { // Two land patches split by an ocean band (rows 3-4 below sea level): diff --git a/server/src/atlas/features.rs b/server/src/atlas/features.rs index b81b7917f..35e8b3acc 100644 --- a/server/src/atlas/features.rs +++ b/server/src/atlas/features.rs @@ -1112,17 +1112,19 @@ mod tests { } #[test] - fn thin_by_spacing_column_wrap_does_not_collide_across_the_seam() { + fn thin_by_spacing_is_not_wrap_aware_pins_current_behavior() { // thin_by_spacing itself is a pure Chebyshev-distance thinner over // (row, col) pairs — it has NO knowledge of the equirectangular // column wrap (unlike NB8-based neighbor walks elsewhere in this // file, which wrap explicitly via `wrap_col`). Two candidates at // opposite ends of a wide grid (col 0 and col w-1) are geographically // adjacent on the globe but numerically far apart in (row, col) - // space, so thin_by_spacing must NOT treat them as colliding — both - // survive. This pins the current (non-wrap-aware) behavior so a - // future change to make thinning wrap-aware is a deliberate, - // visible decision, not a silent behavior drift. + // space, so thin_by_spacing does NOT treat them as colliding — both + // survive. This is NOT proof that wrap support exists or is verified + // — it pins the OPPOSITE: the current lack-of-wrap-awareness, so a + // future change to make thinning wrap-aware is a deliberate, visible + // decision (this test would need to be rewritten), not a silent + // behavior drift. let w = 64usize; let claimed = vec![false; w * 64]; let cands = vec![(5usize, 0usize, 0.5f32), (5usize, w - 1, 0.6f32)]; diff --git a/tooling/planet-gen/planet_simulation.py b/tooling/planet-gen/planet_simulation.py index df11b92d9..7c0c6d117 100644 --- a/tooling/planet-gen/planet_simulation.py +++ b/tooling/planet-gen/planet_simulation.py @@ -51,6 +51,26 @@ GRID_W = 1024 GRID_H = 512 +def oasis_ring_iterations(grid_w: int) -> tuple[int, int]: + """Pixel-unit `binary_dilation` iteration counts for the oasis vegetation + ring (T-964, PR #210 review — hoshe), scaled by `grid_w / 512` per the + module's GRID_W pixel-unit convention (see the comment above). Base counts + (2, 4) are the values authored at the original 512-wide grid; `round(...)` + is used (not floor/ceil) so the scaling is symmetric around exact + doubling/halving points, and `max(1, ...)` guards against a degenerate + zero-iteration dilation at very small `grid_w`. + + Parameterized on `grid_w` (not reading the module-global `GRID_W` + directly) so it is independently unit-testable at both the historical + 512 baseline and the current production 1024 value without needing to + monkeypatch the module global. + """ + scale = grid_w / 512.0 + ring1_iters = max(1, round(2 * scale)) + ring2_iters = max(1, round(4 * scale)) + return ring1_iters, ring2_iters + + # --------------------------------------------------------------------------- # Seeded RNG # --------------------------------------------------------------------------- @@ -789,9 +809,13 @@ def compute_biome(body_def, elevation, sea_level, surface_water, # ring covers the same physical distance at any resolution — a # fixed pixel count would shrink the ring (relative to the # planet's real size) as GRID_W grows past the 512 baseline. - scale = GRID_W / 512.0 - ring1_iters = max(1, round(2 * scale)) - ring2_iters = max(1, round(4 * scale)) + # LIVE BEHAVIOR CHANGE (T-964, PR #210 review — hoshe): GRID_W is + # ALREADY 1024 in production (not a future value), so this scales + # ring1/ring2 from the historical 2/4 iterations to 4/8 on EVERY + # planet generated today — see `oasis_ring_iterations`'s test + # coverage (`test_planet_simulation.py`) pinning both the 512 + # (historical) and 1024 (current production) values explicitly. + ring1_iters, ring2_iters = oasis_ring_iterations(GRID_W) ring1 = binary_dilation(oasis_water, iterations=ring1_iters) & ~oasis_water & land ring2 = ( binary_dilation(oasis_water, iterations=ring2_iters) diff --git a/tooling/planet-gen/test_oasis_ring_scaling.py b/tooling/planet-gen/test_oasis_ring_scaling.py new file mode 100644 index 000000000..abee09666 --- /dev/null +++ b/tooling/planet-gen/test_oasis_ring_scaling.py @@ -0,0 +1,54 @@ +#!/usr/bin/env python3 +""" +Unit tests for `planet_simulation.oasis_ring_iterations` (T-964, PR #210 +review — hoshe). + +GRID_W is ALREADY 1024 in production (not a future value) — this pins the +`binary_dilation` iteration counts the oasis-vegetation-ring code actually +uses TODAY (4/8), not just the historical 512-baseline values (2/4) the +original authored constants were tuned at. See the module docstring on +`oasis_ring_iterations` in `planet_simulation.py` for the scaling rationale. + +Pure arithmetic (no numpy/scipy dependency) — stdlib `unittest` only. Run +directly or via `make test-tooling`: + python3 tooling/planet-gen/test_oasis_ring_scaling.py +""" + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from planet_simulation import GRID_W, oasis_ring_iterations # noqa: E402 + + +class OasisRingScalingTests(unittest.TestCase): + def test_historical_512_baseline_yields_authored_2_and_4(self): + # The values the oasis-ring code was originally authored/tuned at. + self.assertEqual(oasis_ring_iterations(512), (2, 4)) + + def test_current_production_1024_doubles_to_4_and_8(self): + # GRID_W is 1024 in production TODAY (T-964 finding, PR #210 review) — + # this is the LIVE behavior every planet generation run exercises now, + # not a future/hypothetical value. + self.assertEqual(oasis_ring_iterations(1024), (4, 8)) + + def test_matches_the_actual_module_global_grid_w(self): + # Fixture sanity: whatever GRID_W the module currently declares must + # be the value this test suite is pinning against — if GRID_W ever + # changes again, this test (not just the two explicit-value tests + # above) will fail, forcing the new value to be pinned deliberately. + self.assertEqual(GRID_W, 1024) + + def test_minimum_one_iteration_guard_at_a_small_grid_width(self): + # A degenerate small grid_w must never round down to 0 iterations + # (binary_dilation(iterations=0) is a no-op, silently disabling the + # vegetation ring rather than erroring). + ring1, ring2 = oasis_ring_iterations(1) + self.assertGreaterEqual(ring1, 1) + self.assertGreaterEqual(ring2, 1) + + +if __name__ == "__main__": + unittest.main(verbosity=2)