budget frame movement between waypoints - #925
Conversation
🦋 Changeset detectedLatest commit: 617cb15 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 |
ba390de to
be059d1
Compare
|
be059d1 to
fbb2561
Compare
fbb2561 to
fc62136
Compare
fc62136 to
78a2097
Compare
78a2097 to
93cdefc
Compare
93cdefc to
5942bdc
Compare
5942bdc to
ce5614b
Compare
ce5614b to
214af20
Compare
214af20 to
617cb15
Compare
617cb15 to
275ee57
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 4m 59s —— View job Review
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. CorrectnessThe core claim — normalize per-unit before taking the max, not after — is correct, and the
Prototype safety
TestsReference-equality assertions for waypoints are the right call — Nits (inline comments posted)
|
| 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) |
There was a problem hiding this comment.
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:
| 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 |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Same magic number — pair with the alias suggested at line 251.
| 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', () => { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
| 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 |
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
minandmaxon the same descriptor.segmentFrameCostdivides 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
$lib/motion(make motion utils reusable #917)MoveFrameplugin (Motion plan preview #908)Frontend
segmentFrameCost(from, to, budget)costs a segment in frames: each joint's change over the budget for its own kind, largest quotient wins.FrameBudgetcarriesdegrees,millimetresand amotionsmap naming which columns are translational; anything absent reads as rotational.jointMotionsOf(descriptors)builds that map out of a plan'sFrameDescriptors, 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)returnsstepsto render, thewaypointsindices where the planner's own steps landed, and acoarseningfactor. 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,coarseningof 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 issegmentFrameCost's job.DEFAULT_DEGREES_PER_FRAMEis 1.5 andDEFAULT_MILLIMETRES_PER_FRAMEis 5.MAX_INTERPOLATED_FRAMESbounds interpolation at 2,000; planned waypoints are never dropped to fit under it..prettierignoregainsplan-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
jointStepSizeFromLimitsderives one step size per frame rather than one per joint, andsegmentStepCountalso maxes in a Cartesian term this has no analogue for.Why does
jointMotionsOfskip mimic joints?Because a mimic owns no column of its own. Its
jointIndexaddresses 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.
buildMimicMappingsvalidates 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 ontranslationalfor a revolute column under-samples it by 57x. Landing onrotationalfor 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
motionsmap 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?
?? defaultonly catchesnullandundefined. Every other unusable value reaches the arithmetic, and the two ways that fails are opposite and both silent. A zero or a negative clamped toNumber.EPSILONis the finest budget expressible, which costs a quarter turn at 8.1e17 frames and pins every plan to the cap. ANaNstaysNaN, propagating through the cost and the total untildivision < NaNreads 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, soNumber.isFinite(value) && value > 0is 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, soconstructor,toStringand__proto__are all legal names. Read with a plain index each of those resolves throughObject.prototypeto something truthy that is not an array. OnlylerpTrajectoryStepis actually hurt: a prototype value indexes toundefined, which the cost functions' existing per-joint guard absorbs, but it carries a.length, andObject.lengthof 1 beats an emptystartat the grown-joints comparison and calls.sliceon a function. That is aTypeErrorand 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. HenceObject.hasOwnreads 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
fromwhile blending walks the union?Because a component appearing only in
tohas nofromvalue 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:ToFrameSystemInputsserializes 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?
lerpTrajectoryStepis 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
coarseningfactor?Because it is the only signal that the requested resolution was not met. When a plan's total cost exceeds
MAX_INTERPOLATED_FRAMESthe 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 --runpasses 975 tests across 78 files, up 73 tests and one file from the base branch.pnpm exec svelte-checkreports 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 forblock 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
segmentFrameCostto 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 slidefails, andlets the arm keep its own resolution when a long stroke shares the planfails withexpected 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:jointMotionsOfindexed 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;coarseningreturned scaled;waypointFramesreturning 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.
toEqualalso 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 explicitsteps.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 slideis 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 is90.00000000000001 - 50, so the cost is8.000000000000004andMath.ceilrounds 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 alonestates 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 ax 1.1bound would read as 10% slack while really allowing 22.One assertion reads stronger than it is.
holds the %s plan to the requested resolutionis stated in degrees but measured withjointTravelRadians, 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 whylets the arm keep its own resolution when a long stroke shares the planprojects each frame down toarm-1before measuring.The remaining clusters are narrower. Nine tests cover components named after
Object.prototypemembers, split between the cost functions (which absorb the prototype value through their joint guard) andlerpTrajectoryStep(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.