Add collision warnings to the MoveFrame plugin

[Micheal Parks (micheal-parks)]

Overview

Adds collision warnings to the move frame plugin. This is done by instantiating rapier whenever the plugin is active, adding all entities to the rapier world, and collision testing all of the arm descendent entities against all other world entities.

[changeset-bot]

🦋 Changeset detected

Latest commit: 8d47cf2

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	Minor

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-891/
Built to branch gh-pages at 2026-07-29 21:17 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 4m 47s —— View job

---

Code Review

• Read CLAUDE.md
• Read all modified files
• Analyze changes and post inline comments
• Post review summary

---

This is a well-structured PR. The collision detection pipeline is cleanly separated into focused modules with excellent inline documentation — especially the comments explaining why FIXED_FIXED must be opt-in, why ALL doesn't include it, the arm-bit partitioning rationale, and why environment-vs-environment collisions are deliberately suppressed. The tests cover the real Rapier WASM pipeline rather than stubs, which catches exactly the class of bugs the comments warn about.

No correctness bugs found. Issues below are ordered by severity.

---

Moderate

bitCache in collectMembers is dead weight — see inline comment at collisionMembers.ts:69.

The cache is recreated on every collectMembers call, and the seen set guarantees each entity is visited exactly once per call, so bitFor(entity) is called exactly once per entity — the cached !== undefined branch is never hit. The cache adds ~10 lines of scaffolding for zero benefit. Removing it also makes the comment ("One hop only") land directly on the logic it describes.

---

Minor

toReports undefined guard is dead code — see inline comment at collisionReport.ts:40.

displayName always returns a string, so first/second can never be undefined. TypeScript widens toSorted() to string[] which forces the check structurally, but it's misleading as written. A as [string, string] cast removes the dead branch and makes the intent clear.

Module-level scratch variables in collisionWorld.ts — see inline comment at collisionWorld.ts:27.

---

Nit / Accessibility

Collisions.svelte pair list has no accessible label per item.

Each <li> renders as "name name" to screen readers — the ↔ separator is aria-hidden and there's nothing conveying the two names are collision partners. An aria-label on the <li> would fix it:

<li class="flex flex-wrap items-center gap-1.5">

(Could not post this as an inline comment — the file was committed as a binary in the diff.)

---

Missing test coverage

collisionColors.ts and collisionReport.ts are pure functions with no tests. Both are straightforward to exercise:

• collisionColors.ts: syncCollisionColors stash-and-restore cycle, clearCollisionColors teardown, the "already stashed entity is not re-stashed" guard.
• collisionReport.ts: toReports de-duplication, staged-first ordering, the ghost-borrows-source-name case, the 'unnamed' fallback.

The four existing test files cover the physics plumbing (collisionWorld, interactionGroups) and shape derivation (colliderShape, collisionMembers) thoroughly. Adding tests for the two above would round out the suite.

[claude]

The bitCache is created fresh on every collectMembers call and can never produce a cache hit within a single call. The seen set guarantees each entity is visited exactly once, so bitFor(entity) is called exactly once per entity per call — the cached !== undefined branch is dead code every time.

If the intent was to avoid re-walking the parent hierarchy for multiple colliders owned by the same arm (e.g. two Box entities both parented to arm), the cache would need to be keyed by the subject (after resolving through GhostOf), not by entity. Even then, each entity in the loop is unique, so the hit rate would only ever be >0 for two ghosts sharing a source — which can't happen because GhostOf is exclusive: true.

Consider removing the cache entirely since it adds complexity with no benefit:

Suggested change

	const bitCache = new Map<Entity, number>()
	const bitFor = (entity: Entity): number => {
	// One hop only — a ghost's source is never itself a ghost, and resolving
	// through a chain would need a cycle guard for no gain.
	const source = entity.targetFor(GhostOf)
	const subject = source?.isAlive() ? source : entity
	return armBitFor(subject, armBits)
	}