fix(geometry): only extend an opening cap the cutter actually exits through (#3219) - #3340
fix(geometry): only extend an opening cap the cutter actually exits through (#3219)#3340louistrue wants to merge 1 commit into
Conversation
…hrough (#3219) `extend_opening_mesh_through_host` pushes an opening cutter's cap past a coincident host facet so a flush interface becomes a clean transversal crossing instead of a coplanar graze (#1007, host #1112). It qualified a cap on coincidence alone. On a host whose Brep ALREADY carries the hole the cutter's side planes coincide with the hole's jambs, so both jambs read as caps and each was pushed 0.30 * span into the pier beside it. Archicad exports a wall with "Material Preservation: Explode where necessary" exactly that way: the body lives on aggregated IfcBuildingElementPart layers and every layer's Brep carries its own window, while all 22 IfcRelVoidsElement sit on the body-less parent wall and are propagated down. Every window came out scaled by exactly 1.600 with its centre unmoved. On the reporter's model that removed 18.8%, 18.6%, 27.0% and 8.3% of four wall layers. Coincidence is now necessary but not sufficient. A cap must also lie under the cutter's cross-section footprint and face the way an exit faces: an exit facet faces away from the host material, a jamb faces into the hole because the host continues past it. The facing is area-weighted so facet scatter cannot outvote the real surface, and the sign is read from the host's own signed volume rather than assumed, because IFC winding is not reliably outward and the host is not oriented until after the cut. Genuine exit caps keep the identical push, so the #1007 clearance is unchanged. The correct answer for this model is that the subtraction removes NOTHING (the holes are already there), confirmed independently by ifcopenshell 0.8.2 and by manifold3d, which puts the intersection volume of every (part, opening) pair at 0.0000 m3. The four affected layers now land within 0.1%. The decision moved to router/voids/synthesis/exit_cap.rs, reusing OpeningFrame, project_aabb_in_frame, mesh_point and mesh_signed_volume rather than re-deriving them; synthesis.rs drops from 1002 to 950 lines, so its ratchet budget is lowered rather than raised. Four tests assert removed volume or cutter extent, not mechanism. The existing void tests ray-cast "the wall has a hole", which is monotone in the cut and structurally cannot catch an over-cut, which is why this shipped. They also pin the 30% clearance from both sides for the first time: pad = 0 and pad = 1.5 both left the whole suite green before. Not fixed here, tracked separately: two thin IfcCovering layers on the same model come out as torn, non-watertight shells, which survives setting the pad to zero and is therefore independent; and a host whose winding is genuinely MIXED can still mis-tally a cap, which skips the push and costs a rim sliver rather than an over-cut.
|
Warning Review limit reached
This review includes 6 billable files and costs up to $1.50. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 90 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4a2c69ad-18fb-4f54-92b4-72805e340a04) |
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe86be9f07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // convention from the sign of its signed volume instead of assuming one. | ||
| // Without this an inward-wound host loses the #1007 clearance push AND | ||
| // gets the #3219 pier-eating back, both silently. | ||
| let orient = if mesh_signed_volume(host) < 0.0 { -1.0 } else { 1.0 }; |
There was a problem hiding this comment.
On a multi-body host whose disconnected components have opposite windings, this uses the net signed volume of every component even though the later footprint test considers only local facets. An unrelated, larger inward-wound component can therefore make orient negative for an outward-wound pre-cut wall; its jamb normals are then classified as exits and receive the 30% extension, reintroducing the geometry over-cut this change is meant to prevent. The winding convention needs to come from the connected component containing the coincident cap facets rather than the whole merged mesh.
Useful? React with 👍 / 👎.
| a.min(b).min(c) <= hi + band && a.max(b).max(c) >= lo - band | ||
| }; | ||
| spans(&self.frame.cross_a, self.lo.x, self.hi.x) | ||
| && spans(&self.frame.cross_b, self.lo.y, self.hi.y) |
There was a problem hiding this comment.
For a coplanar oblique facet, overlap of the separate u and v intervals does not imply that the projected triangle intersects the cutter rectangle; for example, a triangle can lie wholly beyond a diagonal separating line while its bounding intervals overlap both axes. Such an off-footprint facet is still included in the area-weighted facing tally, so a sufficiently large remote facet can suppress a genuine exit or falsely turn a pre-cut jamb into one. Use a real projected triangle-versus-rectangle intersection test rather than only these two marginal interval checks.
Useful? React with 👍 / 👎.
Parking this behind #3341CI caught a corpus-level regression that no local gate could see: Geometry watertightness census, 32 regressed / 10 improved across 112 models. I chased it through four designs. The short version is that the cap heuristic is the wrong place to fix this, and the evidence for that is measured rather than argued. Four variants, same golden
All four fix #3219 (the four affected wall layers go from -18.8% / -18.6% / -27.0% / -8.3% to within 0.1%). All four regress the corpus. Occupancy is best on regressions and worst on tearing. Seven of its twenty are genuine open-edge increases, not the census misreading a triangle-count drop — Why this points at the kernel, not the cap gateTwo independent discriminators — a signed facet normal, and winding-independent ray parity — fix the bug identically and both regress the corpus. That rules out the discriminator being the problem. What is left is the pad itself. It over-extends by 30% of the opening's span by design (see the calibration note above That is the defect in #3341: Two things I got wrong, recorded so nobody repeats them
State of this branchBranch head is the reviewed winding-sign commit. The ray-parity occupancy variant is not pushed — it is a substantial redesign that has not been through Recommend: land the #3341 kernel fix first, then revisit this with the census as the gate from the start. |
1 participant
Fixes #3219.
What was wrong
extend_opening_mesh_through_hostpushes an opening cutter's cap past a coincident host facet so a flush interface becomes a clean transversal crossing instead of a coplanar graze (#1007, host #1112). It qualified a cap on coincidence alone.Archicad exports a wall with Material Preservation: Explode where necessary like this: the wall body lives on aggregated
IfcBuildingElementPartlayers, every layer's Brep already carries its own window, and all 22IfcRelVoidsElementsit on the body-less parent wall. ifc-lite propagates them down, which is correct (ifcopenshell'sfind_openingswalksDecomposes()the same way).So the cutter's side planes coincide with the hole's jambs. Both read as caps, and each was pushed
0.30 * spaninto the pier beside it. Every window came out scaled by exactly 1.600 with its centre unmoved:Eleven windows, 9.308 m of extra opening, 6.348 m3 implied against a measured 6.366 m3 loss. The pad accounts for the whole error.
The correct answer, established independently
The subtraction should remove nothing on this model, because the holes are already there. Two independent confirmations:
Eliminated 11 disjoint operands/11 touching operandsand returns identical volume with opening subtraction on and off.The fix
Coincidence is now necessary but not sufficient. A cap must also lie under the cutter's cross-section footprint and face the way an exit faces: an exit facet faces away from the host material, a jamb faces into the hole because the host continues past it. The facing is area-weighted so facet scatter cannot outvote the real surface.
The sign is read from the host's own signed volume rather than assumed. IFC winding is not reliably outward and the host is not oriented until after the cut (
element.rsrunsorient_mesh_outward_verdicton the result). Without that term an inward-wound host lost the #1007 clearance push entirely and got the pier-eating back.Genuine exit caps keep the identical push, so the #1007 clearance is unchanged. The pad, its constant, the flush band and the function signature are all untouched.
Tests
Four, all asserting removed volume or cutter extent rather than mechanism. The existing void tests ray-cast "the wall has a hole", which is monotone in the cut and structurally cannot catch an over-cut — that is why this shipped.
Each was verified to fail on the code without its condition, for the right reason:
flush_cap_is_not_pushed_into_a_pre_cut_jambremoved 0.9601 m3remote_coplanar_facet_does_not_outvote_a_local_exit_cappushed clear by only 0.0000 of a 0.9000 spanan_inward_wound_host_is_read_the_same_as_an_outward_one1.400 .. 4.600vs authored2.000 .. 4.000flush_cap_on_a_genuine_exit_is_still_pushed_clearThe last one pins the 30% clearance for the first time: before this PR,
pad = 0andpad = 1.5both left the entire suite green.Structure
The decision moved to
router/voids/synthesis/exit_cap.rs, reusingOpeningFrame,project_aabb_in_frame,mesh_pointandmesh_signed_volumerather than re-deriving them.synthesis.rsdrops 1002 -> 950, so its ratchet budget is lowered, not raised; the new module is under the 400 rule and needs no row.Gates
cargo test -p ifc-lite-geometry --no-fail-fast: 90 binaries, 960 passed, 0 failed, exit 0.cargo test -p ifc-lite-processing --no-fail-fast: 58 binaries, 289 passed, 0 failed, exit 0.cargo clippy -p ifc-lite-geometry --all-targets -- -D warnings: exit 0.Run with all 164 model fixtures fetched, so the fixture-gated void tests actually execute.
Deliberately not fixed here
IfcCoveringlayers on the same model come out as torn, non-watertight shells (198 and 662 edges of degree != 2). This survives setting the pad to zero, so it is independent of this change. Follow-up issue.infer_opening_framepicks the smallest-extent axis with no authored extrusion direction, which for these 44 m deep, 1.21 m wide cutters picks the width. Flipping the comparator was tried and refuted by measurement (+523% on one part of wall #392764). This PR makes the wrong axis harmless at the point it did damage instead.accept_cutincoaxial_union.rsasserts "the watertightness of the cut is guaranteed upstream". Measured false: the first subtract takes a closed 188-triangle host to an open 236-triangle one.Coupling worth knowing
With this fix, roughly 60 remove-nothing outcomes per load of this model now flow through the #635 engulf guard and #964 redundant-void detection instead of being padded into removing real material. Weakening that guard pair would partially re-open #3219.