add preview ghosts - #928
Conversation
🦋 Changeset detectedLatest commit: 927426c 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 |
8f65aa9 to
197e9b5
Compare
197e9b5 to
1b73e1d
Compare
|
1b73e1d to
b6e6274
Compare
b6e6274 to
4cf2fd0
Compare
4cf2fd0 to
b2ae8d9
Compare
b2ae8d9 to
74be7fd
Compare
74be7fd to
cf51477
Compare
cf51477 to
7872671
Compare
8cb9d06 to
927426c
Compare
927426c to
fc54872
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 7m 25s —— View job Review
The implementation is correct and the design is sound. No bugs found. A few inline notes follow. Correctness
TestsCoverage is thorough and well-structured. Highlights:
One acknowledged gap: fixture geometries are typeless, so Minor issues (no blockers)
|
| if (!first) return moving | ||
|
|
||
| for (const [component, values] of Object.entries(first)) { | ||
| const changes = trajectory.some((step) => { |
There was a problem hiding this comment.
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:
| const changes = trajectory.some((step) => { | |
| const changes = trajectory.slice(1).some((step) => { |
| for (const step of trajectory) { | ||
| for (const component of Object.keys(step)) { | ||
| if (!Object.hasOwn(first, component)) moving.add(component) | ||
| } | ||
| } |
There was a problem hiding this comment.
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:
| 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) | |
| } | |
| } |
| 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)) | ||
| } | ||
| ) |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
fc54872 to
fb08bc4
Compare
fb08bc4 to
9f05df3
Compare
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
$lib/motion(make motion utils reusable #917)MoveFrameplugin (Motion plan preview #908)Frontend
spawnPreviewGhosts(world, descriptors, trajectory, ghosts)clears the previous set and fills the caller's map in place, keyed by frame name. Each ghost carriesPreviewOf,NonSelectable,WorldMatrix,Color,Opacityat 0.35, and whichever shape traittraits.Geometryresolves the descriptor'scommon.v1.Geometryinto, plustraits.Centerwhen the geometry has one.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 anawaitstill points at the right entities. Same shape assyncMoveGhosts.PreviewOfis set topreviewComponentName(descriptor.name), which is what routes these entities throughcollectMembers'bitForand 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 withMOTION_TOLERANCEat1e-6, and treating a component that appears or vanishes partway as moving.drivenByMovingJointwalks a frame's parent chain looking for a joint whose component is in that set. Memoized, with the memo entry seeded tofalsebefore recursing so it doubles as the cycle guard.hiddenFrameNames(world)readsInvisibleandInheritedInvisibleoff the live entity carrying each name; those frames get no ghost.geometry: nullare both skipped, so nothing spawns for a bare reference frame.applyPreviewStep(ghosts, worldMatrices)copies out of the mapcreateForwardKinematicsrewrites per step, then signalsentity.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.jsonis the concrete case: 26 columns over 52 steps, of which 24 are zero-DoF padding for scenery,_originframes, cameras and grippers, and the two 6-DoF arms areleft-arm, held, andright-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.
drivenByMovingJointwalks 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:grippercan 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-armin 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 numberMOTION_TOLERANCEis set against:1e-6is about three orders above the drift, and nine below the 2.7 radright-armactually travels. On the other side, joint values are radians or millimeters, so1e-6is a nanometer of prismatic slide, well below anything worth drawing.Why
Object.hasOwnrather than a plain index orin?Component names arrive as object keys off the wire, so
toStringis a name a machine can have. A plainstep[component]on a step that does not own the component reads through toObject.prototype, wheretoStringcomes back as a function whose.lengthis0, which is a perfectly valid DoF count. It compares equal to a zero-DoF column and the component reads as held. Symmetrically,'toString' in firstis true even when no step names atoStringcomponent, so the second pass would never notice one appearing partway. Both loops useObject.hasOwn, and both directions have a case in the spec.Why do ghosts carry no
Nameand noChildOf?resolveOrphansindexes parents by name, so a ghost namedleft-armcould capture the liveleft-cam'sChildOf, 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 whatcreateForwardKinematicsin #924 exists for.Why filter hidden frames explicitly instead of relying on the usual mechanism?
Because the usual mechanism cannot reach these entities.
InheritedInvisibleis maintained byaddInheritedInvisibleListenerswalking theChildOfcascade, and a ghost has noChildOfby 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-enterscollectMembers, re-reporting exactly the pairs the user hid to silence.syncMoveGhostsandcollectMembersboth already treat hiding as "do not consider this", and this matches them.Both
InvisibleandInheritedInvisibleare read. The cascade does stampInheritedInvisibleonto an entity that carriesInvisibleitself, 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,BatchedArrowsandCollisionDetectorall subscribe throughworld.onChange(traits.WorldMatrix, …). AMatrix4rewritten 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.
createForwardKinematicsdocuments that the map and itsMatrix4instances 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 --runpasses 1078 tests across 82 files, up 33 tests and one file from the base branch.pnpm exec svelte-checkreports 0 errors and 0 warnings.The real gate in
which frames earn a ghostis the pair that fails in opposite directions.leaves out a second arm the plan holds stillkills the permissive rule of ghosting anything with a shape;walks past a joint the plan holds still to the moving one above itkills 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 registersis the one assertion built from real robot output rather than a synthetic fixture, and it is what fixes the magnitude ofMOTION_TOLERANCErather than just its existence. The tolerance cases either side of it (Number.EPSILONstill,1e-5moving) only prove a threshold exists somewhere.moves a ghost and tells the renderers it movedinstalls aworld.onChange(traits.WorldMatrix, …)listener and asserts the ghost shows up in it. That assertion, not the one comparingelements, is what catches a droppedchanged().copies the matrix rather than sharing the one it was handedis the equivalent for the reuse contract.Two things read stronger than they are, both worth knowing before trusting the coverage:
common.v1.Geometry, whichtraits.Geometryresolves toReferenceFramerather thanBox,SphereorCapsule. So the spec pins which entities spawn and what traits they carry, not which instanced renderer picks them up.parseGeometrynever 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.movingComponentsis tested directly as well as throughspawnPreviewGhosts, 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_TOLERANCEto an exact comparison fails the captured-plan case, removing the hidden-frame check fails all three cases inhidden geometry, and removingghost.changed(traits.WorldMatrix)fails only the listener assertion.