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

feat(simulation): cache TerrainAnalysis + basin_direction from D8 thalweg (T-1044, T-1047) #170

Closed
jpmschweitzer wants to merge 0 commits from layer1-cache-basin-direction into main
pull from: layer1-cache-basin-direction
merge into: :main
Conversation 2 Commits - Files Changed -
+421 -139
jpmschweitzer commented 2026-06-17 09:48:01 +02:00
Owner
Copy Link
Copy Source

Summary

Two composed Layer-1-pass changes.

T-1044 — drop the redundant per-body drainage re-run. run_layer1 now returns TerrainAnalysis, carried transiently on CascadeSnapshot (dropped after the district + road-graph passes consume it; not persisted on the LRU-cached state, per the D-203/T-1048 per-entry-size concern). The PERF/TODO(T-1044) re-run of drainage::analyze + TerrainAnalysis::analyze in the cascade is eliminated.

T-1047 — derive basin_direction from the real D8 thalweg. run_layer1 aggregates a per-district dominant D8 direction from the live fdir grid (carried transiently on DrainageResult), threaded via Layer1Output.district_basin_dirs → derive_all_districts → DistrictProfile.basin_direction; derive_chunk_context reads it directly. Removed the false derive_basin_direction (it branched on ocean_fraction_q then read seed bits, despite a doc comment claiming an elev_q/slope_q D8 proxy) and corrected the module contract. D-239 §8 (respect the D8 thalweg) is now actually honoured. Threaded through DistrictProfile (the 2 km carrier), not the post-T-1077 RegionProfile.

Verification

  • cargo test: 1559 passed, 0 failed (full suite). clippy --all-targets -D warnings clean; fmt clean.
  • Golden byte-identical — district_basin_dirs is #[serde(skip)] (transient), so the serialized Layer1Output golden is unchanged.
  • No generator sources / data touched → no systems.db regen.

🤖 Generated with Claude Code

## Summary Two composed Layer-1-pass changes. **T-1044 — drop the redundant per-body drainage re-run.** `run_layer1` now returns `TerrainAnalysis`, carried transiently on `CascadeSnapshot` (dropped after the district + road-graph passes consume it; not persisted on the LRU-cached state, per the D-203/T-1048 per-entry-size concern). The `PERF/TODO(T-1044)` re-run of `drainage::analyze` + `TerrainAnalysis::analyze` in the cascade is eliminated. **T-1047 — derive `basin_direction` from the real D8 thalweg.** `run_layer1` aggregates a per-district dominant D8 direction from the live `fdir` grid (carried transiently on `DrainageResult`), threaded via `Layer1Output.district_basin_dirs` → `derive_all_districts` → `DistrictProfile.basin_direction`; `derive_chunk_context` reads it directly. Removed the false `derive_basin_direction` (it branched on `ocean_fraction_q` then read seed bits, despite a doc comment claiming an elev_q/slope_q D8 proxy) and corrected the module contract. D-239 §8 (respect the D8 thalweg) is now actually honoured. Threaded through `DistrictProfile` (the 2 km carrier), not the post-T-1077 `RegionProfile`. ## Verification - `cargo test`: **1559 passed, 0 failed** (full suite). clippy `--all-targets -D warnings` clean; fmt clean. - Golden byte-identical — `district_basin_dirs` is `#[serde(skip)]` (transient), so the serialized Layer1Output golden is unchanged. - No generator sources / data touched → no systems.db regen. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 1 commit 2026-06-17 09:48:02 +02:00
feat(simulation): cache TerrainAnalysis + derive basin_direction from the D8 thalweg (T-1044, T-1047) 0263b68df9 
T-1044: run_layer1 now returns TerrainAnalysis (carried transiently on
CascadeSnapshot, dropped after the district + road-graph passes), eliminating the
redundant per-body drainage::analyze + TerrainAnalysis::analyze re-run flagged by
PERF/TODO(T-1044). Not persisted on the LRU-cached state (D-203/T-1048 size concern).

