Skip to content

add preview ghosts - #928

Open
Devin T. Currie (DTCurrie) wants to merge 6 commits into
feat/plan-do-commandfrom
feat/preview-ghosts
Open

add preview ghosts#928
Devin T. Currie (DTCurrie) wants to merge 6 commits into
feat/plan-do-commandfrom
feat/preview-ghosts

Conversation

@DTCurrie

@DTCurrie Devin T. Currie (DTCurrie) commented Aug 6, 2026

Copy link
Copy Markdown
Member

Draws a previewed plan as a ghost twin of the machine: one translucent entity per frame the plan actually moves, laid over the live scene rather than replacing it. Stacks on #927. Nothing imports it yet; the preview lifecycle in #929 is the consumer, and the panel above that in #908.

The reason it is a twin rather than an animation of the live frames is that the arm is not moving. Driving the real entities would have the scene assert a robot state that is not true, and would fight the pose stream still writing those same matrices. So the preview spawns its own entities, outside the hierarchy, and drives them from createForwardKinematics. What is left is deciding which frames earn one, and that is most of this file.

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. Read mesh data in both the shapes RDK sends it (match rdk mesh data decoding #920)
  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. This PR: Draw a previewed plan as ghost geometry
  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

  • spawnPreviewGhosts(world, descriptors, trajectory, ghosts) clears the previous set and fills the caller's map in place, keyed by frame name. Each ghost carries PreviewOf, NonSelectable, WorldMatrix, Color, Opacity at 0.35, and whichever shape trait traits.Geometry resolves the descriptor's common.v1.Geometry into, plus traits.Center when the geometry has one.
  • The map is the caller's because a ghost carries no Name: it is the only handle anything will ever tear one down by, and filling it in place means a teardown closure that captured it before an await still points at the right entities. Same shape as syncMoveGhosts.
  • PreviewOf is set to previewComponentName(descriptor.name), which is what routes these entities through collectMembers' bitFor and gives the collision layer from handle preview collisions #926 something to read.
  • movingComponents(trajectory) returns the components whose joint values actually change, comparing every step against the first with MOTION_TOLERANCE at 1e-6, and treating a component that appears or vanishes partway as moving.
  • drivenByMovingJoint walks a frame's parent chain looking for a joint whose component is in that set. Memoized, with the memo entry seeded to false before recursing so it doubles as the cycle guard.
  • hiddenFrameNames(world) reads Invisible and InheritedInvisible off the live entity carrying each name; those frames get no ghost.
  • A joint descriptor and a static descriptor with geometry: null are both skipped, so nothing spawns for a bare reference frame.
  • applyPreviewStep(ghosts, worldMatrices) copies out of the map createForwardKinematics rewrites per step, then signals entity.changed(traits.WorldMatrix). Dead ghosts and names the step says nothing about are left alone.
  • clearPreviewGhosts(ghosts) destroys every live ghost and empties the map.

Why?

Why does a component the trajectory names still get no ghost?

Because RDK answers with a column for every component in the frame system, not just the ones it moved. salad-plan.json is the concrete case: 26 columns over 52 steps, of which 24 are zero-DoF padding for scenery, _origin frames, cameras and grippers, and the two 6-DoF arms are left-arm, held, and right-arm, which travels 2.7 rad. "Appears in the trajectory" and "the plan moves it" are different questions.

Getting that wrong in the permissive direction ghosts the entire second arm on a dual-arm rig: a dozen translucent copies laid exactly on the live ones for the whole scrub, z-fighting with what they duplicate, adding colliders that can only report touching themselves, and drifting out of place the moment the real arm moves.

Getting it wrong in the strict direction is worse. drivenByMovingJoint walks the parent chain rather than asking which components the trajectory names, because a gripper bolted to a moving arm owns no column of its own, and dropping it means the preview shows an arm swinging with nothing on the end of it. A joint whose own component is held is also not the end of the walk, for the same reason: gripper can sit at a constant 0.5 the whole plan while the arm above it moves. What the walk requires is that some joint above the frame moves, and that is exactly what separates that gripper from the idle arm across the table.

Why a tolerance rather than exact equality?

Because left-arm in that capture is not byte-identical across the 52 steps even though the plan holds it. Its values drift up to 8.312e-10 rad from step 0 as trajectory optimization runs. I measured that off the fixture directly, and it is the number MOTION_TOLERANCE is set against: 1e-6 is about three orders above the drift, and nine below the 2.7 rad right-arm actually travels. On the other side, joint values are radians or millimeters, so 1e-6 is a nanometer of prismatic slide, well below anything worth drawing.

Why Object.hasOwn rather than a plain index or in?

Component names arrive as object keys off the wire, so toString is a name a machine can have. A plain step[component] on a step that does not own the component reads through to Object.prototype, where toString comes back as a function whose .length is 0, which is a perfectly valid DoF count. It compares equal to a zero-DoF column and the component reads as held. Symmetrically, 'toString' in first is true even when no step names a toString component, so the second pass would never notice one appearing partway. Both loops use Object.hasOwn, and both directions have a case in the spec.

Why do ghosts carry no Name and no ChildOf?

resolveOrphans indexes parents by name, so a ghost named left-arm could capture the live left-cam's ChildOf, or lose its own children to the live arm, depending on query order. Staying out of the hierarchy entirely makes the question moot, and it also keeps the ghosts out of the ECS world-matrix system, which would otherwise recompute the matrices this module just wrote. The price is composing the chain ourselves, which is what createForwardKinematics in #924 exists for.

Why filter hidden frames explicitly instead of relying on the usual mechanism?

Because the usual mechanism cannot reach these entities. InheritedInvisible is maintained by addInheritedInvisibleListeners walking the ChildOf cascade, and a ghost has no ChildOf by design, so it can never inherit one. Without the explicit check, every piece of geometry the user hid behind a / focus comes back at 0.35 opacity and re-enters collectMembers, re-reporting exactly the pairs the user hid to silence. syncMoveGhosts and collectMembers both already treat hiding as "do not consider this", and this matches them.

Both Invisible and InheritedInvisible are read. The cascade does stamp InheritedInvisible onto an entity that carries Invisible itself, but it does so from a listener, so reading only the derived trait would miss a frame hidden in the same tick the preview spawns.

Why signal changed(WorldMatrix) when the matrix was mutated in place, and why copy at all?

Boxes, Spheres, Capsules, AxesHelpers, BatchedArrows and CollisionDetector all subscribe through world.onChange(traits.WorldMatrix, …). A Matrix4 rewritten in place is === its old self, so nothing downstream notices: the ghost sits frozen on screen for the whole scrub while every assertion that reads the trait back directly still passes. That failure mode is invisible to a spec that does not watch the subscription, which is why one of the tests does.

The copy is the other half of the same contract. createForwardKinematics documents that the map and its Matrix4 instances are reused and rewritten on every call, so a ghost holding one rather than copying out of it would show whatever step was evaluated last, all of them at once.

Why 0.35 opacity rather than the staged-goal ghost's 0.5?

The two are on screen together: a whole ghosted machine tracing the plan, behind the single ghosted subtree sitting at the goal. The goal is the thing being decided on, so the trace goes lighter.

Testing

pnpm exec vitest --run passes 1078 tests across 82 files, up 33 tests and one file from the base branch. pnpm exec svelte-check reports 0 errors and 0 warnings.

The real gate in which frames earn a ghost is the pair that fails in opposite directions. leaves out a second arm the plan holds still kills the permissive rule of ghosting anything with a shape; walks past a joint the plan holds still to the moving one above it kills the strict rule of requiring the frame's own component to move. Either rule on its own passes most of the block. Only both together pin the parent walk.

holds a captured plan's idle left-arm still while its moving right-arm registers is the one assertion built from real robot output rather than a synthetic fixture, and it is what fixes the magnitude of MOTION_TOLERANCE rather than just its existence. The tolerance cases either side of it (Number.EPSILON still, 1e-5 moving) only prove a threshold exists somewhere.

moves a ghost and tells the renderers it moved installs a world.onChange(traits.WorldMatrix, …) listener and asserts the ghost shows up in it. That assertion, not the one comparing elements, is what catches a dropped changed(). copies the matrix rather than sharing the one it was handed is the equivalent for the reuse contract.

Two things read stronger than they are, both worth knowing before trusting the coverage:

  • Every fixture geometry is a typeless common.v1.Geometry, which traits.Geometry resolves to ReferenceFrame rather than Box, Sphere or Capsule. So the spec pins which entities spawn and what traits they carry, not which instanced renderer picks them up. parseGeometry never produces a typeless geometry from real data, so this is a fixture artifact rather than a live path, but it does mean no test here exercises the instanced draw.
  • movingComponents is tested directly as well as through spawnPreviewGhosts, so a mistake in it shows up in both places rather than being caught by one and masked by the other.

Each behavior claim was checked by reverting the line that implements it: dropping MOTION_TOLERANCE to an exact comparison fails the captured-plan case, removing the hidden-frame check fails all three cases in hidden geometry, and removing ghost.changed(traits.WorldMatrix) fails only the listener assertion.

@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 927426c

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

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://viamrobotics.github.io/visualization/pr-preview/pr-928/

Built to branch gh-pages at 2026-08-13 18:48 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@DTCurrie

Copy link
Copy Markdown
Member Author

Claude (@claude) review

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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


Review

  • Gather context (CLAUDE.md, PR description)
  • Read changed files in full (previewGhosts.ts, previewGhosts.spec.ts, traits.ts, moveGhosts.ts, frameDescriptors.ts, jointPose.ts)
  • Check for correctness, security, and patterns
  • Post review with inline comments

The implementation is correct and the design is sound. No bugs found. A few inline notes follow.

Correctness

movingComponents handles all the corner cases cleanly:

  • Object.hasOwn in both loops correctly blocks prototype-chain pollution (toString, valueOf, etc. read from Object.prototype on a plain index).
  • The two-loop structure is the right shape: the first loop catches components that appear in first and either change or vanish, the second catches components that appear only in later steps.
  • One minor nit on both loops: they include first (trajectory[0]) itself in the scan. Comparing a step to itself is always a no-op, but trajectory.slice(1) makes the intent self-documenting. See the inline comments.

drivenByMovingJoint is also sound:

  • Seeding memo.set(name, false) before recursing is the correct cycle guard — a loop cannot contain a joint, so false is also the right answer for a cycle member.
  • Calling it with descriptor.parent (not descriptor.name) is the correct choice: after the kind !== 'static' guard, descriptor is already known to be a static frame, so its own joint check would always be false and the call would immediately recurse to the parent. Using parent directly skips one memo entry and one map lookup.
  • The memoization is shared across the outer loop, which is the right scope — moving and byName don't change within a spawnPreviewGhosts call, so the result is deterministic per frame name.

hiddenFrameNames reads both Invisible and InheritedInvisible, which is necessary because the ECS cascade writes InheritedInvisible from a listener, so a frame hidden in the same tick the preview spawns has only Invisible. This matches syncMoveGhosts's InheritedInvisible check and collectMembers' treatment.

applyPreviewStep correctly: copies the matrix (not sharing the createForwardKinematics scratch reference), signals ghost.changed(traits.WorldMatrix) (which the onChange subscribers require), and guards ghost.isAlive() before accessing.

Tests

Coverage is thorough and well-structured. Highlights:

  • The salad-plan.json fixture test (holds a captured plan's idle left-arm still while its moving right-arm registers) uses real RDK output to pin MOTION_TOLERANCE at the right order of magnitude — three above the trajectory-optimization noise, nine below the actual travel. That's the strongest possible grounding for the constant.
  • The two critical test cases noted in the PR (leaves out a second arm the plan holds still, walks past a joint the plan holds still to the moving one above it) are present and correctly falsify the permissive and strict rules individually.
  • The changed(WorldMatrix) assertion installs a real world.onChange subscriber rather than reading trait elements directly — this is the right way to catch the "frozen ghost" failure mode.
  • The toString prototype pollution tests cover both directions. See the inline note on the it.each block.

One acknowledged gap: fixture geometries are typeless, so traits.Geometry always resolves to ReferenceFrame rather than Box/Sphere/Capsule. The instanced-renderer dispatch path is not exercised here. Noted inline.

Minor issues (no blockers)

Nit trajectory.some and the second for..of trajectory loop both include first — harmless, two suggestions inline
Note Spec geometry fixtures are typeless; noted inline for future-reader awareness

if (!first) return moving

for (const [component, values] of Object.entries(first)) {
const changes = trajectory.some((step) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trajectory.some scan includes first itself (index 0). When step === first, !Object.hasOwn(first, component) is false, and comparing the array to itself finds no change, so it always short-circuits harmlessly — but the iteration is wasted. Starting from index 1 makes the intent explicit and saves one pass per component:

Suggested change
const changes = trajectory.some((step) => {
const changes = trajectory.slice(1).some((step) => {

Comment on lines +64 to +68
for (const step of trajectory) {
for (const component of Object.keys(step)) {
if (!Object.hasOwn(first, component)) moving.add(component)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same note as the first loop: iterating first in this second loop is a no-op because every key in first passes !Object.hasOwn(first, component) = false. Starting from trajectory.slice(1) would make the "looking for components that appear after the first step" intent unambiguous:

Suggested change
for (const step of trajectory) {
for (const component of Object.keys(step)) {
if (!Object.hasOwn(first, component)) moving.add(component)
}
}
for (const step of trajectory.slice(1)) {
for (const component of Object.keys(step)) {
if (!Object.hasOwn(first, component)) moving.add(component)
}
}

Comment on lines +143 to +151
it.each<[string, TrajectoryStep[], string[]]>([
['vanishes after the first step', [{ toString: [], arm: [0] }, { arm: [0] }], ['toString']],
['appears after the first step', [{ arm: [0] }, { arm: [0], toString: [1] }], ['toString']],
])(
'reads a component sharing a name with an Object member as data when it %s',
(_label, trajectory, expected) => {
expect(movingComponents(trajectory)).toEqual(new Set(expected))
}
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both directions of the prototype-pollution hazard are covered — component vanishes after step 0 (first loop finds the missing key) and component appears after step 0 (second loop finds the new key). The two-direction structure directly mirrors the two loops in movingComponents and is the right shape for this.

Comment on lines +270 to +280
it('copies the geometry center across, which the renderers offset by', () => {
const centered = link(
'arm:upper',
'arm:waist',
new Geometry({ center: { x: 5, y: 0, z: 0, oX: 0, oY: 0, oZ: 1 } })
)
const ghosts = createPreviewGhosts()

spawnPreviewGhosts(world, [armChain[0]!, armChain[1]!, centered], moves('arm'), ghosts)

expect(ghosts.get('arm:upper')!.get(traits.Center)?.x).toBe(5)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fixture geometries are typeless Geometry({}), so traits.Geometry resolves every one to ReferenceFrame rather than Box/Sphere/Capsule — the spec pins which entities exist and what traits they carry, but the instanced-renderer path is not exercised. The PR description flags this, and it's an acceptable gap since traits.Geometry has its own tests. Worth knowing if a future failure here is geometry-dispatch-related, not ghost-spawn-related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant