Skip to content

interpolation - #930

Open
Devin T. Currie (DTCurrie) wants to merge 3 commits into
motion-plan-previewfrom
feat/preview-interpolation
Open

interpolation#930
Devin T. Currie (DTCurrie) wants to merge 3 commits into
motion-plan-previewfrom
feat/preview-interpolation

Conversation

@DTCurrie

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

Copy link
Copy Markdown
Member

Adds a second way to play a previewed plan: instead of one frame per planned waypoint, fill in frames along the straight joint path between them, in proportion to how far each segment travels. This is the tip of the stack, and it stacks on #908, the move panel this control lives in, rather than on the numerically later #929.

A two-waypoint plan plays as two frames, which tells you where the arm ends up and nothing about how it gets there. This is also the first consumer of the frame budgeting from #925: that rung built segmentFrameCost, jointMotionsOf and interpolatedFrames, and this one is the caller that hands them a real frame system and puts a control on the result.

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. 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. This PR: Fill in the frames between planned waypoints

Frontend

  • PreviewDetail is a new exported type, 'waypoints' | 'interpolated'. PreviewMove gains a settable detail and a read-only waypointIndices.
  • applyDetail replaces applyPlayback. It builds playbackFrames from either waypointFrames(planned) or interpolatedFrames(planned, { motions: jointMotions }), and takes waypointIndices off the same PreviewFrames result, so the marks and the frames they mark can never come from two different builds.
  • The detail setter reframes the plan already in hand rather than re-requesting it: applyDetail(trajectory), player.reset(), renderStep(0). It is a no-op unless status is ready.
  • jointMotionsOf(descriptors) is captured into jointMotions next to createForwardKinematics(descriptors) when a plan resolves, and cleared alongside it in resetPreview.
  • trajectory and plannedSteps keep their meanings exactly. execute is still handed the planner's own waypoints, never playbackFrames.
  • MovePreview.svelte renders prime-core's ToggleButtons under an "Each frame is" legend, with a line beneath giving preview.player.totalSteps for the mode in force, and passes preview.waypointIndices to TrajectoryScrubber as markers.

Why?

Why hold jointMotions next to the kinematics rather than deriving it where it is used?

Because set detail reframes a plan that is already in hand, and there is no descriptor array in scope at that point. interpolatedFrames has to know which trajectory columns are prismatic, and the answer has to be the one this request built. Rebuilding it from frames.parts at toggle time would re-cost the plan against a frame system that may have changed since the plan was computed, which is the exact hazard the rest of this hook goes out of its way to avoid. So it lives where forwardKinematics lives, is set where that is set, and is cleared where that is cleared.

The labels matter because of what #925's segmentFrameCost does with them: each joint's change divided by the budget for its own kind, degrees for a revolute column and millimeters for a prismatic one, with the largest quotient deciding the segment. Normalizing before taking the max is the entire point. Unit-blind, a millimeter is read as a radian, roughly 57 degrees of rotation, and the 40 mm slide captured in gantry-plan.json costs 2,291 degrees and 1,529 frames rather than 10.

To be precise about which rung owns which half: #925 owns segmentFrameCost, jointMotionsOf, interpolatedFrames, the frame cap and the coarsening that fits a long plan under it. This rung owns only the call, meaning which descriptors get labeled, when, and what becomes of the frames that come back. markers on TrajectoryScrubber is likewise not new here; nothing had been passing it.

Why "Waypoints / Interpolated" rather than "Raw / Smoothed"?

Raw versus smoothed reads as honest versus prettified, which is backwards. Nothing is eased or rounded, the same path is just sampled more finely. The names describe what one frame is, and Waypoints leads because it is the plan exactly as returned.

Why show a frame count next to each mode?

The labels alone cannot carry the difference. Seeing the two counts against each other for the same move is the fastest way to understand what the setting changes, and it makes a sparse plan's sparseness impossible to miss. The number comes from player.totalSteps, so it is the count of frames that will actually play rather than a prediction of it.