T-1047: basin_direction is now derived from the real D8 thalweg. run_layer1
aggregates a per-district dominant D8 direction from the live fdir grid (carried
transiently on DrainageResult), threaded via Layer1Output.district_basin_dirs ->
derive_all_districts -> DistrictProfile.basin_direction; derive_chunk_context reads
it directly. Removed the false derive_basin_direction (it branched on ocean_fraction_q
then read seed bits despite a doc comment claiming an elev_q/slope_q D8 proxy) +
corrected the module contract. D-239 §8 (D8 thalweg) now actually honoured.

1559 tests pass; golden byte-identical (district_basin_dirs is #[serde(skip)], transient).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jpmschweitzer added 1 commit 2026-06-17 09:48:23 +02:00
chore(meta): T-1044/T-1047 -> review (PR #170) 7d45d78674 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jpmschweitzer commented 2026-06-17 10:05:05 +02:00
Author
Owner
Copy Link
Copy Source

PR #170 Review — layer1-cache-basin-direction → main (code)

Reviewers: Hoshe (code quality) + Tyre (architecture), both on the branch worktree.

Verdict: CHANGES REQUESTED (Hoshe 2 findings; Tyre approves).

Tyre (Architecture) — APPROVE

Verified and clean: D-203 — TerrainAnalysis (~2 MB) carried transiently on CascadeSnapshot.terrain_analysis, dropped after both consumers + discarded in into_body_world_state; no leak into BodyWorldState; DrainageResult.fdir (~131 KB) lives/dies inside run_layer1. D-239 §8 — basin_direction now from the real D8 fdir (the seed-bit fake is gone); the per-district vote/diagonal-fold/N>S>E>W tie-break matches the flow_direction D8 priority. Carrier — DistrictProfile (the 2 km carrier derive_chunk_context receives) is correct post-T-1077; RegionProfile would be too coarse. serde — #[serde(skip)] (district_basin_dirs, terrain_analysis) + #[serde(default)] (basin_direction) correct; golden byte-identity follows. D-010 — integer math, BTreeMap order, dedup of BasinDirection to scale.rs all clean.

Hoshe (Code Quality) — REQUEST_CHANGES

# Sev File / location Issue
H1 medium cascade.rs district_profile_layer_runs_and_is_deterministic; district_profile.rs derive_all_districts tests The Some(basin_dirs) threading path is untested — derive_all_districts is only exercised with None, and the cascade test asserts river/tectonic/glaciation but never basin_direction. The chain run_layer1 → district_basin_dirs → derive_all_districts → DistrictProfile.basin_direction has no regression guard. Add an assertion that ≥1 district carries a non-North direction (the slope heightmap drains S/E), or a unit test feeding a synthetic basin_dirs map.
H2 low district_profile.rs derive_district (~:1291) The on-demand North-default comment stops at "defaults to North" — clarify it's an accepted limitation of the on-demand path (the batch derive_all_districts path carries the true D8 direction), so the next caller doesn't mistake North for a computed value.

Both will be addressed (a test + a comment).

## PR #170 Review — `layer1-cache-basin-direction` → main (code) Reviewers: **Hoshe** (code quality) + **Tyre** (architecture), both on the branch worktree. **Verdict: CHANGES REQUESTED** (Hoshe 2 findings; Tyre approves). ### Tyre (Architecture) — APPROVE Verified and clean: **D-203** — TerrainAnalysis (~2 MB) carried transiently on `CascadeSnapshot.terrain_analysis`, dropped after both consumers + discarded in `into_body_world_state`; no leak into `BodyWorldState`; `DrainageResult.fdir` (~131 KB) lives/dies inside `run_layer1`. **D-239 §8** — `basin_direction` now from the real D8 `fdir` (the seed-bit fake is gone); the per-district vote/diagonal-fold/`N>S>E>W` tie-break matches the `flow_direction` D8 priority. **Carrier** — `DistrictProfile` (the 2 km carrier `derive_chunk_context` receives) is correct post-T-1077; `RegionProfile` would be too coarse. **serde** — `#[serde(skip)]` (district_basin_dirs, terrain_analysis) + `#[serde(default)]` (basin_direction) correct; golden byte-identity follows. **D-010** — integer math, BTreeMap order, dedup of `BasinDirection` to scale.rs all clean. ### Hoshe (Code Quality) — REQUEST_CHANGES | # | Sev | File / location | Issue | |---|-----|-----------------|-------| | H1 | medium | `cascade.rs` `district_profile_layer_runs_and_is_deterministic`; `district_profile.rs` `derive_all_districts` tests | The `Some(basin_dirs)` threading path is untested — `derive_all_districts` is only exercised with `None`, and the cascade test asserts river/tectonic/glaciation but never `basin_direction`. The chain run_layer1 → `district_basin_dirs` → `derive_all_districts` → `DistrictProfile.basin_direction` has no regression guard. Add an assertion that ≥1 district carries a non-North direction (the slope heightmap drains S/E), or a unit test feeding a synthetic `basin_dirs` map. | | H2 | low | `district_profile.rs` `derive_district` (~:1291) | The on-demand North-default comment stops at "defaults to North" — clarify it's an accepted limitation of the on-demand path (the batch `derive_all_districts` path carries the true D8 direction), so the next caller doesn't mistake North for a computed value. | Both will be addressed (a test + a comment).
jpmschweitzer added 1 commit 2026-06-17 10:12:13 +02:00
test(simulation): cover the Some(basin_dirs) threading + clarify on-demand default (T-1047) 7c09f23b9a 
Addresses PR #170 review (Hoshe H1/H2).

H1: the Some(basin_dirs) path through derive_all_districts was untested (tests
only used None). Added derive_all_districts_threads_supplied_basin_directions
(synthetic per-district directions propagate to DistrictProfile.basin_direction;
unmapped districts fall back to North), and added basin_direction to the cascade
determinism loop so the full run_layer1->district chain is guarded.

H2: clarified the derive_district on-demand comment — the North default is an
accepted limitation (a fallback, not a computed value); the batch path threads
the true D8 direction.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jpmschweitzer commented 2026-06-17 10:12:54 +02:00
Author
Owner
Copy Link
Copy Source

Review findings addressed — 7c09f23b9

Both Hoshe findings resolved. Gate re-run green: cargo test 1560 passed / 0 failed (+1 new), clippy --all-targets -D warnings clean, fmt clean.

# Finding Resolution
H1 Some(basin_dirs) threading path untested Added derive_all_districts_threads_supplied_basin_directions — feeds a synthetic per-district direction map and asserts the directions propagate to DistrictProfile.basin_direction, with unmapped districts falling back to North. Also added basin_direction to the cascade determinism loop (district_profile_layer_runs_and_is_deterministic), guarding the full run_layer1 → district_basin_dirs → DistrictProfile chain. (Note: test_heightmap drains toward NW — predominantly North — so a "non-North on this heightmap" assertion would be unreliable; the synthetic-map test is the robust guard for the Some-path.)
H2 On-demand North-default comment unclear Rewrote the derive_district comment to flag the North default as an accepted limitation (a fallback, not a computed value — the D8 thalweg is only available to the batch path; production voxel-gen uses the batch path, which threads the true direction). Did not cite T-1046 (merged).

Tyre's pass had no comments. Ready to merge.

## Review findings addressed — `7c09f23b9` Both Hoshe findings resolved. Gate re-run green: `cargo test` **1560 passed / 0 failed** (+1 new), clippy `--all-targets -D warnings` clean, fmt clean. | # | Finding | Resolution | |---|---------|------------| | H1 | `Some(basin_dirs)` threading path untested | Added `derive_all_districts_threads_supplied_basin_directions` — feeds a synthetic per-district direction map and asserts the directions propagate to `DistrictProfile.basin_direction`, with unmapped districts falling back to North. Also added `basin_direction` to the cascade determinism loop (`district_profile_layer_runs_and_is_deterministic`), guarding the full run_layer1 → `district_basin_dirs` → DistrictProfile chain. *(Note: `test_heightmap` drains toward NW — predominantly North — so a "non-North on this heightmap" assertion would be unreliable; the synthetic-map test is the robust guard for the Some-path.)* | | H2 | On-demand North-default comment unclear | Rewrote the `derive_district` comment to flag the North default as an **accepted limitation** (a fallback, not a computed value — the D8 thalweg is only available to the batch path; production voxel-gen uses the batch path, which threads the true direction). Did not cite T-1046 (merged). | Tyre's pass had no comments. Ready to merge.
jpmschweitzer closed this pull request 2026-06-17 10:16:21 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#170