fix(simulation): PR #200 review round — cache sizing truth + endorheic assertion
Hoshe finding 1: dropping HydrologySample.elevation was PROVEN unsafe (TerrainAnalysis.elev_pct is a rank percentile, not raw elevation; HydrologyResult carries no elevation) — the copy stays, and the truth moves into the docs instead: gen_queue's TerrainAnalysisCache sizing comment corrected to real 512x256 working-grid numbers (~1.57 -> ~2.62 MB/entry, capacity-8 worst case ~21 MB), clone-on-hit cost documented, byte-safety proof recorded on HydrologySample itself. Arc<TerrainAnalysis> follow-up filed as T-1187. Hoshe finding 2: the endorheic-split test now asserts the bowl basin's BasinOutcome actually diverges (Endorheic at moisture 0, Overflow at 100, straddling ENDORHEIC_MOISTURE_CEILING=60) plus basin-count sanity — mutation-verified by stubbing is_endorheic and watching it fail. Doc/test-only round: goldens byte-unchanged, full suite 2114 green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5
@@ -98,6 +98,27 @@ pub struct TerrainAnalysis {
|
||||
/// `[0.0, 1.0]` normalized domain the raw heightmap and `sea_level` already
|
||||
/// share — so a bilinear sample of one is directly comparable to a bilinear
|
||||
/// sample of the other, no rescaling at the call site.
|
||||
///
|
||||
/// **Size + clone cost (PR #200 review, Hoshe finding 1):** two `Vec<f32>` at
|
||||
/// the real 512×256 working grid = ~1.05 MB/entry, added on top of
|
||||
/// `TerrainAnalysis`'s pre-existing ~1.57 MB of dense fields (~2.62 MB total,
|
||||
/// ×1.67 growth, not quite a doubling) — see the corrected sizing comment on
|
||||
/// `GenWorkItem::DeriveWindow` (`gen_queue.rs`) for the full accounting and
|
||||
/// the `TerrainAnalysisCache` cache-HIT clone-cost note (every hit
|
||||
/// deep-copies both these `Vec`s, not just the first miss/insert).
|
||||
///
|
||||
/// **`elevation` is a deliberate, provably-necessary redundant copy, not an
|
||||
/// oversight.** `TerrainAnalysis` has no OTHER field that retains the raw
|
||||
/// `[0,1]` heightmap: `elev_pct` is a RANK PERCENTILE (`rank(elev[i]) /
|
||||
/// (land_cell_count - 1)`, `compute_elev_percentile`'s own doc/impl) —
|
||||
/// mathematically a different quantity from absolute elevation, and NOT
|
||||
/// safe to compare against `filled` (two cells at different true elevations
|
||||
/// can share adjacent ranks; ocean cells are forced to `0.0` regardless of
|
||||
/// their real depth). `HydrologyResult` itself carries no elevation field
|
||||
/// either (`hydrology_equilibrium.rs`: `basins`, `filled_scaled`,
|
||||
/// `channel_depth_scaled`, `cliff_edge` — no `original`/`elevation` member).
|
||||
/// So there is no existing bit-identical grid this field could point at
|
||||
/// instead — carrying its own copy is the only byte-safe option today.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct HydrologySample {
|
||||
/// The original (unfilled) heightmap elevation, `[0.0, 1.0]`. Not stored
|
||||
|
||||
@@ -197,20 +197,58 @@ pub enum GenWorkItem {
|
||||
/// `drain_generation_completions` documents).
|
||||
///
|
||||
/// **TerrainAnalysis availability (T-1137 binding decision, with numbers;
|
||||
/// corrected 2026-07-21 per PR #187 review — Tyre C1):** `BodyWorldState`
|
||||
/// does NOT retain `TerrainAnalysis` after cascade completion (T-1044
|
||||
/// scoped its transient-carry fix to *within-cascade* reuse only —
|
||||
/// `cascade.rs` drops it once `DistrictProfile`+`RoadGraph` finish; see the
|
||||
/// doc on `CascadeSnapshot::terrain_analysis`). Caching it alongside every
|
||||
/// `BodyWorldStateCache` entry would cost ~2 MB × 50-body capacity ≈
|
||||
/// 100 MB of PERMANENT resident cost, paid by every cached body whether or
|
||||
/// not a window is ever requested for it — the exact D-203 budget concern
|
||||
/// T-1044's own ticket text guarded against. So `run_work_item` re-derives
|
||||
/// via `run_layer1` (matching `aliveness_probe`'s existing `--render`
|
||||
/// workaround) rather than persisting a field on `BodyWorldState` — but
|
||||
/// NOT unconditionally on every work item: the actual model is a small
|
||||
/// **per-body LRU** (`TerrainAnalysisCache`, capacity 8, ~16 MB worst
|
||||
/// case), consulted before every re-derive. The FIRST `DeriveWindow` on a
|
||||
/// corrected 2026-07-21 per PR #187 review — Tyre C1; RE-CORRECTED
|
||||
/// 2026-07-25 per PR #200 review — Hoshe finding 1, T-1184's hydrology
|
||||
/// field addition):** `BodyWorldState` does NOT retain `TerrainAnalysis`
|
||||
/// after cascade completion (T-1044 scoped its transient-carry fix to
|
||||
/// *within-cascade* reuse only — `cascade.rs` drops it once
|
||||
/// `DistrictProfile`+`RoadGraph` finish; see the doc on
|
||||
/// `CascadeSnapshot::terrain_analysis`). Caching it alongside every
|
||||
/// `BodyWorldStateCache` entry would cost **~2.62 MB × 50-body capacity ≈
|
||||
/// 131 MB** of PERMANENT resident cost (real per-entry figure below —
|
||||
/// this multiplier is unchanged, only the per-entry base moved), paid by
|
||||
/// every cached body whether or not a window is ever requested for it —
|
||||
/// the exact D-203 budget concern T-1044's own ticket text guarded
|
||||
/// against. So `run_work_item` re-derives via `run_layer1` (matching
|
||||
/// `aliveness_probe`'s existing `--render` workaround) rather than
|
||||
/// persisting a field on `BodyWorldState` — but NOT unconditionally on
|
||||
/// every work item: the actual model is a small **per-body LRU**
|
||||
/// (`TerrainAnalysisCache`, capacity 8), consulted before every
|
||||
/// re-derive.
|
||||
///
|
||||
/// **Real per-entry size (T-1184, corrected from the stale "~2 MB"
|
||||
/// figure the PR #187 pass computed before hydrology existed):** at the
|
||||
/// real `GRID_W × GRID_H = 512 × 256` working grid (131,072 cells, the
|
||||
/// SAME grid every `TerrainAnalysisCache` entry is built at — NOT the
|
||||
/// 1024×512 stored-heightmap-PNG resolution, D-202, which is downsampled
|
||||
/// away before `run_layer1` ever runs) — pre-T-1184 dense fields
|
||||
/// (`ocean_mask`/`lake_mask`: `Vec<bool>`, 1 B/cell each; `water_dist`:
|
||||
/// `Vec<u16>`, 2 B/cell; `slope_deg`/`elev_pct`: `Vec<f32>`, 4 B/cell
|
||||
/// each) sum to **~1.57 MB**. T-1184's `HydrologySample` (`elevation` +
|
||||
/// `filled`, both `Vec<f32>`, 4 B/cell) adds **~1.05 MB** — **not quite a
|
||||
/// doubling** (×1.67, not ×2), but a real, non-trivial per-entry growth:
|
||||
/// **post-T-1184 total ~2.62 MB/entry.** At capacity 8 that is **~21.0 MB
|
||||
/// worst case** (was ~12.6 MB pre-T-1184; the old "~16 MB" comment was
|
||||
/// itself a round-up of the pre-T-1184 figure, not a post-hydrology one).
|
||||
/// Both this cache and `BodyWorldStateCache`'s counterfactual above use
|
||||
/// the corrected ~2.62 MB base.
|
||||
///
|
||||
/// **Clone-on-HIT, not just insert (Hoshe finding 1, binding for the next
|
||||
/// sizing decision):** `get_or_derive`'s cache-hit branch
|
||||
/// (`return (l1.clone(), ta.clone())`) deep-copies the FULL
|
||||
/// `TerrainAnalysis` — including both new `HydrologySample` `Vec<f32>`
|
||||
/// fields — on EVERY hit, not merely on the first miss/insert. A
|
||||
/// pan-heavy session hitting the same body's cache entry repeatedly pays
|
||||
/// the ~2.62 MB clone cost per `DeriveWindow` work item, not once per
|
||||
/// body. This was already true pre-T-1184 for the smaller ~1.57 MB
|
||||
/// struct; T-1184 makes the per-hit clone cost ~1.67× larger, not a new
|
||||
/// category of cost. **Not fixed this round** (an `Arc<TerrainAnalysis>`
|
||||
/// refactor — sharing one heap allocation across hits instead of
|
||||
/// deep-copying — is the obvious next step if hit-heavy clone cost ever
|
||||
/// shows up in a profile, but is out of scope for T-1184; flagged for a
|
||||
/// follow-up ticket rather than done speculatively here).
|
||||
///
|
||||
/// The FIRST `DeriveWindow` on a
|
||||
/// body pays the full ~45 ms `run_layer1` cost and populates that body's
|
||||
/// cache entry; EVERY SUBSEQUENT window on the SAME body (any `center`/`n`,
|
||||
/// not just an exact repeat — that narrower case is what
|
||||
|
||||
@@ -505,5 +505,68 @@ mod tests {
|
||||
"lake extent (filled_scaled) must be moisture-independent — only \
|
||||
the endorheic/overflow split may vary with moisture_q"
|
||||
);
|
||||
|
||||
// PR #200 review, Hoshe finding 2: the assertion above only proves
|
||||
// EXTENT is moisture-independent — it says nothing about whether the
|
||||
// endorheic/overflow split (this test's OWN namesake claim) actually
|
||||
// moves at all. `TerrainAnalysis.hydrology` doesn't carry
|
||||
// `BasinOutcome` (only the continuous `elevation`/`filled` fields
|
||||
// `derive_morphology_zone`'s lake gate needs — see `HydrologySample`'s
|
||||
// doc), so re-solve directly via `hydrology_equilibrium::solve` to
|
||||
// reach `Basin::outcome`. `moisture_q: 0` and `moisture_q: 100`
|
||||
// straddle `ENDORHEIC_MOISTURE_CEILING = 60` on either side, so the
|
||||
// SAME basin (same elevation geometry, same `bowl_hm`) must classify
|
||||
// differently — a regression that makes the endorheic split dead code
|
||||
// (e.g. `is_endorheic` always returning `false`, or `moisture_q` never
|
||||
// threading through to it) would leave the `filled` assertion above
|
||||
// green but SHOULD fail here.
|
||||
let dry_result = crate::atlas::hydrology_equilibrium::solve(
|
||||
&h.data,
|
||||
h.width,
|
||||
h.height,
|
||||
h.sea_level,
|
||||
crate::atlas::hydrology_equilibrium::ClimateInputs { moisture_q: 0 },
|
||||
);
|
||||
let wet_result = crate::atlas::hydrology_equilibrium::solve(
|
||||
&h.data,
|
||||
h.width,
|
||||
h.height,
|
||||
h.sea_level,
|
||||
crate::atlas::hydrology_equilibrium::ClimateInputs { moisture_q: 100 },
|
||||
);
|
||||
assert_eq!(
|
||||
dry_result.basins.len(),
|
||||
1,
|
||||
"fixture sanity: bowl_hm(64, 32, ...) must produce exactly one basin"
|
||||
);
|
||||
assert_eq!(wet_result.basins.len(), 1, "fixture sanity: same on the wet solve");
|
||||
let dry_outcome = &dry_result.basins[0].outcome;
|
||||
let wet_outcome = &wet_result.basins[0].outcome;
|
||||
assert_ne!(
|
||||
dry_outcome, wet_outcome,
|
||||
"the SAME basin (identical elevation geometry) must classify \
|
||||
differently at moisture_q=0 vs moisture_q=100 — these straddle \
|
||||
ENDORHEIC_MOISTURE_CEILING=60, so a regression making the \
|
||||
endorheic/overflow split dead code would collapse both to the \
|
||||
same outcome here"
|
||||
);
|
||||
// Non-vacuous on BOTH halves of this test's own name: dry actually
|
||||
// IS Endorheic, wet actually IS Overflow — not just "different from
|
||||
// each other" (which alone couldn't rule out both being some other
|
||||
// unrelated pair of values).
|
||||
assert!(
|
||||
matches!(
|
||||
dry_outcome,
|
||||
crate::atlas::hydrology_equilibrium::BasinOutcome::Endorheic { .. }
|
||||
),
|
||||
"moisture_q=0 (well below the ceiling) must classify Endorheic; got {dry_outcome:?}"
|
||||
);
|
||||
assert!(
|
||||
matches!(
|
||||
wet_outcome,
|
||||
crate::atlas::hydrology_equilibrium::BasinOutcome::Overflow { .. }
|
||||
),
|
||||
"moisture_q=100 (well above the ceiling) must classify Overflow; got {wet_outcome:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user