From 9c5aa79852c89e0164ca915dad8f336ef211297c Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 26 Jul 2026 14:28:40 +0200 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20PR=20#216=20review=20fixes?= =?UTF-8?q?=20=E2=80=94=20footprint-wins=20enforcement,=20discriminating?= =?UTF-8?q?=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 2 became a real code fix: interstitial_fill_into now enforces the footprint-wins conflict rule (column_has_voxel range probe) — the FilledChunk absence contract was previously a documented promise the code didn't keep against conflicting inputs; pinned by a fully- overlapping-leaf test asserting per-tile resolution. The tautological overlap test replaced with a real rects_overlap() geometric helper (itself sanity-tested) applied pairwise. The degenerate-setback fix is now a standalone pure fn shrink_lot_or_interstitial with four boundary tests — honestly documented as unreachable from live traffic today (every min_lot exceeds every setback), a robustness guard for future recalibration. Both sub-chunk clip tests now reconstruct the full 32-tile union across the seam (disjoint + complete), including the pre-existing footprint clip test (leave-cleaner). Brief's ChunkLayout claim tightened to the verified no-production-consumer statement. Co-Authored-By: Claude Fable 5 --- docs/architecture/interstitial-fill-t1098.md | 8 +- server/src/atlas/shell.rs | 169 +++++++++++++++++- server/src/atlas/skeleton_gen.rs | 177 +++++++++++++++++-- 3 files changed, 332 insertions(+), 22 deletions(-) diff --git a/docs/architecture/interstitial-fill-t1098.md b/docs/architecture/interstitial-fill-t1098.md index b5d17fdda..fa2e981f3 100644 --- a/docs/architecture/interstitial-fill-t1098.md +++ b/docs/architecture/interstitial-fill-t1098.md @@ -77,8 +77,12 @@ by *construction*, with no residual "what's left over" category to guess at: (`skeleton_gen.rs:872-887`, `// Perimeter street margin`) — it is claimed by the future street-network layer (D-234), not by this ticket. `ChunkLayout` (`spacing`/`offset`/`rotation_steps`, `generator.rs:103`) is the *only* other - street-adjacent data on `BlockSkeleton` today, and it is an unconsumed stub - (grep confirms no reader) reserved for that same future street-network + street-adjacent data on `BlockSkeleton` today; grep confirms it has **no + production consumer** (its only reads are `#[cfg(test)]` assertions in + `skeleton_gen.rs` — `skeleton_has_streets_and_local_lattice` — checking that + the plan-time writer set it; the writer itself, `generate_quarter_skeleton`'s + layout-mode application, populates it but nothing downstream of plan time + consumes it yet). It is reserved for that same future street-network step — not something this ticket needs to or should resolve. Every block-local tile is covered by exactly one of these three at generation diff --git a/server/src/atlas/shell.rs b/server/src/atlas/shell.rs index 2adbfbb55..037c8511e 100644 --- a/server/src/atlas/shell.rs +++ b/server/src/atlas/shell.rs @@ -136,6 +136,17 @@ pub type GroundTilePos = (u8, u8); /// "unknown" — the exhaustiveness lives in the block-plan-time classification /// (`atlas::skeleton_gen::subdivide_block_footprints`'s `BlockSubdivision`), and this /// struct's contract must not reintroduce ambiguity one level up. +/// +/// **Conflicting-input tie-break (PR #216 review finding 2).** The three-way +/// partition above is guaranteed by `subdivide_block_footprints`'s construction on +/// well-formed input, but `fill_chunk`'s `block_tags`/`interstitial_leaves` are two +/// independently-supplied slices (T-987 purity: this layer trusts, not re-validates, +/// its inputs) — so a caller COULD in principle pass a leaf that geometrically +/// overlaps a footprint. The defined outcome for that case: **footprint wins.** +/// `interstitial_fill_into` (`atlas::shell`) explicitly skips stamping any tile that +/// already has a `voxels` entry, so rule 1/2 above hold even under a conflicting +/// input, never silently double-populating both maps for the same tile. See +/// `interstitial_fill_into`'s doc for the mechanism. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] pub struct FilledChunk { /// Stable id of the quarter this chunk belongs to (D-194/D-230). @@ -223,7 +234,7 @@ pub fn fill_chunk( let interstitial_type = resolve_interstitial_type(block_character); for leaf in interstitial_leaves { - interstitial_fill_into(&mut interstitial, leaf, interstitial_type, window); + interstitial_fill_into(&mut interstitial, &voxels, leaf, interstitial_type, window); } FilledChunk { @@ -371,8 +382,30 @@ fn is_perimeter(footprint: &TileRect, tx: i32, ty: i32) -> bool { /// `leaf ∩ window` gets `interstitial_type` — the same value for the whole leaf, /// since character is a frozen block-level fact (`BlockFillContext`), not a /// per-tile roll. +/// +/// **Conflict rule (PR #216 review finding 2 — the contract this doc comment +/// and the tests both pin): footprint wins.** `subdivide_block_footprints` +/// already guarantees footprints and interstitial leaves are geometrically +/// disjoint by construction (§ that function's doc), so this should never +/// fire on well-formed input — but `fill_chunk`'s two inputs +/// (`block_tags`/`interstitial_leaves`) are independently supplied by the +/// caller (T-987 purity: this layer trusts its inputs, it does not +/// re-validate the upstream invariant), so a conflicting pair is reachable in +/// principle (a hand-built work item, a future caller bug, a test fixture). +/// Rather than leaving that case undefined, this function explicitly SKIPS +/// stamping any chunk-local `(x, y)` that already has a `voxels` entry at any +/// `z` — a footprint tile never gains an `interstitial` entry, preserving +/// `FilledChunk`'s absence contract ("footprint-covered ⟹ never present in +/// `interstitial`") even when the caller's inputs conflict, instead of a +/// `debug_assert!` that would panic on a currently-reachable-in-principle +/// input. `voxels` is `BTreeMap<(u8,u8,i32), _>`, so "any z at this (x,y)" is +/// answered with a cheap key-range probe (`(x,y,i32::MIN)..(x,y,i32::MAX)` +/// would miss `i32::MAX` itself since ranges are half-open on `end`, so the +/// probe uses `(x, y+1, i32::MIN)` as the open upper bound instead — see +/// below). fn interstitial_fill_into( interstitial: &mut BTreeMap, + voxels: &BTreeMap, leaf: &TileRect, interstitial_type: InterstitialType, window: (i32, i32, i32, i32), @@ -396,11 +429,36 @@ fn interstitial_fill_into( for ty in lo_y..hi_y { let cx = (tx - win_lo_x) as u8; let cy = (ty - win_lo_y) as u8; + if column_has_voxel(voxels, cx, cy) { + continue; // footprint wins — see this fn's doc, conflict rule + } interstitial.insert((cx, cy), interstitial_type); } } } +/// Whether `voxels` has ANY entry (any `z`) at chunk-local `(cx, cy)` — the +/// footprint-occupancy probe `interstitial_fill_into`'s conflict rule needs. +/// `BTreeMap` range query over the `(x, y, z)` key ordering: fixing `x == cx` +/// and `y == cy` leaves `z` as the only free component, so the range +/// `(cx, cy, i32::MIN)..(cx, cy + 1, i32::MIN)` (upper bound exclusive) covers +/// exactly the keys with that `(cx, cy)`, regardless of `z`'s sign (D-110 +/// basements are negative) — this is why the upper bound bumps `cy` rather +/// than trying to express `z <= i32::MAX` inclusively on a half-open range. +fn column_has_voxel(voxels: &BTreeMap, cx: u8, cy: u8) -> bool { + let lo = (cx, cy, i32::MIN); + let hi = (cx, cy.saturating_add(1), i32::MIN); + if cy == u8::MAX { + // cy+1 would wrap to 0, which is BELOW (cx, cy, _) in key order and + // would make the range empty/backwards — u8::MAX is the last valid + // chunk-local coordinate anyway (CHUNK_M == 64 keeps cy in 0..64 in + // practice), so fall back to an explicit any() over the tail instead + // of constructing a wrapping range. + return voxels.keys().any(|&(x, y, _)| x == cx && y == cy); + } + voxels.range(lo..hi).next().is_some() +} + /// Compile-time sanity: a chunk is 64 voxels on a side, so chunk-local indices fit a u8. const _: () = assert!(VOXELS_PER_CHUNK == CHUNK_M); const _: () = assert!(CHUNK_M <= u8::MAX as i32 + 1); @@ -586,6 +644,36 @@ mod tests { // Right sub-chunk holds block-local x 64..68 → chunk-local x 0..4. assert_ne!(right.voxel_count(), 0); assert!(right.voxels.keys().all(|(x, _, _)| (0..4).contains(x))); + + // PR #216 review finding 4: in-range keys alone don't prove nothing was + // dropped or double-counted at the seam — reconstruct the footprint's + // full 8×4 = 32 block-local tile-columns from the union of both + // sides' touched (x,y) columns (translating each side's chunk-local + // keys back to block-local: left's window origin is 0 so its keys + // are already block-local; right's chunk-local x needs +64), and + // assert that union has exactly 32 distinct columns with zero + // overlap between the two sides. + let left_cols: std::collections::BTreeSet<(u8, u8)> = + left.voxels.keys().map(|&(x, y, _)| (x, y)).collect(); + let right_cols: std::collections::BTreeSet<(u16, u8)> = right + .voxels + .keys() + .map(|&(x, y, _)| (x as u16 + 64, y)) + .collect(); + let left_cols_wide: std::collections::BTreeSet<(u16, u8)> = + left_cols.iter().map(|&(x, y)| (x as u16, y)).collect(); + assert!( + left_cols_wide.is_disjoint(&right_cols), + "left and right sub-chunks must not touch the same block-local column" + ); + let union: std::collections::BTreeSet<(u16, u8)> = + left_cols_wide.union(&right_cols).copied().collect(); + assert_eq!( + union.len(), + 8 * 4, + "left ∪ right must reconstruct all 32 columns of the 8×4 footprint, \ + none dropped and none double-counted at the sub-chunk seam" + ); } #[test] @@ -762,9 +850,11 @@ mod tests { } #[test] - fn footprint_and_interstitial_tiles_are_mutually_exclusive() { + fn footprint_and_interstitial_tiles_are_mutually_exclusive_when_disjoint() { // A building footprint and an adjacent-but-disjoint interstitial leaf - // in the same chunk: no chunk-local tile appears in both maps. + // in the same chunk: no chunk-local tile appears in both maps. (This + // fixture is disjoint BY CONSTRUCTION — see the companion test below + // for the case that actually exercises the conflict rule.) let building = tag((0, 0), (4, 4), 0, 1); let leaf = TileRect::new(10, 10, 4, 4); let ctx = BlockFillContext { @@ -788,6 +878,52 @@ mod tests { } } + #[test] + fn conflicting_footprint_and_interstitial_leaf_resolves_footprint_wins() { + // PR #216 review finding 2: feed inputs that ACTUALLY conflict — a + // leaf whose tile-space intersects a footprint's — and assert the + // defined outcome. `fill_chunk`'s two inputs are independently + // supplied (T-987 purity: no cross-validation against + // `subdivide_block_footprints`'s upstream disjointness guarantee), so + // this is reachable in principle even though real callers won't hit + // it. The pinned rule (see `FilledChunk`'s and + // `interstitial_fill_into`'s doc comments): **footprint wins** — the + // overlapping tiles keep their `voxels`/`surface_material` entries + // and gain NO `interstitial` entry, preserving the absence contract + // rather than leaving the tile double-classified. + let building = tag((2, 2), (6, 6), 0, 1); // footprint covers x/y 2..8 + let leaf = TileRect::new(0, 0, 10, 10); // leaf fully overlaps the footprint + let ctx = BlockFillContext { + interstitial_character: InterstitialCharacter::OpenSpace, + setback_tier: SetbackTier::Standard, // → Garden if it were stamped + }; + let fc = fill_chunk(1, (0, 0), (0, 0), &[building], &[leaf], ctx); + + // Every footprint tile: voxels entry present, interstitial entry ABSENT. + for tx in 2..8u8 { + for ty in 2..8u8 { + assert!( + fc.voxels.keys().any(|&(x, y, _)| x == tx && y == ty), + "footprint tile ({tx},{ty}) must still have a voxels entry" + ); + assert_eq!( + fc.interstitial.get(&(tx, ty)), + None, + "footprint wins: ({tx},{ty}) is inside both the footprint and the \ + conflicting leaf, so it must NOT gain an interstitial entry" + ); + } + } + // The leaf's non-overlapping remainder (e.g. (0,0), outside the 2..8 + // footprint box) still resolves normally — the conflict rule is + // per-tile, not "drop the whole leaf". + assert_eq!( + fc.interstitial.get(&(0, 0)), + Some(&InterstitialType::Garden), + "the leaf's non-conflicting tiles must still resolve normally" + ); + } + #[test] fn interstitial_leaf_clipped_to_sub_chunk() { // A leaf spanning the block's sub-chunk seam at x=64: only the portion @@ -804,6 +940,33 @@ mod tests { assert!(left.interstitial.keys().all(|(x, _)| (60..64).contains(x))); assert!(!right.interstitial.is_empty()); assert!(right.interstitial.keys().all(|(x, _)| (0..4).contains(x))); + + // PR #216 review finding 4: reconstruct the leaf's full 8×4 = 32 + // block-local tile-columns from left ∪ right (translating right's + // chunk-local x by +64 back to block-local), asserting exactly 32 + // distinct columns and zero overlap — not just in-range keys. + let left_cols: std::collections::BTreeSet<(u16, u8)> = left + .interstitial + .keys() + .map(|&(x, y)| (x as u16, y)) + .collect(); + let right_cols: std::collections::BTreeSet<(u16, u8)> = right + .interstitial + .keys() + .map(|&(x, y)| (x as u16 + 64, y)) + .collect(); + assert!( + left_cols.is_disjoint(&right_cols), + "left and right sub-chunks must not stamp the same block-local column" + ); + let union: std::collections::BTreeSet<(u16, u8)> = + left_cols.union(&right_cols).copied().collect(); + assert_eq!( + union.len(), + 8 * 4, + "left ∪ right must reconstruct all 32 columns of the 8×4 leaf, \ + none dropped and none double-counted at the sub-chunk seam" + ); } #[test] diff --git a/server/src/atlas/skeleton_gen.rs b/server/src/atlas/skeleton_gen.rs index 181657236..f3b1dab87 100644 --- a/server/src/atlas/skeleton_gen.rs +++ b/server/src/atlas/skeleton_gen.rs @@ -857,6 +857,36 @@ struct BlockSubdivision { interstitial: Vec, } +/// The result of shrinking a BSP lot that WON its coverage roll by `setback` +/// (T-1098, PR #216 review finding 3). +enum LotOutcome { + /// Setback shrink left a valid (≥1×1) building footprint. + Footprint(TileRect), + /// Degenerate shrink — `setback` consumed the whole lot on at least one + /// axis (`w < 1 || h < 1` post-shrink). The lot still reads as ground, + /// not a building, so it falls through to interstitial rather than + /// vanishing from both lists. + Interstitial(TileRect), +} + +/// Shrink a coverage-winning BSP `lot` by `setback` on both axes (`saturating_sub`, +/// so a lot no bigger than `setback` floors at 0, never wraps). Pure, unit-testable +/// in isolation from `bsp`'s leaf-size behavior — this is the arithmetic PR #216 +/// review finding 3 asked to be pinned directly, since realistic `bsp` output never +/// exercises the degenerate branch (every `min_lot` floor in `subdivide_block_footprints` +/// is comfortably above every `setback` value, so a leaf this small cannot occur via +/// the normal call path; the branch exists for robustness against a future +/// min_lot/setback recalibration, not today's live data). +fn shrink_lot_or_interstitial(lot: &TileRect, setback: u8) -> LotOutcome { + let w = lot.size.0.saturating_sub(setback); + let h = lot.size.1.saturating_sub(setback); + if w >= 1 && h >= 1 { + LotOutcome::Footprint(TileRect::new(lot.origin.0, lot.origin.1, w, h)) + } else { + LotOutcome::Interstitial(lot.clone()) + } +} + /// Subdivide one block into building footprints (D-220/D-229/D-233/D-234). /// /// Lot size + setback scale with `density_pct` (Frontier → few big lots, wide @@ -924,15 +954,9 @@ fn subdivide_block_footprints( interstitial.push(lot.clone()); // T-1098: the whole leaf, unshrunk continue; } - let w = lot.size.0.saturating_sub(setback); - let h = lot.size.1.saturating_sub(setback); - if w >= 1 && h >= 1 { - footprints.push(TileRect::new(lot.origin.0, lot.origin.1, w, h)); - } else { - // Degenerate shrink (setback ate the whole lot) — the lot still - // reads as ground, not a building; T-1098 keeps it interstitial - // rather than silently vanishing from both lists. - interstitial.push(lot.clone()); + match shrink_lot_or_interstitial(lot, setback) { + LotOutcome::Footprint(fp) => footprints.push(fp), + LotOutcome::Interstitial(leaf) => interstitial.push(leaf), } } BlockSubdivision { @@ -1851,13 +1875,57 @@ mod tests { } } + /// Real geometric-overlap test over tile-space coordinate ranges (not + /// struct equality) — two axis-aligned `TileRect`s overlap iff their x + /// ranges AND y ranges both intersect. + fn rects_overlap(a: &TileRect, b: &TileRect) -> bool { + let a_lo_x = a.origin.0 as i32; + let a_hi_x = a_lo_x + a.size.0.max(1) as i32; // exclusive + let a_lo_y = a.origin.1 as i32; + let a_hi_y = a_lo_y + a.size.1.max(1) as i32; + + let b_lo_x = b.origin.0 as i32; + let b_hi_x = b_lo_x + b.size.0.max(1) as i32; + let b_lo_y = b.origin.1 as i32; + let b_hi_y = b_lo_y + b.size.1.max(1) as i32; + + a_lo_x < b_hi_x && b_lo_x < a_hi_x && a_lo_y < b_hi_y && b_lo_y < a_hi_y + } + + #[test] + fn rects_overlap_agrees_with_known_cases() { + // Sanity-check the helper itself before trusting it in the real test: + // identical rects overlap, disjoint rects don't, and a rect that is + // fully inside another overlaps it. + let a = TileRect::new(0, 0, 10, 10); + assert!(rects_overlap(&a, &a), "a rect overlaps itself"); + let disjoint = TileRect::new(20, 20, 5, 5); + assert!( + !rects_overlap(&a, &disjoint), + "disjoint rects must not overlap" + ); + let touching_edge = TileRect::new(10, 0, 5, 5); // shares the x=10 boundary, exclusive + assert!( + !rects_overlap(&a, &touching_edge), + "rects that only touch at an edge (hi == lo) must not overlap" + ); + let inside = TileRect::new(2, 2, 3, 3); + assert!( + rects_overlap(&a, &inside), + "a fully-contained rect must overlap" + ); + } + #[test] fn footprints_and_interstitial_leaves_do_not_overlap() { // Every BSP leaf becomes exactly one of a (shrunk) footprint or an - // (unshrunk) interstitial rect — never both, never neither is - // reflected in this test by checking no footprint origin coincides - // with an interstitial leaf's origin (BSP leaves are disjoint, so - // distinct origins is a sufficient proxy for "different leaves"). + // (unshrunk) interstitial rect — the two lists must be geometrically + // disjoint (no shared tile-space), checked pairwise over every + // footprint x interstitial-leaf pair by actual coordinate-range + // intersection (T-1098 PR #216 review finding 1: struct equality is a + // tautology here, since footprints are always setback-shrunk while + // leaves keep the full BSP-leaf size, so no footprint can ever be + // `==` to a leaf regardless of whether the fill is correct). let sub = subdivide_block_footprints( 50, &BulkClass::NonPhysical, @@ -1865,11 +1933,86 @@ mod tests { None, SeedChain::root(3), ); + assert!( + !sub.footprints.is_empty() && !sub.interstitial.is_empty(), + "fixture must exercise both lists for this assertion to be meaningful" + ); for fp in &sub.footprints { - assert!( - !sub.interstitial.contains(fp), - "a footprint rect must not also appear as an interstitial leaf" - ); + for leaf in &sub.interstitial { + assert!( + !rects_overlap(fp, leaf), + "footprint {fp:?} must not geometrically overlap interstitial leaf {leaf:?}" + ); + } + } + } + + // ── T-1098 PR #216 review finding 3: degenerate setback shrink ────────── + // + // A lot that WINS its coverage roll but is no bigger than `setback` on + // some axis must fall through to interstitial, not vanish from both + // lists. Unit-tested directly against `shrink_lot_or_interstitial` + // (rather than hunting for a real `bsp()` call that produces a + // pathologically small leaf — every `min_lot` floor comfortably exceeds + // every `setback` value today, so the branch is unreachable from live + // `subdivide_block_footprints` traffic; that does not make it untested + // code, it makes it a robustness guard that must still be pinned). + + #[test] + fn shrink_keeps_a_normal_lot_as_a_footprint() { + let lot = TileRect::new(4, 4, 10, 10); + match shrink_lot_or_interstitial(&lot, 2) { + LotOutcome::Footprint(fp) => { + assert_eq!(fp.origin, (4, 4)); + assert_eq!(fp.size, (8, 8), "10 - setback(2) = 8 on both axes"); + } + LotOutcome::Interstitial(_) => { + panic!("a lot comfortably larger than setback must become a footprint") + } + } + } + + #[test] + fn shrink_falls_through_to_interstitial_when_width_is_degenerate() { + // lot.size.0 (3) <= setback (3) → w = 0 post-shrink → degenerate on x, + // even though y has headroom (10 > 3). Must fall through to + // Interstitial, not silently vanish. + let lot = TileRect::new(0, 0, 3, 10); + match shrink_lot_or_interstitial(&lot, 3) { + LotOutcome::Interstitial(leaf) => { + assert_eq!( + leaf, lot, + "degenerate shrink must keep the WHOLE unshrunk lot, not a partial rect" + ); + } + LotOutcome::Footprint(_) => { + panic!("w=0 post-shrink must not produce a footprint") + } + } + } + + #[test] + fn shrink_falls_through_to_interstitial_when_height_is_degenerate() { + // Mirror of the width case: lot.size.1 (2) < setback (3) → h = 0. + let lot = TileRect::new(5, 5, 10, 2); + match shrink_lot_or_interstitial(&lot, 3) { + LotOutcome::Interstitial(leaf) => assert_eq!(leaf, lot), + LotOutcome::Footprint(_) => { + panic!("h=0 post-shrink must not produce a footprint") + } + } + } + + #[test] + fn shrink_falls_through_when_lot_exactly_equals_setback_on_both_axes() { + // Exact-equality boundary: size == setback → saturating_sub floors at + // exactly 0 (not merely near it) on both axes simultaneously. + let lot = TileRect::new(1, 1, 2, 2); + match shrink_lot_or_interstitial(&lot, 2) { + LotOutcome::Interstitial(leaf) => assert_eq!(leaf, lot), + LotOutcome::Footprint(_) => { + panic!("a lot exactly the size of the setback must not become a footprint") + } } }