geometry decode fixes - #912
Conversation
🦋 Changeset detectedLatest commit: 56eca67 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Hey Devin T. Currie (@DTCurrie) — CI is green and no reviewer is assigned yet. Could you request one when you have a chance? Auto-comment from overwatch. Will not re-nudge for 7 days. |
809c09b to
14597b2
Compare
504223c to
b1bdd31
Compare
b1bdd31 to
d65b02f
Compare
4c1dcd3 to
55ef0ee
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 6m 46s —— View job Review
No bugs found. The two fixes are correct and minimal. Inline comments are observations, not action items. Rotation fix (
|
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 5m 11s —— View job Review: geometry decode fixes
SummaryTwo clean, focused bug fixes with thorough tests and one of the most detailed PR descriptions I've seen. No correctness issues. A few minor observations below.
|
| input | result |
|---|---|
'ply' |
'ply' ✓ |
'STL' |
'stl' ✓ |
'model/stl' |
'stl' ✓ |
'application/ply; charset=binary' |
'ply' ✓ |
'meshes/ur20/collision/base.stl' |
'base.stl' → undefined ✓ |
'package://arm/link_1.stl' |
'link_1.stl' → undefined ✓ |
File-path rejection falls out naturally: extensions like base.stl or link_1.stl don't equal 'stl' exactly.
src/lib/stl.ts — STL parser
String path is correct. stlLoader.parse(decoded) accepts binary strings from atob because STLLoader.parse calls ensureBinary first, which converts a binary string to ArrayBuffer via charCodeAt. This is well-defined Three.js behavior, not a coincidence.
Subarray handling uses mesh.buffer.slice(...) vs parsePlyInput's new Uint8Array(mesh).buffer — both produce a fresh copy, just different idioms. No bug, minor inconsistency.
See inline comment on line 26 for the one unguarded edge case (valid-length, corrupt triangle count) that carries through both parsers.
Tests
mesh.spec.ts: The binary fixture is exactly the right choice — the comment explaining why is important and should be preserved. See inline comment on line 55 for a minor fragility in the base64 helper.
meshGeometryTrait.spec.ts: The rationale doc-comment (PLY answers STL bytes with empty geometry rather than throwing) is the key insight that makes this spec necessary and non-redundant. Good.
build-frame-descriptors.spec.ts: The parameterized mesh skip table (reads a mesh declared as %s and the updated skip table) cover the normalization path end-to-end. The regression table in the PR description mapping each production change to the test that catches it is unusually useful.
Pre-existing asymmetry (not this PR's concern)
parsePlyInput calls atob(mesh) with no try/catch for malformed base64, while parseStlInput wraps it. Since the callers are ECS trait loops with no error boundary, parsePlyInput could bubble a DOMException if it ever receives a malformed base64 string. Follow-up candidate.
Verdict: LGTM with minor nits. The rotation fix and mesh content-type path are both correct, the test strategy is smart, and the PR description is thorough enough to serve as the authoritative reference for future readers.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } catch { | ||
| return new BufferGeometry() | ||
| } | ||
| return decoded.length < STL_MIN_BYTES ? new BufferGeometry() : stlLoader.parse(decoded) |
There was a problem hiding this comment.
The string-path length guard and the atob try/catch together catch everything that STLLoader throws by design. Worth noting one unguarded gap that carries through to the binary path too: for data ≥84 bytes where the triangle-count field at offset 80 claims more triangles than the body actually contains, parseBinary reads out of bounds in DataView and throws a RangeError. parsePlyInput has the same gap for corrupt binary PLY headers, so this is consistent — just worth knowing if callers ever get untrusted mesh data rather than RDK-produced blobs.
| return new Uint8Array(buffer) | ||
| } | ||
|
|
||
| const base64 = (data: Uint8Array) => btoa(String.fromCodePoint(...data)) |
There was a problem hiding this comment.
Nit: String.fromCodePoint(...data) spreads the array into the call stack. Fine for this 134-byte fixture, but it'll RangeError on anything beyond ~65 k bytes. Array.from(data, c => String.fromCharCode(c)).join('') is the safe form if this helper is ever reused for larger fixtures.
| const base64 = (data: Uint8Array) => btoa(String.fromCodePoint(...data)) | |
| const base64 = (data: Uint8Array) => btoa(Array.from(data, (c) => String.fromCharCode(c)).join('')) |
| } catch { | ||
| return new BufferGeometry() | ||
| } | ||
| return decoded.length < STL_MIN_BYTES ? new BufferGeometry() : stlLoader.parse(decoded) |
There was a problem hiding this comment.
atob returns a binary string where each character's code point is one byte, so decoded.length equals the byte count here — correct for the STL_MIN_BYTES check. Three.js's STLLoader.ensureBinary then converts it back via charCodeAt, so binary STL round-trips correctly through the string path.
| // The loader takes a whole ArrayBuffer, so a view into a larger one has to be cut out first. | ||
| // Getting this wrong is silent on binary STL: `isBinary` stops matching and the parse returns | ||
| // nothing. | ||
| const whole = mesh.byteOffset === 0 && mesh.byteLength === mesh.buffer.byteLength |
There was a problem hiding this comment.
ply.ts does the same via new Uint8Array(mesh).buffer — both create a fresh ArrayBuffer from the view's slice. slice is slightly more direct. Consistent intent, defensible divergence in spelling.
| * `ply` or `stl`, so the folding and trimming are defense on a proto this repo does not own. | ||
| */ | ||
| export const meshContentType = (raw: string | undefined): MeshContentType | undefined => { | ||
| const value = (raw ?? '').toLowerCase().split(';')[0]?.trim().split('/').at(-1) |
There was a problem hiding this comment.
The chain handles 'model/stl', 'application/ply; charset=binary', and bare 'stl' all in one expression. The test table confirms path-like strings (meshes/base.stl) fall through correctly because .at(-1) on 'base.stl' does not equal 'stl'.
| } | ||
| // Unconditional: an absent orientation means identity in the parent's frame, which is still | ||
| // R_frame⁻¹ once expressed locally. `quatFromJson` writes identity when it finds nothing. | ||
| quatFromJson(geoOrient, tmpQGeo) |
There was a problem hiding this comment.
This is the crux of the rotation fix. Before: absent orientation skipped setFromQuaternion, leaving the center with no rotation applied, so the geometry was placed in the parent frame rather than the link frame. Now quatFromJson writes identity when it finds nothing, and tmpQInv * identity = tmpQInv — the frame rotation is undone regardless of whether the geometry declared one explicitly.
| * Binary on purpose: `STLLoader` classifies ASCII by regex, so an ASCII fixture parses out of an | ||
| * oversized buffer and the subarray and short-input cases below pass with their guards deleted. | ||
| */ | ||
| const binaryStl = (triangles = 1): Uint8Array => { |
There was a problem hiding this comment.
Smart fixture choice. ASCII STL is classified by a regex in STLLoader, so ASCII bytes would pass the subarray guard even when the fix is absent — the test would be vacuously green. Binary forces the isBinary path, where the wrong offset is silent (returns empty geometry) rather than throwing.
| * Not covered by `mesh.spec.ts`: a regression to `parsePlyInput` here still puts an entity in the | ||
| * world, since `PLYLoader` answers STL bytes with an empty geometry rather than throwing. | ||
| */ | ||
| describe('mesh geometry reaches the trait layer', () => { |
There was a problem hiding this comment.
Good motivation for a separate spec file. PLYLoader returns empty geometry on STL bytes rather than throwing, so a regression to parsePlyInput in the trait would produce an entity with no vertices — indistinguishable from success without this coverage.
55ef0ee to
56eca67
Compare
2 participants
Fixes two geometry decoding bugs in the plan replayer. An unoriented collision shape on a rotated link is drawn rotated by the link's own rotation, and an STL collision mesh is dropped by the plan parser and misread as PLY by the renderer. Stacks on #910, base branch
fix/output-frame.Stack
$lib/motion(make motion utils reusable #917)MoveFrameplugin (Motion plan preview #908)Frontend
geometryCenterInFrame($lib/spatialJson) composes the geometry's local rotation unconditionally. It previously did so only when the geometry declared anorientation, which left an unoriented shape aligned to its parent rather than to its own link.parseStlInput(new,src/lib/stl.ts) parses STL bytes through three'sSTLLoader. It cuts an exactArrayBufferout of aUint8Arrayview so a subarray does not hand the loader its neighbors, and answers input shorter than an STL header with an empty geometry rather than throwing.meshContentTypeandparseMeshInput(new,src/lib/mesh.ts) pick a parser from the declared content type, normalizing case,model/stlstyle prefixes and; charset=suffixes. An unrecognized or absent type falls back to PLY.GeometryandupdateGeometryTraitinsrc/lib/ecs/traits.tscallparseMeshInputwith the mesh'scontentType. They previously calledparsePlyInputfor every mesh.useDrawAPI'sdrawGeometrydoes the same, so all three mesh render paths read the content type rather than two of them ignoring it.parseGeometryaccepts any content typeparseMeshInputhandles and stores the normalized value on the protoMesh. It previously required an exactplyand skipped everything else with a warning.Why?
Why not just relax the content type check in the plan parser?
Because the check was load bearing.
parsePlyInputisPLYLoaderand nothing else, and the ECS geometry traits called it for every mesh without looking at the content type, so accepting STL at the parser alone would have turned a logged skip into a wrong render inside the viewer, far from the cause. The gate was the right guard for a viewer that only spoke PLY. Teaching the viewer STL is what removes it.Why does PLY stay the fallback for an unrecognized content type?
It is what the renderer assumed before STL was handled at all, so making an unlabeled mesh an error would change behavior for data that renders correctly today. The plan parser does not depend on the fallback: it gates on
meshContentTypefirst and skips anything it does not recognize, so the fallback is only ever reached from the render path, where a named warning would arrive too late to be useful anyway.Why normalize the content type when both RDK producers write a bare token?
They do:
GeometryConfigwritesstring(fileType), which is only everplyorstl, and the URDF loader picks one of the same two off the file extension and errors on anything else. The normalization is defense on a free string field of a proto this repo does not own, not a response to a shape anyone has observed. It stops at content types on purpose.meshes/ur20/base.stlis rejected, because that form belongs inmesh_file_path, and reading an extension here would commit us to reading one out of apackage://URI too.Why does
parseStlInputanswer short or malformed input with an empty geometry instead of throwing?STLLoaderreads the triangle count as a uint32 at offset 80 before it checks the length, so 1 to 83 bytes throw aRangeErrorout of theDataView, andatobthrows on malformed base64 ahead of that.PLYLoaderanswers both with an empty geometry. The callers areGeometryandupdateGeometryTrait, which run inside an unguarded loop over every geometry on a resource with no error boundary above them, so one truncated mesh throwing would cost every mesh behind it. The guard sits at 84 bytes, where RDK draws the same line innewMeshFromSTLBytes.Why was the rotation only wrong sometimes?
An absent
orientationand an explicit identity quaternion make the same claim: the geometry is unrotated relative to its parent. Only the explicit form gotR_frame⁻¹applied, so the two forms disagreed by exactly the link's own rotation.That the pose is parent relative is RDK's convention rather than a guess about it. RDK has two static frame kinds that differ precisely in which end of the frame's transform the geometry pose is measured from:
staticFrame.Geometriesreturnsgeometry.Transform(NewZeroPose()), the stored pose unchanged, whiletailGeometryStaticFrame.Geometriesreturnsgeometry.Transform(sf.transform).frame.gosays it directly, that a tail geometry frame is "a static frame whose geometry is placed at the end of the frame's transform, rather than at the beginning". So a plain static frame's geometry pose is parent relative, its link local form isT_frame⁻¹ ∘ G, and the rotation half of that isR_frame⁻¹when the geometry declares nothing.The same evidence says the two frame kinds must not be treated alike, and they are not.
parseGeometryonly undoes the parent frame when it is handed aframePose, and the only caller that passes one is the model link path. Top leveltail_geometry_staticframes keep their pose as is, which is correct, and they are not rare: all four captures carry them, 19 inplan.jsonalone.Why fix a rotation branch no captured plan reaches?
Because it is one line and the branch is reachable by construction. RDK's marshaller always writes an orientation, since
OrientationOffsetis a struct value whoseomitemptydoes nothing andNewGeometryConfigassigns it unconditionally, so no marshalled dump takes this path. Hand authored kinematics do. A bare geometry on a link that is itself rotated appears inur20.json(base_link,wrist_1_link),xarm6.jsonandlite6.json(gripper_mountin both).ur20.json'swrist_1_linkis the clearest, a capsule with a translation and no orientation on a link turned byeuler_angles.Why does a mesh from a live machine still route to PLY?
Because RDK says it is PLY.
Mesh.ToProtobufhardcodesContentType: "ply", andnewMeshFromSTLByteskeeps the raw STL it was handed, so a live part sends STL bytes under aplylabel. STL therefore decodes from the plan dump path, whereGeometryConfigwrites the truefileType, and from the draw service. Guessing the format from the bytes would mean ignoring a content type RDK stated, which is worse than the mislabel, so this belongs upstream rather than in a client side sniffer.Testing
pnpm exec vitest --runpasses 693 tests across 69 files, up from 655 across 67 onfix/output-frame.svelte-checkreports 0 errors and 0 warnings across 9963 files.New
src/lib/__tests__/mesh.spec.ts(30 tests) covers content type normalization, ASCII and binary STL and PLY parsing to real vertex counts, the PLY fallback, empty meshes, base64 input in both STL encodings, a mesh held in a subarray, and truncated input. Its STL fixture is binary on purpose:STLLoaderclassifies ASCII with a regex, so an ASCII fixture parses out of an oversized buffer and would pass with the subarray cut deleted.New
src/lib/__tests__/meshGeometryTrait.spec.ts(3 tests) drives aGeometryproto with a mesh case throughtraits.Geometryandtraits.updateGeometryTraitout to aBufferGeometrywith real vertices. A regression toparsePlyInputis invisible without it, becausePLYLoaderanswers STL bytes with an empty geometry rather than throwing, so the entity still reaches the world.In
build-frame-descriptors.spec.ts,treats an absent geometry orientation the same as an explicit identityasserts the two forms agree and that the shared answer is the link's rotation undone rather than identity.reads a mesh declared as %scoversstl,STL,model/stlandply; charset=binary. The existing skip table keepsobjand a missing content type skipping.Each production change was checked by reverting it and naming the test that catches it:
returns an empty geometry for a 1/19/83 byte stl rather than throwingreturns an empty geometry for an empty string / a truncated base64 payloadparses a binary stl mesh held in a subarraygeometryCenterInFrametreats an absent geometry orientation the same as an explicit identityparseMeshInputback toparsePlyInputin the traitsmeshGeometryTrait.spec.ts