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

feat(simulation): interstitial fill — ground-tile character between building footprints (T-1098) #216

Closed
jpmschweitzer wants to merge 0 commits from interstitial-fill into main
pull from: interstitial-fill
merge into: :main
Conversation 2 Commits - Files Changed -
jpmschweitzer commented 2026-07-26 14:14:53 +02:00
Owner
Copy Link
Copy Source

Summary

Batch-8 server track: the second Phase-4 walkability item. Between-footprint ground tiles previously had zero signal (implicit Void); FilledChunk now carries a defined character for every one of them.

  • Geometry model (design brief, lead-approved checkpoint: docs/architecture/interstitial-fill-t1098.md): the BSP leaves that lose the D-233 coverage roll in subdivide_block_footprints were already computed and discarded — they ARE the interstitial set. Surfacing them makes the ground-plane classification exhaustive by construction: footprint / interstitial / street-margin-or-reserved (D-234 future scope).
  • Threading: FillChunk gains the leaves + a minimal BlockFillContext (interstitial_character, setback_tier re-derived at block level via the existing pure fn) — not the whole BlockSkeleton.
  • Resolution (pure, no RNG): OperationsSurface (D-233) short-circuits; else setback_tier maps onto five of D-235's seven interstitial values (ZeroLot→Void, Tight→Court, Standard→Garden, Generous→Plaza, Campus→OpenLawn). dock_slip/market_pad have no trigger condition specified in D-235 — deliberately not invented; T-1209 filed and cross-referenced in code + brief.
  • Absence contract stated on FilledChunk: a missing key means footprint/street/reserved, never unknown — the implicit-Void ambiguity this ticket kills is not reintroduced one level up.
  • Bonus fix: a degenerate setback shrink (setback ate the whole lot) previously vanished from BOTH lists silently; now falls through to interstitial.

Test plan

  • 14 new tests (resolution fn ×3, BSP split ×4, fill/clip/determinism/mutual-exclusion ×7); lib suite 1990 passed / 0 failed
  • Full cargo test green including believability, cascade, derivation and golden harnesses — no Gauntlet room or golden fixture touched (additive)
  • Purity per T-987: plan-time compute, pre-resolved work items, no cache reads in run_work_item
  • Data-only until Phase 5 consumes FilledChunk — no rendered path changes, no eyeball owed
