Skip to content

fix(geometry): close small boundary gaps consolidate leaves on overlapping/rotated operands - #3373

Open
BIMvoice wants to merge 1 commit into
mainfrom
fix-3353-boolean-tearing
Open

fix(geometry): close small boundary gaps consolidate leaves on overlapping/rotated operands#3373
BIMvoice wants to merge 1 commit into
mainfrom
fix-3353-boolean-tearing

Conversation

@BIMvoice

Copy link
Copy Markdown
Collaborator

Summary

  • Refs Booleans still tear on overlapping and rotated operands after the #3341 parity fix #3353. A family of boolean tears survives the Multi-opening cuts tear thin shells open, and the kernel removes volume from a merely touching operand #3341 parity-segment fix on overlapping and rotated operand pairs. Reproduced with a sweep of rotated-cutter / axis-aligned-host box pairs over Difference, Union and Intersection, watertightness checked by welding the output at 0.1 mm and counting directed half-edges that fail to pair one-forward/one-reverse.
  • On one pinned case (proptest_shrunk) the raw kernel output is already watertight; consolidate_coplanar's per-bucket independent re-triangulation is what tears it, emitting the same boundary vertex twice from adjacent buckets a few hundred micrometres apart. On another (sweep_261) the raw arrangement output is already torn before consolidation runs at all.
  • close_micro_gaps (rust/geometry/src/csg/consolidate.rs) is a bounded, last-resort weld applied to whatever consolidate_coplanar is about to return: it fires only when that mesh already has an open boundary edge at 0.1 mm (a sound mesh returns byte-identical), and it keeps the weld only when it strictly reduces the open-edge count without increasing spike-triangle count. This is a repair, not a gate — unlike the closed-in/closed-out enforcement already tried and rejected for ClippingProcessor::validate_mesh (measured to regress the corpus watertightness census), a weld that doesn't help is simply discarded and the mesh returned exactly as it was.
  • This does not close the whole Booleans still tear on overlapping and rotated operands after the #3341 parity fix #3353 family. The sweep still measures a residual, smaller tear rate after the fix, and at least one case has open-edge endpoints too far apart for a vertex weld to fix — pointing at a deeper near-degenerate arrangement defect the issue's own investigation had already flagged as unproven.

Root cause

Two vertices meant to be the same point — one bucket's earcut output and its neighbour's, or two near-degenerate exact intersections in the raw arrangement itself — differ by a few hundred micrometres: well under any real feature this crate triangulates, well over the exact kernel's own snap grid.

