Skip to content

fix(geometry): gate opening classification on triangle count, not vertex-buffer length - #4178

Open
BIMvoice wants to merge 2 commits into
mainfrom
fix-4119-opening-triangle-count
Open

fix(geometry): gate opening classification on triangle count, not vertex-buffer length#4178
BIMvoice wants to merge 2 commits into
mainfrom
fix-4119-opening-triangle-count

Conversation

@BIMvoice

@BIMvoice BIMvoice commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Refs #4119

Summary

classify_openings_impl's >100-vertex fallback gate (issue #635's high-vertex AABB fallback) read opening_mesh.positions.len() / 3 — the raw position-buffer length. That count also includes per-IfcFace vertex duplication the faceted-brep mesher emits, and welding (#4103) merges duplicate vertex slots without touching indices. So the same cutter geometry could land on either side of the 100 threshold depending purely on how redundantly its vertices happened to be stored. The issue measured this on a real corpus: 7 of 618 openings moved branch from welding alone, with zero triangles changed.

This switches the gate to triangle count (indices.len() / 3), which is invariant to vertex duplication and welding. OpeningDiagnostic gains a triangle_count field alongside the existing vertex_count (kept for diagnostics display only, and now documented as not what the classifier reads).

Reproduction

Confirmed on upstream/mainrouter/voids/synthesis.rs line ~102 reads opening_mesh.positions.len() / 3 and gates the >100 branch on it, matching the issue's citation exactly.

What's NOT in this PR

The issue also raises a separate, harder question: whether the numeric threshold itself (100) should be retuned once measured in triangle terms rather than vertex terms, since the two units aren't equivalent. The issue explicitly flags that as needing "its own before/after over the corpus and its own review" — I don't have access to the 618-opening corpus the issue measured against, so I kept the threshold value unchanged (100) and only fixed what it measures. Requesting unqueued since this is partial.

Note on that: leaving the constant at 100 while switching the metric is not "behavior unchanged at the boundary" — vertex counts typically run 2-3x triangle counts for faceted geometry, so an opening that previously needed roughly 33-50 triangles to hit the >100 fallback now needs 100, making the gate more permissive. That branch exists because those meshes "won't fit through the CSG safety thresholds" (synthesis.rs), so under-classification is the risky direction — a genuinely complex opening that used to get the safe AABB fallback could now attempt exact CSG instead; over-classification is merely conservative. Measured against every .ifc file in this repo (samples plus geometry test fixtures), the largest opening is 92 vertices / 55 triangles, well under 100 on either metric, so zero openings here change classification and no output geometry differs.

CI follow-ups on this branch

  • Module-size ratchet: the triangle-count comments above pushed rust/geometry/src/router/voids/synthesis.rs to 973 lines against its 955-line allowlist budget. Extracted remove_internal_membrane (the AC20 glued-cap-to-cap deseaming helper — self-contained, no other state from synthesis.rs) verbatim into a new sibling module rust/geometry/src/router/voids/synthesis/membrane.rs (206 lines, under the 400-line new-file threshold, no allowlist entry needed). synthesis.rs is now 791 lines.

  • Geometry watertightness census (tests/manifests/watertightness_census.tsv): the larger CI corpus (1170 void hosts vs. the handful measurable locally) contains openings this metric switch does reclassify — the local measurement in "What's NOT in this PR" above only covered this repo's own small fixtures. Two hosts in ara3d/ISSUE_126_model.ifc (opening ids #98282 and #111997) moved from the >100-vertex AABB fallback to exact CSG and, per CI, went from torn (26 and 27 unmatched edges) to fully watertight (0), with fewer triangles each (220→186, 51→32) — i.e. the exact path produced a cleaner cut than the fallback did, not a worse one. Every other host is unchanged: the CI run shows 0 regressed, 0 coverage loss, 0 added, 0 volume-moved, and every corpus total (torn hosts, unmatched/strict edges, genuine defects) moved down or held. This is what the "What's NOT in this PR" risk note flagged as possible in this larger corpus, confirmed here as an improvement rather than a regression, so the golden is re-blessed for these 2 hosts (IFCLITE_CENSUS_BLESS=1 cargo test -p ifc-lite-geometry --features triangulation-alt --test triangulation_invariance).

    The golden diff touches only those two rows — nothing else in the 1170-host manifest changed. Both rows go from torn to fully watertight with real enclosed-volume readings now populated (#98282: 1776930 cm³, #111997: 290818 cm³, both previously blank on the torn/unmeasurable path). Corpus totals all moved down or held: torn hosts 104→102, unmatched edges 2848→2795, strict-rule edges 3752→3699, genuine defects 75→73, collapsed/non-invariant counts unchanged. Void-host population is identical on both sides, 1170/1170 — the improvement isn't fewer things being examined, it's the same population classified more accurately. Verified independently of CI: fetched the full 164-file corpus locally, re-ran with IFCLITE_CENSUS_BLESS=1 to produce this diff, then re-ran again without bless to confirm the census now passes clean against it.

Test plan

  • Regression test router::voids::synthesis::tests::issue_4119_triangle_count_gate::nonrectangular_opening_keeps_the_welded_mesh_not_the_raw_one (rust/geometry/src/router/voids/synthesis_tests.rs) drives the real production path (GeometryRouter::classify_openings) with a faceted-brep opening authored with per-cell duplicate points (8x8 grid → 128 triangles, 256 raw vertices, 81 unique vertices after the automatic weld inside apply_placement). Asserts the classifier's returned NonRectangular mesh is the welded one (81 vertices).
  • Mutation-verified: reverting the gate back to vertex_count > 100 makes the same assertion fail with 256 instead of 81 (welded vertex count already ≤100 skips the fallback branch and falls through to the unwelded per-item path) — confirmed RED, then restored the fix and confirmed GREEN again.
  • cargo test -p ifc-lite-geometry --lib: 860 passed, 0 failed, 1 ignored (pre-existing, unrelated).
  • All void/opening-related integration test files in rust/geometry/tests/ (engulfing_solid_void, issue_068/635/832/964/1007/1167/1367, multi_body_void_spike, opening_void_cut_local_frame_test, voids_inline_matrix_test, voids_production_test, voids_submesh_test, wall_opening_cut_regression): all pass, no regressions.
  • cargo clippy -p ifc-lite-geometry --lib --tests: clean.
  • check-module-size.mjs, check-test-wiring.mjs, check-source-text-assertions.mjs: all exit 0.
  • Changeset added for @ifc-lite/wasm (patch).
  • cargo test -p ifc-lite-processing --test module_size_ratchet: 6 passed.
  • cargo test -p ifc-lite-geometry --features triangulation-alt --test triangulation_invariance watertightness_census_and_triangulator_invariance: passes against the re-blessed golden.

https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

… count, not vertex-buffer length

`classify_openings_impl`'s >100-vertex fallback gate (issue #635) read
`opening_mesh.positions.len() / 3`, which also counts per-`IfcFace` vertex
duplication the faceted-brep mesher emits. Welding (#4103) merges those
duplicate vertex slots without touching `indices`, so the same cutter
geometry could land on either side of the threshold depending purely on
how redundantly its vertices happened to be stored — measured on a real
corpus in the issue: 7 of 618 openings moved branch from welding alone,
with zero triangles changed.

Switch the gate to triangle count (`indices.len() / 3`), which is
invariant to vertex duplication and welding. Add `triangle_count` to
`OpeningDiagnostic` alongside the existing `vertex_count` (kept for
diagnostics only, documented as no longer the classification signal).

Regression test (`router/voids/synthesis_tests.rs`) drives the real
production path with a faceted-brep opening authored with per-cell
duplicate points (8x8 grid, 128 triangles, 256 raw vertices, 81 after
weld) and asserts the classifier keeps the welded mesh. Reverting the
fix flips the assertion from 81 to 256 vertices (verified).

Refs #4119
@BIMvoice
BIMvoice requested a review from louistrue as a code owner September 8, 2026 13:48
@BIMvoice

BIMvoice commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Requesting unqueued: this fixes the classification defect (vertex-count -> triangle-count gate) but deliberately does not retune the numeric threshold (100), which the issue itself flags as needing its own before/after measurement over a corpus I don't have access to. Refs #4119, not Closes.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

Or wait 22 minutes for your next included review.

Check out review usage here.

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 648905b7-3ea4-46fd-99c9-cc05b1b7384e

📥 Commits

Reviewing files that changed from the base of the PR and between 0ac3136 and f4c857b.

⛔ Files ignored due to path filters (1)
  • rust/geometry/tests/manifests/watertightness_census.tsv is excluded by !**/*.tsv
📒 Files selected for processing (5)
  • .changeset/fix-opening-classification-triangle-count.md
  • rust/geometry/src/router/diagnostics_recording.rs
  • rust/geometry/src/router/voids/synthesis.rs
  • rust/geometry/src/router/voids/synthesis/membrane.rs
  • rust/geometry/src/router/voids/synthesis_tests.rs

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Claude review - no findings for 876e29c93

Reviewed this diff and found nothing to flag.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Viewer benchmark

✅ No threshold regressions detected.

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 1654ms 2905ms -43.1% +50%
firstVisibleGeometryMs 2307ms 3652ms -36.8% +50%
streamCompleteMs 3323ms 3598ms -7.6% +50%
spatialReadyMs 1137ms 1032ms +10.2% +50%
metadataCompleteMs 1526ms 3063ms -50.2% +50%
totalWallClockMs 3500ms 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 357ms 1075ms -66.8% +50%
firstVisibleGeometryMs 2113ms 1572ms +34.4% +50%
streamCompleteMs 803ms 1980ms -59.4% +50%
spatialReadyMs 873ms 915ms -4.6% +50%
metadataCompleteMs 991ms 1392ms -28.8% +50%
totalWallClockMs 2200ms 3300ms -33.3% +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).

@github-actions github-actions Bot added the llm-reviewed A review was verified as posted for this PR's head. label Sep 8, 2026
…atertightness census for the triangle-count gate

Two self-inflicted CI failures from the triangle-count classification
change:

- Module-size ratchet: the new triangle-count comments pushed
  router/voids/synthesis.rs to 973 lines against its 955-line
  allowlist budget. Extracted `remove_internal_membrane` (the AC20
  glued-cap-to-cap deseaming helper) verbatim into a new sibling
  module, router/voids/synthesis/membrane.rs (206 lines, no allowlist
  entry needed). synthesis.rs is now 791 lines.

- Geometry watertightness census: the larger CI corpus (1170 void
  hosts, vs. what's measurable against this repo's own small
  fixtures) contains two openings this metric switch does reclassify.
  Both are in ara3d/ISSUE_126_model.ifc (opening ids #98282 and
  #111997): previously routed to the >100-vertex AABB fallback, they
  now qualify for exact CSG under the >100-triangle gate. Per the
  re-run, both hosts went from torn (26 and 27 unmatched edges) to
  fully watertight (0), with fewer triangles each (220->186, 51->32) -
  the exact path produced a cleaner cut than the fallback did, not a
  worse one. Every other host is unchanged: 0 regressed, 0 coverage
  loss, 0 added, 0 volume-moved, and every corpus total (torn hosts,
  unmatched/strict edges, genuine defects) moved down or held. Golden
  re-blessed for exactly these 2 rows.

Refs #4119
@vercel

vercel Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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

2 Skipped Deployments
Project Deployment Actions Updated
ifc-lite-dev Ignored Ignored Sep 8, 2026 3:03pm UTC
ifc-lite-viewer-embed Ignored Ignored Sep 8, 2026 3:03pm UTC

@github-actions github-actions Bot removed the llm-reviewed A review was verified as posted for this PR's head. label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Claude review - no findings for f4c857b41

Reviewed this diff and found nothing to flag.

@github-actions github-actions Bot added the llm-reviewed A review was verified as posted for this PR's head. label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llm-reviewed A review was verified as posted for this PR's head.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant