Skip to content

budget frame movement between waypoints - #925

Open
Devin T. Currie (DTCurrie) wants to merge 4 commits into
feat/client-forward-kinematicsfrom
feat/preview-frame-budget
Open

budget frame movement between waypoints#925
Devin T. Currie (DTCurrie) wants to merge 4 commits into
feat/client-forward-kinematicsfrom
feat/preview-frame-budget

Conversation

@DTCurrie

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

Copy link
Copy Markdown
Member

Decides how many frames to draw between two planned waypoints, budgeting each joint against the unit it actually moves in so an arm keeps its own resolution when a prismatic axis shares the plan. Stacks on #924. Nothing imports it yet; the move preview in #908 is the consumer.

A plan comes back as joint values with no timing, so anything that plays one has to choose its own sampling. A joint's value arrives in its column's own unit, radians for a revolute joint and millimeters for a prismatic one, not degrees like the sibling min and max on the same descriptor. segmentFrameCost divides each joint's change by the budget for its own kind and takes the largest quotient. Normalizing before the max is the whole change: a raw max across the two units lets one millimeter outweigh one radian by 57x.

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. This PR: Budget preview frames per joint unit
  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. Fill in the frames between planned waypoints (interpolation #930)

Frontend

  • segmentFrameCost(from, to, budget) costs a segment in frames: each joint's change over the budget for its own kind, largest quotient wins. FrameBudget carries degrees, millimetres and a motions map naming which columns are translational; anything absent reads as rotational.
  • jointMotionsOf(descriptors) builds that map out of a plan's FrameDescriptors, indexed the way a trajectory step is (motions.get('gantry-1')[0]), so a caller does not have to carry the kinematics separately. Mimic joints are skipped.
  • interpolatedFrames(trajectory, budget) returns steps to render, the waypoints indices where the planner's own steps landed, and a coarsening factor. A trajectory shorter than two steps comes back untouched.
  • waypointFrames(trajectory) is the other mode: the plan exactly as the planner returned it, one frame per waypoint, coarsening of 1.
  • lerpTrajectoryStep(from, to, t) blends over the union of both steps' components, copies the plan's arrays rather than aliasing them, and carries joints a component gained or lost between steps.
  • jointTravelRadians(from, to) stays deliberately unit-blind and has no production caller. It compares configurations; budgeting frames is segmentFrameCost's job.
  • DEFAULT_DEGREES_PER_FRAME is 1.5 and DEFAULT_MILLIMETRES_PER_FRAME is 5. MAX_INTERPOLATED_FRAMES bounds interpolation at 2,000; planned waypoints are never dropped to fit under it.
  • .prettierignore gains plan-linear-constrained.json, a 225-step capture stored minified because it is 2,000 lines pretty-printed and nothing reads it by eye.

Why?

Why normalize before the max rather than after?

Taking the max first means comparing a value in millimeters against a value in radians and keeping whichever number is larger, which is not a comparison of anything. A 40 mm gantry stroke reads as 40 "radians", about 2,300 degrees, and beats every joint on the arm: the gantry gets thousands of frames and the arm gets one per segment. Dividing each change by the budget for its own kind first makes the two quotients commensurable, and then the max means what it says, that the joint most in need of subdivision decides how finely the segment is cut. This ends up stricter than RDK's own resolution check rather than a copy of it, since jointStepSizeFromLimits derives one step size per frame rather than one per joint, and segmentStepCount also maxes in a Cartesian term this has no analogue for.

Why does jointMotionsOf skip mimic joints?

Because a mimic owns no column of its own. Its jointIndex addresses its source's, so writing its motion at that index labels a column it does not own, and which label survives is decided by whichever descriptor the frame system happens to yield last.

RDK permits the two to disagree. buildMimicMappings validates only that the source frame exists and has degrees of freedom, never that the joint types match, and a rack and pinion is exactly a prismatic joint driven by a revolute one. Landing on translational for a revolute column under-samples it by 57x. Landing on rotational for a prismatic one reproduces the 1,529-frame slide this module exists to remove, arrived at through the label rather than through the arithmetic. The source's own descriptor always carries the right answer, so skipping the mimic costs nothing.

Why does an unlabeled joint count as revolute?

Every joint in the captured plans here except the gantry's is revolute, and reading an unknown joint as angular fails in the safe direction: an angular budget over-samples a prismatic joint, where a millimeter budget applied to a revolute one under-samples it into a visible jump. The motions map is how a caller says otherwise.

Why is 5 mm the millimeter default?

It is the arc that DEFAULT_DEGREES_PER_FRAME's 1.5 degrees sweeps at a 191 mm radius, which is a mid-link distance rather than a typical arm's reach; at an 850 mm reach the same angle sweeps 22 mm. Erring short only over-samples a prismatic axis, so the short figure is the deliberate one, and the radius it came from is stated in the doc comment so the derivation is checkable instead of the number being a bare constant.

Why validate the budget rather than clamp it?

?? default only catches null and undefined. Every other unusable value reaches the arithmetic, and the two ways that fails are opposite and both silent. A zero or a negative clamped to Number.EPSILON is the finest budget expressible, which costs a quarter turn at 8.1e17 frames and pins every plan to the cap. A NaN stays NaN, propagating through the cost and the total until division < NaN reads as false and every segment collapses to one frame: the raw waypoint teleport this module exists to prevent, arrived at without a symptom. Zero is what an emptied numeric input or a slider at its minimum sends, so Number.isFinite(value) && value > 0 is not only a guard against nonsense.

Why is a component name read as an own key?

A component is named by an RDK resource name, and : and + are its only reserved characters, so constructor, toString and __proto__ are all legal names. Read with a plain index each of those resolves through Object.prototype to something truthy that is not an array. Only lerpTrajectoryStep is actually hurt: a prototype value indexes to undefined, which the cost functions' existing per-joint guard absorbs, but it carries a .length, and Object.length of 1 beats an empty start at the grown-joints comparison and calls .slice on a function. That is a TypeError and a blank preview. Assigning __proto__ on a plain object is the writing half: it hits the prototype setter, so the component vanishes from the frame and the frame's prototype is replaced. Hence Object.hasOwn reads and a null-prototype accumulator, in that one function and not in the two where the guard would be dead code.

Why does costing walk from while blending walks the union?

Because a component appearing only in to has no from value to measure against, so there is no defined distance to charge it, whereas holding a value it does have is always defined. It does not come up on a real reply either: ToFrameSystemInputs serializes every node of a plan from a single schema, so a plan's steps all carry the same keys.

Why blend straight across the pi boundary?

lerpTrajectoryStep is not angle-aware, and that is deliberate rather than an omission. The argument is not that the data is unwrapped, since joint limits routinely exceed pi and nothing normalizes them. It is that RDK collision-checks a segment with this identical expression, so a preview that wrapped to the short way round would draw an interior RDK never validated.

Why report a coarsening factor?

Because it is the only signal that the requested resolution was not met. When a plan's total cost exceeds MAX_INTERPOLATED_FRAMES the module coarsens rather than truncates, so the plan still plays end to end and still lands on every planned waypoint; the one thing lost is the guarantee that no drawn frame exceeds the budget. Returning 1 when the budget was met and the stretch factor when it was not lets a consumer say so without recomputing the total itself. No consumer reads it yet.

Testing

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

Most of that volume is buying two things: fixtures that stand in for the density regimes the planner actually produces, and a mutation check on every behavior claim the module makes.

Four captured plans cover the regimes: free space (2 steps, one 270 degree segment), linear-constrained (225 steps, sub-degree), obstacle-routed (21 steps, including a zero-length segment that exercises the divide-by-zero guard), and gantry (2 steps, 40 mm on one prismatic axis). The captured plans still have the shape these rules were built for block tests the fixtures rather than the module, which is the point: every threshold in the rest of the spec is calibrated against those step counts and worst-segment bands, and a re-captured fixture would quietly stop meaning what the assertion says it means. The zero-length segment is called out as structural rather than incidental, since a CBiRRT-solved goal returns a path whose first node repeats the segment's start configuration.

Reverting segmentFrameCost to a unit-blind max fails 7 tests. Two of them are the bug in its two forms: spends exactly the budgeted frames on a 40 mm slide fails, and lets the arm keep its own resolution when a long stroke shares the plan fails with expected 1 to be greater than 1. The second is the one that matters on a real rig, since it is the arm that loses resolution. Every other behavior was checked the same way, by reverting the line that implements it, including the mutants a weaker spec survives: jointMotionsOf indexed by frame name instead of component name, or rebuilding the array instead of accumulating into it (both of which drop every joint of a component but the last); waypoint indices shifted only when the frame cap binds; coarsening returned scaled; waypointFrames returning all-zero indices; the millimeter budget ignored in favor of its default; and a one-frame cost floor.

Three assertions are shaped deliberately.

Waypoints are asserted by reference, not by value. toEqual also passes for a blend that happens to land on the waypoint, which is exactly what walking a segment's interior one step further and dropping the explicit steps.push(to) produces, so only identity distinguishes the planner's own object from a float reconstruction of it. That single change takes the mutant from being killed on one fixture to being killed on all three.

spends exactly the budgeted frames on a 40 mm slide is exact rather than a band. A band wide enough to read as sane also accepts the millimeter budget being anything from 2 to 20, and that constant is a documented judgment call. It is 10 frames and not 9 because the capture's stroke is 90.00000000000001 - 50, so the cost is 8.000000000000004 and Math.ceil rounds up a ninth division. Rounding up on a hair of float residue only ever adds a frame, so it is left visible rather than papered over with an epsilon.

leaves an already-dense plan essentially alone states frames added rather than a fraction of the plan's length. Only 6 of that plan's 224 segments exceed the budget and they contribute all 11 added frames, so a x 1.1 bound would read as 10% slack while really allowing 22.

One assertion reads stronger than it is. holds the %s plan to the requested resolution is stated in degrees but measured with jointTravelRadians, which is unit-blind, so it only means what it says on the three all-revolute fixtures. The gantry case has to reconstruct the arm's column on its own before it can make the same claim, which is why lets the arm keep its own resolution when a long stroke shares the plan projects each frame down to arm-1 before measuring.

The remaining clusters are narrower. Nine tests cover components named after Object.prototype members, split between the cost functions (which absorb the prototype value through their joint guard) and lerpTrajectoryStep (which does not, and needs both the own-key read and the null-prototype write). The mimic case is asserted in both descriptor orders, because the winner is decided by frame-system key order and pinning only the order that happens to fail leaves half of it green.

@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 617cb15

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 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-925/

Built to branch gh-pages at 2026-08-12 14:44 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 4m 59s —— View job


Review

  • Read CLAUDE.md and gather context
  • Read main implementation file (interpolateTrajectory.ts, 228 lines)
  • Read test file (interpolateTrajectory.spec.ts, 527 lines)
  • Read fixture files and supporting types (frameDescriptors.ts, jointPose.ts)
  • Post inline comments
  • Post review summary

Solid implementation. The arithmetic is correct, the prototype-pollution defense is thorough and tested, and the spec does real mutation-testing work rather than just asserting happy paths. A few nits below, nothing blocking.

Correctness

The core claim — normalize per-unit before taking the max, not after — is correct, and the segmentFrameCost implementation matches it. The perFrame validator correctly catches zero, negative, NaN, and Infinity (the four bad inputs the PR description walks through). The coarsening path is right: Math.max(1, total / MAX_INTERPOLATED_FRAMES) gives 1 when the budget is met and the stretch factor otherwise, and Math.max(1, Math.ceil(segment / resolution)) keeps zero-length segments from producing 0 divisions.

lerpTrajectoryStep's handling of gained/lost joints is correct: lost joints hold their from value, gained joints copy the to value directly (consistent with the PR's reasoning that there's no defined distance to charge). The pi-boundary choice (no angle wrapping) is correctly documented and matches RDK's own collision-check expression.

Prototype safety

Object.create(null) for the accumulator and Object.hasOwn for reads in lerpTrajectoryStep are the right tools. The cost functions' reliance on the if (target === undefined) continue guard to absorb prototype values is correct: Object.prototype.constructor[0], Object.prototype.toString[0], etc. all return undefined, so the guard fires and cost stays 0. The nine tests covering this are well-split between the cost functions (guard absorbs) and lerp (needs both the own-key read and the null-prototype write).

Tests

Reference-equality assertions for waypoints are the right call — toEqual would survive the "blend that happens to land on the waypoint" mutant. The fixture-validation block at the top is a smart pattern: it pins the captured data's shape so threshold calibrations in the rest of the spec stay meaningful if a fixture is ever recaptured. The mimic ordering test in both directions is exactly what's needed given the frame-system key-order dependency.

Nits (inline comments posted)

  • interpolateTrajectory.ts line 37: FrameBudget.motions is missing a JSDoc comment while the other two fields have one. It's also the most consequential field to leave absent.
  • spec line 251 and line 279: MAX_INTERPOLATED_FRAMES is hardcoded as 2000 in both assertions. A local alias (const MAX_FRAMES = 2000 // must track MAX_INTERPOLATED_FRAMES) would make the dependency grep-findable.
  • spec line 300: DEFAULT_MILLIMETRES_PER_FRAME is never imported into the test file; it's tested implicitly through the exact-count assertion. Worth importing explicitly so the constant name appears rather than just its value.

expect(coarsening).toBeGreaterThan(1)
// The cap bounds interpolation; each planned waypoint keeps its own frame regardless, so the
// ceiling is the budget plus the plan's own length.
expect(steps.length).toBeLessThanOrEqual(2000 + plans.linear.length)

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.

2000 is hardcoded here and again at line 279. MAX_INTERPOLATED_FRAMES is intentionally unexported, but that means a change to the cap would leave these tests quietly under-constrained (if the cap grows) or failing with a confusing number (if it shrinks). The easiest fix without exporting the constant is a single local:

Suggested change
expect(steps.length).toBeLessThanOrEqual(2000 + plans.linear.length)
// Must track MAX_INTERPOLATED_FRAMES in interpolateTrajectory.ts.
const MAX_FRAMES = 2000
expect(steps.length).toBeLessThanOrEqual(MAX_FRAMES + plans.linear.length)

…and the same alias at line 279. That makes the dependency explicit and grep-findable.

degrees?: number
/** Millimetres of prismatic travel one frame represents. */
millimetres?: number
motions?: JointMotions

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.

motions is missing a JSDoc comment. The field is the most important one in FrameBudget — without it the budget is purely angular — but it has no inline documentation while degrees and millimetres both do. Even a one-liner stating what happens when it's absent (defaults to all-rotational) would match the pattern of the other fields.

Suggested change
motions?: JointMotions
/**
* Which joints are prismatic, indexed by component name. Absent means all joints are treated as
* rotational. Build this with {@link jointMotionsOf} from a plan's descriptors.
*/
motions?: JointMotions

.slice(1)
.reduce((sum, step, index) => sum + segmentFrameCost(plans.linear[index]!, step, budget), 0)

expect(interpolatedFrames(plans.linear, budget).coarsening).toBeCloseTo(total / 2000, 6)

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 magic number — pair with the alias suggested at line 251.

Suggested change
expect(interpolatedFrames(plans.linear, budget).coarsening).toBeCloseTo(total / 2000, 6)
expect(interpolatedFrames(plans.linear, budget).coarsening).toBeCloseTo(total / MAX_FRAMES, 6)

* 2 to 20. Ten and not nine because the stroke is `90.00000000000001 - 50`, which `Math.ceil`
* rounds up.
*/
it('spends exactly the budgeted frames on a 40 mm slide', () => {

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.

DEFAULT_MILLIMETRES_PER_FRAME is never imported. Its value (5) is tested implicitly here — 40 mm ÷ 5 = 8, then Math.ceil(8.000…) = 9, plus the final endpoint = 10. If the constant changes, this assertion is the only thing that breaks, which is fine, but importing it would make the dependency explicit and let the comment reference it by name rather than prose.

degrees?: number
/** Millimetres of prismatic travel one frame represents. */
millimetres?: number
motions?: JointMotions

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.

motions is missing a JSDoc comment. The field is the most important one in FrameBudget — without it the budget is purely angular — but it has no inline documentation while degrees and millimetres both do. Even a one-liner saying what happens when it's absent (defaults to all-rotational) would match the pattern of the other fields.

Suggested change
motions?: JointMotions
/**
* Which joints are prismatic, indexed by component name. Absent means all joints are treated as
* rotational. Build this with {@link jointMotionsOf} from a plan's descriptors.
*/
motions?: JointMotions

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