## Summary Batch-8 server track: the second Phase-4 walkability item. Between-footprint ground tiles previously had zero signal (implicit Void); FilledChunk now carries a defined character for every one of them. - **Geometry model (design brief, lead-approved checkpoint: docs/architecture/interstitial-fill-t1098.md):** the BSP leaves that lose the D-233 coverage roll in subdivide_block_footprints were already computed and discarded — they ARE the interstitial set. Surfacing them makes the ground-plane classification exhaustive by construction: footprint / interstitial / street-margin-or-reserved (D-234 future scope). - **Threading:** FillChunk gains the leaves + a minimal BlockFillContext (interstitial_character, setback_tier re-derived at block level via the existing pure fn) — not the whole BlockSkeleton. - **Resolution (pure, no RNG):** OperationsSurface (D-233) short-circuits; else setback_tier maps onto five of D-235's seven interstitial values (ZeroLot→Void, Tight→Court, Standard→Garden, Generous→Plaza, Campus→OpenLawn). dock_slip/market_pad have no trigger condition specified in D-235 — deliberately not invented; T-1209 filed and cross-referenced in code + brief. - **Absence contract stated on FilledChunk:** a missing key means footprint/street/reserved, never unknown — the implicit-Void ambiguity this ticket kills is not reintroduced one level up. - **Bonus fix:** a degenerate setback shrink (setback ate the whole lot) previously vanished from BOTH lists silently; now falls through to interstitial. ## Test plan - 14 new tests (resolution fn ×3, BSP split ×4, fill/clip/determinism/mutual-exclusion ×7); lib suite 1990 passed / 0 failed - Full cargo test green including believability, cascade, derivation and golden harnesses — no Gauntlet room or golden fixture touched (additive) - Purity per T-987: plan-time compute, pre-resolved work items, no cache reads in run_work_item - Data-only until Phase 5 consumes FilledChunk — no rendered path changes, no eyeball owed
jpmschweitzer added 2 commits 2026-07-26 14:14:54 +02:00
feat(simulation): interstitial fill — ground-tile character between building footprints (T-1098) 1d676a91f8 
The BSP leaves that lose the D-233 coverage roll in
subdivide_block_footprints were computed and discarded; they are now
surfaced as the interstitial rect set (BlockSubdivision), making the
ground-plane classification exhaustive by construction: footprint /
interstitial / street-margin-or-reserved. FillChunk carries the leaves
plus a minimal BlockFillContext (interstitial_character + setback_tier,
re-derived at block level via the existing pure fn); FilledChunk gains
a sparse interstitial map whose absence contract is stated on the
struct (missing key = footprint/street/reserved, never unknown). The
pure resolution maps OperationsSurface (D-233) first, else setback_tier
onto five of D-235's seven interstitial values — dock_slip/market_pad
have no specified trigger in the record and point at T-1209 rather
than an invented mapping. Design brief with the geometry model at
docs/architecture/interstitial-fill-t1098.md (lead-approved
checkpoint). Bonus fix: a degenerate setback shrink previously vanished
from BOTH lists silently; it now falls through to interstitial. 14 new
tests; full cargo test green incl. all golden harnesses; purity per
T-987 (plan-time compute, pre-resolved work items, no cache reads).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merge remote-tracking branch 'origin/main' into interstitial-fill 5b5cdc85a3
jpmschweitzer commented 2026-07-26 14:20:17 +02:00
Author
Owner
Copy Link
Copy Source

Review: interstitial-fill -> main (type: code)

Reviewers: Hoshe (QA) + Tyre (architecture/governance). Both REQUEST_CHANGES (Tyre's is a single wording nit under the any-comment rule; Hoshe's four are substantive test-quality findings).

Hoshe (QA): REQUEST_CHANGES

The IMPLEMENTATION verified sound on every focus — exhaustiveness of the three-way partition (bsp/split_rect read directly), clipping conventions matching shell_derive_into, resolution-fn totality, purity/determinism. The findings are all about tests that don't discriminate:

# File Issue
1 skeleton_gen.rs:1859-1880 The "no overlap" test is a tautology: TileRect's PartialEq covers origin AND size, and footprints are always setback-shrunk while interstitial leaves keep full BSP size — contains(fp) can essentially never fire regardless of geometric overlap. No test constructs actually-overlapping rects.
2 shell.rs:765-789 The mutual-exclusion fixture is trivially disjoint (6-tile gap); fill_chunk computes the two maps independently with no cross-check, so exclusion is a property of the inputs, never exercised. Needs an intersecting or boundary-sharing case.
3 skeleton_gen.rs:917-936 The advertised degenerate-setback bonus fix has NO discriminating test — reverting the `w < 1
4 shell.rs:792-806 The sub-chunk clip test checks each side's keys stay in-range but never that the two sides SUM to the leaf's full 32 tiles — a seam off-by-one dropping a row would pass. (Same limitation exists in the pre-existing footprint clip test — fix the new one; the old one may follow.)

Tyre (architecture/governance): REQUEST_CHANGES

# File Issue
1 interstitial-fill-t1098.md:80-82 "ChunkLayout ... unconsumed stub (grep confirms no reader)" is imprecise — it has plan-time writers and one #[cfg(test)] read. The conclusion (no PRODUCTION consumer, reserved for D-234) is verified correct; tighten the wording so the binding brief doesn't carry a trivially-falsifiable claim.

Tyre's confirmations and rulings on record: citation fidelity the best reviewed in this class — every governance claim in brief and code verified against record bodies, T-1209 cross-referenced at all four required points; block-level derive_setback_tier is provably the identical pure call every building makes (zero drift); goldens provably unaffected. RULINGS: no D-235 amendment needed for the 5-of-7 subset (vocabulary vs trigger-spec distinction; T-1209 owns the gap); the brief belongs permanently in docs/architecture (mechanics, not decisions — no D-record migration); the sibling-map API shape and BlockFillContext minimal-carry are correct as-is, do not merge.

Verdict: CHANGES REQUESTED

All five findings dispatched to the authoring agent. Fix round → gate → merge (no eyeball owed — data-only until Phase 5).

# Review: interstitial-fill -> main (type: code) Reviewers: Hoshe (QA) + Tyre (architecture/governance). Both REQUEST_CHANGES (Tyre's is a single wording nit under the any-comment rule; Hoshe's four are substantive test-quality findings). ## Hoshe (QA): REQUEST_CHANGES The IMPLEMENTATION verified sound on every focus — exhaustiveness of the three-way partition (bsp/split_rect read directly), clipping conventions matching shell_derive_into, resolution-fn totality, purity/determinism. The findings are all about tests that don't discriminate: | # | File | Issue | |---|------|-------| | 1 | skeleton_gen.rs:1859-1880 | The "no overlap" test is a tautology: TileRect's PartialEq covers origin AND size, and footprints are always setback-shrunk while interstitial leaves keep full BSP size — `contains(fp)` can essentially never fire regardless of geometric overlap. No test constructs actually-overlapping rects. | | 2 | shell.rs:765-789 | The mutual-exclusion fixture is trivially disjoint (6-tile gap); fill_chunk computes the two maps independently with no cross-check, so exclusion is a property of the inputs, never exercised. Needs an intersecting or boundary-sharing case. | | 3 | skeleton_gen.rs:917-936 | **The advertised degenerate-setback bonus fix has NO discriminating test** — reverting the `w < 1 || h < 1` fall-through to the old silent `continue` passes every test in the diff. The one behavior change most likely to regress silently is unpinned. | | 4 | shell.rs:792-806 | The sub-chunk clip test checks each side's keys stay in-range but never that the two sides SUM to the leaf's full 32 tiles — a seam off-by-one dropping a row would pass. (Same limitation exists in the pre-existing footprint clip test — fix the new one; the old one may follow.) | ## Tyre (architecture/governance): REQUEST_CHANGES | # | File | Issue | |---|------|-------| | 1 | interstitial-fill-t1098.md:80-82 | "ChunkLayout ... unconsumed stub (grep confirms no reader)" is imprecise — it has plan-time writers and one #[cfg(test)] read. The conclusion (no PRODUCTION consumer, reserved for D-234) is verified correct; tighten the wording so the binding brief doesn't carry a trivially-falsifiable claim. | Tyre's confirmations and rulings on record: citation fidelity the best reviewed in this class — every governance claim in brief and code verified against record bodies, T-1209 cross-referenced at all four required points; block-level derive_setback_tier is provably the identical pure call every building makes (zero drift); goldens provably unaffected. RULINGS: no D-235 amendment needed for the 5-of-7 subset (vocabulary vs trigger-spec distinction; T-1209 owns the gap); the brief belongs permanently in docs/architecture (mechanics, not decisions — no D-record migration); the sibling-map API shape and BlockFillContext minimal-carry are correct as-is, do not merge. ## Verdict: CHANGES REQUESTED All five findings dispatched to the authoring agent. Fix round → gate → merge (no eyeball owed — data-only until Phase 5).
jpmschweitzer added 1 commit 2026-07-26 14:29:23 +02:00
fix(simulation): PR #216 review fixes — footprint-wins enforcement, discriminating tests 9c5aa79852 
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>
jpmschweitzer commented 2026-07-26 14:29:29 +02:00
Author
Owner
Copy Link
Copy Source

Fix round — all 5 findings addressed in 9c5aa7985

Reviewer # Resolution
Hoshe 1 Fixed — real rects_overlap() coordinate-range helper (itself pinned by a known-cases sanity test: self/disjoint/edge-touching/contained) applied pairwise over every footprint × interstitial leaf
Hoshe 2 Fixed, and it became a real code fix — conflict semantics ruled footprint wins and now ENFORCED: interstitial_fill_into probes voxels per column and skips occupied tiles. The prior code had no guard — the absence contract was a documented promise the code didn't keep against conflicting inputs. Contract stated on both FilledChunk and the fn; pinned by a fully-overlapping-leaf test asserting per-tile resolution (overlap tiles keep voxels, gain no interstitial entry; the remainder resolves normally). Old disjoint fixture renamed _when_disjoint as the companion case
Hoshe 3 Fixed — the shrink decision extracted to pure shrink_lot_or_interstitial() with four discriminating tests (normal / width-degenerate / height-degenerate / exact-equality boundary), each asserting the whole unshrunk lot lands in interstitial rather than vanishing. Honestly documented: the branch is unreachable from live traffic today (every min_lot floor exceeds every setback) — a robustness guard for future recalibration, now pinned so reverting is a visible act
Hoshe 4 Fixed — both clip tests (new interstitial AND the pre-existing footprint one, leave-cleaner) now reconstruct the full 32-tile union across the seam: left/right disjoint, union exactly the leaf/footprint area (column-ized for the multi-z footprint case)
Tyre 1 Fixed — brief wording tightened to the verified claim: no PRODUCTION consumer; plan-time writers + #[cfg(test)] reads named explicitly

No retractions — all five findings accurate, and finding 2 exposed a genuine enforcement gap beyond its test-quality framing. Touched suites green (shell 23, skeleton_gen 61); full cargo test 45/45 blocks ok, lib 1996 passed / 0 failed, all golden harnesses green. Gate green on push. Merging.

# Fix round — all 5 findings addressed in `9c5aa7985` | Reviewer | # | Resolution | |----------|---|------------| | Hoshe | 1 | **Fixed** — real `rects_overlap()` coordinate-range helper (itself pinned by a known-cases sanity test: self/disjoint/edge-touching/contained) applied pairwise over every footprint × interstitial leaf | | Hoshe | 2 | **Fixed, and it became a real code fix** — conflict semantics ruled **footprint wins** and now ENFORCED: `interstitial_fill_into` probes `voxels` per column and skips occupied tiles. The prior code had no guard — the absence contract was a documented promise the code didn't keep against conflicting inputs. Contract stated on both `FilledChunk` and the fn; pinned by a fully-overlapping-leaf test asserting per-tile resolution (overlap tiles keep voxels, gain no interstitial entry; the remainder resolves normally). Old disjoint fixture renamed `_when_disjoint` as the companion case | | Hoshe | 3 | **Fixed** — the shrink decision extracted to pure `shrink_lot_or_interstitial()` with four discriminating tests (normal / width-degenerate / height-degenerate / exact-equality boundary), each asserting the whole unshrunk lot lands in interstitial rather than vanishing. Honestly documented: the branch is unreachable from live traffic today (every min_lot floor exceeds every setback) — a robustness guard for future recalibration, now pinned so reverting is a visible act | | Hoshe | 4 | **Fixed** — both clip tests (new interstitial AND the pre-existing footprint one, leave-cleaner) now reconstruct the full 32-tile union across the seam: left/right disjoint, union exactly the leaf/footprint area (column-ized for the multi-z footprint case) | | Tyre | 1 | **Fixed** — brief wording tightened to the verified claim: no PRODUCTION consumer; plan-time writers + #[cfg(test)] reads named explicitly | No retractions — all five findings accurate, and finding 2 exposed a genuine enforcement gap beyond its test-quality framing. Touched suites green (shell 23, skeleton_gen 61); full cargo test 45/45 blocks ok, lib 1996 passed / 0 failed, all golden harnesses green. Gate green on push. Merging.
jpmschweitzer closed this pull request 2026-07-26 14:32:46 +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#216