Test plan

  • New pinned regression: rust/geometry/tests/issue_3353_rotated_overlap_tearing.rs — two real cases from a 60000-pair sweep, checked across Difference/Union/Intersection.
  • RED confirmed: reverting close_micro_gaps locally fails the new test (sweep_261 Union: 3 unmatched edges); restoring it passes.
  • cargo test -p ifc-lite-geometry — full crate suite green, no failures.
  • cargo test -p ifc-lite-geometry --test touching_operand --test csg_property_test --test csg_quality_regression --test issue_3353_rotated_overlap_tearing — all pass (no regression on the Multi-opening cuts tear thin shells open, and the kernel removes volume from a merely touching operand #3341 gates or the existing quality/watertightness regressions).

🤖 Generated with Claude Code

https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

…pping/rotated operands (#3353)

A family of tears survives the #3341 parity-segment fix on overlapping and
rotated operand pairs. Reproduced with a sweep of rotated-cutter /
axis-aligned-host box pairs over Difference, Union and Intersection: two
pinned cases each come back with unmatched directed half-edges after a
closed-in operand pair.

On one, consolidate_coplanar's per-bucket independent re-triangulation emits
the same boundary vertex twice from adjacent buckets, a few hundred
micrometres apart. On the other, the raw kernel arrangement output is
already torn before consolidation runs at all.

close_micro_gaps is a bounded, last-resort weld applied to whatever
consolidate_coplanar is about to return: it only fires when that mesh
already has an open boundary edge at 0.1 mm, so a sound mesh is returned
untouched, and it keeps the weld only when it strictly reduces the open-edge
count without increasing spike-triangle count. Unlike the closed-in/
closed-out gate already tried and rejected for validate_mesh, a weld that
doesn't help is simply discarded rather than reverting the cut.

This does not close the whole #3353 family — the sweep still measures a
residual, smaller tear rate, and at least one case has open edges too far
apart for a vertex weld to fix, pointing at a deeper near-degenerate
arrangement defect.

Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
@BIMvoice
BIMvoice requested a review from louistrue as a code owner August 28, 2026 04:47
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 3 billable files and costs up to $0.75.

Or wait 30 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: 6e89c2ad-dd46-4641-bab9-2050753bcc0f

📥 Commits

Reviewing files that changed from the base of the PR and between 5a431e5 and 7b9d3f1.

📒 Files selected for processing (3)
  • .changeset/close-micro-gaps-issue-3353.md
  • rust/geometry/src/csg/consolidate.rs
  • rust/geometry/tests/issue_3353_rotated_overlap_tearing.rs

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

@github-actions

Copy link
Copy Markdown
Contributor

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 2065ms 2905ms -28.9% +50%
firstVisibleGeometryMs 3090ms 3652ms -15.4% +50%
streamCompleteMs 3095ms 3598ms -14.0% +50%
spatialReadyMs 1709ms 1032ms +65.6% +50%
metadataCompleteMs 2228ms 3063ms -27.3% +50%
totalWallClockMs 3300ms 3700ms -10.8% +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 317ms 1075ms -70.5% +50%
firstVisibleGeometryMs 2059ms 1572ms +31.0% +50%
streamCompleteMs 1000ms 1980ms -49.5% +50%
spatialReadyMs 1071ms 915ms +17.0% +50%
metadataCompleteMs 1141ms 1392ms -18.0% +50%
totalWallClockMs 2100ms 3300ms -36.4% +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).

@BIMvoice

Copy link
Copy Markdown
Collaborator Author

Do not merge this as written. The census failure is a real regression, not a bless-needed improvement, and investigating it surfaced a worse problem than the red check.

58 hosts regressed against the golden, and they are not the scenario #3353 targets — they are SweptSolid/Clipping/Tessellation hosts on unrelated models:

ara3d/FM_ARC_DigitalHub.ifc #188333  SweptSolid  triangles 74 -> 56 (geometry lost), open=0 both sides
ara3d/ISSUE_129_...V17.ifc  #296868  SweptSolid  open edges 621 -> 628; triangles 572 -> 504
ara3d/ISSUE_129_...V17.ifc  #247051  Clipping    open edges 29 -> 34
various/rvt01.ifc           #7295    SweptSolid  open edges 65 -> 82
various/rvt01.ifc           #10335   Tessellation open edges 9 -> 11

Note the first row: a host with zero open edges on both sides lost 18 triangles. Nothing was torn there, so nothing needed repairing.

Root cause. close_micro_gaps calls Mesh::welded_by_position, which quantizes and merges every vertex in the mesh within the tolerance, globally by position. It has no concept of "boundary only". And consolidate_coplanar is not CSG-only — it is reached from the ordinary opening-cut path (router/voids/{prism_cut,sweep,coaxial_union}), so this runs on routine door and window cuts, not just the rotated-overlap case the fix targets.

The guard checks the aggregate outcome of each call — open-edge count strictly down, badness not up. That cannot detect "closed one tear, silently merged two unrelated closed features elsewhere in the same mesh", which is what the open=0 row above shows.

On the tolerance. SNAP_GRID is 1.0 / 65536.0, a fixed compile-time constant, so 128 * SNAP_GRID is absolute in caller units. Per mesh_bridge.rs's own note those are "CALLER units, not metres (#2684)" — so its physical meaning tracks the file's length unit. On a metre-unit model that is ~1.95mm, which is the size of real features: a thin wall leaf, a narrow reveal, flashing, a chamfer. A global weld at that size can merge two genuinely distinct features, and the aggregate guard would not notice.

Aggregate improvement is not sufficient evidence. The tear count dropping 522 -> 411 across 180,000 op-instances is compatible with these 58 hosts getting worse, which is exactly what happened. I checked whether #3366's misclassification bug was masking anything here — it was not: reclassified: 0, and the 16 hosts filed as IMPROVED show genuine directional movement.

A candidate direction, not proposed for merge yet: restrict the weld to vertices that actually sit on an open boundary edge (an eligibility mask passed into a welded_by_position_restricted), so a closed feature elsewhere in the mesh can never be collapsed. That compiles and keeps the PR's own regression test green, but it has not been verified against the full corpus census — which is the check that caught this — so I am not putting it forward as done.

Flagging rather than closing; the call is yours.

@BIMvoice

Copy link
Copy Markdown
Collaborator Author

Follow-up: I tried the boundary-restricted weld I sketched above, and it is not sufficient. Reporting it as a dead end rather than pushing it.

Change tested: Mesh::welded_by_position_restricted(tol, eligible) plus an open_boundary_vertex_mask(), so only vertices actually sitting on an open boundary edge can merge. Ineligible vertices get a dedupe key tagged with their own vertex index, so they cannot collide with anything — a strong guarantee, and it does work.

Census result, run locally with the CI command (cargo test -p ifc-lite-geometry --features triangulation-alt --test triangulation_invariance, 209s over the full corpus):

53 host(s) REGRESSED against the golden — an existing mesh got worse

Down from 58, not zero.

The canary is fixed. ara3d/FM_ARC_DigitalHub.ifc #188333 — the host with open=0 on both sides that lost 18 triangles — no longer appears in the regression list at all. So the mask does what it was designed to do: a mesh with nothing torn is no longer touched.

Why that is not enough. The remaining 53 hosts have genuine open boundaries — they are already torn. Restricting the weld to boundary vertices does not help there, because that is exactly where their damage is. Welding at ~1.95mm along a real open boundary merges features that legitimately sit that close:

various/rvt01.ifc #31083  Tessellation  triangles 42 -> 15 (geometry lost)
various/rvt01.ifc #16286  SweptSolid    triangles 146 -> 122 (geometry lost)
various/rvt01.ifc #15917  SweptSolid    open edges 64 -> 73
various/rvt01.ifc #14638  SweptSolid    open edges 50 -> 54; newly depends on the triangulator's diagonal choice

That last one is the clearest signal: the output became triangulator-dependent, which is the property this census exists to protect.

There are real improvements in the same run (e.g. #138037 open 12 -> 0, triangles 301 -> 364; several hosts losing snap-collapsed triangles), so the weld does help some meshes. But a repair that fixes some geometry by silently deleting other geometry is not a repair, and the aggregate tear count dropping does not license the individual losses.

Conclusion: a position weld is the wrong mechanism for this class, at any eligibility scoping I can see. Closing a tear needs to know which two boundary loops should join — topology — and a distance threshold cannot recover that. The gap sizes here (up to centimetres in at least one case from the earlier sweep) are also well past what any weld tolerance can bridge without merging real features.

Nothing pushed; the branch is unchanged at 7b9d3f1b6. Flagging rather than closing — the call is yours, and #3353 itself should stay open regardless.

@BIMvoice

Copy link
Copy Markdown
Collaborator Author

One more data point that I think settles this, from trying to rescue the approach rather than abandon it.

The obvious tightening is to make the accept guard also refuse any weld that loses triangles — welded.triangle_count() >= before_tris. That does eliminate the whole remaining regression class: the 53 are almost all [triangles N -> M (geometry lost)] on heavily torn meshes.

But it breaks this PR's own regression test: overlapping_rotated_operands_never_tear fails with sweep_261 Union: 3 unmatched edges, because a legitimate gap-closing weld in that very fixture does reduce the triangle count.

So the two requirements are in direct conflict under a distance-threshold weld:

No tolerance or eligibility scoping resolves that, because both cases look identical to a position weld — it cannot tell "these two boundary vertices are the two sides of one tear" from "these two boundary vertices belong to unrelated loops that happen to sit 2mm apart". That is topology, and a distance threshold does not have it.

The eligibility mask is still worth keeping as a concept — it demonstrably fixed the closed-feature corruption (#188333) — but the remaining eligibility test is "touches any open edge anywhere in the mesh", which on a mesh with hundreds of open edges is still a large set. Making it correct means pairing each vertex against a specific candidate open-edge partner, which is a materially different and bigger change than this PR.

Nothing pushed. fix-3353-boolean-tearing remains at 7b9d3f1b6.

@BIMvoice

Copy link
Copy Markdown
Collaborator Author

New diagnosis: this PR is aimed at the wrong stage. Reporting it here because it invalidates the premise, not just the implementation.

A fresh investigation traced sweep_261 Union (one of the two cases this PR's own test pins) and found:

1. consolidate_coplanar is a byte-identical no-op on this mesh. Comparing its input and output directly: raw.positions == cons.positions and raw.indices == cons.indices are both true. The per-bucket re-triangulation this PR's weld targets does not touch this mesh at all, so it cannot be what tears it.

2. The tear already exists in pure Vid-space — in the exact arrangement's own symbolic vertex identity, before any float conversion and before any position-based welding. Measured via a temporary debug hook computing edge balance purely from Vid pairs, with no coordinates involved: 3 non-manifold edges, with arrangement.unrecovered == 0 (the retriangulation conformed fully; every constraint was forced as an edge).

That second point matters most: no position weld at any tolerance or eligibility scoping can fix a tear that exists in symbolic identity before positions are consulted. It explains why the weld could only ever trade one corruption for another, and why tightening its guard broke this PR's own test.

3. The actual mechanism. The bad edge (shared Vids 13–14) is used by three kept triangles instead of two: A#15 (regime-3 centroid ray-cast), A#22 (regime-1 coincident-with-B face, which should imply B's matching copy is dropped), and B#18 (regime-3, kept as independent). The reason both survive is that each operand uses a different coincidence detector:

  • A's side: BComponents::surface_normal, tested against B's original triangles — fired.
  • B's side: c_on_or_near_a, tested against A's original triangles — did not fire.

Root cause in one sentence: boolean_vids_components classifies each operand's sub-triangles with two structurally different "is this a coincident duplicate of the other operand's face" tests, and at a near-degenerate rotated overlap the two disagree about the same shared edge, so a redundant triangle survives on both operands and leaves that edge non-manifold.

That is this repo's most familiar defect shape — two implementations of one rule with nothing enforcing that they agree — and it sits in classify.rs, upstream of and unrelated to consolidation.

No fix is proposed here, deliberately. Reconciling the two detectors (or replacing both with one symmetric test) changes the correctness-critical core every CSG operation depends on, and warrants its own scope with the full census plus cross-platform determinism validation that a classify.rs change requires. Pushing a speculative change there would repeat this PR's mistake at a more dangerous location.

Provenance, so you can weigh it: the no-op comparison and the Vid-space edge count come from temporary #[doc(hidden)] debug hooks and a scratch example, all since reverted — I have not independently re-run them, and they are worth confirming before anyone builds on them. proptest_shrunk, the other pinned case, passes on current main as-is; only sweep_261 Union still reproduces.

Recommendation: leave #3353 open, and treat this PR as superseded rather than fixable — the weld cannot address a tear that predates the stage it operates on.

@BIMvoice

Copy link
Copy Markdown
Collaborator Author

Follow-up: the diagnosis above has now been independently re-verified from scratch, on current main, by a separate run. I flagged it earlier as resting on since-reverted debug hooks that had not been re-run — that caveat can be lifted.

Claim A — confirmed. consolidate_coplanar is a byte-identical no-op on sweep_261 Union:

raw positions=747 indices=249   cons positions=747 indices=249
positions_eq=true  indices_eq=true

Claim B — confirmed. The tear exists in pure Vid-space, before any float conversion or position weld:

arr.unrecovered=0
kept triangle count=83
non-manifold Vid edges (use count != 2): [((13, 17), 1), ((14, 17), 3), ((13, 14), 3)]

Three non-manifold symbolic edges, and the arrangement reports full recovery (unrecovered == 0) — so this is not a recovery failure. Classification emits a non-manifold complex from a cleanly recovered arrangement.

Third claim — confirmed. Running this PR's own pinned test file against current main: only sweep_261 Union fails, with 3 unmatched edges, matching the three non-manifold Vid edges exactly. proptest_shrunk and every other case/op combination pass.

Nothing in the earlier comment needs correcting.

The practical consequence is unchanged and now on firmer footing: a position weld inside consolidate_coplanar cannot fix this, because the stage it operates in provably does not alter the mesh, and the defect is already present in symbolic identity upstream of it. That is why the weld's guard requirements were irreconcilable — it was being asked to repair damage that had already happened somewhere else.

Still not proposing a fix here. Reconciling BComponents::surface_normal and c_on_or_near_a into one symmetric coincident-face test touches the core every CSG operation depends on, and deserves its own scope with the full census and cross-platform determinism validation. Flagging that as the shape of the real fix, for whenever you want it picked up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant