Add the GLB and glTF 3D model FileDef families - #5782
Conversation
Introduce a shared ModelDef base (extends FileDef) with two leaves: StlDef (.stl) and ThreeMfDef (.3mf). Each extracts format metadata in extractAttributes (pure-JS parsers ported from the handoff realm) and renders a deterministic inline SVG silhouette across atom/fitted/ embedded/isolated — no WebGL, prerender-safe. - ModelDef: shared Model3DInfoField scene facts, silhouette preview, and a thumbnailUrl seam for the shaded-PNG follow-up (CS-12401). - StlDef: ASCII/binary detection, facet/normal/degenerate counts, color-data flag, bounding-box extents. - ThreeMfDef: OPC ZIP unzip (fflate), model-part + slicer-config parse (objects, materials, plates, print parts, extruders, bounds). - Register .stl/.3mf in file-def-code-ref; add fflate dependency. Shaded raster PNG previews are intentionally deferred to CS-12401; the SVG silhouette is the preview until then. CS-12053, CS-12054 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Register `fflate` as an async module shim in host externals.ts and add it to host deps. Bare imports in realm-served base modules resolve only via the shim registry, not node_modules, so the extractor's `import 'fflate'` would 404 at runtime without this. - Render 3MF materialNames via a joined getter instead of interpolating a string[] directly (glint: string[] is not a ContentValue). - prettier formatting. CS-12053, CS-12054
isolated + embedded now mount an interactive three.js orbit viewer (ModelViewer + renderModel modifier), matching the handoff prototype. three.js + OrbitControls + STL/3MFLoader load lazily from a CDN (esm.sh) at client render time only — the sanctioned Boxel pattern for card libraries — so the engine never runs during extraction/indexing. - fitted keeps the pure SVG silhouette (collection-tile budget). - The silhouette is the loading placeholder and the fallback whenever WebGL/the CDN engine is unavailable (e.g. prerender), so the static prerendered HTML degrades gracefully. - Full GPU teardown on unmount (dispose + forceContextLoss). CS-12053, CS-12054
The WebGL viewer's `fetch(url, { credentials: 'include' })` was blocked
cross-origin (host 4200 → realm 4201): a credentialed CORS request is
illegal against the realm's wildcard Access-Control-Allow-Origin, so the
STL/3MF bytes never loaded and every model fell back to the silhouette.
Drop `credentials: 'include'` — realm auth is carried by an Authorization
header injected by the host auth service worker on the GET (the same path
that lets <img src> load realm images), not by cookies. Verified in the
browser: complex STL and moderate 3MF now render in the live orbit viewer.
CS-12053, CS-12054
Six sample model files (stl + three-mf, simple/moderate/complex) under experiments/model-samples/ so StlDef/ThreeMfDef can be exercised manually in the app — each is indexed directly as its FileDef subtype. CS-12053, CS-12054
Rework the isolated view to mirror the handoff prototype: a header bar with a mono extension pill, a bordered live-viewer stage, and a two-column (stage + property inspector) body that collapses to one column on narrow containers. Metadata reads as grouped "3D model" / "STL mesh" / "3MF package" sections with uppercase mono labels; the viewer hint is a small mono chip bottom-right. Also aligns tokens with the design conventions: semantic role tokens (--foreground/--muted-foreground/--card/--muted/--border) instead of numbered palette values, rem units, and no `font` shorthand. CS-12053, CS-12054
Match the handoff realm's inspector: wrap the property groups in a bordered, rounded --card panel and separate rows with hairline top borders (first row none), with mono uppercase group headings. Applied to the shared "3D model" group and the leaf STL/3MF groups. CS-12053, CS-12054
Fitted now mounts the WebGL viewer instead of only the silhouette, but: - lazy via IntersectionObserver — the engine boots only when the tile is on-screen, so a grid of many models doesn't exhaust the browser's WebGL context budget; off-screen tiles keep the silhouette. - non-interactive — no orbit controls, no gesture trapping, no hint, so fitted tiles render a static model and the enclosing grid scrolls normally. During prerender/indexing the engine never loads (no CDN/WebGL), so fitted still falls back to the silhouette there. Verified in the browser across badge/strip tile sizes. CS-12053, CS-12054
Extract the STL/3MF metadata parsers into pure `.ts` modules (stl-meta-extractor / three-mf-meta-extractor, mirroring png-meta-extractor) so they're unit-testable without the card-api harness, and harden them per review: - STL parsing is now a single streaming pass over the DataView with no per-vertex array, so index-time memory stays flat regardless of model size; degeneracy is counted per facet as read, fixing the misaligned count when non-finite vertices were dropped. - parseThreeMf returns undefined for any unparseable input (non-ZIP, no model part, malformed XML) so the leaf falls back cleanly to base FileDef; removed a redundant modelPart regex clause. - Both leaves take an optional fileSizeLimitBytes, defaulting to the realm's standard DEFAULT_FILE_SIZE_LIMIT_BYTES. Over the cap they skip the parse and return base attributes only (keeping the 3D type + live viewer), rather than throwing and demoting to a plain file. - Share Model3dData and getExtension; replace the ~40-line inspector style block duplicated across the three isolated templates with a data-driven ModelInspectorSection component. - Document that model3d.vertices counts vertex records as the format stores them (not comparable across formats). - Add unit tests for parseStl, parseThreeMf, and silhouettePath. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drives the real render/file-extract route (the indexer's path) over an ASCII STL and a real 3MF ZIP served from a test realm, asserting the file-meta search doc carries the parsed model3d / stlMetadata / threeMfMetadata, plus the mismatch-fallback for a .stl whose bytes aren't STL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "Build test web assets" (rolldown) job failed with MISSING_EXPORT cascading from card-api.gts: the unit test value-imported silhouettePath from model-file-def.gts, which pulled model-file-def -> card-api into the host test bundle, where card-api's long-standing loose type imports (CodeRef/ToolContext/LocalPath/SingleCardDocument imported without the `type` keyword) become hard rolldown errors. Move silhouettePath into a pure model-silhouette.ts (no card-api dependency) and import the test from there; model-file-def re-exports it for back-compat. No host test now value-imports a card-api-chained .gts, so card-api is no longer bundled into the test assets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ent-side size Rework STL/ThreeMf FileDefs per review feedback: - Rename ModelDef -> ThreeDModelDef (and model-file-def.gts -> three-d-model-def.gts) so the base class no longer reads like a shared base type. - Fitted view now renders a static cube icon + filename (mirroring the audio FileDef) instead of a live per-tile WebGL viewer, so a grid of tiles can no longer exhaust the browser's WebGL context budget. The shaded-thumbnail seam (`thumbnailUrl`) is kept for CS-12401. - Drop the deterministic SVG silhouette (model-silhouette.ts) and the index-time bounding box (sizeX/Y/Z). Physical dimensions now come from the live client-side viewer, which reads the true, transform-correct bounds off the loaded geometry — so the 3MF-transform approximation disappears too. - STL extraction is header-only: it reads the binary header (facet count, COLOR= flag) or the ASCII prologue (solid name) and never scans the facet body. - 3MF extraction is a bounded, DOM-free prologue read: fflate's filter decompresses only the `.model` + `model_settings.config` entries (skipping embedded thumbnails/textures and refusing oversized entries as a ZIP-bomb backstop), and metadata is regex-read from the model part's prologue rather than DOM-parsing the geometry. Also validates the 3MF `<model>` core root so unrelated `.model` XML is rejected. - Thread the realm's configured fileSizeLimitBytes through FileDefAttributesExtractor -> the leaf extractAttributes, so the size cap tracks the write-path limit instead of a hard-coded default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…creen Reframe the WebGL/static split around render context rather than format, and fix the context-budget leak the review flagged: - Prerender gate: `ModelViewer`'s modifier no-ops when `globalThis.__boxelRenderContext` is set (server-side indexing/prerender), so WebGL and the CDN engine import are never started there — the static thumbnail is the deterministic prerender representation, instead of relying on a doomed CDN import failing. - Viewport gate: the viewer now lazy-boots when the element scrolls on-screen and disposes its WebGL context (renderer.dispose + forceContextLoss) when it scrolls off, re-booting from cached bytes on re-entry. So an embedded strip of models holds a context only for the visible ones and never exhausts the browser's ~16-context budget — the actual fix to the "contexts grow monotonically" finding. Fitted stays on the static thumbnail (a grid can show more tiles than the context cap at once; pooled-renderer live fitted is tracked in CS-12401). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends the 3D model family (StlDef/ThreeMfDef, CS-12053/CS-12054) with the two glTF forms, so `.glb` and `.gltf` files get the same interactive viewer and inspector as the other model formats. GlbDef and GltfDef are thin ThreeDModelDef leaves — they inherit the shared Three.js orbit viewer, whose GLTFLoader default path already renders both the binary and JSON forms, plus the fitted thumbnail and the isolated body. Each adds a `gltfMetadata` field and an extension-guarded, size-capped `extractAttributes`, mirroring StlDef. The metadata is read from the glTF structure itself rather than a geometry scan: a glTF describes its accessors, so `gltf-meta-extractor.ts` reads the vertex and triangle counts, mesh/material/node/animation/texture counts, and the bounding-box dimensions straight from the header — the JSON of a `.gltf`, or the JSON chunk of a `.glb` (its BIN chunk is never touched). Returns undefined for non-glTF bytes, which the leaf turns into a FileContentMismatchError so a mislabeled file falls back to the plain FileDef. Routes `.glb` → GlbDef and `.gltf` → GltfDef, and adds unit tests for the extractor (both container forms, indexed/non-indexed/strip triangle counts, bounds, and the non-glTF fallback) plus code-ref routing assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Preview deploymentsHost Test Results 1 files 1 suites 1h 27m 6s ⏱️ Results for commit fa2265c. Realm Server Test Results 1 files ±0 1 suites ±0 14m 53s ⏱️ - 2m 58s Results for commit 3ddb1c0. ± Comparison against earlier commit cd4adf3. |
b97f29e to
b30b584
Compare
…subclasses' into cs-12243-3d-model-formats-glb-gltf-stl-3mf # Conflicts: # packages/base/stl-model-def.gts # packages/base/three-d-model-def.gts # packages/base/three-mf-def.gts # packages/host/tests/acceptance/model-file-extract-test.gts # packages/host/tests/unit/file-def-code-ref-test.ts # packages/host/tests/unit/model-meta-extractor-test.ts # packages/runtime-common/file-def-code-ref.ts
|
[Claude Code 🤖] The merge from the base branch was more than marker cleanup, because the base's squashed commit rewrote the 3D family onto the shared file-format shells while this branch's GLB/glTF leaves were written against the earlier hand-rolled architecture. The merge commit (cd4adf3) ports them:
Typechecks pass across runtime-common and host; template-lint is clean on the touched |
…mats-glb-gltf-stl-3mf
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] I reviewed this as a format-registration change plus a parser: whether a new extension landed on every site that has to learn about it, whether the header read is faithful to the glTF spec, and whether the numbers it surfaces mean what their labels say. Everything below was traced against the checked-out branch.
Bottom line: the registration is complete, the container handling is spec-correct, and the design decision to read the header rather than scan geometry is the right one for this format. One value is wrong as labelled — the bounding box is mesh-space, not model-space, so any glTF whose nodes carry a scale or translation reports the wrong Size — and that is the one thing I would settle before merge. Everything else is non-blocking.
What lands right
- Registration is genuinely complete, which was not obvious.
FILEDEF_CODE_REF_BY_EXTENSIONwas the only site that needed adding:file-type-profile.tsalready carriesmodel/gltf-binaryandmodel/gltf+jsonprofiles and theglb/gltfentries in its extension→MIME map, andmodel3d-preview.gtsalready falls through toGLTFLoaderfor anything that is not.3mfor.stl. I checked each rather than assuming, because a partly-registered extension fails silently as a generic binary. - The GLB container read is correct against the spec. Magic at 0, version at 4, chunk length at 12, chunk type at 16, JSON chunk required first, payload at 20 — all right, and the 4-byte space padding is tolerated because
JSON.parseignores trailing whitespace. The test'sbuildGlbbuilds exactly that layout, padding included. trianglesForgets the topology math right, including thecount - 2for strips and fans and theMath.max(0, …)floor for a degenerate primitive, and correctly contributes zero for point and line modes rather than silently treating everything as triangles.- The indexed / non-indexed distinction is right: an indexed primitive counts its index accessor, a non-indexed one draws its POSITION vertices directly. That is the detail most header readers get wrong, and both arms have a test.
- The size-cap branch deliberately does not throw, keeping the 3D card with an empty inspector rather than demoting the file. That is the right trade and the comment explaining it is the most valuable one in the change — see recommendation 3, which is about applying the same reasoning one branch over.
Recommendations
- Apply the node transforms before reporting a bounding box, or stop calling it the model's size. Mechanism, two concrete failure shapes, and both ways out are in the thread on the bounds loop in
gltf-meta-extractor.ts. The one item I'd resolve before merge. - Require
asset.versionrather than a truthyasset. Today{ asset: 'hello' }parses as glTF, so a mislabelled.gltfkeeps the 3D card instead of falling back. One-line suggestion in that thread. Non-blocking. - Decide whether an unreadable-but-real glTF container should demote to a plain
FileDef. The size-cap branch argues it shouldn't; the parse-failure branch does it anyway. Follow-up. - Drop the full-file copy in
extractGltfAttributes(pass theUint8Arraythrough), and narrow the module comment's "never touched" claim to describe the parse rather than the I/O — the whole file is already buffered by then. Non-blocking. - Add the scene-graph, malformed-container, and truthy-
assettest cases.buildGlbmakes all three cheap. Non-blocking. - Confirm whether the ~2.1 MB of unreferenced STL/3MF sample binaries are meant to ship with this change — the description says the opposite, and there are no glTF samples among them. Needs your answer, not a code change.
Adjacent, out of scope
vertexCountsums POSITION counts across primitives, so a mesh split into several primitives counts its shared seam vertices more than once. That is the conventional reading of "vertex count" for glTF and matches what most tools report, so it is right — worth knowing it is a deliberate convention rather than a total of distinct positions.- The
.gltfexternal-resources limitation is called out honestly in the description, and it is a real one: a.gltfreferencing external.bin/textures gets correct metadata and a thumbnail-only viewer. The inspector gives no signal about that, so a user sees full statistics next to a scene that never loads. AbufferViews/bufferscheck could tell them ("references external resources"), which would be a genuinely useful field for this format. Not this change. Number(position.min[axis])is not validated, so a file with non-numeric bounds yieldsNaN × NaN × NaNwithhaveBoundsstill true. Only reachable from a malformed file, and it degrades to a strange string rather than a throw — but it is the same input-validation gap as recommendation 2, and a singleNumber.isFiniteguard covers both the axis reads and thehaveBoundsdecision.
Generated by Claude Code
|
|
||
| if ( | ||
| Array.isArray(position?.min) && | ||
| Array.isArray(position?.max) && | ||
| position.min.length >= 3 && | ||
| position.max.length >= 3 | ||
| ) { | ||
| for (let axis = 0; axis < 3; axis++) { | ||
| min[axis] = Math.min(min[axis]!, Number(position.min[axis])); | ||
| max[axis] = Math.max(max[axis]!, Number(position.max[axis])); | ||
| } | ||
| haveBounds = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Claude Code 🤖] The bounding box is the union of mesh-local POSITION bounds, but it is presented as the model's size. For any glTF whose nodes carry a scale or a translation — which is most of them — the "Size" row is wrong, sometimes by orders of magnitude.
The mechanism. A glTF accessor's min/max are in the coordinate space of the mesh they belong to. Meshes are placed into the scene by nodes, and a node carries either a matrix or a TRS triple (translation / rotation / scale), applied down the children hierarchy from the roots listed in scenes[n].nodes. This loop walks doc.meshes directly and never looks at doc.nodes or doc.scenes, so the transforms are not applied.
Two concrete failures:
- Scale. A Blender export of a 2 m cube commonly writes unit-cube POSITION bounds plus a node with
scale: [2, 2, 2]. This reports1 × 1 × 1. - Instancing. A scene that references one mesh from several nodes at different translations — a row of chairs, a bolt pattern — reports the extent of a single instance, not the row. The union over
meshesdeduplicates exactly the thing that makes the scene big.
Rotation compounds both: an axis-aligned box in mesh space is not axis-aligned in scene space, so even a pure rotation changes the true AABB.
Why it matters more than an off-by-a-bit. This value lands in the inspector's Size row next to STL's, and stl-meta-extractor derives its dimensions from an actual geometry scan — so the same row means "true extent" for one format and "extent of one mesh before it was placed" for the other, with nothing on screen distinguishing them.
Two ways out.
- Walk the scene graph. It stays a header-only read —
nodes,scenes, and the TRS values are all in the JSON, and the buffers are still never touched. Roughly: recurse fromscenes[doc.scene ?? 0].nodes, compose each node's local matrix into the parent's, and for a node with amesh, transform that mesh's eight AABB corners and fold them into the running min/max. About 40 lines, and it makesdimensionsmean what the label says. - Say what it is. If the graph walk is out of scope, then don't call it Size — label the row "Mesh bounds" for glTF, and change the comment here and on
Model3dMetadataField.dimensionsto say the transforms are not applied. A field documented as mesh-space is honest; one documented as "model-space bounding box" is not, since model space is precisely what the node transforms produce.
I'd take (1) — the data is already parsed and this is the ticket's headline "bounding box" deliverable — but (2) is a legitimate scoping call as long as the label moves with it.
Scope. Correctness of a user-facing value, introduced here. This is the one item I would resolve before merge, in one form or the other.
There was a problem hiding this comment.
Fixed in 39e7c53. `dimensions` is now model-space: each mesh's POSITION AABB is placed through the scene graph, composing every node's `matrix`/TRS parent-to-child and transforming all eight corners (so it stays correct under rotation, not just scale/translation). A scaled cube, an instanced row, and an explicit-matrix node are asserted in the unit tests. A document that never places a mesh through a node keeps the mesh-local union, so node-less fixtures are unchanged. Took option (1) as you recommended.
| // Every glTF asset carries an `asset` object with a version; its absence is | ||
| // the cheapest reliable "this isn't glTF" signal. | ||
| if (!doc || typeof doc !== 'object' || !doc.asset) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] This accepts any JSON with a truthy asset of any shape, so a non-glTF .gltf file is typed as a glTF model instead of falling back. The comment two lines up says the check is for "an asset object with a version" — the code checks neither that it is an object nor that it has a version.
Traceable in one line, using this module's own entry point:
parseGltf(toArrayBuffer(new TextEncoder().encode(JSON.stringify({ asset: 'hello' }))))
// → { gltfMetadata: { container: 'gltf' } } — not undefinedEvery subsequent read is optional-chained (doc.asset?.version on a string is undefined), so nothing throws; the result is a metadata object holding only container. extractGltfAttributes sees a truthy parsed, skips the FileContentMismatchError, and the file keeps GltfDef — a card with a WebGL viewer that will fail to load anything and an inspector with a single Format: glTF JSON row. The fallback to plain FileDef that the mismatch error exists to trigger never happens.
The existing negative test ({ hello: 'world' }) passes because that object has no asset at all, so it doesn't reach this case.
The fix matches the comment:
| // Every glTF asset carries an `asset` object with a version; its absence is | |
| // the cheapest reliable "this isn't glTF" signal. | |
| if (!doc || typeof doc !== 'object' || !doc.asset) { | |
| return undefined; | |
| } | |
| // Every glTF asset carries an `asset` object with a version string; its | |
| // absence is the cheapest reliable "this isn't glTF" signal. | |
| if ( | |
| !doc || | |
| typeof doc !== 'object' || | |
| typeof (doc as any).asset?.version !== 'string' | |
| ) { | |
| return undefined; | |
| } |
Worth adding { asset: 'hello' } and { asset: {} } to the "returns undefined for non-glTF content" test — both pass today.
Scope. Regression risk introduced here, narrow (needs a .gltf-named JSON that isn't glTF). Non-blocking.
There was a problem hiding this comment.
Fixed in 39e7c53. The sniff now requires `typeof doc.asset?.version === 'string'` rather than a truthy `asset`, so `{ asset: 'hello' }` and `{ asset: {} }` fall back instead of keeping a glTF card. Both are now in the "returns undefined for non-glTF content" test.
| if (!parsed) { | ||
| throw new FileContentMismatchError( | ||
| `File does not contain a parseable ${containerLabel}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] This throw and the size-cap branch twenty lines up answer the same question in opposite ways, and the comment on that branch is the better answer.
The size-cap path says it explicitly: "Do NOT throw FileContentMismatchError here: that would demote the file to a plain FileDef and lose the 3D card entirely." That reasoning is right, and it applies to more than size. parseGltf returns undefined for several inputs that are real glTF the viewer would happily render — it just can't summarize them:
- A GLB that isn't version 2.
readGlbJsonrejects onview.getUint32(4, true) !== 2. glTF 1.0 GLB files exist; they're old, but they're glTF. - A GLB whose JSON chunk doesn't fit the declared length —
20 + chunkLength > buf.byteLength, i.e. a truncated download or a file with unusual chunk padding. The bytes on disk may still be enough for the client-side loader. - A GLB whose first chunk isn't JSON. The spec requires JSON first, so this is a malformed file — but again, malformed-for-summarizing, not necessarily malformed-for-rendering.
In each of those the extension already said .glb, getExtension already matched, and the user gets a plain file card with a download link instead of the 3D viewer. Compare with a 200 MB .glb, which keeps the viewer.
Distinguishing the two cases is what makes the demotion correct where it is correct: undefined because the bytes aren't glTF at all (no GLB magic and the JSON has no asset.version) should still throw — that's what makes a mislabelled .gltf fall back properly. undefined because the container is glTF but unreadable should follow the size-cap precedent and return { ...base }. parseGltf currently collapses both into one undefined, so the call site cannot tell them apart; returning a discriminated result (or a second isGltfContainer(buf) check here) is what unblocks it.
Scope. Follow-up rather than a defect in this change — the common paths are right. Worth resolving because the file already contains the argument for the other behaviour, and a reader hitting both in one function will not know which rule applies.
There was a problem hiding this comment.
Addressed in d79b7bd. `parseGltf` still returns `undefined` for a real-but-unreadable GLB (glTF 1.0 / truncated / chunks out of order), but the call site now branches on a new `isGlbContainer(bytes)` check: a GLB container keeps its 3D card (returns base facts only, mirroring the size-cap branch) instead of throwing, while bytes that aren't a glTF container at all still demote to a plain FileDef. An acceptance test proves an unreadable `.glb` keeps its GlbDef card with no mismatch flag, and unit assertions confirm the unreadable fixtures still register as containers while non-glTF bytes do not.
| let parsed = parseGltf( | ||
| bytes.buffer.slice( | ||
| bytes.byteOffset, | ||
| bytes.byteOffset + bytes.byteLength, | ||
| ) as ArrayBuffer, | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Two things here, both non-blocking.
The slice copies the whole file to read its first few kilobytes. bytes.buffer.slice(byteOffset, byteOffset + byteLength) allocates a second full copy of the file so that parseGltf can take an ArrayBuffer. For a .glb just under the cap that is a second multi-hundred-megabyte allocation whose only purpose is a type signature. parseGltf can take the Uint8Array directly and build its views from it — new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength) and new Uint8Array(bytes.buffer, bytes.byteOffset + 20, chunkLength) — with no copy and no change to its logic. That also removes the as ArrayBuffer cast, which is currently papering over slice()'s ArrayBufferLike return.
The module comment claims a saving the pipeline doesn't deliver. gltf-meta-extractor.ts opens with "The heavy buffers (a .glb BIN chunk, a .gltf's external or base64 buffers) are never touched." That is true of the parse and it is the right design, but by the time it runs, memoizedStream() has already drained the entire stream into memory via byteStreamToUint8Array, and the line below copies it again. So the claim reads as "we don't download the geometry", which isn't what happens — the whole file is fetched and buffered either way. Worth narrowing the sentence to what it actually means: the JSON chunk alone is decoded and parsed, so the cost is independent of geometry size in parsing, not in I/O. The distinction matters to whoever next wonders why a large .glb is slow to index.
The base ThreeDModelDef.extractAttributes needs the bytes anyway, so avoiding the read entirely isn't on the table here — this is about the comment matching the code, and about the one avoidable copy.
There was a problem hiding this comment.
Both fixed. `parseGltf` now takes the caller's `Uint8Array` directly and builds its views from `byteOffset` — no full-file copy, no `as ArrayBuffer` cast (39e7c53). The module/field comments were narrowed to describe the parse rather than the I/O, and updated to say the glTF bounding box is now transform-composed at index time rather than deferred to the viewer (d79b7bd).
|
|
||
| test('returns undefined for non-glTF content', function (assert) { | ||
| assert.strictEqual( | ||
| parseGltf(toArrayBuffer(new TextEncoder().encode('not a model'))), | ||
| undefined, | ||
| 'random text', | ||
| ); | ||
| assert.strictEqual( | ||
| parseGltf(gltfJson({ hello: 'world' })), | ||
| undefined, | ||
| 'JSON without an asset object', | ||
| ); | ||
| assert.strictEqual( | ||
| parseGltf(new ArrayBuffer(0)), | ||
| undefined, | ||
| 'empty buffer', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
[Claude Code 🤖] The glTF coverage is solid where it exists — building both containers in code (including the 4-byte space padding the spec requires) is the right call, and buildGlb is a genuinely reusable fixture. Three gaps, each corresponding to a thread above.
No scene-graph case. Every fixture here has meshes and no node transforms, which is exactly the shape for which the bounding-box math is correct. A fixture with nodes: [{ mesh: 0, scale: [2, 2, 2] }] and scenes: [{ nodes: [0] }] would assert 4 × 8 × 12 and currently fails with 2 × 4 × 6 — see the thread on the bounds loop. That one test is the difference between "we chose mesh-space bounds" and "we didn't notice".
No malformed-container cases. buildGlb makes these nearly free, and each pins a decision that is currently implicit:
// version 1 GLB: view.setUint32(4, 1, true) → today: undefined
// truncated: buf.slice(0, total - 8) → today: undefined
// first chunk not JSON: setUint32(16, 0x004E4942 /* BIN */, true)Whether those should demote the file to a plain FileDef or keep the 3D card is the open question in the thread on extractGltfAttributes; either answer deserves a test, because both are currently reachable by accident.
The negative test doesn't cover the shape that gets through. { hello: 'world' } is rejected for having no asset at all. { asset: 'hello' } and { asset: {} } both return a metadata object rather than undefined — that's the thread on the asset guard.
Scope. Test coverage, non-blocking; the first of the three is the one tied to a value users will read.
There was a problem hiding this comment.
Added across both commits. Scene-graph: node-scale, explicit-matrix, and instanced-translation cases assert the transformed extent (39e7c53). Malformed containers: version-1, truncated, and BIN-first GLBs, each asserting `parseGltf` → undefined and `isGlbContainer` → true so the keep-the-card path is pinned (d79b7bd). Truthy-`asset`: `{ asset: 'hello' }` and `{ asset: {} }` (39e7c53). Plus a non-finite-bounds case (no "NaN × NaN × NaN") and an acceptance test for the unreadable-`.glb` card.
| @@ -0,0 +1,2018 @@ | |||
| solid DiskBase | |||
There was a problem hiding this comment.
[Claude Code 🤖] This change carries ~2.1 MB of STL and 3MF sample files into the repo — six binaries in packages/experiments-realm/model-samples/, added by 0c50c373b5 on this branch — and nothing references them. I grepped the whole tree for model-samples: no card, test, fixture, or doc points at the directory.
Worth flagging for three reasons rather than one:
- The description says the opposite. "Extractor unit tests … all fixtures built in-code, no binaries" is accurate about the tests, and reads as a statement about the change as a whole. A reviewer taking the description at its word would not expect 2.1 MB of binaries in the diff.
- They are for the other two formats.
stl-*.stlandthree-mf-*.3mf, with no.glbor.gltfamong them — so if the intent is browsable sample content for the experiments realm, this change adds samples for everything except the formats it is about. - They are permanent. Git keeps the blobs whether or not the files are later deleted, so this is a one-way ~2.1 MB addition to every clone, for content that is currently unreachable from the app.
None of that makes them wrong — an experiments realm is a reasonable home for demo assets, and stl-complex.stl at 1.2 MB is a fair "complex" sample. But it should be a decision rather than a side effect. If they are intended: say so in the description, add the .glb/.gltf equivalents so the 3D family is covered evenly, and ideally reference them from a fixture card so they are discoverable. If they rode along from the STL/3MF work on this shared branch, they belong in whichever change ships that content, not this one.
Scope. Scope/repo-weight question, non-blocking — but it needs an answer from you rather than a code change I can suggest.
There was a problem hiding this comment.
Removed in d79b7bd — all six STL/3MF sample binaries are gone from this branch. They were unreferenced and belonged to the STL/3MF work rather than this glTF change; they can come back in a dedicated sample-content change that also includes `.glb`/`.gltf` samples and references them from a fixture card.
The reported dimensions now place each mesh's POSITION bounds through the scene graph's node transforms (matrix or TRS, composed parent-to-child), so scaled, translated, and instanced meshes report the assembled scene's extent; documents without a scene graph keep the mesh-space union. The glTF sniff requires asset.version to be a string rather than any truthy asset, non-finite accessor bounds are skipped instead of producing NaN dimensions, and parseGltf takes the caller's Uint8Array directly instead of a full-buffer copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follows up the model-space bounds / parse-guard commit with the remaining review points on #5782. - Distinguish an unreadable-but-real GLB container from bytes that aren't glTF at all. `parseGltf` still returns undefined for a glTF 1.0 / truncated / mis-ordered GLB, but the call site now checks `isGlbContainer` and keeps the 3D card (returning base facts only, like the size-cap branch) instead of demoting to a plain FileDef via the mismatch error. Only non-container bytes fall back. - Add `isGlbContainer` (the GLB-magic sniff), reused by `parseGltf` and the call site so the container test lives in one place. - Refresh the stale `Model3dMetadataField` comments: the glTF bounding box is now transform-composed at index time, not deferred to the viewer. - Tests: an acceptance case proving an unreadable `.glb` keeps its GlbDef card without a mismatch flag; unit assertions that the unreadable-GLB fixtures still register as containers and that non-glTF bytes do not. - Remove the ~2.1 MB of unreferenced STL/3MF sample binaries that rode in on this branch; they belong in a dedicated sample-content change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
All review points addressed across two commits:
|
…mats-glb-gltf-stl-3mf
Part of CS-12243 (3D model formats). Stacked on #5658 (StlDef/ThreeMfDef, CS-12053/CS-12054) — base this PR against that branch so the diff shows only the glTF additions. Per that decision, GLB/glTF are added in #5658's existing style (Three.js
ThreeDModelDefleaves), not a separate shared-shell rebuild.What
Adds the two glTF forms so
.glband.gltfget the same interactive orbit viewer and inspector as STL/3MF.GlbDef/GltfDef— thinThreeDModelDefleaves. They inherit the shared Three.js viewer (whoseGLTFLoaderdefault path already renders both the binary and JSON forms), the fitted thumbnail, and the isolated body. Each adds agltfMetadatafield and an extension-guarded, size-cappedextractAttributes, mirroringStlDef.gltf-meta-extractor.ts— reads metadata from the glTF structure itself rather than a geometry scan: a glTF describes its accessors, so vertex/triangle counts, mesh/material/node/animation/texture counts, and bounding-box dimensions come straight from the header — the JSON of a.gltf, or the JSON chunk of a.glb(the BIN chunk is never touched). This directly delivers the ticket's "vertex/triangle counts, bounding box." Non-glTF bytes →undefined→FileContentMismatchError→ graceful fallback..glb→GlbDef,.gltf→GltfDef.Tests
model-meta-extractor-test.ts): both container forms, indexed / non-indexed / triangle-strip counts, bounding box, and the non-glTF fallback — all fixtures built in-code, no binaries.GlbDef/GltfDef.Validation
ember-tsc --noEmit(host): 0 errors; base template-lint clean.Notes / limitations
.gltfthat references external.bin/texture resources renders its metadata fine but the live viewer (parsing with an empty resource path) will fall back to the thumbnail for the 3D scene; self-contained.gltf(embedded base64) and.glbrender fully.dimensionsis presented unitless (matching STL's honesty), anddisplayUnitstays inherited-empty.🤖 Generated with Claude Code