interpolation - #930
Conversation
🦋 Changeset detectedLatest commit: 824cd07 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
9705ccc to
f6f0b29
Compare
f6f0b29 to
7af58a4
Compare
7af58a4 to
86475e9
Compare
|
86475e9 to
686486a
Compare
686486a to
1bc6992
Compare
d1c7e66 to
f468591
Compare
f468591 to
42e4480
Compare
42e4480 to
705adcc
Compare
6eacfe3 to
2a6eb90
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 4m 49s —— View job Review
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
API design
TestsFive new tests, all in The marker tests pin both ends of Nits (inline)Two inline comments posted:
|
| waypoints: 'Waypoints', | ||
| interpolated: 'Interpolated', | ||
| } | ||
| const detailByLabel = new Map<string, PreviewDetail>( |
There was a problem hiding this comment.
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.
| const detailByLabel = new Map<string, PreviewDetail>( | |
| const detailByLabel = new Map<string, PreviewDetail>( | |
| Object.entries(detailLabels).map(([detail, label]) => [label, detail as PreviewDetail]) | |
| ) |
| {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. |
There was a problem hiding this comment.
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.
| {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. |
2a6eb90 to
357e65d
Compare
357e65d to
824cd07
Compare
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,jointMotionsOfandinterpolatedFrames, and this one is the caller that hands them a real frame system and puts a control on the result.Stack
$lib/motion(make motion utils reusable #917)MoveFrameplugin (Motion plan preview #908)Frontend
PreviewDetailis a new exported type,'waypoints' | 'interpolated'.PreviewMovegains a settabledetailand a read-onlywaypointIndices.applyDetailreplacesapplyPlayback. It buildsplaybackFramesfrom eitherwaypointFrames(planned)orinterpolatedFrames(planned, { motions: jointMotions }), and takeswaypointIndicesoff the samePreviewFramesresult, so the marks and the frames they mark can never come from two different builds.detailsetter reframes the plan already in hand rather than re-requesting it:applyDetail(trajectory),player.reset(),renderStep(0). It is a no-op unlessstatusisready.jointMotionsOf(descriptors)is captured intojointMotionsnext tocreateForwardKinematics(descriptors)when a plan resolves, and cleared alongside it inresetPreview.trajectoryandplannedStepskeep their meanings exactly.executeis still handed the planner's own waypoints, neverplaybackFrames.MovePreview.svelterenders prime-core'sToggleButtonsunder an "Each frame is" legend, with a line beneath givingpreview.player.totalStepsfor the mode in force, and passespreview.waypointIndicestoTrajectoryScrubberasmarkers.Why?
Why hold
jointMotionsnext to the kinematics rather than deriving it where it is used?Because
set detailreframes a plan that is already in hand, and there is no descriptor array in scope at that point.interpolatedFrameshas to know which trajectory columns are prismatic, and the answer has to be the one this request built. Rebuilding it fromframes.partsat 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 whereforwardKinematicslives, is set where that is set, and is cleared where that is cleared.The labels matter because of what #925's
segmentFrameCostdoes 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 ingantry-plan.jsoncosts 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.markersonTrajectoryScrubberis 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
currentSteppast 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
lerpTrajectoryStepdeliberately does not wrap angles. But it is what the planner approved, not a promise of what the arm traces. RDK'sbuiltIn.executebatches itsGoToInputscalls 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.
trajectoryis whatexecuteis handed;playbackFramesis 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. ReturningplaybackFramesfromget trajectory(), and so handing the robot the interpolated frames, passed the entire suite.Testing
pnpm exec vitest --runpasses 1119 tests across 84 files, up 5 tests and no new files from the base branch.pnpm exec svelte-checkreports 0 errors and 0 warnings.All five new tests are in
usePreviewMove.svelte.spec.ts. Nothing tests the toggle inMovePreview.svelte; the two component specs only gaindetailandwaypointIndiceson theirPreviewMovefixtures 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
GANTRYpart built fromgantry-plan.json's capturedtype: "prismatic"model, not from a hand-written['translational']map, so it holds the whole chain: the fixture's joint type, throughframeSystemToPlanFramesandbuildFrameDescriptors, intojointMotionsOf, and out assegmentFrameCost's divisor.interpolateTrajectory.spec.tsalready 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 assertstrajectorystill equals the planner's reply andplannedStepsis 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
currentStepis 1, flipsdetail, and asserts it is back to 0.Screen.Recording.2026-08-12.at.10.33.18.AM.mov