You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
feat(geometry,wasm,cache): representation-item identity crosses the boundary, disjoint from material ids (#3199) (#3210)
* feat(geometry,wasm,cache): representation-item identity crosses the boundary, disjoint from material ids (#3199)
The router already kept each representation item's STEP id, and it already
reached the server REST payload. `MeshDataJs::from_mesh_data` did not copy it,
so the browser never saw it and a host could not drill from a rendered piece to
the entity that produced it -- an `IfcWindow`'s pane or frame -- and navigate
there in source (#2985).
AND IT WAS WRONG FOR LAYERED WALLS. `router/layers.rs` pushes
`SubMesh::new(layer.material_id, slab)`, so for material-layered walls and slabs
the same field carried an `IfcMaterial` id. Following it to source landed on the
wrong entity, with nothing to tell the caller which kind of id it had.
TWO DISJOINT FIELDS, never both: `geometry_item_id` is always a representation
item, `material_layer_id` is always an `IfcMaterial`. `with_style_metadata`
takes the DISCRIMINATOR rather than the destination field, so a caller cannot
put a material id in the wrong one by picking the wrong setter -- a two-setter
API would leave the confusion one typo away.
THE DISCRIMINATOR LIVES ON `SubMeshCollection`, set where the layered slabs are
built, because it is a property of how the collection was built and is uniform
across it. `geometry_class == GEOM_CLASS_LAYER_SLICE` cannot answer it, and this
was checked rather than assumed: that class is stamped in `element.rs` from
`is_material_layer_sliceable`, a static material-index check made BEFORE the
geometry runs, while `try_layered_sub_meshes` can still return `None` at several
points and fall through to the representation-item path. Rep-item ids can carry
class 3.
The voided rebuild in `voids/mod.rs` INHERITS the flag rather than hard-coding
rep-item. It is false there today, but hard-coding would let a future layered
producer be silently relabelled -- the exact confusion this removes.
CACHE FORMAT v14, because `sections/geometry.ts` writes `MeshData` field by
field and a cache-restored session would otherwise lose the identity. Bumped
rather than read leniently: the record is positional, so a v13 reader handed a
v14 record would take the origin's first f64 out of the two new u32 and every
field after it would be garbage. A v14 reader still reads v13 records -- the
fields are version-gated and degrade to `undefined`, the same state the runtime
uses where identity is genuinely merged away, so an old cache degrades to
"unknown" and never to a WRONG id.
THE ABSENT SENTINEL IS 0xFFFFFFFF, NOT 0, and a test caught me getting this
wrong. `layers.rs` uses `0` for a layer with no material -- an air gap -- so 0
is a REAL value that must round trip. The first version treated it as absent,
which is the valid-but-falsy trap and would have dropped exactly the layers
hardest to notice missing. Every read now compares against the sentinel or
`undefined`; nothing tests these ids for truthiness.
MODULE SIZE. Five Rust rows are raised with a per-row justification and the
digest re-pinned, because adding a field to a boundary struct necessarily grows
the boundary file and `zero_copy/mesh.rs` cannot absorb 2 fields + 2 getters + a
paired setter + propagation at three clone sites within its budget. The TS
allowlist forbids raises outright, so `types.ts` was SPLIT instead: the
self-contained symbolic-representation family moves to `symbolic-types.ts` (it
is re-exported, so the public surface is unchanged), taking types.ts from 459 to
381 and letting its row be DELETED -- which is what the gate asks for. The
worker was brought back to exactly its budget.
VERIFIED AT THE REAL BOUNDARY, not by reading:
duplex.ifc through processGeometryBatch:
436 meshes with geometryItemId, 186 with materialLayerId,
BOTH = 0, neither = 0
Paired probe: conflating the two fields (setting both, as before the fix)
turns the new contract test red with "186 meshes carried BOTH".
The contract test asserts non-vacuity in both directions -- some mesh must carry
each field -- because "never both" is satisfied trivially by never setting
either.
One instrument bug found on the way: `scripts/lib/mesh-via-prepass.mjs` mirrors
`convertMeshCollectionToBatch` field by field, so it silently dropped the new
ids and my first probe read zeros through it. A field the real converter carries
and that facade drops is invisible to every script that reads through it.
2162 Rust tests, clippy -D warnings clean, 621 script tests, 106 cache tests,
73 wasm-contract assertions, typecheck clean.
Closes#3199
* fix(3199): never emit a source id of 0, mirror the ids on the REST wire, and rename material_layer_id (#3210 review)
Review round on #3210. The three defects below were raised by the peer session
that filed and claimed #3199 and had built its own branch for it; the rest came
from Codex, CodeRabbit and a /simplify + /code-review pass. Three of these are defects in
the change as first written.
A SOURCE ID OF 0 IS NOT AN ENTITY, and the first version shipped one.
`IfcMaterialLayer.Material` is OPTIONAL, so `material_layer_index.rs` reads it
as `get_ref(0).unwrap_or(0)` and an air gap arrives with `material_id == 0`.
That is the DECODER'S ABSENCE SENTINEL, not an id -- STEP instance names start
at #1. Storing it made the slab claim to be a slice of `IfcMaterial #0`, and a
host following it landed on nothing, which is the exact defect #3199 exists to
remove, one field over. MEASURED at the real boundary: 12 slabs of duplex.ifc
reported `IfcMaterial #0` before the filter. `with_style_metadata` now maps 0 to
`None` on BOTH fields, at the setter rather than at each producer, so the next
producer does not have to remember.
This makes my earlier reasoning WRONG, not just incomplete: I had argued 0 was a
real value that must round trip, and built the cache's 0xFFFFFFFF sentinel on
that. The sentinel is still right -- an absence marker the domain can produce is
one upstream change from being wrong again -- but its stated REASON was false in
three places (the writer, the v14 ledger, and a cache test citing a measurement
the same PR had invalidated). All three now tell one story.
THE REST MIRROR was untouched. `apps/server/src/types/mesh.rs` re-exports the
very struct that changed, so the server had started sending `material_id` while
`packages/server-client/src/types.ts` declared no such field and
`convertServerMesh` dropped it -- the REST path's identity died at the
converter. Both are fixed, with a Rust test pinning the wire NAMES and the
ABSENT ENCODING: `skip_serializing_if` makes `None` a MISSING KEY, and a TS
optional means absent rather than nullable, so the two agree only while that
attribute stays on.
RENAMED `material_layer_id` -> `material_id`. It holds an `IfcMaterial` express
id, not an `IfcMaterialLayer` one, and `IfcMaterialLayer` is a real and
different entity -- so the old name did not merely abbreviate, it pointed at the
wrong schema type. AGENTS.md: "New surface still gets the EXPRESS name, once."
Cheaper now than after it ships as public API on four packages.
TESTS THAT COULD NOT FAIL, found by mutation rather than by reading:
- Deleting BOTH spreads from `convertMeshCollectionToBatch` left
packages/geometry at 353/353 green. That converter is the only path by
which the viewer's main thread sees the ids, and every other #3199 test
reads the raw MeshCollection, which is upstream of it. Three tests added;
they fail with the spreads removed.
- The block header claimed "one test below reads through the facade on
purpose" and none did, so the `mesh-via-prepass.mjs` fix shipped with no
coverage -- the facade being the thing that actually went wrong. Added, and
it cross-checks against the raw collection rather than a fixed count, so it
cannot go vacuous when the fixture changes.
- `voided_submesh_entry_reports_layer_ids_as_material_ids` claimed to cover
the sub-mesh-by-sub-mesh rebuild. It does not: a layered wall returns at the
`try_layered_sub_meshes` early exit and never reaches it, and replacing the
inheritance with a hard-coded `false` leaves everything green. The comment
now says so. The code stays an inheritance deliberately; it is defensive and
honestly untested rather than dishonestly claimed.
MODULE SIZE, redone: the TS allowlist now has NO raises at all. Splitting the
self-contained symbolic wire shapes out of `server-client/src/types.ts` (the
same split already made in `@ifc-lite/geometry`, re-exported so the surface is
unchanged) ratchets that row 772 -> 627, and `packages/geometry/src/types.ts`'s
row is deleted. Both directions the gate asks for, neither a raise.
STATED, NOT SILENTLY SHIPPED -- three things this PR does not fix:
#3211 five style lookups in element.rs still read a sub-mesh id as a
representation item on the layered path; safe only because express ids
are unique per file, which is an accident doing load-bearing work
#3215 the Parquet transport carries neither id, so drill-to-source works on
JSON and not on the binary path
#3216 npm drives the Cargo version, so this change -- breaking for the
published `ifc-lite-processing` crate -- ships as a minor bump, with no
cargo-semver-checks anywhere to catch it
BYTE IDENTITY, verified independently rather than inherited: duplex.ifc through
scripts/perf/ab.sh gives fingerprintDrift=false, 486/486 meshes, 52053/52053
vertices, 30264/30264 triangles. motif-ifc-ad measured the same on
schependomlaan (6057/423949/257790). Neither of us has a trustworthy TIMING
verdict: ab.sh refused one both times with tooNoisy, this machine running five
Claude sessions at load ~10.
2164 Rust tests, clippy -D warnings clean, 73 wasm-contract assertions, 106
cache tests, 356 geometry tests, viewer shard 2 at 1374, typecheck and lint
clean, module-size gate green.
Carry representation-item identity across the wasm boundary, and stop delivering material ids in the same field.
9
+
10
+
`MeshData` gains two DISJOINT fields. `geometryItemId` is always the `IfcRepresentationItem` a mesh was tessellated from, so a host can drill from a rendered piece into an `IfcWindow`'s pane or frame and navigate to that entity in source. `materialId` is always the `IfcMaterial` whose layer a mesh slices. Never both — a consumer that ignores the distinction still cannot read one as the other.
11
+
12
+
The router already kept each item's STEP id and it already reached the server REST payload; `MeshDataJs::from_mesh_data` did not copy it, so the browser never saw it. And for material-layered walls and slabs the same field carried the layer's `IfcMaterial` id, so following it to source landed on the wrong entity with nothing to warn the caller.
13
+
14
+
`geometryClass === 3` cannot discriminate the two: it is stamped from a static material-index check made before the geometry runs, while the layered path can bail at runtime and emit representation-item submeshes under that class. The discriminator therefore lives on `SubMeshCollection`, set where the layered slabs are built.
15
+
16
+
Neither field is ever `0`. `IfcMaterialLayer.Material` is optional, so an air gap reaches the mesher as `material_id 0` — that is the decoder's "no reference" sentinel, not an entity, and STEP instance names start at `#1`. Twelve slabs of `duplex.ifc` reported `IfcMaterial #0` before this was filtered at the setter. An air-gap slab is still meshed; it simply reports no material.
17
+
18
+
Both fields cross the boundary, both wasm converters carry them, the REST wire shape and `convertServerMesh` carry them, and the cache format gains them at v14 — without that, a cache-restored session silently lost the identity.
19
+
20
+
BREAKING FOR THE RUST CRATE, and this changeset cannot express it. `ifc-lite-processing` is published to crates.io (`scripts/release-crates.mjs`), `MeshData` gains a public field, and `with_style_metadata` goes from two arguments to three — both break a struct literal or a call downstream, which `rust/export/src/usd/tests.rs` demonstrates in-repo. `scripts/sync-versions.js` derives the Cargo workspace version from the highest npm package version, so a `minor` here ships 6.0.0 → 6.1.0 and a consumer pinned to `ifc-lite-processing = "6"` breaks on `cargo update`. Nothing gates this: there is no `cargo-semver-checks` anywhere in the repo.
0 commit comments