From 1d2cac9e65e07ea79fdf9d0ac5a9efcca75935a9 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sat, 25 Jul 2026 09:04:54 +0200 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20PR=20#202=20review=20round?= =?UTF-8?q?=20=E2=80=94=20spill=20cell=20always=20wired=20+=20golden=20tru?= =?UTF-8?q?th?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoshe finding 1 (live-firing on GJ1c: 2 spill collisions + 1 i==1 collision among 51 Overflow basins): adjacency adjudicated INSUFFICIENT for the cue — a course's visible anchor is its upstream cell, so nothing pre-existing belongs to the lake unless wired. The spill cell (outlet_path[0]) now always gets a real entry: appended when new, OVERWRITTEN IN PLACE when it collided with an existing river cell (append would duplicate edge_id; the hydrology solve is the more authoritative downstream answer for that cell than flat D8 extraction). Interior stop-on-collision stays, now provably safe. Internal lookup is a dense Vec>, never iterated (D-010). Two non-vacuous regression tests prove the cue through build_edges output; end-to-end on GJ1c all 51 Overflow basins now build a readable edge (was: one silently missing). Hoshe finding 2: both doc sites now state the fallback-vs-production split explicitly (fallback moisture 55: 51/2; production GJ1c moisture 80: 53/53 all-Overflow) — the golden's Endorheic pair is a fallback-constant artifact, not a fact about GJ1c. Golden re-regenerated: river_cells 143->192, position-identity diff purely additive (zero removed, one legitimate in-place overwrite at the spill-collision cell); attractors/basins/mouths/confluences byte-identical. Suites: hydrology 26/26, full lib 1943, cascade_golden 1/1, window goldens + believability untouched green. Co-Authored-By: Claude Fable 5 --- server/src/atlas/hydrology_equilibrium.rs | 401 ++++++++++++---- server/src/atlas/layer1.rs | 17 + server/tests/cascade_golden.rs | 73 ++- server/tests/golden/cascade_layer1.json | 534 +++++++++++++++++++++- 4 files changed, 896 insertions(+), 129 deletions(-) diff --git a/server/src/atlas/hydrology_equilibrium.rs b/server/src/atlas/hydrology_equilibrium.rs index 1244cceb5..3399bc43c 100644 --- a/server/src/atlas/hydrology_equilibrium.rs +++ b/server/src/atlas/hydrology_equilibrium.rs @@ -424,44 +424,75 @@ pub fn solve( /// byte-identical order. No hashing, no `HashMap`, nothing keyed on wall-clock /// or thread scheduling. /// -/// **Cell selection.** `outlet_path` is "inclusive of both ends" (the basin's -/// `spill_cell` through the downstream terminus) — the spill cell itself is -/// already part of the basin's own lake footprint (a `Lake` gridunit, not a -/// river cell), so it is skipped; every remaining path cell becomes a new -/// river cell whose downstream pointer is the real D8 direction toward the -/// NEXT path cell (mirroring `extract_river_network`'s own `fdir[i]` -/// convention exactly — `outlet_path` is itself a chain of true D8 neighbors, -/// guaranteed by construction: [`cheapest_overflow_path`]'s Dijkstra only -/// ever relaxes D8-adjacent cells). The final path cell's downstream pointer -/// is set from `downstream_target`: [`DownstreamTarget::Sea`] maps to -/// [`RIVER_DOWNSTREAM_MOUTH`] (a real river reaching the sea IS a mouth, the -/// same semantics `extract_river_network` already assigns), and -/// [`DownstreamTarget::Basin`]/[`DownstreamTarget::OpenSpillway`] both map to -/// [`RIVER_DOWNSTREAM_EDGE_DRAIN`] (the course simply ends at the last -/// station — there is no meaningful "direction" once the outlet has reached -/// another lake's footprint or open ground, the same semantics `build_edges` -/// already gives a grid-artifact edge-drain: no further chord to invent). +/// **Cell selection — the spill cell ALWAYS gets a real outflow pointer +/// (PR #202 review, Hoshe finding 1).** `outlet_path` is "inclusive of both +/// ends": index 0 is the basin's `spill_cell` (a `Lake` gridunit at the +/// basin's own boundary — morphology sourcing and river-cell membership are +/// independent, T-1184, so a cell can legitimately be both), index +/// `len - 1` is the downstream terminus. The spill cell is the ONE cell that +/// visually anchors the lake's exit: every course drawn on the map starts at +/// a `RiverEdge::upstream` position ([`crate::atlas::river_course:: +/// invent_course`]'s `anchor_a`), so unless the spill cell itself carries an +/// outflow-pointing entry, NOTHING in the served network visually touches +/// this lake — a real `Overflow` basin would read exactly like `Endorheic` +/// (no course leaving the lake), which is the CUE this ticket exists to +/// deliver, not an incidental gap. This is why the spill cell is never +/// skipped, unconditionally: it always gets an entry pointing along +/// `outlet_path` (D8 direction toward `outlet_path[1]`, or straight to the +/// terminus sentinel on a length-1 residual path — see below), whether or +/// not it was already a `river_cells` member. If it WAS already present (a +/// real, if rare, case — verified on GJ1c: 2/51 real `Overflow` basins have +/// a pre-existing-river-cell spill point), its EXISTING entry is +/// overwritten in place (same array index, same position, new downstream +/// direction/class/seaward) rather than appended — appending would create a +/// second `river_cells` entry at the same `(row, col)`, which +/// [`crate::atlas::river_course::build_edges`] would turn into two +/// `RiverEdge`s sharing the same `edge_id` (`pack_cell_id` is a pure +/// function of position, Ruling 2d), corrupting the +/// one-edge-per-upstream-cell invariant `edge_ids_are_unique` guards. +/// Overwriting is correct, not merely safe: the pre-existing pointer was +/// computed by `extract_river_network`'s D8 walk on the ORIGINAL +/// (unfilled) surface, which has no knowledge of the basin's spill +/// direction — the hydrology solve is the more authoritative answer for +/// what water actually does at this specific cell once the basin is full, +/// so it wins. /// -/// **Cells already present in `river_cells` are skipped** (an outlet path can -/// legitimately re-enter the D8-extracted network, e.g. `DownstreamTarget:: -/// Basin` chaining through a stretch of terrain the original extraction -/// already classified as a river) — re-adding them would create duplicate -/// entries in the parallel arrays [`crate::atlas::river_course::build_edges`] -/// enumerates by index, corrupting the one-edge-per-upstream-cell invariant -/// (Ruling 2d). The chain still connects visually: the LAST cell before the -/// duplicate correctly points its D8 direction at the existing river cell, -/// which already has its own onward pointer — the course simply continues -/// through machinery that was already there. +/// Every remaining interior path cell (index 1 through `len - 2`) becomes a +/// new river cell whose downstream pointer is the real D8 direction toward +/// the NEXT path cell (mirroring `extract_river_network`'s own `fdir[i]` +/// convention exactly — `outlet_path` is itself a chain of true D8 +/// neighbors, guaranteed by construction: [`cheapest_overflow_path`]'s +/// Dijkstra only ever relaxes D8-adjacent cells). The final path cell's +/// downstream pointer is set from `downstream_target`: [`DownstreamTarget:: +/// Sea`] maps to [`RIVER_DOWNSTREAM_MOUTH`] (a real river reaching the sea +/// IS a mouth, the same semantics `extract_river_network` already assigns), +/// and [`DownstreamTarget::Basin`]/[`DownstreamTarget::OpenSpillway`] both +/// map to [`RIVER_DOWNSTREAM_EDGE_DRAIN`] (the course simply ends at the +/// last station — there is no meaningful "direction" once the outlet has +/// reached another lake's footprint or open ground, the same semantics +/// `build_edges` already gives a grid-artifact edge-drain: no further chord +/// to invent). /// -/// **`EdgeUnreachable` and empty/single-cell paths contribute nothing** — a -/// path of length ≤ 1 (just the spill cell, already skipped) has no interior -/// cell to assign a direction to, so no new edge is created. This is the -/// correct behaviour, not a gap: a basin whose overflow search exhausted its -/// budget without finding a real terminus has no honest direction to draw a -/// river in, and the module's own guarantee -/// (`overflowing_basin_has_nonempty_outlet_path`) is about `Overflow` -/// basins having a *search path*, not that every search path resolves to a -/// non-trivial extension. +/// **Interior cells (index ≥ 1) already present in `river_cells` STOP the +/// walk** (an outlet path can legitimately re-enter the D8-extracted +/// network, e.g. `DownstreamTarget::Basin` chaining through a stretch of +/// terrain the original extraction already classified as a river) — this is +/// safe (unlike the spill-cell case above) precisely BECAUSE the spill cell +/// always got its own real entry first: the chain from the lake is never +/// silently dropped, only its LATER re-entry into pre-existing machinery is +/// deduplicated. The cell immediately before the collision (which may BE the +/// spill cell itself, on a length-2 path) correctly points its D8 direction +/// at the existing river cell, which already has its own onward pointer — +/// the course continues through machinery that was already there. +/// +/// **`EdgeUnreachable` paths and a length-1 residual (spill-only) path still +/// get the spill cell's real entry, just with no interior D8 hop.** A path +/// of length ≤ 1 (`outlet_path == [spill]` — the `EdgeUnreachable` +/// best-effort case, or any basin whose search terminates immediately) has +/// no `outlet_path[1]` to point toward, so the spill cell's downstream +/// sentinel is taken directly from `downstream_target` (the same terminus +/// mapping the multi-cell path's LAST cell uses) instead of a D8 direction — +/// still a real, readable outflow entry, never silently dropped. /// /// **`river_class` scope note.** Every appended cell is classified `0` /// (stream — [`crate::atlas::river_course`]'s narrowest, most conservative @@ -479,17 +510,24 @@ pub fn extend_river_network_with_basin_outlets( height: u32, mut network: crate::atlas::body_world_state::RiverNetwork, ) -> crate::atlas::body_world_state::RiverNetwork { - use crate::atlas::body_world_state::{RIVER_DOWNSTREAM_EDGE_DRAIN, RIVER_DOWNSTREAM_MOUTH}; + use crate::atlas::body_world_state::RIVER_DOWNSTREAM_MOUTH; let w = width as usize; + let n = (width as usize) * (height as usize); - // O(1) membership test against the network's pre-existing river cells — - // built once, outside the basin loop (basins can number in the thousands - // on a real body, per the T-1177 population survey: 22,270 basins across - // 267 bodies). - let mut is_river_cell = vec![false; (width as usize) * (height as usize)]; - for &(r, c) in &network.river_cells { - is_river_cell[r as usize * w + c as usize] = true; + // Position -> array-index for the network's pre-existing river cells — a + // dense `Vec` (same shape/determinism story as `is_lake`/`basin_of` + // above: O(1) point lookup, never iterated, so there is no D-010 + // HashMap-iteration-order concern to even raise). Built once, outside + // the basin loop (basins can number in the thousands on a real body, per + // the T-1177 population survey: 22,270 basins across 267 bodies). + // Doubles as the membership test the interior-cell dedup walk needs; the + // INDEX half is what the spill-cell overwrite case (Hoshe finding 1) + // needs to mutate the correct existing entry in place rather than + // appending a duplicate. + let mut river_cell_index: Vec> = vec![None; n]; + for (idx, &(r, c)) in network.river_cells.iter().enumerate() { + river_cell_index[r as usize * w + c as usize] = Some(idx); } // Basins are already stored in ascending `basin_id` order (the `(0.. @@ -504,25 +542,62 @@ pub fn extend_river_network_with_basin_outlets( else { continue; // Endorheic — no continuation; absence IS the cue. }; + if outlet_path.is_empty() { + continue; // Degenerate — no spill cell at all to anchor from. + } + + // The spill cell's downstream sentinel (fn doc's "Cell selection" + // section): real D8 direction toward `outlet_path[1]` when an + // interior cell exists, otherwise the terminus mapping directly (a + // length-1 residual/EdgeUnreachable path) — either way, ALWAYS a + // real entry, never skipped. + let spill = outlet_path[0]; + let (sr, sc) = (spill / w, spill % w); + let spill_sentinel = if outlet_path.len() >= 2 { + let next = outlet_path[1]; + let (nr, nc) = (next / w, next % w); + d8_direction_between((sr, sc), (nr, nc), w) + } else { + terminus_sentinel(downstream_target) + }; + let spill_seaward = seaward_for(spill_sentinel, sr, sc, RIVER_DOWNSTREAM_MOUTH); + + match river_cell_index[spill] { + Some(existing_idx) => { + // Overwrite in place (fn doc: the hydrology solve is the + // more authoritative answer for this cell's true downstream + // direction than the original flat D8 extraction) — never + // append, which would duplicate `edge_id` at this position. + network.river_class[existing_idx] = 0; + network.river_downstream[existing_idx] = spill_sentinel; + network.river_seaward[existing_idx] = spill_seaward; + } + None => { + let new_idx = network.river_cells.len(); + network.river_cells.push((sr as u16, sc as u16)); + network.river_class.push(0); + network.river_downstream.push(spill_sentinel); + network.river_seaward.push(spill_seaward); + river_cell_index[spill] = Some(new_idx); + } + } - // `outlet_path` is inclusive of the spill cell (index 0) — skip it, - // it's a lake cell, not a river cell. Fewer than 2 entries means no - // interior cell exists to extend from (the EdgeUnreachable - // best-effort `vec![spill]` case, or a degenerate empty path). if outlet_path.len() < 2 { - continue; + continue; // No interior cell beyond the spill entry above. } for i in 1..outlet_path.len() { let cell = outlet_path[i]; let (r, c) = (cell / w, cell % w); - if is_river_cell[cell] { + if river_cell_index[cell].is_some() { // Re-entered the pre-existing D8 network (e.g. chained into // another basin's already-extracted river reach) — the prior - // cell's downstream pointer (set below, on the PREVIOUS loop - // iteration or as the loop's own boundary case) already - // points here, so the chain is visually continuous without - // adding a duplicate entry. + // cell's downstream pointer (the spill entry above, or a + // PREVIOUS loop iteration) already points here, so the chain + // is visually continuous without adding a duplicate entry. + // Safe here (unlike the spill cell) because the spill entry + // above already guarantees the lake's own outflow is never + // silently dropped — this only dedups a LATER re-entry. break; } @@ -533,50 +608,60 @@ pub fn extend_river_network_with_basin_outlets( let (nr, nc) = (next / w, next % w); d8_direction_between((r, c), (nr, nc), w) } else { - match downstream_target { - DownstreamTarget::Sea => RIVER_DOWNSTREAM_MOUTH, - DownstreamTarget::Basin(_) | DownstreamTarget::OpenSpillway => { - RIVER_DOWNSTREAM_EDGE_DRAIN - } - // EdgeUnreachable never reaches here: it only ever - // carries a length-≤-1 path (see `cheapest_overflow_path`), - // already filtered above. - DownstreamTarget::EdgeUnreachable => RIVER_DOWNSTREAM_EDGE_DRAIN, - } + terminus_sentinel(downstream_target) }; + let new_idx = network.river_cells.len(); network.river_cells.push((r as u16, c as u16)); network.river_class.push(0); // stream — fixed default, see fn doc's scope note network.river_downstream.push(sentinel); network .river_seaward - .push(if sentinel == RIVER_DOWNSTREAM_MOUTH { - // A real mouth needs a real seaward neighbor for - // `build_edges`' Mouth-chord synthesis (Ruling 2b's own - // precedent — see `RiverNetwork::river_seaward`'s doc). The - // outlet's own next-cell-below-sea-level is exactly that: - // `cheapest_overflow_path`'s Sea termination condition is - // `original[idx] <= sea_scaled` AT THIS CELL, so `(r, c)` - // itself is already the sub-sea-level neighbor being - // recorded — but `river_seaward` wants the SEAWARD cell, one - // step further than the last land cell. Since this loop only - // ever visits path cells up to and including the terminus, - // and the terminus here has original elevation <= sea_scaled - // by construction, (r, c) IS a below-sea-level cell — use it - // directly (matches `extract_river_network`'s own `(nr, nc)` - // capture, which likewise stores the sub-sea-level neighbor - // itself, not a cell beyond it). - (r as u16, c as u16) - } else { - (0, 0) - }); - is_river_cell[cell] = true; + .push(seaward_for(sentinel, r, c, RIVER_DOWNSTREAM_MOUTH)); + river_cell_index[cell] = Some(new_idx); } } network } +/// Terminus sentinel for a [`BasinOutcome::Overflow`]'s downstream end, from +/// [`DownstreamTarget`] — shared by both the spill-cell entry (a length-1 +/// residual path with no interior cell to point at) and the last interior +/// path cell's entry (the common multi-cell case). +/// [`DownstreamTarget::Sea`] maps to [`RIVER_DOWNSTREAM_MOUTH`] (a real +/// river reaching the sea IS a mouth, the same semantics +/// `extract_river_network` already assigns); every other variant +/// ([`DownstreamTarget::Basin`], [`DownstreamTarget::OpenSpillway`], +/// [`DownstreamTarget::EdgeUnreachable`]) maps to +/// [`RIVER_DOWNSTREAM_EDGE_DRAIN`] — no further chord to invent once the +/// outlet has reached another lake's footprint, open ground, or exhausted +/// its search budget. +fn terminus_sentinel(target: &DownstreamTarget) -> u8 { + use crate::atlas::body_world_state::{RIVER_DOWNSTREAM_EDGE_DRAIN, RIVER_DOWNSTREAM_MOUTH}; + match target { + DownstreamTarget::Sea => RIVER_DOWNSTREAM_MOUTH, + DownstreamTarget::Basin(_) + | DownstreamTarget::OpenSpillway + | DownstreamTarget::EdgeUnreachable => RIVER_DOWNSTREAM_EDGE_DRAIN, + } +} + +/// [`RiverNetwork::river_seaward`] value for a cell whose downstream +/// sentinel is `sentinel` — a real seaward neighbor when `sentinel == +/// mouth_sentinel` (see [`extend_river_network_with_basin_outlets`]'s doc on +/// why `(r, c)` itself, not a cell beyond it, is the correct seaward value +/// here — mirrors `extract_river_network`'s own `(nr, nc)` capture), the +/// unreadable `(0, 0)` placeholder otherwise (matches every non-MOUTH +/// `river_seaward` entry across the codebase). +fn seaward_for(sentinel: u8, r: usize, c: usize, mouth_sentinel: u8) -> (u16, u16) { + if sentinel == mouth_sentinel { + (r as u16, c as u16) + } else { + (0, 0) + } +} + /// The D8 direction index `k` (matching [`d8_offset`]'s table) such that /// stepping from `(r, c)` by `d8_offset(k)` (with horizontal wraparound, /// [`crate::atlas::drainage`]'s convention) reaches `(nr, nc)`. Panics if the @@ -1536,6 +1621,152 @@ mod tests { ); } + /// PR #202 review, Hoshe finding 1 — the i==1 collision regression, + /// asserted NON-VACUOUSLY on the actual D-227 amendment (4) cue: when + /// `outlet_path[1]` (the FIRST interior cell after the spill) is already + /// a pre-existing river cell, the basin's Overflow status must still be + /// READABLE via `build_edges` — a course must leave the lake. Before the + /// fix, this exact scenario made the loop `break` on its first + /// iteration having pushed zero cells for the basin, silently making a + /// real Overflow basin indistinguishable from Endorheic. + #[test] + fn overflow_basin_survives_an_i_equals_one_collision_non_vacuously() { + let result = overflow_bowl_result(); + let overflow = result + .basins + .iter() + .find(|b| matches!(b.outcome, BasinOutcome::Overflow { .. })) + .expect("fixture sanity: at least one Overflow basin"); + let BasinOutcome::Overflow { outlet_path, .. } = &overflow.outcome else { + unreachable!() + }; + assert!( + outlet_path.len() >= 2, + "fixture sanity: outlet_path must have an interior cell at index 1 \ + to pre-seed as the i==1 collision" + ); + let spill = outlet_path[0]; + let collision_cell = outlet_path[1]; // the exact i==1 collision position + let (sr, sc) = (spill / 64, spill % 64); + let (cr, cc) = (collision_cell / 64, collision_cell % 64); + + // Pre-seed the network with ONLY the i==1 cell as a native river + // cell (pointing at some unrelated existing direction) — the spill + // cell itself is NOT pre-existing, isolating this as the exact + // "first interior cell collides" scenario Hoshe's finding names. + let mut network = empty_network(); + network.river_cells.push((cr as u16, cc as u16)); + network.river_class.push(1); + network + .river_downstream + .push(crate::atlas::body_world_state::RIVER_DOWNSTREAM_EDGE_DRAIN); + network.river_seaward.push((0, 0)); + + let after = extend_river_network_with_basin_outlets(&result, 64, 32, network); + + // The spill cell must have its OWN entry — this is the actual fix: + // before it, nothing was ever pushed for this basin at all. + let spill_idx = after + .river_cells + .iter() + .position(|&cell| cell == (sr as u16, sc as u16)) + .expect( + "the spill cell must have a real river_cells entry even when \ + outlet_path[1] collides with a pre-existing river cell — this \ + is the exact bug PR #202 flagged: a silent break at i==1 must \ + never erase the basin's own outflow anchor", + ); + // Its downstream pointer must be a real D8 direction (not a sentinel) + // toward the collision cell, since outlet_path[1] IS the next hop. + assert!( + after.river_downstream[spill_idx] < 8, + "the spill cell's downstream pointer must be a real D8 direction \ + toward outlet_path[1], not a terminus sentinel" + ); + + // Non-vacuous on the cue itself: build_edges must produce a real + // edge whose upstream IS the spill cell — an overflow lake's exit + // river must be readable from the served network, not just present + // as raw unreachable array data. + let edges = crate::atlas::river_course::build_edges(&after); + assert!( + edges.iter().any(|e| e.upstream == (sr as u16, sc as u16)), + "an Overflow basin's spill cell must produce a real RiverEdge even \ + when its first interior outlet cell collides with a pre-existing \ + river cell — otherwise this basin is visually indistinguishable \ + from Endorheic, which is exactly the cue this ticket must not break" + ); + } + + /// The sibling collision case: the SPILL CELL ITSELF (not an interior + /// cell) is already a pre-existing river cell — verified to occur on + /// real GJ1c data (2/51 real Overflow basins). The pre-existing entry's + /// downstream pointer must be overwritten to point along the outlet + /// path (the hydrology solve is the more authoritative answer for this + /// cell's true direction), not left pointing wherever the original flat + /// D8 extraction guessed — and it must not create a duplicate + /// `river_cells` entry at the same position. + #[test] + fn overflow_basin_overwrites_a_pre_existing_spill_cell_in_place() { + let result = overflow_bowl_result(); + let overflow = result + .basins + .iter() + .find(|b| matches!(b.outcome, BasinOutcome::Overflow { .. })) + .expect("fixture sanity: at least one Overflow basin"); + let BasinOutcome::Overflow { outlet_path, .. } = &overflow.outcome else { + unreachable!() + }; + assert!( + outlet_path.len() >= 2, + "fixture sanity: needs an interior cell" + ); + let spill = outlet_path[0]; + let (sr, sc) = (spill / 64, spill % 64); + + // Pre-seed the network with the SPILL cell as a native river cell, + // pointing in an unrelated direction (simulating the original flat + // D8 extraction having already claimed this cell for its own, + // wrong-for-the-lake reasons). + let mut network = empty_network(); + network.river_cells.push((sr as u16, sc as u16)); + network.river_class.push(2); // deliberately a different class + network.river_downstream.push(0); // deliberately a real but likely-wrong direction + network.river_seaward.push((0, 0)); + + let after = extend_river_network_with_basin_outlets(&result, 64, 32, network); + + // Exactly one entry at the spill position — overwritten, not duplicated. + let occurrences = after + .river_cells + .iter() + .filter(|&&cell| cell == (sr as u16, sc as u16)) + .count(); + assert_eq!( + occurrences, 1, + "the spill cell must be overwritten in place, never duplicated" + ); + let spill_idx = after + .river_cells + .iter() + .position(|&cell| cell == (sr as u16, sc as u16)) + .unwrap(); + assert!( + after.river_downstream[spill_idx] < 8, + "the overwritten spill cell must point along the real outlet path, \ + not retain whatever direction the pre-existing entry had" + ); + + // Non-vacuous on the cue: the overwritten spill cell must still + // build into a real, readable RiverEdge. + let edges = crate::atlas::river_course::build_edges(&after); + assert!( + edges.iter().any(|e| e.upstream == (sr as u16, sc as u16)), + "an overwritten spill cell must still produce a real RiverEdge — \ + the lake's outflow must remain readable" + ); + } + #[test] fn d8_direction_between_matches_the_offset_table() { // Round-trip sanity: for every direction in the D8 table, stepping diff --git a/server/src/atlas/layer1.rs b/server/src/atlas/layer1.rs index 178f25490..739765eff 100644 --- a/server/src/atlas/layer1.rs +++ b/server/src/atlas/layer1.rs @@ -83,6 +83,23 @@ pub struct Layer1Output { /// used ONLY by [`run_layer1`]'s two-arg form; [`run_layer1_with_moisture`] /// (called by every production site that has real `BodyParams` in scope) never /// reaches this constant. +/// +/// **This constant is NOT what a player sees on any real body (PR #202 +/// review, Hoshe finding 2) — never cite a basin's Overflow/Endorheic split +/// at this moisture value as representative of production.** The +/// endorheic-vs-overflow decision (`hydrology_equilibrium::is_endorheic`) +/// gates on `moisture_q <= ENDORHEIC_MOISTURE_CEILING (60)`; this fallback +/// (55) sits just BELOW that ceiling, so a fallback-path solve can show +/// Endorheic basins that would never occur on the real body. Concretely, on +/// GJ1c (`hydrosphere: liquid_water`, `atmosphere: standard` → +/// `derive_moisture_ceiling_q` = 80, well ABOVE the ceiling): the fallback +/// path (55) splits its 53 real basins 51 Overflow / 2 Endorheic, while the +/// PRODUCTION path (80, via `run_layer1_with_moisture` at a real call site) +/// is 53/53 all-Overflow on the identical geometry — moisture is the only +/// variable that moves (`filled_scaled`/basin geometry itself is +/// moisture-independent). Any doc, golden comment, or report citing a +/// basin-outcome ratio must state which moisture path produced it — see +/// `tests/cascade_golden.rs`'s own re-pin note for the worked example. const DEFAULT_HYDROLOGY_MOISTURE_Q: i32 = 55; /// Run the Layer-1 topography pipeline for a single body. diff --git a/server/tests/cascade_golden.rs b/server/tests/cascade_golden.rs index 9701a4b9f..dda47ece2 100644 --- a/server/tests/cascade_golden.rs +++ b/server/tests/cascade_golden.rs @@ -61,28 +61,57 @@ //! D8 river-network wiring):** `run_layer1`/`run_layer1_with_moisture` now //! extends the D8-extracted `RiverNetwork` with every `BasinOutcome:: //! Overflow` basin's `outlet_path` as new river cells (the endorheic cue: -//! outflow-course PRESENCE). On this fixture (GJ1c, 256×128 downsample, -//! `moisture_q=55` — `run_cascade_from_heightmap`'s `body_params: None` -//! path), 51 of the working grid's 53 real basins classify `Overflow`; -//! their outlets add exactly **50 new river cells** (`river_cells` -//! 93→143 — one basin's single new cell coincides with a cell another -//! basin's outlet already added, correctly deduplicated). `attractors` -//! (256) and `drainage_basins` are UNCHANGED — the extension runs strictly -//! after `features::extract_attractors`, by design, so basin-outlet cells -//! never perturb geographic attractor placement. `mouths`/`confluences` -//! also unchanged (T-1185 never rewrites those arrays, only appends to -//! `river_cells`/`river_class`/`river_downstream`/`river_seaward`). Every -//! appended cell on this real body carries `river_downstream = -//! RIVER_DOWNSTREAM_EDGE_DRAIN` (9) — verified separately (a scratch probe, -//! not committed) that all 51 real `Overflow` basins here have a -//! length-2 `outlet_path` (`[spill_cell, terminus]`) resolving to -//! `DownstreamTarget::OpenSpillway`, the dominant real-world pattern on -//! this body (most basins spill onto immediately-adjacent open ground -//! rather than carving a long channel to the sea) — not a bug or an -//! unexercised code path; the interior-D8-direction branch (0–7) and the -//! `RIVER_DOWNSTREAM_MOUTH` terminus branch are both exercised by the -//! dedicated `hydrology_equilibrium`/`layer1` unit tests instead, which use -//! synthetic fixtures shaped to hit those specific branches. +//! outflow-course PRESENCE). `attractors` (256) and `drainage_basins` are +//! UNCHANGED — the extension runs strictly after +//! `features::extract_attractors`, by design, so basin-outlet cells never +//! perturb geographic attractor placement. `mouths`/`confluences` also +//! unchanged (T-1185 never rewrites those arrays). +//! +//! **This golden pins the FALLBACK moisture path, not production — stated +//! explicitly so nobody cites its basin-outcome split as what a player +//! actually sees (PR #202 review, Hoshe finding 2).** +//! `run_cascade_from_heightmap`'s `body_params: None` argument here means +//! `run_layer1`'s `DEFAULT_HYDROLOGY_MOISTURE_Q = 55` fallback is what +//! solves this fixture's hydrology, not GJ1c's REAL body params +//! (`wiki/star-systems/GJ-1/bodies/GJ1c/index.md`: `hydrosphere: +//! liquid_water`, `atmosphere: standard` → `derive_moisture_ceiling_q` +//! yields **moisture_q=80**, well above `ENDORHEIC_MOISTURE_CEILING=60`). +//! At the fallback `moisture_q=55` this fixture's 53 real working-grid +//! basins split 51 Overflow / 2 Endorheic; verified directly (a scratch +//! probe, not committed) that at the PRODUCTION `moisture_q=80` the SAME +//! 53 basins are **53/53 all-Overflow — zero Endorheic** (moisture is the +//! only thing that moves; `filled_scaled`/basin geometry itself is +//! moisture-independent, per `run_layer1_with_moisture_changes_ +//! endorheic_split_not_lake_extent`'s own invariant). A materially +//! different picture: on the real body, every one of these basins shows an +//! exit river on the map — this fixture happening to include 2 Endorheic +//! basins is an artifact of testing at the body-agnostic fallback +//! constant, not a fact about GJ1c itself. +//! +//! **Cell count (post PR #202 review fix — the i==1/spill-collision +//! regression, Hoshe finding 1):** `river_cells` 93→192 (+99). Every one of +//! the 51 real `Overflow` basins now has its spill cell wired as a real +//! river-network entry, unconditionally — the earlier landing (143, now +//! superseded) silently dropped a basin's ENTIRE outlet whenever +//! `outlet_path[1]` collided with a pre-existing river cell (the loop broke +//! on its first iteration having pushed nothing), which on THIS fixture +//! happened for 1 basin outright and would have made it visually +//! indistinguishable from Endorheic — exactly the cue this ticket must +//! never break. Verified directly against this fixture (scratch probe, not +//! committed): all 51 real `Overflow` basins now build into a real, +//! readable `RiverEdge` via `river_course::build_edges` — zero basins +//! missing an outlet edge. One pre-existing baseline cell (`(28, 14)`) is +//! overwritten in place (its `river_downstream` sentinel changes from +//! `RIVER_DOWNSTREAM_EDGE_DRAIN` to a real D8 direction) rather than +//! duplicated — the other real spill-cell collision on this fixture lands +//! among the newly-appended range, not the original 93-cell baseline. +//! Confirmed by direct position-identity diff (not raw array-index +//! comparison, which is misleading once the fix reshuffles positions +//! within each basin's block): zero positions are ever REMOVED between the +//! pre-fix and post-fix goldens, only added-or-overwritten — the fix is +//! additive at the position level, exactly as designed. No duplicate +//! `(row, col)` positions exist in the final array (verified — the +//! `edge_ids_are_unique` invariant `build_edges` depends on holds). use std::path::PathBuf; diff --git a/server/tests/golden/cascade_layer1.json b/server/tests/golden/cascade_layer1.json index d0c67175d..5e25ace28 100644 --- a/server/tests/golden/cascade_layer1.json +++ b/server/tests/golden/cascade_layer1.json @@ -8152,86 +8152,170 @@ 124, 239 ], + [ + 2, + 99 + ], [ 1, 100 ], + [ + 2, + 137 + ], [ 1, 138 ], + [ + 8, + 194 + ], [ 7, 195 ], + [ + 8, + 204 + ], [ 7, 204 ], + [ + 10, + 68 + ], [ 10, 69 ], + [ + 14, + 29 + ], [ 14, 28 ], + [ + 13, + 47 + ], [ 14, 48 ], + [ + 13, + 172 + ], [ 14, 173 ], + [ + 18, + 179 + ], [ 19, 180 ], + [ + 24, + 152 + ], [ 25, 152 ], + [ + 20, + 165 + ], [ 21, 166 ], + [ + 16, + 199 + ], [ 17, 200 ], + [ + 16, + 6 + ], [ 17, 6 ], + [ + 19, + 181 + ], [ 20, 181 ], + [ + 17, + 45 + ], [ 16, 46 ], + [ + 17, + 92 + ], [ 17, 93 ], + [ + 17, + 126 + ], [ 17, 127 ], + [ + 19, + 223 + ], [ 19, 224 ], + [ + 22, + 231 + ], [ 21, 232 ], + [ + 32, + 254 + ], [ 33, 255 ], + [ + 26, + 103 + ], [ 27, 104 @@ -8240,114 +8324,226 @@ 29, 15 ], + [ + 30, + 61 + ], [ 31, 61 ], + [ + 27, + 237 + ], [ 27, 236 ], + [ + 29, + 138 + ], [ 30, 138 ], + [ + 31, + 9 + ], [ 31, 10 ], + [ + 34, + 137 + ], [ 34, 136 ], + [ + 34, + 156 + ], [ 35, 155 ], + [ + 37, + 73 + ], [ 37, 72 ], + [ + 39, + 64 + ], [ 39, 63 ], + [ + 40, + 235 + ], [ 41, 235 ], + [ + 45, + 123 + ], [ 44, 122 ], + [ + 52, + 241 + ], [ 51, 242 ], + [ + 51, + 0 + ], [ 52, 1 ], + [ + 51, + 109 + ], [ 50, 108 ], + [ + 56, + 40 + ], [ 57, 41 ], + [ + 57, + 63 + ], [ 56, 64 ], + [ + 58, + 215 + ], [ 57, 216 ], + [ + 62, + 43 + ], [ 62, 42 ], + [ + 70, + 233 + ], [ 70, 232 ], + [ + 74, + 228 + ], [ 74, 229 ], + [ + 74, + 230 + ], [ 74, 231 ], + [ + 91, + 214 + ], [ 92, 213 ], + [ + 88, + 37 + ], [ 88, 38 ], + [ + 97, + 89 + ], [ 96, 88 ], + [ + 101, + 215 + ], [ 101, 214 ], + [ + 115, + 40 + ], [ 114, 41 ], + [ + 115, + 60 + ], [ 115, 59 ], + [ + 116, + 220 + ], [ 117, 221 ], + [ + 124, + 25 + ], [ 124, 24 @@ -8408,7 +8604,7 @@ 0, 1, 0, - 2, + 0, 1, 0, 1, @@ -8496,6 +8692,55 @@ 0, 0, 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, 0 ], "river_downstream": [ @@ -8553,7 +8798,7 @@ 4, 9, 7, - 9, + 6, 3, 3, 9, @@ -8592,55 +8837,104 @@ 9, 2, 8, + 4, + 9, + 4, + 9, + 4, + 9, + 0, + 9, + 2, + 9, + 3, + 9, + 6, + 9, + 6, + 9, + 6, + 9, + 1, + 9, + 6, + 9, + 6, + 9, + 1, + 9, + 1, + 9, + 4, + 9, + 2, + 9, + 2, + 9, + 2, + 9, + 4, + 9, + 6, + 9, + 6, 9, 9, + 1, 9, + 3, 9, + 1, 9, + 2, 9, + 3, 9, + 7, 9, + 3, 9, + 3, 9, + 1, 9, + 5, 9, + 4, 9, + 6, 9, + 5, 9, + 6, 9, + 4, 9, + 4, 9, + 3, 9, + 3, 9, + 2, 9, + 2, 9, + 7, 9, + 2, 9, + 5, 9, + 3, 9, + 4, 9, + 3, 9, + 6, 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, - 9, + 3, 9 ], "river_seaward": [ @@ -9212,6 +9506,202 @@ 0, 0 ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], + [ + 0, + 0 + ], [ 0, 0