core: shared pure plan-footprint math for spatial-grid and MCP - #600
core: shared pure plan-footprint math for spatial-grid and MCP#600ActArtech wants to merge 3 commits into
Conversation
Aymericr
left a comment
There was a problem hiding this comment.
Thanks for picking up the note I left at the end of #569 — this is the right direction and I checked the math rather than assuming: planFootprintAABB's abs-cos/abs-sin extents are provably the same box as the rotated-corner loop it replaces in alignment-anchors.ts, planFootprintCorners preserves spatial-grid's rotation convention exactly (different corner order, nothing depends on order), the attachTo null-list matches the enum in item.ts, and it stays clear of the R3F guard in architecture.test.ts. The alignment-anchors.ts comment admitting it "mirrors the rotated-corner math the spatial-grid manager uses" was a real duplication and −58 lines is a real removal.
But as it stands the PR moves the number in the wrong direction, and that's what I want to fix before it lands.
The ask on #569 was that MCP stop being a third copy. With the rewire declared out of scope, aabbsOverlap in door-clearance.ts survives — and the new aabbsOverlapPlan is a line-for-line reimplementation of it. Post-merge there are three copies of expand-then-intersect instead of two, and MCP still uses its own. The extraction is the easy half; the consolidation was the point.
Three of the five exports have no callers. aabbsOverlapPlan and planFootprintAABBForItem exist for the deferred MCP work; planFootprintAABBFromCorners is called only by tests, which then assert it's identical to planFootprintAABB — so it's an identity helper proving itself. On #569 I asked you to delete aabbFromPlan and nodeItemAabb for exactly this, and AGENTS.md names dead code and speculative abstractions specifically.
Two paths, either fine by me:
- Finish it. Keep all five exports and rewire
door-clearance.ts/layout-clearance.ts/check-collisions.tsto import from@pascal-app/core/plan-footprint, deleting MCP'saabbsOverlapandPlanAabb. Heads-up on the trap that probably made you defer:@pascal-app/coreis only a devDependency ofpackages/mcpwhiledoor-clearance.tsalready runtime-importsgetScaledDimensionsfrom@pascal-app/core/schema— that's a latent packaging bug, and moving more runtime code across that edge means promoting it to a real dependency. Please also keep thegap: 0versus0.08distinction from #569 intact per call site; that's the bug I'd most want a test to hold. - Or narrow it. Ship only
planFootprintCornersandplanFootprintAABB, drop the other three exports and their tests, and leave the subpath export in as the seam for the follow-up.
On the wiki page: I'd rather it didn't say "Aymericr noted…" — the other wiki/architecture/ pages state the rule, not who asked for it, and 43 lines of page restating the header of a 133-line math module is more provenance than durable rule. Fold the gap-semantics table into the module header and skip the page, or cut it to the invariant plus the call-site table.
Useful side-finding worth carrying into the rewire: your aabbsOverlapPlan comment ("expand each box by gap") is right and MCP's existing one ("expand each box by gap/2") describes something the code doesn't do. Fix that comment while you're in there.
Extract rotation-aware XZ footprint AABB/corners and expand-then-intersect gap overlap into packages/core/lib/plan-footprint. Spatial-grid and alignment-anchors delegate to it. Export via @pascal-app/core/plan-footprint and spatial-grid. Design note documents one source and gap call-site rules after pascalorg#569.
Answer CHANGES_REQUESTED on pascalorg#600 with the Narrow path: keep planFootprintCorners and planFootprintAABB, drop the three unused exports and their tests, and remove the named-attribution wiki page. Gap call-site meanings stay in the module header as the seam for the MCP consolidation follow-up.
6384a73 to
b66908b
Compare
|
Took the Narrow path from the review:
Follow-up (not in this PR): rewire MCP |
What does this PR do?
Extracts rotation-aware plan (XZ) footprint math into a pure, Node-safe core module so spatial-grid and alignment anchors share one formula (follow-up seam for MCP layout clearance after #569).
packages/core/src/lib/plan-footprint.tswithplanFootprintCornersandplanFootprintAABBonly (plusPlanAabb/PlanVec2types)@pascal-app/core/plan-footprint(pure) and re-export the same two helpers from@pascal-app/core/spatial-gridgap ≈ 0.08packing vsgap = 0hard verify) documented in the module header for the MCP consolidation follow-upNarrow response to review: dropped unused
aabbsOverlapPlan,planFootprintAABBForItem, andplanFootprintAABBFromCorners(no dead / speculative exports).Out of scope (follow-up): rewiring MCP
door-clearance/layout-clearance/check-collisionsonto these helpers and deleting MCP's duplicate overlap path.How to test
bun test packages/core/src/lib/plan-footprint.test.ts— all passbun test packages/core/src/services/alignment-anchors.test.ts— still greenbun run check packages/core/src/lib/plan-footprint.ts packages/core/src/lib/plan-footprint.test.ts packages/core/src/hooks/spatial-grid/spatial-grid-manager.ts packages/core/src/services/alignment-anchors.ts— cleanplanFootprintAABBfrom@pascal-app/core/plan-footprintand from@pascal-app/core/spatial-gridand confirm the same boxScreenshots / screen recording
N/A — non-visual change (pure geometry helpers + tests).
Checklist
bun devbun checkto verify)mainbranchNote: this change is pure math with no UI surface; verification was
bun teston plan-footprint + alignment-anchors,bun checkon touched files, and green GitHub CI (quality/mcp-ci/cli-smoke).