make trajectory playback reusable - #922
Conversation
🦋 Changeset detectedLatest commit: 2a7b8e3 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 |
|
1e14638 to
173bb0d
Compare
e513c8f to
af05112
Compare
26f94bd to
f2fe86a
Compare
355bcdc to
f525214
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 4m 22s —— View job Review
SummaryThe architecture is solid: Three things to address: Bug (minor) Convention (required by Observation (nit) What looks good:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| toggle: () => { | ||
| if (isPlaying) { | ||
| pause() | ||
| return | ||
| } | ||
| if (atEnd) moveTo(0) | ||
| play() | ||
| }, |
There was a problem hiding this comment.
Edge case: play() fires even when the rewind is refused
If atEnd is true and moveTo(0) returns false (e.g. snapshot for step 0 is missing), play() is still called unconditionally. isPlaying becomes true, but on the very next timer tick currentStep >= lastStep fires park(), so isPlaying flips back to false 100ms later — a brief flicker.
The fix is one guard:
| toggle: () => { | |
| if (isPlaying) { | |
| pause() | |
| return | |
| } | |
| if (atEnd) moveTo(0) | |
| play() | |
| }, | |
| toggle: () => { | |
| if (isPlaying) { | |
| pause() | |
| return | |
| } | |
| if (atEnd && !moveTo(0)) return | |
| play() | |
| }, |
Step 0 is usually renderable (it's what loadPlan applies on select), so this won't trigger in practice, but the timer-and-park path is real and subtle enough to eliminate.
| $effect(() => { | ||
| if (total > 0) return | ||
| isPlaying = false | ||
| currentStep = 0 | ||
| }) |
There was a problem hiding this comment.
$effect writing state — add an in-code comment explaining the exception
svelte.md says "Never use $effect to derive state — use $derived". This block writes to both isPlaying and currentStep, which looks like exactly that. The PR description explains why a $derived doesn't work here (the two state variables have independent writers, and the safety net has to silence the timer immediately), but that reasoning lives only in the PR and not next to the code.
Please add a brief inline comment so a future reader doesn't refactor this into a $derived and wonder why the tests start flaking. Something like:
| $effect(() => { | |
| if (total > 0) return | |
| isPlaying = false | |
| currentStep = 0 | |
| }) | |
| // $effect, not $derived: both `isPlaying` and `currentStep` have independent writers, so they | |
| // can't be derived from `total` alone. This is a safety net — a consumer that swaps the step | |
| // source without calling `reset()` would otherwise leave a timer walking an empty range. | |
| $effect(() => { | |
| if (total > 0) return | |
| isPlaying = false | |
| currentStep = 0 | |
| }) |
| readonly currentStep: number | ||
| readonly totalSteps: number |
There was a problem hiding this comment.
currentStep and totalSteps are now redundant on the context
player.currentStep and player.totalSteps expose exactly the same values now that player is public. Keeping both means two sources of truth and two things to keep in sync. Removing them would be a breaking change for callers already destructuring ctx.currentStep, which may be why they're still here — but if no external caller uses them, this is a good opportunity to drop them and point readers to player.* instead.
Worth confirming whether any consumer outside this PR uses ctx.currentStep / ctx.totalSteps directly, and either removing them or adding a brief comment that they're kept for compatibility.
f525214 to
41badc9
Compare
41badc9 to
2a7b8e3
Compare
1 participant
Splits trajectory playback out of the plan replayer's scrubber so the move panel can drive the same state with its own presentation. Stacks on #921, and its base branch is
fix/replayer-snapshot-keys.Stack
$lib/motion(make motion utils reusable #917)interpolateTrajectory, budgeting preview frames per joint unit (budget frame movement between waypoints #925)planandexecutedo-commands (add do command wiring for planning and execution #927)MoveFrameplugin (Motion plan preview #908)Frontend
createTrajectoryPlayer(new,$lib/motion/trajectoryPlayer.svelte.ts) ownscurrentStep,isPlaying,play/pause/toggle/seek/stepBy/reset, and the playback interval.totalStepsandintervalMsare getters rather than values, so a caller can re-pace a run in flight, andonStepreports every index the player moves to and can returnfalseto refuse one.TrajectoryScrubber.svelte(new,$lib/components/motion/) renders a player as a range track, five transport buttons and a step counter. It also draws waypoint tick marks over the track and a· waypoint N / Mcounter, both fed by an optionalmarkersprop and both suppressed when every frame is a marker. Nothing passesmarkersuntil interpolation #930 hands itPreviewFrames.waypoints.useMotionPlanReplayer.svelte.tsbuilds a player and publishes it asMotionPlanReplayerContext.player.applyStepnow returns abooleanand refuses a step with no snapshot behind it, andsetStepdelegates toplayer.seek, which clamps and also pauses.MotionPlanReplayerUI.svelterendersTrajectoryScrubberwithlabel="motion plan". The oldMotionPlanReplayerScrubber.svelte, which owned both the playback state machine and the transport markup, is deleted.plugins/index.tsre-exports theTrajectoryPlayertype, becauseMotionPlanReplayerContext.playeris public and its type has to be nameable.Why?
Why does
onSteprun beforecurrentStepcommits?The callback is what actually draws the frame, and it can fail: the replayer returns
falsewhen the snapshot array and the step count have come apart. Committing the index first means the counter reads a frame that was never drawn, and every relative step after that counts from a position the scene is not in.Why does a refused step clear the interval instead of just pausing?
pause()setsisPlayingtofalse, which takes the timer down when the effect re-runs rather than immediately. In a browser that gap is usually invisible, because Svelte flushes effects in a microtask and the microtask checkpoint drains before the next timer callback fires.park()deliberately does not lean on that. Nothing aboutsetIntervalpromises a checkpoint between callbacks, and under fake timers there is none, so clearing the interval directly is what makes the refusal take effect on the tick that produced it instead of some number of retries later against a scene that by definition cannot change.Why must
createTrajectoryPlayerbe called during initialization?The timer lives in an
$effect, so calling the factory from a click handler, from module scope, or after anawaitthrowseffect_orphanrather than returning a player that quietly never advances. A consumer that wants a player per preview therefore builds one up front and re-points it at new steps, which is whytotalStepsis a getter.Why does the player rewind itself when its step count drops to zero?
Unloading the steps out from under a running player would otherwise leave it walking an empty range. The replayer cannot reach that today, since all four of its paths to zero steps call
reset()first, so this is a safety net rather than a load-bearing path. It stays because nothing obliges a future consumer to callreset()before swapping its step source out, and a playing timer over an empty range is a worse failure than a redundant guard.Why does the scrubber pause the player on unmount?
The player outlives the component that renders it: the replayer builds one at its plugin root, above both the monitor-mode gate and the panel's own
{#if isOpen}, and the move panel will build one per preview.FloatingPanelrenders its children behind that{#if}, so closing the panel really does unmount the scrubber while the player keeps running. Nothing else stops the timer, so playback would keep reconciling the world every frame with no way on screen to stop it.Why
aria-disabledrather thandisabledon the transport?The ends of a trajectory flip two of the five buttons unavailable while you are using them, and
disableddrops the focused control out of the tab order underneath a keyboard user mid-scrub.aria-disabledkeeps it focusable, which is whatsvelte.mdasks for and what the upload button in this same panel already does. It leaves the click live, so each handler re-checks:stepBywould clamp to a no-op on its own, butseekwould re-render the step it is already on.Why do the tick marks correct for the thumb?
A range input's thumb center travels from half a thumb in to half a thumb short of the end, not the full width of the track. A mark placed at a bare percentage is therefore off by up to half a thumb, 6px left at the first frame and 6px right at the last, and correct only in the middle. The ends are exactly where someone looks to check whether the thumb is sitting on a mark. The correction lives in the
.tickrule ascalc(var(--tick-fraction) * (100% - var(--thumb-size)) + var(--thumb-size) / 2), with the thumb diameter passed in from the one place it is defined.Why does the scrubber not validate
markers?Because the only thing that could produce a bad one is a caller, and a defensive sort or clamp here would be code no test could ever reach.
markershas to be ascending, deduplicated and within range: the counter walks until it passescurrentStep, so an unsorted array stops early and reads a plausible wrong number, a duplicate inflates both halves of the count, and an out-of-range entry places a tick off the end of the track.PreviewFrames.waypointsis documented to start at 0, end at the last index, and give every planned waypoint its own frame, so it satisfies all three. The contract is stated on the prop as well as on the producer.Why is this a
minorrelease rather than apatch?patchis reserved for internal refactors. This addsreadonly playertoMotionPlanReplayerContext, which is exported from./plugins, and it changes whatsetStepdoes for anyone already calling it: it now pauses. Neither is internal.Testing
npx vitest runpasses 820 tests across 74 files, up 52 fromfix/replayer-snapshot-keys's 768 across 72.svelte-checkreports 0 errors and 0 warnings.npx prettier --checkis clean.Two new spec files carry 48 of the 52.
trajectoryPlayer.spec.tsdrives a real player through an$effect.rootharness under fake timers, covering clamping, truncation and the non-finite guard onseek, relative stepping, the interval advancing and parking itself at the end, re-pacing a run in flight, rewinding ontoggleat the end, and the refusal channel: a refused step leaves the index on the frame still on screen, the next relative step counts from there, and playback parks rather than retrying the same index every tick.TrajectoryScrubber.svelte.spec.tsrenders the component against a stub player for the readouts and the transport wiring, and againstScrubberHarnesswhen the player has to actually respond, which is how the unmount test proves the timer stops rather than thatpausewas called.The remaining 4 are in
useMotionPlanReplayer.svelte.spec.ts, covering the seam:setSteppauses, reselecting or removing the active plan rewinds the index, and a plan whose snapshot array is short or gappy holds the index on the frame that is still drawn.