Why does switching restart playback?

The two settings are different framings of one motion, so a frame index does not carry across. Playing a two-frame preview to the end and switching to interpolated would leave the scrubber reading index 1 of a much longer track with the ghosts already at the goal pose; going the other way would leave currentStep past the end of the shorter framing entirely. The setter resets and re-renders step 0 instead of trying to map an index between the two.

Is the interpolated path what the arm will actually do?

No, and the callout above the toggle says so. The straight joint path is not invented here: it is the one RDK collision-checks when it validates a segment, which is also why lerpTrajectoryStep deliberately does not wrap angles. But it is what the planner approved, not a promise of what the arm traces. RDK's builtIn.execute batches its GoToInputs calls precisely so that a component can blend between the inputs it is handed, and what any given component does with them is its own decision.

Why keep the planned trajectory separate from the played frames?

Because only one of them may ever reach the robot. trajectory is what execute is handed; playbackFrames is what the scrubber walks. Until this PR they held the same steps, which is why nothing could tell them apart, and the spec said as much in a comment on the block that covers them. Returning playbackFrames from get trajectory(), and so handing the robot the interpolated frames, passed the entire suite.

Testing

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

All five new tests are in usePreviewMove.svelte.spec.ts. Nothing tests the toggle in MovePreview.svelte; the two component specs only gain detail and waypointIndices on their PreviewMove fixtures so they still typecheck.

The real gate is the gantry test, because it is the only assertion here that could not have been written before. It drives the hook with a new GANTRY part built from gantry-plan.json's captured type: "prismatic" model, not from a hand-written ['translational'] map, so it holds the whole chain: the fixture's joint type, through frameSystemToPlanFrames and buildFrameDescriptors, into jointMotionsOf, and out as segmentFrameCost's divisor. interpolateTrajectory.spec.ts already covers the arithmetic with labels supplied by hand; what it cannot cover is whether anything on this route ever supplies them. The bound looks weak on purpose. toBeLessThan(50) sits against a true value of 10, because the failure it separates from is 1,529.

The trajectory-versus-playback split is now pinned by keeps handing out the waypoints when playback is subdivided: it subdivides playback and then asserts trajectory still equals the planner's reply and plannedSteps is still 2. That is the assertion that fails if the two are ever conflated.

The marker tests pin both ends. In waypoint mode every index is a marker. In interpolated mode there are exactly two, the first is 0, and the last is player.lastStep. The last-index assertion is the one carrying weight, since that is what keeps the scrubber's ticks from drifting off the frames they mark on a track whose length the toggle changes.

The switch test seeks to the last step, confirms currentStep is 1, flips detail, and asserts it is back to 0.

Screen.Recording.2026-08-12.at.10.33.18.AM.mov

@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 824cd07

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

github-actions Bot commented Aug 7, 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-930/

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

@DTCurrie
Devin T. Currie (DTCurrie) force-pushed the feat/preview-interpolation branch 2 times, most recently from d1c7e66 to f468591 Compare August 10, 2026 20:51
Devin T. Currie (DTCurrie) added a commit that referenced this pull request Aug 11, 2026
Devin T. Currie (DTCurrie) added a commit that referenced this pull request Aug 11, 2026
Devin T. Currie (DTCurrie) added a commit that referenced this pull request Aug 11, 2026
@DTCurrie
Devin T. Currie (DTCurrie) force-pushed the feat/preview-interpolation branch 2 times, most recently from 6eacfe3 to 2a6eb90 Compare August 12, 2026 14:42
Devin T. Currie (DTCurrie) added a commit that referenced this pull request Aug 12, 2026
@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 4m 49s —— View job


Review

  • Read CLAUDE.md and project conventions
  • Read all changed files
  • Run git diff for full context
  • Analyze correctness, API design, patterns, tests
  • Post inline comments
  • Post review summary

