Instanced rendering of all boxes

[Micheal Parks (micheal-parks)]

Overview

Migrates rendering of all box geometries to one InstancedMesh2.

This PR should serve as the template for how we handle instanced rendering for other geometries in the future.

[changeset-bot]

🦋 Changeset detected

Latest commit: 345410e

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-773/
Built to branch gh-pages at 2026-06-22 21:20 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[Micheal Parks (micheal-parks)]

Claude (@claude) review

[claude]

Claude finished Micheal Parks (@micheal-parks)'s task in 5m 10s —— View job

---

Review

• Read CLAUDE.md and gather context
• Read all changed files
• Run diff analysis
• Post inline comments
• Post review summary

---

Overall this is solid work. The architecture is well-thought-out — microtask coalescing mirrors the WorldMatrix system, the transform/appearance dirty split is a nice optimization, and the boundingSphere workaround is well-documented. The refactor of createEntityEvents is clean.

Bug

Missing onAdd/onRemove for Colors trait → Boxes.svelte:265

Color gets onAdd + onChange + onRemove; Colors only has onChange. If a Colors trait is freshly added (not just changed), or removed, the appearance won't update. The box will stay stale-colored until something else triggers a re-appearance.

Important

Free-list sync between face and edge meshes is implicit → Boxes.svelte:160

Both InstancedMesh2 instances must assign the same ID to corresponding instances. This is true today because every addInstances/removeInstances is mirrored in order — but there's no assertion to catch divergence. A dev-only console.error guard when edgeId !== id would catch this immediately if the assumption ever breaks.

No tests for composeBoxMatrix → composeBoxMatrix.ts:1

This function is used in three separate places (rendering, selection, focus framing) and is the canonical box-matrix composition logic the PR description says should be a template. Missing WorldMatrix, missing Box, center-offset composition, and mm→m scaling are all worth a spec file before this becomes the reference pattern.

Minor

new Matrix4() allocated on every hover → useEntityEvents.svelte.ts:44 — easy fix: hoist to module scope alongside tempHoverMatrix.

Version note on the Object.assign hack → Boxes.svelte:91 — the "LineBasicMaterial shares the basic shader chunks" assumption is a library internals bet; a one-line comment with the version makes a future upgrade audit easier.

[Matthew MacFarquhar (mattmacf98)]

nice!

[Matthew MacFarquhar (mattmacf98)]

nit: this should probably go into a general color helper asp

[Micheal Parks (micheal-parks)]

going to be refactoring colors in a bit so that this resolution isn't even necessary.

[Matthew MacFarquhar (mattmacf98)]

why split these up if they just get enqueued and processed together anyway?

[Micheal Parks (micheal-parks)]

They're only processed together here, in the one-time initial sync. At runtime the two sets diverge by design: WorldMatrix fires changed() for every box every kinematics tick, so a moving robot enqueues transform work only.

[Micheal Parks (micheal-parks)]

Claude (@claude) Final review

[claude]

Claude finished Micheal Parks (@micheal-parks)'s task in 5m 30s —— View job

---

Final Review

• Read CLAUDE.md and gather context
• Read all changed files in full
• Run diff analysis
• Check prior review feedback was addressed
• Post final review

---

All four issues from the previous review have been addressed. This is ready to merge.

What was fixed

Bug: onAdd/onRemove for Colors trait — Fixed. Boxes.svelte:264-266 now has the full onAdd + onChange + onRemove triple for both Color and Colors, matching the Center/Box/WorldMatrix transform trait pattern.

Free-list sync assumption — Resolved architecturally, which is better than an assertion. The InstanceIds = { face: number; edge: number } type at Boxes.svelte:101 explicitly tracks separate IDs per mesh; the comment at lines 95–100 documents the invariant. The IDs no longer need to be equal — each entity stores its own pair. removeInstance uses ids.face / ids.edge correctly.

Tests for composeBoxMatrix — Comprehensive spec at __tests__/composeBoxMatrix.spec.ts. Covers all four requested cases: Box + WorldMatrix only, Box + WorldMatrix + Center (verifying the rotation-under-90° case), missing Box, missing WorldMatrix. The it.each on the early-return cases is the right pattern. Test infrastructure runs in Playwright browser mode — I can't execute it in this environment (no Playwright binary), but the test logic is correct.

Version note on the Object.assign hack — Added at Boxes.svelte:90-91. Clear, actionable for a future library upgrade.

One nit still open

The new Matrix4() on useEntityEvents.svelte.ts:42 was flagged previously as hoist-to-module-scope. On re-reading: the allocation is actually load-bearing — traits.InstancedMatrix stores the matrix by reference, so each entity needs its own Matrix4 instance. Hoisting would require .clone() on the trait store and gains nothing. The original comment was slightly off; the code as-written is correct.

Minor observation (no action required)

Entities.svelte queries (worldStateEntities, drawServiceEntities, meshEntities) still include Or(traits.Box, ...), so box entities flow through <Frame {entity} /> → Mesh.svelte. Since Mesh.svelte lost its box geometry branch (13 lines deleted), those Frame instances render empty meshes. They exist in the scene graph (named by entity ID), which Selected.svelte and FocusBox.svelte explicitly skip via the composeBoxMatrix early-continue. Cost is O(n) empty objects — real but not blocking. Worth a follow-up Not(traits.Box) exclusion in those queries once this lands.

[viamrobotics-overwatch]

Hey Micheal Parks (@micheal-parks) — this PR has been approved and CI has been green for 3+ business days. Ready to merge?

Auto-comment from overwatch. Will not re-nudge for 7 days.