fix(geometry): hoist the AABB into point_inside instead of rescanning (#3354)

[BIMvoice]

Summary

• point_inside in rust/geometry/src/kernel/arrangement/classify.rs used to call tris_aabb(tris) — an O(N) scan of every triangle — on every ray-parity query, purely to size the far endpoint. It now takes that box as a parameter instead.
• Every caller already holds a box that contains the same triangles: BComponents::inside its cached, padding-inflated per-component box; boolean_vids_components's regime-2 probe the BVH's already-computed root box; union_all each mesh's tris_aabb, hoisted out of its per-probe loop instead of recomputed for every other mesh's boundary triangle.
• Passing a superset box cannot change the parity verdict: sound_far only extends the far endpoint to clear whichever box it is handed, and clearing a bigger box means clearing the real one too, past which there are no more triangles to cross. Checked all three call sites' held boxes are genuine supersets of tris_aabb(tris) — two are exact, one is padding-inflated — so this is a redundant-scan removal, not a behavior change.

What this finishes

This resumes 2fae5a8bd (preserved on upstream/wip/3354-point-inside-aabb), an earlier attempt that was cut off by a full disk. That commit's message was explicit about what was missing:

• The equivalence test's non-vacuous half (a shrunk, non-superset box must be able to flip a verdict, or the superset-safety assertions prove nothing). The prior grid-search draft never found a divergence and tripped sound_far's commensurability debug_assert trying. Replaced with a constructed case: the ray's default far endpoint is placed strictly inside the exact box but within the shrink margin of one face, so the exact box triggers sound_far's extend-to-escape branch (correct verdict) while the shrunk box's early-return hands back an endpoint that is still inside the real solid (corrupted verdict) — and never reaches the debug_assert's branch.
• A before/after timing measurement. Not done here either — a release-mode benchmark was attempted, but the shared CARGO_TARGET_DIR used in this environment had its release cache corrupted by a concurrent build (duplicate lang-item errors from two conflicting libcore builds), and available disk did not allow a clean rebuild to retry. This PR is a redundant-computation removal with an unmeasured effect on wall time, not a measured speedup — flagged explicitly in the changeset.
• Running the affected tests. Done below.

Test plan

• cargo test -p ifc-lite-geometry --lib classify_tests — 4/4 pass, including the completed non-vacuous equivalence test
• cargo test -p ifc-lite-geometry --lib kernel::arrangement — 16/16 pass
• cargo test -p ifc-lite-geometry --lib (full crate unit suite) — 728 passed, 0 failed, 1 ignored
• cargo test -p ifc-lite-geometry --test touching_operand (the Multi-opening cuts tear thin shells open, and the kernel removes volume from a merely touching operand #3341 end-to-end regression gate) — 2/2 pass

Refs #3354

https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 22 minutes for your next included review.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 52a16b22-3003-4ebe-adde-b4f59cabed03

📥 Commits

Reviewing files that changed from the base of the PR and between 5a431e5 and 8ef32a5.

📒 Files selected for processing (4)

• .changeset/point-inside-aabb-hoist.md
• rust/geometry/src/kernel/arrangement/boolean.rs
• rust/geometry/src/kernel/arrangement/classify.rs
• rust/geometry/src/kernel/arrangement/classify_tests.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Viewer benchmark

⚠ 1 metric(s) exceeded the regression threshold (advisory only, not blocking).

01_Snowdon_Towers_Sample_Structural(1).ifc

Baseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric	Current	Baseline	Delta	Threshold	Status
firstBatchWaitMs	2300ms	2905ms	-20.8%	+50%	✅
firstVisibleGeometryMs	3489ms	3652ms	-4.5%	+50%	✅
streamCompleteMs	3750ms	3598ms	+4.2%	+50%	✅
spatialReadyMs	1957ms	1032ms	+89.6%	+50%	❌
metadataCompleteMs	2475ms	3063ms	-19.2%	+50%	✅
totalWallClockMs	3900ms	3700ms	+5.4%	+50%	✅

AC20-FZK-Haus.ifc

Baseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric	Current	Baseline	Delta	Threshold	Status
firstBatchWaitMs	367ms	1075ms	-65.9%	+50%	✅
firstVisibleGeometryMs	1251ms	1572ms	-20.4%	+50%	✅
streamCompleteMs	1064ms	1980ms	-46.3%	+50%	✅
spatialReadyMs	1090ms	915ms	+19.1%	+50%	✅
metadataCompleteMs	1173ms	1392ms	-15.7%	+50%	✅
totalWallClockMs	1400ms	3300ms	-57.6%	+50%	✅

Refresh the baseline from a CI run: dispatch the Benchmark workflow with record_baseline, download the benchmark-baseline artifact, and commit baseline.json (see tests/benchmark/README.md).

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
ifc-lite-dev	Ignored	Preview	Aug 28, 2026 8:46am
ifc-lite-viewer-embed	Ignored		Aug 28, 2026 8:46am

[BIMvoice]

This PR is red on the Rust module-size ratchet, and I do not think it should be forced green. Your call.

rust/geometry/src/kernel/arrangement/classify.rs is 720 lines on main — exactly its recorded budget. This PR's additions took it to 754, failing no_module_grows_past_its_ratchet_budget.

I compressed this PR's own comments and compacted solid_side's signature, keeping every claim: 754 → 739. Still 19 over.

The remaining gap cannot be closed without deleting the soundness rationale — the comment explaining why passing a caller-held box is verdict-identical to rescanning (a superset only lengthens the far endpoint, past where no triangles remain, so parity is unchanged). That is precisely the comment that must not go: it is the argument the whole change rests on, on a file every CSG operation depends on.

So the options are:

1. Raise the budget by ~19. The documented escape hatch, but the ratchet's own notes record a raise reaching main and being undone (fix(geometry): return coaxial_union.rs to its 704-line budget #2658), and CodeRabbit flagged a budget raise on a sibling PR today as contradicting the allowlist's "ratchet down only" header.
2. Split classify.rs. Correct long-term, but a refactor of correctness-critical CSG core does not belong inside an optimisation PR.
3. Don't land it.

My honest read is that (3) is defensible and possibly right. This is an unmeasured optimisation — the benchmark could not be run (a shared cargo target dir was corrupted mid-session), and the PR says so plainly rather than implying a speedup. Trading a budget raise on correctness-critical code for an unquantified gain is a poor deal, and I would rather say that than quietly raise the number.

What the PR does have going for it, independent of speed: union_all and boolean_vids_components were rescanning boxes their callers already held, and the equivalence test now includes a genuine non-vacuous half — a shrunk, non-superset box is shown to actually flip a verdict, so the superset argument is pinned rather than assumed.

Happy to do (1) or (2) if you prefer either; I did not want to pick unilaterally.

Separately: an earlier commit here (6c6a2ae13) briefly swept in 267 lines of unrelated in-progress work from a concurrent worker — my error, git add -A in a shared checkout. Reverted in 8ef32a517; the PR now touches only boolean.rs, classify.rs and classify_tests.rs. Flagging it so the history reads clearly rather than looking like a silent force-push.

[louistrue]

The geometry reasoning here is sound and I checked it on three legs rather than one, so this is close to ready. The red check is mechanical.

Why "Rust tests" is red: it is not a behaviour failure. It is the module-size ratchet at rust/processing/tests/module_size_ratchet.rs:297 reporting classify.rs: 754 lines, budget 720. Your roughly 43 added lines, mostly doc comments, pushed the file 34 past its recorded budget. Reproduced locally with cargo test -p ifc-lite-processing --test module_size_ratchet. "Build + WASM + Rust + Node" is just the aggregator gating on that. Every geometry test passed in the same CI job, including your new classify tests.

Behaviour preservation confirmed:

1. sound_far uses the box only to decide whether to lengthen the far endpoint, so a superset box pushes the endpoint further along the same ray, past the true AABB where no triangles remain. Parity is identical. Bvh::root_aabb() is the unpadded exact box, union_all passes the exact tris_aabb, and BComponents passes a small-pad superset with far_l at or above box scale, so the commensurability debug_assert cannot fire.
2. cargo test -p ifc-lite-geometry --no-fail-fast on your head: exit 0, zero failures.
3. The full watertightness census lane was green on this head, 12m33s of real corpus, so no host's classification moved. Debug builds throughout, so the assert was armed the whole time.

a_superset_aabb_matches_the_exact_box_and_a_shrunk_one_does_not is a good test. The shrunk-box half proves the argument is not simply ignored, so the superset half cannot pass vacuously.

What to do: get classify.rs back under 720. The ratchet's own message says shrink or split, and splitting reads better here: the ray-parity block (sound_far plus point_inside, roughly 120 lines) is a clean unit for its own module. Condensing the four new doc comments would also do it, but they are worth keeping.

Please do not raise the budget. And if you regenerate the allowlist, check the diff afterwards, because --update annexes rows for files the PR never touched (it happened on #3367 and #3330 in this same batch).

Thanks for flagging in the changeset that the speedup is unmeasured. That is the right way to write it up.

[louistrue]

Do not raise the budget, and do not delete the rationale. Split the file.

You are right that the comment is the part that must not go. It is the argument the whole change rests on, and classify.rs is on the path of every CSG operation, so the next person to touch point_inside needs to be able to read why a caller-held box is verdict-identical to a rescan.

The third option the ratchet's own message points at is the right one here: move the ray-parity block, sound_far plus point_inside, roughly 120 lines, into its own module. It is a clean unit with one entry point, it takes the doc comment with it where it reads better than buried in a 750-line file, and it drops classify.rs well under 720 without touching a word of the reasoning. arrangement/ray_parity.rs or similar.

That is also the honest answer to the ratchet's question. The file is at its budget because it is genuinely doing several jobs; the budget is telling you to separate one out, not to write less about what you did.

For the record, I verified the change itself on three legs before deciding this, so the only thing between this PR and merge is the line count:

• sound_far uses the box only to decide whether to lengthen the far endpoint, so a superset pushes the endpoint further along the same ray, past the true AABB where no triangles remain. Parity is identical. Bvh::root_aabb() is the unpadded exact box, union_all passes the exact tris_aabb, and BComponents passes a small-pad superset with far_l at or above box scale, so the commensurability debug_assert cannot fire.
• cargo test -p ifc-lite-geometry --no-fail-fast on your head: exit 0.
• The full watertightness census lane was green on this head, 12m33s of real corpus, debug builds throughout so the assert was armed the whole time. No host's classification moved.

a_superset_aabb_matches_the_exact_box_and_a_shrunk_one_does_not is a good test, and the shrunk-box half is what stops the superset half passing vacuously.

Thanks for the revert commit dropping the files the earlier commit swept in. That sweep is a real trap in this repo and it caught two other PRs in this same batch: check-module-size --update emits a "lower the budget" note for every stale row anywhere in the repo, not just the ones your PR grows, so a regenerate annexes them. I have restored them on #3367 and #3330 myself. Worth knowing next time you regenerate anything.