Skip to content

Commit ffe80a7

Browse files
authored
refactor(geometry,processing,wasm): split four modules back under budget, and collapse three hand-written clones (#3199 follow-up) (#3235)
* refactor(geometry,processing,wasm): split four modules back under budget, and collapse three hand-written clones (#3199 follow-up) #3210 raised four allowlist budgets to land the representation-item work. AGENTS.md says prefer splitting to allowlisting, so this pays that back: every budget goes DOWN, none up, each exactly its file's line count with no slack, and no new file needs a row. rust/geometry/src/mesh.rs 1543 -> 1012 tests to mesh_tests.rs rust/processing/src/element.rs 966 -> 812 colour to element/element_color.rs rust/wasm-bindings/src/zero_copy/mesh.rs 764 -> 754 three clones collapsed rust/geometry/src/router/layers.rs 686 -> 637 tests to layers_tests.rs The two test moves use the house `#[cfg(test)] #[path = "<name>_tests.rs"]` pattern; layers.rs already used it for `layers_cycle_tests.rs`. Test-fn counts are unchanged (24 and 4) and the moved blocks are byte-identical modulo the licence header. `element_color.rs` is a real seam: colour and material-name resolution for an element's sub-meshes is a pure function of (geometry id, style map) with its own bounded walk, and the mesh production it serves stays in element.rs. MeshDataJs had THREE hand-written 21-field copies -- `get`, `takeMesh`, and `MeshCollection`'s own `Clone` -- which that file's allowlist row names as a cost in as many words. Deriving `Clone` reduces them to `.cloned()`, `mem::take` and `.clone()`. Be precise about what that buys, because the obvious claim is wrong: an exhaustive struct literal that OMITS a field is a COMPILE error, so those literals were never silently lossy. What they were is three places to edit for one field. `Default` is what makes `takeMesh` a one-liner, and it also makes `..Default::default()` legal, which WOULD be lossy. One literal remains, in `new` (spelled `Self { .. }`, which is why grepping `MeshDataJs {` misses it), and it must stay exhaustive. `Default` is hand-written, not derived, because the derive DISAGREED with `new`: it gives `false` for the texture repeat flags where `new` sets `true`, so a `default()`-built mesh plus setters would clamp a texture that should tile. `default_agrees_with_new_on_the_fields_new_does_not_take` pins that, and it has teeth -- flipping one flag in the impl fails it with the intended message. It runs natively in `cargo test --workspace`, which the wasm crate is part of. The second `takeMesh` on one index now reports a default mesh rather than a metadata-bearing husk. That is an observable change on a published package, so it carries a patch changeset for `@ifc-lite/wasm` rather than riding in silently -- the earlier draft claimed "no published API changes", which conflated the type surface with the behaviour. The contract test anticipated the change ("a switch to `mem::take` on the whole struct would change what a second call reports, and this suite should be the thing that notices") and permitted both answers; that permissive form is spent, so it pins the exact behaviour now. The only in-repo caller, geometry.worker.ts, takes each index exactly once. Gates, all run here: `cargo test --workspace` 172 binaries 0 failures; clippy `--exclude ifc-lite-wasm --all-targets -D warnings` clean; `pnpm typecheck`; `pnpm lint`; `pnpm test` 94/94; `check:api-surface` matches; docs regions clean; `pnpm test:wasm-contract` 73 passed 0 failed WITH fixtures present, since it exits 0 silently without them; module-size ratchet 5/5. Ten things review caught across three rounds. Recording them because the pattern in most is one thing: a check that could not observe the case it was trusted for. Removing a `pub(crate) use` only `#[cfg(test)]` consumed left `cargo build` green and broke the test build -- the trap AGENTS.md names. Re-adding it failed clippy as unused in the non-test build. Importing the constant from core in the test then SHADOWED the glob, making the assertion compare it with itself. Now read back through element_color.rs; mutation-proved a private copy there fails it. The third clone site survived a draft that claimed the mechanism was gone. "Removes every `MeshDataJs { .. }` literal" was false: `new` spells one `Self { .. }`, invisible to that grep. `#[derive(Default)]` silently disagreed with `new`. The `takeMesh` behaviour change needed a changeset. `element.rs`'s budget carried 4 lines of unearned slack. Two stale pointers to `element.rs` for a constant that moved. Two contract-test comments describing a divergence this change removes. * docs(wasm): name the JS-visible property in the takeMesh contract CodeRabbit on #3235, minor and correct. The doc said `express_id`, the Rust field name. This text is generated into `packages/wasm/pkg/ifc-lite.d.ts`, whose audience is JS callers, and they read `expressId` — so the declaration described a property they cannot access. Fixed in the source and regenerated via scripts/build-wasm.sh rather than editing the generated output, which AGENTS.md forbids.
1 parent 180a896 commit ffe80a7

18 files changed

Lines changed: 958 additions & 846 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@ifc-lite/wasm": patch
3+
---
4+
5+
`MeshCollection.takeMesh(i)` now leaves a DEFAULT mesh behind rather than a metadata-bearing husk.
6+
7+
`takeMesh` is read-once by contract and moves the vertex data out. It used to move the buffers field by field and copy the scalars, so a second read of the same index still reported the real `expressId`, `color`, `geometryClass`, `origin`, `localBounds` and `localToWorld` alongside empty buffers. It now moves the whole struct, so a second read reports `expressId 0`, `color [0,0,0,0]`, `geometryClass 0`, `origin [0,0,0]` and no bounds.
8+
9+
This only affects a consumer that calls `takeMesh(i)` and then reads metadata for the same `i` again. That second read is affected whether it goes through `takeMesh` or `get`, because the data is gone from the collection either way. `get` on an index never taken is unaffected. The documented contract was already read-once and the in-repo streaming path takes each index exactly once, so nothing here changes. Read the metadata before taking, or use `get` for every read of that index.
10+
11+
The change comes from collapsing three hand-written 21-field copies of `MeshDataJs` into a derived `Clone`, which is what removes the per-field edit cost that #3199 paid three times over.

packages/wasm/pkg/ifc-lite.d.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -891,7 +891,10 @@ export class MeshCollection {
891891
* worker reads each mesh exactly once, so moving avoids the full vertex-
892892
* data clone `get` pays — one fewer copy of positions/normals/indices/uvs/
893893
* texture per mesh (the JS getters still do the single Rust→JS copy). Calling
894-
* it twice for the same index yields the second call an empty mesh.
894+
* it twice for the same index yields the second call a DEFAULT mesh:
895+
* `expressId` 0 and every buffer empty, rather than the metadata-bearing
896+
* husk the hand-written copy used to leave. The method is read-once by
897+
* contract and the wasm-contract test pins this.
895898
*/
896899
takeMesh(index: number): MeshDataJs | undefined;
897900
/**
@@ -1015,6 +1018,27 @@ export class MeshCollection {
10151018

10161019
/**
10171020
* Individual mesh data with express ID and color (matches MeshData interface)
1021+
*
1022+
* `Clone` is derived so the three sites that used to enumerate all 21 fields
1023+
* by hand -- `get`, `takeMesh` and `MeshCollection`'s own `Clone` -- reduce to
1024+
* `.cloned()`, `mem::take` and `.clone()`. Adding a field to #3199 meant
1025+
* editing three literals in lockstep; the allowlist row for this file records
1026+
* exactly that cost.
1027+
*
1028+
* To be precise about what this does and does not buy, because the obvious
1029+
* claim is wrong: an exhaustive struct literal that OMITS a field is a compile
1030+
* error, so those literals were never silently lossy. What they were is three
1031+
* places to edit for one field, and `..Default::default()` is the shortcut a
1032+
* hurried author reaches for when the compiler complains -- which WOULD be
1033+
* silently lossy. `Default` is what makes `takeMesh` a one-liner, so the rule
1034+
* is: TWO literals remain, `new` and `Default` below (both spelled
1035+
* `Self { .. }`, which is why a grep for `MeshDataJs {` finds neither). Both
1036+
* must stay exhaustive AND must agree on every field `new` does not take as an
1037+
* argument. The compiler catches an omitted field in either; it cannot catch
1038+
* the two DISAGREEING, which is the failure the pair exists to prevent, so
1039+
* `default_agrees_with_new_on_the_fields_new_does_not_take` covers that.
1040+
* Never spread `Default` into `new` -- `new` is the only place a field's
1041+
* initial value is decided, so a field defaulted there is inert everywhere.
10181042
*/
10191043
export class MeshDataJs {
10201044
private constructor();

0 commit comments

Comments
 (0)