match rdk mesh data decoding

[Devin T. Currie (DTCurrie)]

Decodes mesh_data in both of the shapes RDK sends it, so a mesh that reaches the client through frameSystemConfig rather than through a plan dump draws instead of vanishing. Stacks on #919.

Stack

1. Read a plan model's output frame from where RDK writes it (read plan model output frame #910)
2. Geometry decode fixes (geometry decode fixes #912)
3. Mimic joint fixes (mimic joint fixes #913)
4. Move the shared plan kinematics into $lib/motion (make motion utils reusable #917)
5. Drive plan joints by RDK's schema order (match rdk joint numbering #918)
6. Infer an untyped geometry from its dimensions (infer collisions #919)
7. This PR: Read mesh data in both the shapes RDK sends it
8. Keep a plan's snapshots with the plan when another is removed (replayer plan snapshot cleanup #921)
9. Share trajectory playback between the replayer and the move panel (make trajectory playback reusable #922)
10. Draw a part's configured geometry even when it has a kinematic model (reconstructed flattened frames from rdk #923)
11. Place a plan's frames by running its kinematics on the client (forward kinematics for player #924)
12. Budget preview frames per joint unit (budget frame movement between waypoints #925)
13. Report a previewed collision as a warning about the move (handle preview collisions #926)
14. Ask RDK to check the start state before executing a previewed plan (add do command wiring for planning and execution #927)
15. Draw a previewed plan as ghost geometry (add preview ghosts #928)
16. Run a previewed plan's lifecycle (preview lifecycle #929)
17. Add move preview to the MoveFrame plugin (Motion plan preview #908)
18. Fill in the frames between planned waypoints (interpolation #930)

Frontend

• meshBytes turns a raw mesh_data value into bytes, or into one of three named problems: absent, empty, unreadable. It reads a number array or a base64 string and refuses anything else. A number array is accepted whole or refused whole; a string goes through protoBase64.dec inside a try, since dec throws a bare Error that loadPlan would otherwise report as an unparseable plan rather than as one unreadable shape.
• The mesh case in parseGeometry calls meshBytes instead of decoding inline, and maps each problem to its own skip, so the warning both names the frame and says which of the three happened.
• inferGeometryType returns mesh when mesh_data or mesh_content_type is set. It previously read only x/y/z, l and r, none of which a mesh sets, so an untyped mesh fell past the whole chain into the empty-type arm that means "no geometry" and dropped with no warning at all, while every other unreadable mesh here gets one that names its frame.

Why?

Why does the same field arrive in two shapes?

The route decides, not the data. A plan dump reaches the client through SimpleModel.MarshalJSON, so Go's encoding/json writes the []byte as base64. frameSystemConfig reaches it through protoutils.StructToStructPb, which reflects over the struct rather than marshalling it, and whose marshalSlice walks a slice element by element. The same field therefore arrives as an array of numbers.

Go itself never needs an equivalent of meshBytes, which is why the asymmetry is easy to miss from that side: encoding/json unmarshals both a base64 string and a number array into a []byte. Only a decoder written by hand has to know there are two shapes.

Why refuse a whole array rather than filter the bad elements out of it?

Because dropping one element shifts every byte after it, and a byte-shifted mesh does not fail loudly. A binary STL fails isBinary's size check, falls through to the ASCII parser, whose facet regex matches nothing, and yields an empty BufferGeometry with no error at all. A collision volume that silently renders as nothing is worse than one that was refused and named in a warning. encoding/json, the decoder on the other end of this contract, rejects every one of these cases too: a non-number element, a value past 255, a negative, a non-integer.

Why does the length check sit after the branch rather than inside the array arm?

Because both shapes can carry nothing, and only one of them looks like it can. [] is truthy where '' is falsy, which reads as though the array is the arm that needs a length check and the string arm is already covered. But only the literal empty string is falsy. protoBase64.dec ignores whitespace and tolerates missing padding, so '=', '====', ' ' and '\n' all decode to zero bytes without throwing, and a Uint8Array(0) is exactly as truthy as []. Each of those would otherwise build the entity this guard exists to prevent, one that renders nothing and still costs a draw pass.

Why keep three skip reasons instead of one message?

Because they have different causes and different owners. absent and empty say the robot's config carries no mesh, which is a question for whoever wrote that config. unreadable says this decoder was handed bytes it could not read, which is a question for this file. That distinction is what separates a mesh arriving in the array shape from a mesh whose data is genuinely corrupt, and reading only base64 makes the first look exactly like the second. Collapsed into one string, the two are indistinguishable at the console.

Why does inferGeometryType need a mesh branch at all?

Because a mesh is the one geometry the existing chain cannot see, and it is the one where falling through is silent rather than warned. An authored config can omit type entirely, which this file already pins for a sphere, and nothing stops such a config from being a mesh. Nothing reaches it today: buildFrameDescriptors skips model frames, and no capture pairs an absent type with mesh data. It costs two comparisons and closes the gap before a caller that reaches it exists.

Testing

pnpm exec vitest --run passes 752 tests across 70 files, up 17 tests and no new files from the base branch. pnpm exec svelte-check reports 0 errors and 0 warnings.

I checked each behavior claim by reverting it alone. Dropping the number-array branch fails reads mesh data delivered as a number array, as frameSystemConfig sends it. Filtering the array instead of refusing it fails all four refuses a number array with ... cases. Moving the length check inside the array arm fails the four base64-decodes-to-nothing cases. Collapsing the three reasons into one fails four of the six skips a mesh with ..., and says which cases. Dropping the inferGeometryType fallback fails reaches the mesh branch, not the silent no-geometry arm, when type is absent.

Three things in these specs are not what they look like:

• The skip cases assert the warning now, not just the null. They previously asserted only toBeNull(), so deleting console.warn from parseGeometry's skip helper outright, which also covers the unsupported-geometry-type branch, passed the entire suite. Each case now asserts the reason string and the frame name, the frame name because a warning that cannot be traced back to a frame is not actionable on a rig carrying forty geometries.
• The two shapes are asserted against the literal bytes, not against each other. Comparing the two results only proves they agree, which they would if both were broken the same way. Both now run as one it.each against the same TextEncoder output, so a failure prints the bytes rather than collapsing to a bare false.
• reaches the mesh branch, not the silent no-geometry arm, when type is absent asserts that warn was never called, and that holds only because of clearMocks: true in vite.config.ts. The spy is installed once at describe scope and is never cleared by hand, and the test immediately before it in the block does warn.

parseMeshInput's subarray handling is now pinned for both loaders. parses a binary %s mesh held in a subarray runs over stl and ply, the PLY side against a hand-built binary fixture with a binary_little_endian header, three float vertices and one face list. The fixtures are binary on purpose: an ASCII mesh survives the padding either way, since PLY decodes text from the view itself and STL's ASCII parser matches facet blocks by regex, which leading NUL bytes do not disturb. Only the binary path reads the underlying buffer positionally, so only a binary mesh at a nonzero offset distinguishes a correct slice from .buffer. The ASCII pair is kept beside it so a change to either loader's ASCII-versus-binary sniffing cannot quietly break the text route, but it is not what guards the offset. The failure being guarded against is silent in both loaders: an unsliced view starts them four bytes early and each answers with an empty BufferGeometry rather than a throw.

parse-plan.spec.ts's builds every geometry in the capture without skipping one still passes, so the captured plan takes no new skip path.

[changeset-bot]

🦋 Changeset detected

Latest commit: 4a3b4f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@viamrobotics/motion-tools	Patch

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

[github-actions]

PR Preview Action v1.8.1
🚀 View preview at https://viamrobotics.github.io/visualization/pr-preview/pr-920/
Built to branch gh-pages at 2026-08-13 18:47 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[Devin T. Currie (DTCurrie)]

Claude (@claude) review

[claude]

Claude finished Devin T. Currie (@DTCurrie)'s task in 7m 42s —— View job

---

Review

• Read CLAUDE.md and gather context
• Read changed files in full
• Run git diff against base branch
• Analyze correctness, security, patterns, and tests
• Post inline comments for line-specific feedback
• Post review summary

---

This is a well-constructed fix. The PR description is unusually complete — every design decision has a clear rationale. Inline comments cover two minor nits; nothing blocks merging.

Correctness

meshBytes is the right abstraction. The three-way absent | empty | unreadable return type maps cleanly to three different skip calls, and the caller now says exactly which happened instead of collapsing them. The important behaviors are correct:

• An empty array [] passes the every guard vacuously (✓) and then hits the length check → 'empty'.
• '' is caught by the early === '' guard → 'absent', not 'empty'. This is intentional but slightly confusing at the call-site level — see inline comment at frameDescriptors.spec.ts:1110.
• protoBase64.dec is lenient enough to handle Go's standard-base64 output (with + and /), so plan-dump decoding is unaffected. The existing captured-plan test (builds every geometry in the capture without skipping one) confirms this end-to-end.
• The array path rejects the whole payload rather than filtering — exactly right, since a dropped element silently byte-shifts the rest and both PLY and STL binary parsers answer that with an empty BufferGeometry rather than an error.

inferGeometryType mesh branch: correct placement (after x/y/z/l/r, before the empty fallback). A mesh sets none of those fields, so it was falling through to '' silently. The fix is minimal and the test that pins it (reaches the mesh branch, not the silent no-geometry arm, when type is absent) is rightly marked as pinning a path that no existing capture reaches yet.

Order change in parseGeometry: The original code checked data before content type; the new code checks content type first. See inline comment on frameDescriptors.ts:238. Not blocking, just worth knowing.

Tests

Significant improvements over the previous state:

• Skip assertions now check the frame name and the reason string, not just toBeNull(). A console.warn deletion would have passed the old suite; it would break the new one.
• The two shapes are asserted against the literal bytes (TextEncoder().encode(ply)) rather than against each other — a correct design since identical-but-wrong decodings would agree.
• Invalid array elements are refused whole (four cases), not filtered.
• The four base64-decodes-to-nothing cases are explicitly tested because protoBase64.dec ignores whitespace and missing padding, which is non-obvious.
• binaryPly fixture: the header, vertex layout, and face list (uchar int32[]) match the PLY binary-little-endian spec. The subarray test correctly targets the binary path (which reads the underlying buffer positionally), and the ASCII pair is kept beside it so sniffing changes can't quietly break the text route.
• warn spy at describe scope: the file already uses this pattern in three other describe blocks. With clearMocks: true, call history is cleared before each test, so expect(warn).not.toHaveBeenCalled() on line 1061 is reliable even though the preceding test does warn.

Nits (inline)

• frameDescriptors.spec.ts:1110 — test label "empty mesh data" vs reason 'no mesh_data' — one-liner rename suggestion posted.
• frameDescriptors.ts:238 — content-type checked before data (reversed from original); no test covers the both-wrong case.

[claude]

Nit: the test label "empty mesh data" and the reason 'no mesh_data' point in different directions. Internally '' returns 'absent' (caught by the early raw === '' guard in meshBytes), so the warning says "carries no mesh_data", not "carries empty mesh_data". A future maintainer eyeballing the table might expect 'empty mesh_data' here (mirroring the [] row below) and be confused when it fails.

Renaming the case to 'an empty string' would match how the code thinks about it:

Suggested change

	['empty mesh data', { mesh_content_type: 'ply', mesh_data: '' }, 'no mesh_data'],
	['an empty string', { mesh_content_type: 'ply', mesh_data: '' }, 'no mesh_data'],

[claude]

The original code checked data (!meshData) before content type. The order is now reversed — content type is validated first. This is fine when only one is wrong, but if both are simultaneously wrong (absent mesh_data and an unsupported content type), the user sees "unsupported content type" and must fix it before discovering there is no data. There are no tests for the both-wrong case because every failing fixture has exactly one bad field.

Not a blocking issue, but worth noting since the original order produced the arguably more actionable first error.