Overall this is a clean, well-reasoned piece of work. The logic is sound, the tests cover the critical path, and the design decisions are well-justified.

Correctness

set detail orderingdetail = next runs before applyDetail, which reads detail in closure to pick the framing. Correct.

jointMotions lifecycle — initialized to an empty Map, set immediately before applyDetail in requestPreview, cleared in resetPreview alongside forwardKinematics. The empty-Map default means a plan that resolves before jointMotionsOf is reached would treat all joints as rotational (safe), though that path doesn't exist in practice. Correct.

player.reset() before renderStep(0) in the setterreset() explicitly does not call onStep (per its doc comment), so the manual renderStep(0) is required. Correct, and consistent with requestPreview.

on:input event syntax — Confirmed against prime-core's type declaration: ToggleButtons dispatches CustomEvent<string> on input, making on:input the right handler. event.detail is the label string. Correct.

detail survives resetPreview — intentional: user preference persists across re-plans and errors. Picked up by applyDetail(result.trajectory) when the next plan resolves. Correct.

API design

PreviewDetail is a new additive export. waypointIndices and detail are additive additions to PreviewMove. Both mock fixtures (MoveControls and MovePreview specs) have been updated to include the new fields. No backwards-compatibility issues.

Tests

Five new tests, all in usePreviewMove.svelte.spec.ts. The gantry test is the standout — it threads the full chain from fixture joint type through frameSystemToPlanFrames, buildFrameDescriptors, jointMotionsOf and segmentFrameCost, and the toBeLessThan(50) vs 1,529 bound is exactly the right magnitude to separate signal from noise. The trajectory-vs-playback split test (keeps handing out the waypoints when playback is subdivided) pins the invariant that execute is never handed interpolated steps, which the PR description identifies as the most dangerous conflation.

The marker tests pin both ends of waypointIndices in interpolated mode; the middle is implicitly covered if boundary and count are right.

Nits (inline)

Two inline comments posted:

  • detailByLabel destructuring: [value, label] aliases are reversed relative to what they hold — value is the PreviewDetail key, label is the display string. Suggested renaming to [detail, label].
  • Frame count text: {frameCount} frames doesn't pluralize. "1 frames" won't come up often in practice but the text is still wrong.

waypoints: 'Waypoints',
interpolated: 'Interpolated',
}
const detailByLabel = new Map<string, PreviewDetail>(

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.

Nit: the destructuring aliases [value, label] are misleading — Object.entries gives [key, value], so value here holds the PreviewDetail key ('waypoints') and label holds the display string ('Waypoints'). Names are backwards relative to what they represent.

Suggested change
const detailByLabel = new Map<string, PreviewDetail>(
const detailByLabel = new Map<string, PreviewDetail>(
Object.entries(detailLabels).map(([detail, label]) => [label, detail as PreviewDetail])
)

Comment on lines +92 to +95
{frameCount} frames — one per configuration the planner returned, and nothing between.
{:else}
{frameCount} frames across {preview.plannedSteps} planned waypoints, along the straight joint
path the planner checks between them.

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.

Nit: frameCount isn't pluralized, so a 1-step result reads "1 frames". In practice the already-at-goal path probably catches most 1-waypoint replies before they reach here, but the text is still wrong for that case.

Suggested change
{frameCount} frames — one per configuration the planner returned, and nothing between.
{:else}
{frameCount} frames across {preview.plannedSteps} planned waypoints, along the straight joint
path the planner checks between them.
{#if preview.detail === 'waypoints'}
{frameCount} {frameCount === 1 ? 'frame' : 'frames'} — one per configuration the planner returned, and nothing between.
{:else}
{frameCount} {frameCount === 1 ? 'frame' : 'frames'} across {preview.plannedSteps} planned {preview.plannedSteps === 1 ? 'waypoint' : 'waypoints'}, along the straight joint
path the planner checks between them.

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