fix(geometry): weld source vertices in the object frame, not after the bake (#4103) - #4121
fix(geometry): weld source vertices in the object frame, not after the bake (#4103)#4121louistrue wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
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_3671715a-17b8-4bc2-8e97-09e6e86db800) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (10)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughMesh welding now occurs in object coordinates before placement transforms. Ordinary meshes use the shared welding API after processing, while shared geometry skips a second weld. Tests cover instancing, UV handling, vertex identity, and flat-shading tolerance. ChangesFrame-aware mesh welding
Assessment at Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal This change welds shared geometry before placement so mapped occurrences can reuse identical mesh buffers and instance efficiently. The supplied regression coverage and passing workspace checks indicate no remaining merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant PlacementApplier
participant mesh_weld
participant build_mesh_data
participant Collation
PlacementApplier->>mesh_weld: weld mesh in object coordinates
mesh_weld->>PlacementApplier: return welded geometry
PlacementApplier->>PlacementApplier: apply placement and world transforms
build_mesh_data->>mesh_weld: weld ordinary mesh
build_mesh_data->>Collation: provide matching mesh buffers
Collation->>Collation: reuse shared geometry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Claude review - no findings for
|
github-actions
Bot
added
the
llm-reviewed
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 |
…e bake (#4103) Ten `IfcFurnishingElement` sharing one `IfcRepresentationMap` produced ten different mesh buffers, and no route deduplicated them. Same triangle counts, vertex counts spread over 0.9%. `mesh_weld`'s vertex key carries the raw f32 BIT PATTERN of the position, so what it merges depends on the magnitude of the coordinates it is handed. One f32 ULP is ~6e-8 m at 0.5 m and ~2e-6 m at 30 m, so welding baked world coordinates applied an epsilon that grew with the element's distance from the origin: the same source geometry welded differently at every placement. That broke a contract the pipeline depends on and states in `instancing/mod.rs`. Every occurrence of one `IfcRepresentationMap` is a clone of ONE cached source mesh (`router::mapped_item`), and every direct-solid `rep_identity` is a hash of the mesh BEFORE placement (`router::processing::direct_rep_identity`), so occurrences of one representation are meant to be bit-identical. A post-bake weld rewrote each of them differently, and `collate_refs` refuses a group whose members disagree on vertex count (`instancing/group.rs:209`, `verify.rs:119`), so nothing collated and the content-hash tier could not fire either. The weld now runs in the object frame, from the two placement appliers, which is the last point at which the vertices are still in the frame the geometry was authored in. All three placement sites in `router/processing.rs` go through those two functions. That covers the whole shipped path, but it is not every bake in the crate: `voids::probe::get_opening_item_meshes_world` calls `transform_mesh_world_framed` directly, so it does produce unwelded meshes that carry `instance_meta`. Those are void cutters and volume probes and never become element MeshData, so nothing ships unwelded, but the pairing is inferred rather than recorded. #4122 tracks putting the answer on the mesh. Normals had to move with it. They are computed in `element.rs` AFTER the bake, and `weld_indexed` refuses a mesh whose normals do not match its positions 1:1, so a weld inserted before placement without them merges NOTHING - which is why this looks free to measure and is not. `weld_mesh`/`weld_sub_mesh` call `calculate_normals` first; it accumulates from the triangle winding, so it needs only positions and indices, and `transform_mesh_world_framed` already rotates normals into world with the positions. `build_mesh_data` still welds, but only meshes carrying no `instance_meta`: a void cut (`voids::process_element_with_voids` clears it precisely because a cut body no longer reproduces its representation), a layer slice, a #858 palette split, a textured type part. What they have in common is that none has a cross-occurrence identity to protect, so `weld_baked` welds them in the frame they ship in. Not that none reaches a placement applier - a void host does, and is welded there before it is cut, so for that population this is a second weld. `weld_baked`'s doc names both cases. Without that split, the kernel's per-face output ships unwelded and voided walls inflate: C20-Institute-Var-2 by 90%, ISSUE_098 by 7%. Measured, vertices that must ship after `collate_refs(&refs, 2, [0,0,0])`, the same call `parquet_instancing.rs` makes: dental_clinic 309,684 -> 179,840 (-42%) Office_A_20110811 81,766 -> 54,591 (-33%) duplex 40,785 -> 28,668 (-30%) ISSUE_098 (site-local) 3,461,037 -> 3,278,926 (-5%) AC20-FZK-Haus 32,116 -> 32,116 (0%) C20-Institute-Var-2 16,097 -> 16,117 (+0.1%) Collation rejections drop to 0 on duplex (4), dental_clinic (8) and Office_A (4); schependomlaan keeps its 2. End to end on the #4103 attachment, both binaries built here, each with a fresh CACHE_DIR. Main reproduces the reporter's bytes exactly: as authored /parse/parquet 30,408,159 -> 30,329,804 /parse/parquet/optimized 16,804,132 -> 16,676,055 site placement /parse/parquet 30,329,870 -> 30,255,284 neutralised /parse/parquet/optimized 16,785,313 -> 1,682,818 and `optimization_stats` on the neutralised file goes `unique_meshes: 30, mesh_reuse_ratio: 1.0` to `unique_meshes: 3, mesh_reuse_ratio: 10.0`. The attachment as authored improves only 0.8%, and that is honest: its IfcSite placement carries a 34 degree rotation, so `element.rs:766` discards every mesh's instancing metadata before any collation can see it. That is a separate defect with a separate mechanism, filed as #4118 with this measurement. Two tests were measuring the wrong layer and are corrected here, not weakened: - `issue_846_revolved_beam` demanded bit-identical normals within a triangle at the ROUTER output, where the mesh was still unwelded and every triangle owned its three vertices. That bar never described the shipped mesh: on main the weld ran afterwards at `build_mesh_data` and merged normals within the same 1e-3 cell, putting the identical jitter into the `MeshData` users get. Moving the weld ahead of the placement makes the router's output show what the shipped mesh always showed. Measured on the branch: of 644 triangles, 25 exceed 1e-5 and 4 exceed 1e-4, worst within-triangle deviation 1.6e-4. The threshold moves to the weld's own normal quantization grid (1e-3), and a new control smooth-shades the same mesh and measures 2.0 there - four orders the other way - so the bar sits in the gap and still catches the #846 regression it exists for. - 11 `geometry_correctness_harness` snapshots move. Every VALUE that changes is `total_vertices` and every one goes DOWN; triangles, surface area, bbox and error counts are untouched. Two of the 11 also drop a stale `assertion_line: 628` header, matching the nine others here: that is insta metadata, it was already stale on main, and it churns on any unrelated edit to the harness. All 25 were diffed before any was accepted. The new regression test is a 41-entity inline IFC: one `IfcRepresentationMap` holding two coplanar quads whose x coordinates differ by 1e-6 m, each authored as two triangular faces so the source carries genuine per-face duplication, mapped at x = 0, 64 and 128. It discriminates three outcomes rather than two: 12 vertices means nothing welded, 8 at x=0 and 4 at x=128 means the weld ran on baked coordinates, 8 everywhere is the fix. It also asserts that at x=128 the two quads' four corner pairs share f32 world positions AND normals and are still separate vertices, which is only true of a mesh welded in the object frame. One behaviour change outside the mesh buffers, disclosed rather than tuned. `voids/synthesis.rs:102` classifies an opening by its cutter's RAW vertex count and sends `> 100` single-body cutters down the #635 AABB fallback. Welding the cutter reduces that count without changing its geometry, so 7 of 618 openings in the fixture corpus move to the exact CSG path: 250 -> 92, 250 -> 92, 122 -> 80, and 108 -> 56 four times. The move is toward what the gate's own comment says it is for ("won't fit through the CSG safety thresholds"), because 92 distinct vertices really is a simpler cutter than a 250-slot soup, and the full suite is green either way since nothing asserts the classification. The gate measuring authoring redundancy instead of complexity is a defect in its own right, filed as #4119: retuning it re-classifies all 618 openings, not these 7, so it needs its own before/after and its own review. Review, all three outcomes on the record: - The claim "no fourth site to forget" was false as written, and the same claim in `element.rs`'s guard comment ("every producer that sets `instance_meta` routes through a placement applier") was false the same way. `probe.rs:521` is the counterexample. Fixed above: both now say what is actually true, which is that everything ARRIVING at `build_mesh_data` came through an applier. - "Every change is total_vertices" was false: two snapshots also moved `assertion_line`. Fixed by stripping that header from those two as well, so the claim is true of the diff rather than the diff being described loosely. - Deferred to #4122: a real `welded_in_object_frame` bit on `Mesh` so the guard can assert instead of trust. It touches ~25 struct literals, which is a wider diff than this fix. /simplify, second pass: - `weld_baked` was a byte-identical alias of the private `weld_in_place`, and two reviewers found it independently. Five entry points collapsed to three: one public `weld` carrying the body, plus `weld_mesh` and `weld_sub_mesh`. `ensure_normals` is inlined into it, which also merges two doc paragraphs that read as contradicting each other (they described different call-site populations 15 lines apart). `mesh_weld.rs` 327 -> 316 lines. - Dropped the position-delta loop from the regression test. Any tolerance loose enough for f32 world storage at 128 m (one ULP is 1.5e-5 there) is far looser than the 1e-6 separating the two quads, so it could not tell them apart and only restated the vertex count. Removing it left `world()` dead, which is the tell that it was the odd assertion out. The index and normal equality stay (they do not depend on `collate`'s internals) and so does the exact coincident-pair count, which is the sharp form. - Rejected: replacing that coincident count with `weld_indexed(..).is_some()`. Bit-exact equality is strictly STRONGER than the weld's quantized key, so a bit-identical pair is necessarily one the key would merge, and `== 4` pins the fixture's structure where `is_some()` would not. - Rejected: dropping `flat_indices.is_empty()` as derivable from the other two collation assertions. An assertion that names its own failure mode earns its line. - Deferred: replacing the `FRAME_OVERRIDE` mutex with a `LazyLock`. The mutex exists because this test hit that exact flake; swapping it buys nothing behavioural. Perf, measured rather than argued (`scripts/perf/ab.sh`, 15 interleaved rounds, `profiling` profile, base `4638f7491`): FM_ARC_DigitalHub.ifc geometry 162 -> 159 ms (-1.9%, noise +/-23%) dental_clinic.ifc geometry 58 -> 57 ms (-1.7%, noise +/-17%) `parse` flat on both as the control. Every delta is inside the noise band: NO proven regression and no proven win on native. Native runs with `local_frame_enabled() == false`, so this never exercises the relativizing branch in `mesh_world.rs` that allocates 24 B/vertex per element; that is the wasm and viewer path, where the saving should be larger and is unmeasured here. Two costs the reorder introduces, both recorded on issues rather than fixed here: `ensure_normals`' output is discarded whenever the orienter flips (and the processors that leave normals absent are the ones that flip), and opening cutters are now welded although `mesh_to_tris` ignores normals and they never reach `build_mesh_data`. The second is the mechanism behind the 7-of-618 opening reclassification described above, so it is on #4119; the first is on #4122. /code-review, third pass. It traced every router entry point `element.rs` calls and every producer of `instance_meta` and found NO live hole in the discriminator, and confirmed the `geom_closure` claim (`mesh_orient` really does quantize to a 10 um grid, which an exact-bit weld refines). Four findings: - FIXED: a dangling intra-doc link to `weld_baked`, which the /simplify collapse above had just deleted. `cargo doc` warns and the docs lane is `continue-on-error`, so nothing would have caught it. `mesh_weld` now emits zero doc warnings. - FIXED: the `issue_846` tolerance derivation was wrong by a factor of two. The comment reasoned over ONE merge class, but the assertion compares three vertices that can survive in three DIFFERENT cells, so straddling representatives can differ by about two cells (~2e-3), which is ABOVE the 1e-3 bar. It passes at 1.6e-4, six times inside, but the comment was telling the next maintainer to re-derive from a bad model. Corrected, and it now says plainly that the control is what proves the bar, not the constant. (Fable raised the same point earlier and it was left; two reviewers agreeing is what made me look properly.) - DISCLOSED, not fixed: a THIRD residual the module doc did not list. Welding is not the only stage that can make two occurrences disagree; anything editing INDICES post-bake can too. `degenerate::clean` compares a triangle height from f32 world positions against an ABSOLUTE 15.3 um threshold, and `mesh_orient`'s 10 um adjacency grid is finer than the f32 world grid past 128 m (one ULP is 1.5e-5 m there). Either can leave two occurrences with different index buffers, which `instancing::group` rejects: the #4103 symptom by another route. Reasoned from the thresholds, NOT observed, and this fixture's half-unit squares cannot exercise it, so it is recorded as unmeasured rather than claimed. - DEFERRED to #4122: non-instanced elements now pay two weld passes where main paid one. Real, but the A/B above bounds it at inside-noise on two real models, and skipping the second pass needs the `welded` marker that issue is about. Closes #4103 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfAavu3wBCfAPxEsv9BJ4K
4013904 to
dce6016
Compare
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_21b0b7cb-7352-4e93-a3b2-5c378ec8c3ca) |
|
Pushed Two false claims corrected. "No fourth site to forget" was wrong, and the matching "Every change is A tolerance comment that was wrong by 2x. The A third residual, disclosed rather than found later. Welding is not the only stage Perf, measured ( Deferred with the arguments recorded: #4122 (carry a welded bit on Two things a reviewer should know rather than infer:
|
github-actions
Bot
removed
the
llm-reviewed
Claude review - no findings for
|
github-actions
Bot
added
the
llm-reviewed
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
|
|
@coderabbitai full review |
Action performedFull review triggered. |
1 participant
Closes #4103.
Ten
IfcFurnishingElementsharing oneIfcRepresentationMapproduced tendifferent mesh buffers, and no route deduplicated them. Same triangle counts,
vertex counts spread over 0.9%.
The reporter's diagnosis was right. The mechanism is sharper than "welding runs
after placement".
Root cause
mesh_weld's vertex key carries the raw f32 BIT PATTERN of the position:So what it merges depends on the magnitude of the coordinates it is handed. One
f32 ULP is ~6e-8 m at 0.5 m and ~2e-6 m at 30 m. Welding baked world coordinates
therefore applied an epsilon that grew with the element's distance from the
origin, and the same source geometry welded differently at every placement.
That broke a contract the pipeline depends on and states in
instancing/mod.rs:It is not, if a later stage rewrites the vertices per occurrence.
collate_refsrefuses a group whose members disagree on vertex count(
instancing/group.rs:209,verify.rs:119), all-or-nothing per group, sonothing collated. The content-hash tier in
parquet_optimized.rscould not fireeither.
The fix
The weld runs in the object frame, from the two placement appliers, the last
point at which the vertices are still in the frame the geometry was authored in.
All three placement sites in
router/processing.rsgo through those twofunctions, so there is no fourth site to forget.
Normals had to move with it, and this is the part that makes the change
non-obvious. They are computed in
element.rsAFTER the bake, andweld_indexedrefuses a mesh whose normals do not match its positions 1:1. Aweld moved earlier without them merges NOTHING, on every model, while looking
byte-identical to a fingerprint check.
weld_mesh/weld_sub_meshcallcalculate_normalsfirst; it accumulates from triangle winding, so it needs onlypositions and indices, and
transform_mesh_world_framedalready rotates normalsinto world with the positions.
build_mesh_datastill welds, via the newweld_baked, but only meshes carryingno
instance_meta: a void cut (voids::process_element_with_voidsclears itprecisely because a cut body no longer reproduces its representation), a layer
slice, a textured type part. None of those reaches a placement applier and none
has a cross-occurrence identity to protect, so they are welded in the frame they
ship in. Without that split the kernel's per-face output ships unwelded and
voided walls inflate: C20-Institute-Var-2 by 90%, ISSUE_098 by 7%.
Measured
Vertices that must ship after
collate_refs(&refs, 2, [0,0,0]), the same callparquet_instancing.rsmakes:Collation rejections drop to 0 on duplex (4), dental_clinic (8) and Office_A (4).
schependomlaan keeps its 2.
End to end on the #4103 attachment, both server binaries built here, each with a
fresh
CACHE_DIR. Main reproduces the reporter's bytes exactly, so this ismeasured against their environment:
/parse/parquet/parse/parquet/optimized/parse/parquet/parse/parquet/optimizedoptimization_statson the neutralised file goesunique_meshes: 30, mesh_reuse_ratio: 1.0tounique_meshes: 3, mesh_reuse_ratio: 10.0.The attachment as authored improves only 0.8%, and that is the honest headline
for this PR. Its
IfcSiteplacement carries a 34 degree rotation, soelement.rs:766discards every mesh's instancing metadata before collation cansee it. Separate mechanism, separate fix, filed as #4118 with this measurement.
What this PR does prove on the reporter's own file is the thing they reported:
within-group vertex spread 84 / 527 / 2712 goes to 0 / 0 / 0.
Two tests were measuring the wrong layer
Corrected, not weakened:
issue_846_revolved_beamdemanded bit-identical normals within a triangle atthe ROUTER output. The SHIPPED mesh for that same beam has had 42 triangles
over that 1e-5 bar since the weld landed (measured on
main); under thischange it has 19, and the worst within-triangle deviation is 6.4e-5, three
orders of magnitude below a crease. The threshold moves to the weld's own
normal quantization grid, and a new control smooth-shades the same mesh and
asserts it blows past the threshold, so the bar still catches the IfcBeam geometry error with IfcRevolvedAreaSolid #846
regression it exists for.
geometry_correctness_harnesssnapshots move. Every change istotal_verticesand every one goes DOWN; triangles, surface area, bbox anderror counts are untouched. All 25 were diffed before any was accepted.
The regression test
41 entities, inline, no external fixture. One
IfcRepresentationMapholding twocoplanar quads whose x coordinates differ by 1e-6 m, each authored as two
triangular faces so the source carries genuine per-face duplication, mapped at
x = 0, 64 and 128. It discriminates three outcomes rather than two:
It also asserts that at x=128 the two quads' four corner pairs share f32 world
positions AND normals and are still separate vertices, which is only true of a
mesh welded in the object frame. The local frame is forced off rather than
inherited, so it measures the same thing in every environment.
A residual, measured rather than hidden
The weld's no-duplicate-key invariant now holds in the frame the weld runs in.
A rigid placement can still map two distinct object-frame vertices onto one f32
world position, and those are no longer merged, because merging them is exactly
the placement-dependent behaviour this PR removes. Across six ara3d models that
is 104 meshes of 15,211 and 494 vertices of 1,093,616: 0.68% of meshes, 0.045%
of vertices.
soupymeshes (>= 2.9 vertices per triangle) carryinginstance_metaare identical on main and this PR, so nothing became soup.Verification
No changeset: no
packages/*files changed.🤖 Generated with Claude Code
https://claude.ai/code/session_01NfAavu3wBCfAPxEsv9BJ4K
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests