fix(simulation): PR #216 review fixes — footprint-wins enforcement, discriminating tests

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 <noreply@anthropic.com>
This commit is contained in:
2026-07-26 14:28:40 +02:00
co-authored by Claude Fable 5
parent 5b5cdc85a3
commit 9c5aa79852
3 changed files with 332 additions and 22 deletions
+6 -2
View File
@@ -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
+166 -3
View File
@@ -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<GroundTilePos, InterstitialType>,
voxels: &BTreeMap<ShellVoxelPos, ShellVoxel>,
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<ShellVoxelPos, ShellVoxel>, 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]
+160 -17
View File
@@ -857,6 +857,36 @@ struct BlockSubdivision {
interstitial: Vec<TileRect>,
}
/// 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")
}
}
}