From e4cf1031e05dd3fbe5a4de43d711f1ba661f5b86 Mon Sep 17 00:00:00 2001 From: Devin C Date: Thu, 6 Aug 2026 13:48:42 -0400 Subject: [PATCH 1/4] replayer plan snapshot cleanup --- .changeset/eighty-crabs-shave.md | 5 + .../__fixtures__/ReplayerHarness.svelte | 28 +++++ .../useMotionPlanReplayer.svelte.spec.ts | 103 ++++++++++++++++++ .../useMotionPlanReplayer.svelte.ts | 30 ++++- 4 files changed, 160 insertions(+), 6 deletions(-) create mode 100644 .changeset/eighty-crabs-shave.md create mode 100644 src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte create mode 100644 src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts diff --git a/.changeset/eighty-crabs-shave.md b/.changeset/eighty-crabs-shave.md new file mode 100644 index 000000000..d8b03619c --- /dev/null +++ b/.changeset/eighty-crabs-shave.md @@ -0,0 +1,5 @@ +--- +'@viamrobotics/motion-tools': patch +--- + +Keep a plan's snapshots with the plan when another one is removed diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte new file mode 100644 index 000000000..d997f7a5d --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte @@ -0,0 +1,28 @@ + diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts new file mode 100644 index 000000000..5d0173277 --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts @@ -0,0 +1,103 @@ +import '@testing-library/jest-dom/vitest' +import { render } from '@testing-library/svelte' +import { describe, expect, it } from 'vitest' + +import type { Snapshot } from '$lib/buf/draw/v1/snapshot_pb' + +import type { MotionPlanReplayerContext } from '../useMotionPlanReplayer.svelte' + +import { parsePlan } from '../parse-plan' +import { parsedPlanToSnapshots } from '../plan-to-snapshots' +import gantryPlan from './__fixtures__/gantry-plan.json?raw' +import ReplayerHarness from './__fixtures__/ReplayerHarness.svelte' + +const mount = (): MotionPlanReplayerContext => { + let ctx!: MotionPlanReplayerContext + render(ReplayerHarness, { onReady: (c: MotionPlanReplayerContext) => (ctx = c) }) + return ctx +} + +/** + * Real snapshots, cycled to whatever length the test needs: what matters below is how many steps a + * plan has relative to its neighbours, and reconcile keys on `Transform.uuid` so repeats are simply + * re-applied. + */ +const stepsOfLength = (length: number): Snapshot[] => { + const base = parsedPlanToSnapshots(parsePlan(gantryPlan)) + return Array.from({ length }, (_, i) => base[i % base.length]!) +} + +const addPlans = (ctx: MotionPlanReplayerContext, lengths: number[]) => { + for (const [i, length] of lengths.entries()) { + ctx.addPlan(`plan-${i}`, `content-${i}`, stepsOfLength(length)) + } +} + +describe('removing a plan', () => { + /** + * Snapshots used to be keyed by the plan's position in `plans`, which `removePlan` reindexes. The + * active plan then read whichever array had inherited its old slot: shorter, and the step the + * player clamped to (against the *plan's* step count) ran off the end of it, so reconcile was + * handed `undefined` and threw out of whatever input handler got there. + */ + it('leaves the active plan reading its own snapshots, not its neighbour’s', () => { + const ctx = mount() + addPlans(ctx, [2, 2, 6]) + + expect(ctx.activePlanIndex).toBe(2) + expect(ctx.totalSteps).toBe(6) + + ctx.removePlan(0) + + expect(ctx.activePlanIndex).toBe(1) + expect(ctx.totalSteps).toBe(6) + + ctx.setStep(5) + expect(ctx.currentStep).toBe(5) + }) + + it('lets a plan that shifted down still be reselected', () => { + const ctx = mount() + addPlans(ctx, [2, 6, 3]) + + ctx.removePlan(0) + ctx.selectPlan(0) + + expect(ctx.plans[0]!.name).toBe('plan-1') + expect(ctx.totalSteps).toBe(6) + ctx.setStep(5) + expect(ctx.currentStep).toBe(5) + }) + + it('clears the scene when the removed plan is the active one', () => { + const ctx = mount() + addPlans(ctx, [2, 4]) + + ctx.removePlan(1) + + expect(ctx.activePlanIndex).toBeNull() + expect(ctx.totalSteps).toBe(0) + expect(ctx.plans.map((p) => p.name)).toEqual(['plan-0']) + }) + + it('holds the index still when the removed plan sits after the active one', () => { + const ctx = mount() + addPlans(ctx, [2, 4]) + ctx.selectPlan(0) + + ctx.removePlan(1) + + expect(ctx.activePlanIndex).toBe(0) + expect(ctx.totalSteps).toBe(2) + }) + + it('ignores an index that names no plan', () => { + const ctx = mount() + addPlans(ctx, [2]) + + ctx.removePlan(7) + + expect(ctx.plans).toHaveLength(1) + expect(ctx.activePlanIndex).toBe(0) + }) +}) diff --git a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts index ac77119f4..199fa0bbf 100644 --- a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts +++ b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts @@ -37,6 +37,11 @@ export interface PlanEntry { // Only primitives here — proto objects (Snapshot[]) live outside $state to avoid Svelte 5 deep proxy interface PlanState { + /** + * Survives the reindexing that `removePlan` does to `plans`, which a position cannot. Everything + * held outside `plans` is keyed by this rather than by where the plan currently sits. + */ + id: number name: string content: string status: 'idle' | 'ready' | 'error' | 'no-trajectory' @@ -64,11 +69,16 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { const world = useWorld() const relationships = useRelationships() - // Proto objects stored here — never inside $state to avoid Svelte 5 deep proxy + // Proto objects stored here — never inside $state to avoid Svelte 5 deep proxy. + // Keyed by `PlanState.id`: keyed by position, removing any plan ahead of the active one left every + // later entry pointing at its neighbour's snapshots. const snapshotStore = new Map() + let nextPlanId = 0 + let plans = $state( (initialPlans ?? []).map((e) => ({ + id: nextPlanId++, name: e.name, content: e.content, status: 'idle' as const, @@ -146,7 +156,8 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { const setStep = (step: number) => { if (activePlanIndex === null) return - const snapshots = snapshotStore.get(activePlanIndex) + const active = plans[activePlanIndex] + const snapshots = active && snapshotStore.get(active.id) if (!snapshots || snapshots.length === 0) return applyStep(snapshots, Math.max(0, Math.min(snapshots.length - 1, step))) } @@ -155,7 +166,7 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { const planState = plans[index] if (!planState) return - const stored = snapshotStore.get(index) + const stored = snapshotStore.get(planState.id) if (stored) { activePlanIndex = index currentStep = 0 @@ -172,7 +183,7 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { activePlanIndex = index return } - snapshotStore.set(index, snapshots) + snapshotStore.set(planState.id, snapshots) plans[index] = { ...planState, status: 'ready', stepCount: snapshots.length, error: null } activePlanIndex = index currentStep = 0 @@ -191,13 +202,15 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { } const addPlan = (name: string, content: string, precomputedSnapshots?: Snapshot[]) => { + const id = nextPlanId++ const index = plans.length if (precomputedSnapshots && precomputedSnapshots.length > 0) { - snapshotStore.set(index, precomputedSnapshots) + snapshotStore.set(id, precomputedSnapshots) } plans = [ ...plans, { + id, name, content, status: precomputedSnapshots && precomputedSnapshots.length > 0 ? 'ready' : 'idle', @@ -209,9 +222,14 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { } const removePlan = (index: number) => { + const removed = plans[index] + if (!removed) return + if (activePlanIndex === index) clearActivePlan() - snapshotStore.delete(index) + snapshotStore.delete(removed.id) plans = plans.filter((_, i) => i !== index) + // Every remaining plan keeps its snapshots because they were never keyed by position; only + // `activePlanIndex`, which is one, has to follow the shift. if (activePlanIndex !== null && activePlanIndex > index) { activePlanIndex = activePlanIndex - 1 } From 4f6c15eaf8d9db8126594b048200d35d665f5116 Mon Sep 17 00:00:00 2001 From: Devin C Date: Fri, 7 Aug 2026 17:42:48 -0400 Subject: [PATCH 2/4] cleanup --- .../MotionPlanReplayerUI.svelte | 7 +- .../__fixtures__/ReplayerHarness.svelte | 18 +- .../useMotionPlanReplayer.svelte.spec.ts | 247 ++++++++++++++++-- .../useMotionPlanReplayer.svelte.ts | 4 +- 4 files changed, 239 insertions(+), 37 deletions(-) diff --git a/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte b/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte index b59740e34..1144b7c4c 100644 --- a/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte +++ b/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte @@ -97,7 +97,12 @@ {/if} - {#each ctx.plans as plan, i (plan.name)} + + {#each ctx.plans as plan, i (plan.id)} {@const isActive = ctx.activePlanIndex === i}
void + /** Handed the live context and the world it draws into, once, during init. */ + onReady: (ctx: MotionPlanReplayerContext, world: World) => void } const { onReady }: Props = $props() provideWorld() provideRelationships() - // Once, at init: the context is a stable object, so re-reporting it would say nothing new. - untrack(() => onReady(provideMotionPlanReplayer())) + // `untrack` to say the once-at-init read of `onReady` is deliberate. Without it the compiler + // warns that the reference captures only the prop's initial value, which is exactly the intent: + // the context and the world are both stable, so there is nothing later to report. + untrack(() => onReady(provideMotionPlanReplayer(), useWorld())) diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts index 5d0173277..c082d5073 100644 --- a/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts @@ -1,47 +1,94 @@ -import '@testing-library/jest-dom/vitest' import { render } from '@testing-library/svelte' +import { type Entity, type World } from 'koota' +import { UuidTool } from 'uuid-tool' import { describe, expect, it } from 'vitest' -import type { Snapshot } from '$lib/buf/draw/v1/snapshot_pb' +import { PoseInFrame, Transform } from '$lib/buf/common/v1/common_pb' +import { Snapshot } from '$lib/buf/draw/v1/snapshot_pb' +import { traits } from '$lib/ecs' import type { MotionPlanReplayerContext } from '../useMotionPlanReplayer.svelte' -import { parsePlan } from '../parse-plan' -import { parsedPlanToSnapshots } from '../plan-to-snapshots' import gantryPlan from './__fixtures__/gantry-plan.json?raw' import ReplayerHarness from './__fixtures__/ReplayerHarness.svelte' -const mount = (): MotionPlanReplayerContext => { - let ctx!: MotionPlanReplayerContext - render(ReplayerHarness, { onReady: (c: MotionPlanReplayerContext) => (ctx = c) }) - return ctx +interface Mounted { + ctx: MotionPlanReplayerContext + world: World +} + +const mount = (): Mounted => { + let mounted: Mounted | undefined + render(ReplayerHarness, { + onReady: (ctx: MotionPlanReplayerContext, world: World) => (mounted = { ctx, world }), + }) + if (!mounted) throw new Error('ReplayerHarness never called onReady') + return mounted } /** - * Real snapshots, cycled to whatever length the test needs: what matters below is how many steps a - * plan has relative to its neighbours, and reconcile keys on `Transform.uuid` so repeats are simply - * re-applied. + * Snapshots that say which plan they came from. + * + * This matters more than it looks. Cycling one fixture's snapshots across every plan makes each + * plan's geometry byte-identical, so a test can only ever notice that it read an array of the wrong + * *length*. Reading the wrong plan's array of the same length, which is the actual bug this module + * had, draws a completely different robot and would go unnoticed. Naming the frame per plan and per + * step is what lets the assertions below be about identity rather than about arithmetic. */ -const stepsOfLength = (length: number): Snapshot[] => { - const base = parsedPlanToSnapshots(parsePlan(gantryPlan)) - return Array.from({ length }, (_, i) => base[i % base.length]!) -} +const planSnapshots = (plan: string, steps: number): Snapshot[] => + Array.from( + { length: steps }, + (_, step) => + new Snapshot({ + transforms: [ + new Transform({ + referenceFrame: `${plan}-frame`, + poseInObserverFrame: new PoseInFrame({ referenceFrame: 'world' }), + // Stable across steps and distinct across plans, matching a real plan: reconcile keys + // on this, so repeating it is what makes a scrub update rather than respawn. + uuid: Uint8Array.from( + UuidTool.toBytes(`${plan}-0000-4000-8000-00000000000${step % 10}`) + ), + }), + ], + }) + ) const addPlans = (ctx: MotionPlanReplayerContext, lengths: number[]) => { for (const [i, length] of lengths.entries()) { - ctx.addPlan(`plan-${i}`, `content-${i}`, stepsOfLength(length)) + ctx.addPlan(`plan-${i}`, `content-${i}`, planSnapshots(`plan-${i}`, length)) } } +/** + * Which plan's geometry is actually in the world right now. The `-frame` suffix separates the + * drawn transforms from the plan's own root entity, which carries the plan's name. + */ +const drawnFrames = (world: World): string[] => + world + .query(traits.Name) + .map((entity: Entity) => entity.get(traits.Name)) + .filter((name): name is string => typeof name === 'string' && name.endsWith('-frame')) + .toSorted() + +/** The entity drawn for a transform, as opposed to the plan root that shares the `Name` trait. */ +const drawnEntity = (world: World): Entity => + world.query(traits.Name).find((entity) => entity.get(traits.Name)?.endsWith('-frame'))! + describe('removing a plan', () => { /** * Snapshots used to be keyed by the plan's position in `plans`, which `removePlan` reindexes. The - * active plan then read whichever array had inherited its old slot: shorter, and the step the - * player clamped to (against the *plan's* step count) ran off the end of it, so reconcile was - * handed `undefined` and threw out of whatever input handler got there. + * active plan then read whichever array had inherited its old slot, so it drew a different plan's + * geometry while its own step count still came from `plans[i].stepCount`. + * + * Nothing threw. `setStep` clamped against the array it had just fetched, so the read was always + * in range. What the user got was worse than a wrong drawing: with `currentStep` pinned to the + * short array's last index and `lastStepIdx` still derived from the plan's own count, `atEnd` was + * never true, so the scrubber's play loop re-reconciled one frame at 10 Hz forever with the + * counter stuck partway and every forward control still enabled. */ it('leaves the active plan reading its own snapshots, not its neighbour’s', () => { - const ctx = mount() + const { ctx, world } = mount() addPlans(ctx, [2, 2, 6]) expect(ctx.activePlanIndex).toBe(2) @@ -51,13 +98,16 @@ describe('removing a plan', () => { expect(ctx.activePlanIndex).toBe(1) expect(ctx.totalSteps).toBe(6) + // The identity assertion, not just the arithmetic one: plan-2 is on screen, not plan-1. + expect(drawnFrames(world)).toEqual(['plan-2-frame']) ctx.setStep(5) expect(ctx.currentStep).toBe(5) + expect(drawnFrames(world)).toEqual(['plan-2-frame']) }) it('lets a plan that shifted down still be reselected', () => { - const ctx = mount() + const { ctx, world } = mount() addPlans(ctx, [2, 6, 3]) ctx.removePlan(0) @@ -65,12 +115,35 @@ describe('removing a plan', () => { expect(ctx.plans[0]!.name).toBe('plan-1') expect(ctx.totalSteps).toBe(6) + expect(drawnFrames(world)).toEqual(['plan-1-frame']) ctx.setStep(5) expect(ctx.currentStep).toBe(5) }) + /** + * The same bug in its destructive form, and the one the fix's own call site could still have had: + * `addPlan` computes `index = plans.length`, so after a removal that index belongs to a plan that + * is still loaded. Keyed by position, the new plan's snapshots overwrite the survivor's outright + * rather than merely being read in its place. + */ + it('does not overwrite a surviving plan when a new one is added after a removal', () => { + const { ctx, world } = mount() + addPlans(ctx, [2, 3, 7]) + + ctx.removePlan(0) + ctx.addPlan('plan-3', 'content-3', planSnapshots('plan-3', 4)) + + ctx.selectPlan(1) + + expect(ctx.plans[1]!.name).toBe('plan-2') + expect(ctx.totalSteps).toBe(7) + expect(drawnFrames(world)).toEqual(['plan-2-frame']) + ctx.setStep(6) + expect(ctx.currentStep).toBe(6) + }) + it('clears the scene when the removed plan is the active one', () => { - const ctx = mount() + const { ctx, world } = mount() addPlans(ctx, [2, 4]) ctx.removePlan(1) @@ -78,10 +151,12 @@ describe('removing a plan', () => { expect(ctx.activePlanIndex).toBeNull() expect(ctx.totalSteps).toBe(0) expect(ctx.plans.map((p) => p.name)).toEqual(['plan-0']) + // The removed plan's geometry goes with it rather than being left in the world. + expect(drawnFrames(world)).toEqual([]) }) it('holds the index still when the removed plan sits after the active one', () => { - const ctx = mount() + const { ctx } = mount() addPlans(ctx, [2, 4]) ctx.selectPlan(0) @@ -91,13 +166,127 @@ describe('removing a plan', () => { expect(ctx.totalSteps).toBe(2) }) - it('ignores an index that names no plan', () => { - const ctx = mount() - addPlans(ctx, [2]) + /** + * An index naming no plan now returns before anything else happens. That is not only tidiness: + * the id lookup needs the entry, and the shift below used to run unconditionally, so a negative + * or fractional index moved the active plan onto its neighbour without removing anything. Those + * are only reachable from outside, `removePlan` being public API through `./plugins`, but they + * are the same class of bug as the one this fixes. + */ + // The active plan is the last one so that a negative or fractional index would satisfy the + // `activePlanIndex > index` shift. Held at index 1 instead, both compare false and the case + // would pass whether or not the guard exists. + it.each([ + ['out of range', 7], + ['negative', -1], + ['fractional', 1.5], + ])('ignores a(n) %s index', (_label, index) => { + const { ctx } = mount() + addPlans(ctx, [2, 4, 6]) + expect(ctx.activePlanIndex).toBe(2) - ctx.removePlan(7) + ctx.removePlan(index) - expect(ctx.plans).toHaveLength(1) - expect(ctx.activePlanIndex).toBe(0) + expect(ctx.plans.map((p) => p.name)).toEqual(['plan-0', 'plan-1', 'plan-2']) + expect(ctx.activePlanIndex).toBe(2) + expect(ctx.totalSteps).toBe(6) + }) +}) + +describe('plan identity', () => { + /** + * Ids, not names, are what the store and the panel's `{#each}` key on. Names are only deduplicated + * on the upload path, so `addPlan` and the `plans` prop can both produce a collision, and a + * duplicate `{#each}` key throws in production builds as well as in dev. + */ + it('gives two plans with the same name distinct ids', () => { + const { ctx } = mount() + ctx.addPlan('same.json', 'content-a', planSnapshots('plan-a', 2)) + ctx.addPlan('same.json', 'content-b', planSnapshots('plan-b', 5)) + + const [first, second] = ctx.plans + expect(first!.id).not.toBe(second!.id) + + // And they keep their own snapshots, which is the whole point of the id. + ctx.selectPlan(0) + expect(ctx.totalSteps).toBe(2) + ctx.selectPlan(1) + expect(ctx.totalSteps).toBe(5) + }) + + /** + * Every other test hands `addPlan` precomputed snapshots, which is the hosted path. Without a + * `resolvePlanSnapshots` the plan is parsed here instead, and that branch is the one that writes + * the store itself and rewrites `plans[index]` through a spread. The spread has to carry the id + * across, or the plan is left pointing at snapshots it can no longer find. + */ + it('keys a plan it parsed itself the same way, without landing on a live plan', () => { + const { ctx, world } = mount() + // A removal first, so the parsed plan's position and its id genuinely differ. Added straight + // into an untouched list the two coincide, and keying by either one would pass. + addPlans(ctx, [2, 3]) + ctx.removePlan(0) + ctx.addPlan('gantry.json', gantryPlan) + + expect(ctx.plans.map((p) => p.name)).toEqual(['plan-1', 'gantry.json']) + expect(ctx.plans[1]!.status).toBe('ready') + expect(ctx.totalSteps).toBe(2) + + // plan-1 now sits at the position the parsed plan was written from. Its snapshots have to be + // untouched, which is the same corruption as the one above, on the branch that parses. + ctx.selectPlan(0) + + expect(ctx.totalSteps).toBe(3) + expect(drawnFrames(world)).toEqual(['plan-1-frame']) + }) +}) + +describe('scrubbing', () => { + it.each([ + ['below the first step', -3, 0], + ['past the last step', 99, 5], + ])('clamps a seek %s', (_label, requested, expected) => { + const { ctx } = mount() + addPlans(ctx, [6]) + + ctx.setStep(requested) + + expect(ctx.currentStep).toBe(expected) + }) + + it('rewinds when the active plan is cleared', () => { + const { ctx, world } = mount() + addPlans(ctx, [6]) + ctx.setStep(3) + + ctx.clearActivePlan() + + expect(ctx.currentStep).toBe(0) + expect(ctx.activePlanIndex).toBeNull() + expect(drawnFrames(world)).toEqual([]) + }) + + /** + * Reconcile runs `updateMetadata` on every step, which resets `Opacity` to its default and drops + * `Invisible` / `ShowAxesHelper`. Without the capture-and-restore around it, scrubbing wipes + * whatever the user set from the Details panel or the tree, one frame at a time. That is a + * regression this module has already had once, and nothing was holding it. + */ + it('keeps display edits made while scrubbing', () => { + const { ctx, world } = mount() + addPlans(ctx, [4]) + + const entity = drawnEntity(world) + entity.set(traits.Opacity, 0.25) + entity.add(traits.Invisible) + entity.add(traits.ShowAxesHelper) + + ctx.setStep(1) + ctx.setStep(2) + + expect(entity.isAlive()).toBe(true) + expect(entity.get(traits.Opacity)).toBeCloseTo(0.25) + expect(entity.has(traits.Invisible)).toBe(true) + expect(entity.has(traits.ShowAxesHelper)).toBe(true) }) }) diff --git a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts index 199fa0bbf..51f0719d1 100644 --- a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts +++ b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts @@ -15,8 +15,8 @@ import * as planRelations from './relations' const PLAN_COLOR = { r: 0, g: 0.47, b: 1 } const PLAN_OPACITY = 0.6 -// koota's `set` writes the trait's store slot but will not add an absent trait — the entity's -// mask is untouched, so `has` stays false and nothing querying the trait ever sees the value. +// koota's `set` on a trait the entity does not have throws, because it reaches through a store +// slot that was never allocated: `TypeError: Cannot read properties of undefined (reading 'store')`. // Plan transforms carry no color metadata, so `Color` is always absent on spawn; `Opacity` only // happens to be present because `drawTransform` adds it unconditionally. Guard both rather than // depend on that. From c5867999de539ed144e19abc4080cffa385a0fef Mon Sep 17 00:00:00 2001 From: Devin C Date: Mon, 10 Aug 2026 20:08:06 -0400 Subject: [PATCH 3/4] apply review findings for #921 --- .../MotionPlanReplayerUI.svelte.spec.ts | 62 +++++++++++++++++++ .../__fixtures__/MockDashboardPortal.svelte | 11 ++++ .../__fixtures__/MockFloatingPanel.svelte | 33 ++++++++++ .../__fixtures__/ReplayerUIHarness.svelte | 30 +++++++++ .../useMotionPlanReplayer.svelte.spec.ts | 39 +++++++++++- .../useMotionPlanReplayer.svelte.ts | 31 +++++----- 6 files changed, 188 insertions(+), 18 deletions(-) create mode 100644 src/lib/plugins/MotionPlanReplayer/__tests__/MotionPlanReplayerUI.svelte.spec.ts create mode 100644 src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockDashboardPortal.svelte create mode 100644 src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockFloatingPanel.svelte create mode 100644 src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/MotionPlanReplayerUI.svelte.spec.ts b/src/lib/plugins/MotionPlanReplayer/__tests__/MotionPlanReplayerUI.svelte.spec.ts new file mode 100644 index 000000000..cc406db72 --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/MotionPlanReplayerUI.svelte.spec.ts @@ -0,0 +1,62 @@ +import '@testing-library/jest-dom/vitest' +import { fireEvent, render, screen } from '@testing-library/svelte' +import { describe, expect, it, vi } from 'vitest' + +import ReplayerUIHarness from './__fixtures__/ReplayerUIHarness.svelte' + +// FloatingPanel reads `useThrelte().dom` unconditionally to seed its default position; the +// globally-mocked `@threlte/core` (vitest-setup-client.ts) has no `dom` field, so the real +// component throws on mount outside a Canvas. Swap it for a plain open/closed shell — the panel's +// own positioning isn't what this spec exercises. +vi.mock('$lib/components/overlay/FloatingPanel.svelte', async () => { + const MockFloatingPanel = await import('./__fixtures__/MockFloatingPanel.svelte') + return { default: MockFloatingPanel.default } +}) + +// `MotionPlanReplayerUI` reaches `DashboardPortal` through the package's `$lib` barrel, which also +// re-exports `App.svelte` and drags in the whole Threlte-dependent component tree (`T`, which the +// global `@threlte/core` mock doesn't provide). Replacing the barrel with just the one export this +// component uses avoids loading any of that; `DashboardPortal` itself is only a `Portal` (already +// mocked globally to a passthrough) around its children, so a bare passthrough stands in for it too. +vi.mock('$lib', async () => { + const MockDashboardPortal = await import('./__fixtures__/MockDashboardPortal.svelte') + return { DashboardPortal: MockDashboardPortal.default } +}) + +// useToast requires a `provideToast` ancestor; nothing here checks toast content. +vi.mock('@viamrobotics/prime-core', async (importOriginal) => ({ + ...(await importOriginal()), + useToast: () => vi.fn(), +})) + +const open = async () => { + await fireEvent.click(screen.getByRole('radio', { name: 'Motion Plan Replayer' })) +} + +describe('MotionPlanReplayerUI', () => { + /** + * The `{#each ctx.plans as plan, i (plan.id)}` key used to be `plan.name`. Only the upload path + * (`handlePlanFile`) rejects a duplicate name — neither `addPlan` nor the `plans` prop does — so + * two plans sharing a name reach the template unfiltered. Svelte throws `each_key_duplicate` on + * a repeated key in production builds as well as dev, which would take the whole panel down on + * mount rather than merely mis-rendering one row. + * + * The store-level spec ("gives two plans with the same name distinct ids") only pins the id + * *generator*; it renders nothing, so it stays green whether the template keys on `id` or + * `name`. This is the one test that actually constrains the template's key. + */ + it('renders two plans that share a name as distinct rows', async () => { + render(ReplayerUIHarness, { + props: { + plans: [ + { name: 'same.json', content: 'content-a' }, + { name: 'same.json', content: 'content-b' }, + ], + }, + }) + + await open() + + expect(screen.getAllByRole('button', { name: 'Remove plan' })).toHaveLength(2) + }) +}) diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockDashboardPortal.svelte b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockDashboardPortal.svelte new file mode 100644 index 000000000..60ab9f04e --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockDashboardPortal.svelte @@ -0,0 +1,11 @@ + + +{@render children()} diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockFloatingPanel.svelte b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockFloatingPanel.svelte new file mode 100644 index 000000000..0a7ee3897 --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/MockFloatingPanel.svelte @@ -0,0 +1,33 @@ + + +
+ {#if title} +

{title}

+ {/if} + + + + {#if isOpen} + {@render children()} + {/if} +
diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte new file mode 100644 index 000000000..b67114d6f --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte @@ -0,0 +1,30 @@ + + + diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts index c082d5073..d821c2e6a 100644 --- a/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts @@ -3,7 +3,7 @@ import { type Entity, type World } from 'koota' import { UuidTool } from 'uuid-tool' import { describe, expect, it } from 'vitest' -import { PoseInFrame, Transform } from '$lib/buf/common/v1/common_pb' +import { Geometry, PoseInFrame, Sphere, Transform } from '$lib/buf/common/v1/common_pb' import { Snapshot } from '$lib/buf/draw/v1/snapshot_pb' import { traits } from '$lib/ecs' @@ -290,3 +290,40 @@ describe('scrubbing', () => { expect(entity.has(traits.ShowAxesHelper)).toBe(true) }) }) + +describe('display defaults', () => { + /** + * `setOrAddColor` used to be a local reimplementation of `setOrAddTrait`; the spawn path now + * calls the shared helper directly. Plan transforms carry no color metadata, so this is the + * "entity doesn't have the trait yet" branch `setOrAddTrait` exists for — the one koota's own + * `entity.set` would write into an unallocated store slot and lose silently, since `has()` + * would stay false and no query would ever see it. + * + * Needs a `physicalObject` on the transform: `drawTransform` only pushes `traits.Geometry` + * (here, `Sphere`) when one is present, and `applyStep` only colors entities that got real + * geometry rather than the bare `ReferenceFrame` marker — `planSnapshots` above omits it, so it + * can't be reused for this one. + */ + it('colors a freshly spawned plan entity even though Color is always absent on spawn', () => { + const { ctx, world } = mount() + ctx.addPlan('plan-0', 'content-0', [ + new Snapshot({ + transforms: [ + new Transform({ + referenceFrame: 'plan-0-frame', + poseInObserverFrame: new PoseInFrame({ referenceFrame: 'world' }), + physicalObject: new Geometry({ + geometryType: { case: 'sphere', value: new Sphere({ radiusMm: 10 }) }, + }), + uuid: Uint8Array.from(UuidTool.toBytes('plan-0-0000-4000-8000-000000000000')), + }), + ], + }), + ]) + + const entity = drawnEntity(world) + + expect(entity.has(traits.Color)).toBe(true) + expect(entity.get(traits.Color)).toEqual({ r: 0, g: 0.47, b: 1 }) + }) +}) diff --git a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts index 51f0719d1..7e201b13c 100644 --- a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts +++ b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts @@ -4,7 +4,7 @@ import { onDestroy } from 'svelte' import type { Snapshot } from '$lib/buf/draw/v1/snapshot_pb' -import { traits, useWorld } from '$lib/ecs' +import { setOrAddTrait, traits, useWorld } from '$lib/ecs' import { useRelationships } from '$lib/hooks/useRelationships.svelte' import { reconcileSnapshotEntities, type SnapshotEntity } from '$lib/snapshot' @@ -15,20 +15,16 @@ import * as planRelations from './relations' const PLAN_COLOR = { r: 0, g: 0.47, b: 1 } const PLAN_OPACITY = 0.6 -// koota's `set` on a trait the entity does not have throws, because it reaches through a store -// slot that was never allocated: `TypeError: Cannot read properties of undefined (reading 'store')`. // Plan transforms carry no color metadata, so `Color` is always absent on spawn; `Opacity` only -// happens to be present because `drawTransform` adds it unconditionally. Guard both rather than -// depend on that. -const setOrAddColor = (entity: Entity, value: typeof PLAN_COLOR) => { - if (entity.has(traits.Color)) entity.set(traits.Color, value) - else entity.add(traits.Color(value)) -} - -const setOrAddOpacity = (entity: Entity, value: number) => { - if (entity.has(traits.Opacity)) entity.set(traits.Opacity, value) - else entity.add(traits.Opacity(value)) -} +// happens to be present because `drawTransform` adds it unconditionally. Neither can be assumed +// present, so both go through `setOrAddTrait` (`$lib/ecs`) rather than a raw `entity.set`. +// +// koota's `entity.set` on a trait this entity lacks does not throw here: it writes the trait's +// store slot without touching the entity's mask, so `has()` stays false and the write is silently +// lost to every query that reads it. `set` only throws when the trait was never registered on the +// *world* at all (`TypeError: Cannot read properties of undefined (reading 'store')`) — a +// different precondition than "this entity doesn't have it", and not one plan entities can hit, +// since other entities register `Color`/`Opacity` on this world well before a plan ever loads. export interface PlanEntry { name: string @@ -136,14 +132,15 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { // Defaults land on first appearance only. Re-forcing them every step is what wiped // the user's Details-panel edits. - if (!spawned.entity.has(traits.ReferenceFrame)) setOrAddColor(spawned.entity, PLAN_COLOR) - setOrAddOpacity(spawned.entity, PLAN_OPACITY) + if (!spawned.entity.has(traits.ReferenceFrame)) + setOrAddTrait(spawned.entity, traits.Color, PLAN_COLOR) + setOrAddTrait(spawned.entity, traits.Opacity, PLAN_OPACITY) } // Restore captured config onto entities that survived this step. for (const [entity, prev] of preserved) { if (!entity.isAlive()) continue - setOrAddOpacity(entity, prev.opacity) + setOrAddTrait(entity, traits.Opacity, prev.opacity) if (prev.invisible) entity.add(traits.Invisible) else entity.remove(traits.Invisible) if (prev.showAxes) entity.add(traits.ShowAxesHelper) From 758210074464ea6e9177804c1904d8b6b9eb4c0a Mon Sep 17 00:00:00 2001 From: Devin C Date: Tue, 11 Aug 2026 13:01:16 -0400 Subject: [PATCH 4/4] apply comment, test and description review for #921 --- .../MotionPlanReplayerUI.svelte | 7 +- .../MotionPlanReplayerUI.svelte.spec.ts | 36 ++----- .../__fixtures__/ReplayerHarness.svelte | 15 +-- .../__fixtures__/ReplayerUIHarness.svelte | 9 +- .../useMotionPlanReplayer.svelte.spec.ts | 93 ++++--------------- .../useMotionPlanReplayer.svelte.ts | 23 +---- 6 files changed, 39 insertions(+), 144 deletions(-) diff --git a/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte b/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte index 1144b7c4c..60669bc70 100644 --- a/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte +++ b/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte @@ -97,11 +97,8 @@
{/if} - + {#each ctx.plans as plan, i (plan.id)} {@const isActive = ctx.activePlanIndex === i}
{ const MockFloatingPanel = await import('./__fixtures__/MockFloatingPanel.svelte') return { default: MockFloatingPanel.default } }) -// `MotionPlanReplayerUI` reaches `DashboardPortal` through the package's `$lib` barrel, which also -// re-exports `App.svelte` and drags in the whole Threlte-dependent component tree (`T`, which the -// global `@threlte/core` mock doesn't provide). Replacing the barrel with just the one export this -// component uses avoids loading any of that; `DashboardPortal` itself is only a `Portal` (already -// mocked globally to a passthrough) around its children, so a bare passthrough stands in for it too. +// The `$lib` barrel re-exports `App.svelte` and pulls the whole Threlte component tree in with it. +// `DashboardPortal` is only a `Portal`, already mocked globally to a passthrough. vi.mock('$lib', async () => { const MockDashboardPortal = await import('./__fixtures__/MockDashboardPortal.svelte') return { DashboardPortal: MockDashboardPortal.default } @@ -29,23 +25,11 @@ vi.mock('@viamrobotics/prime-core', async (importOriginal) => ({ useToast: () => vi.fn(), })) -const open = async () => { - await fireEvent.click(screen.getByRole('radio', { name: 'Motion Plan Replayer' })) -} - describe('MotionPlanReplayerUI', () => { - /** - * The `{#each ctx.plans as plan, i (plan.id)}` key used to be `plan.name`. Only the upload path - * (`handlePlanFile`) rejects a duplicate name — neither `addPlan` nor the `plans` prop does — so - * two plans sharing a name reach the template unfiltered. Svelte throws `each_key_duplicate` on - * a repeated key in production builds as well as dev, which would take the whole panel down on - * mount rather than merely mis-rendering one row. - * - * The store-level spec ("gives two plans with the same name distinct ids") only pins the id - * *generator*; it renders nothing, so it stays green whether the template keys on `id` or - * `name`. This is the one test that actually constrains the template's key. - */ + // The store spec's duplicate-name case pins the id generator and renders nothing, so this is the + // only test that constrains the `{#each}` key. it('renders two plans that share a name as distinct rows', async () => { + const user = userEvent.setup() render(ReplayerUIHarness, { props: { plans: [ @@ -55,7 +39,7 @@ describe('MotionPlanReplayerUI', () => { }, }) - await open() + await user.click(screen.getByRole('radio', { name: 'Motion Plan Replayer' })) expect(screen.getAllByRole('button', { name: 'Remove plan' })).toHaveLength(2) }) diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte index ea56b94a5..d66618921 100644 --- a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte @@ -1,12 +1,8 @@ diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte index b67114d6f..446923aae 100644 --- a/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte @@ -1,9 +1,7 @@ diff --git a/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts index d821c2e6a..5ba8e2a80 100644 --- a/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts @@ -27,28 +27,21 @@ const mount = (): Mounted => { } /** - * Snapshots that say which plan they came from. - * - * This matters more than it looks. Cycling one fixture's snapshots across every plan makes each - * plan's geometry byte-identical, so a test can only ever notice that it read an array of the wrong - * *length*. Reading the wrong plan's array of the same length, which is the actual bug this module - * had, draws a completely different robot and would go unnoticed. Naming the frame per plan and per - * step is what lets the assertions below be about identity rather than about arithmetic. + * Snapshots that name their plan. One fixture cycled across every plan makes each plan's geometry + * byte-identical, so a test could only notice an array of the wrong length, not the wrong plan. */ const planSnapshots = (plan: string, steps: number): Snapshot[] => Array.from( { length: steps }, - (_, step) => + () => new Snapshot({ transforms: [ new Transform({ referenceFrame: `${plan}-frame`, poseInObserverFrame: new PoseInFrame({ referenceFrame: 'world' }), - // Stable across steps and distinct across plans, matching a real plan: reconcile keys - // on this, so repeating it is what makes a scrub update rather than respawn. - uuid: Uint8Array.from( - UuidTool.toBytes(`${plan}-0000-4000-8000-00000000000${step % 10}`) - ), + // Stable across a plan's steps, distinct across plans: reconcile keys on this, so + // repeating it is what makes a scrub update the entity rather than respawn it. + uuid: Uint8Array.from(UuidTool.toBytes(`${plan}-0000-4000-8000-000000000000`)), }), ], }) @@ -61,8 +54,8 @@ const addPlans = (ctx: MotionPlanReplayerContext, lengths: number[]) => { } /** - * Which plan's geometry is actually in the world right now. The `-frame` suffix separates the - * drawn transforms from the plan's own root entity, which carries the plan's name. + * Which plan's geometry is in the world now. The `-frame` suffix separates the drawn transforms + * from the plan's own root entity, which is named for the plan. */ const drawnFrames = (world: World): string[] => world @@ -71,22 +64,10 @@ const drawnFrames = (world: World): string[] => .filter((name): name is string => typeof name === 'string' && name.endsWith('-frame')) .toSorted() -/** The entity drawn for a transform, as opposed to the plan root that shares the `Name` trait. */ const drawnEntity = (world: World): Entity => world.query(traits.Name).find((entity) => entity.get(traits.Name)?.endsWith('-frame'))! describe('removing a plan', () => { - /** - * Snapshots used to be keyed by the plan's position in `plans`, which `removePlan` reindexes. The - * active plan then read whichever array had inherited its old slot, so it drew a different plan's - * geometry while its own step count still came from `plans[i].stepCount`. - * - * Nothing threw. `setStep` clamped against the array it had just fetched, so the read was always - * in range. What the user got was worse than a wrong drawing: with `currentStep` pinned to the - * short array's last index and `lastStepIdx` still derived from the plan's own count, `atEnd` was - * never true, so the scrubber's play loop re-reconciled one frame at 10 Hz forever with the - * counter stuck partway and every forward control still enabled. - */ it('leaves the active plan reading its own snapshots, not its neighbour’s', () => { const { ctx, world } = mount() addPlans(ctx, [2, 2, 6]) @@ -98,7 +79,6 @@ describe('removing a plan', () => { expect(ctx.activePlanIndex).toBe(1) expect(ctx.totalSteps).toBe(6) - // The identity assertion, not just the arithmetic one: plan-2 is on screen, not plan-1. expect(drawnFrames(world)).toEqual(['plan-2-frame']) ctx.setStep(5) @@ -120,12 +100,8 @@ describe('removing a plan', () => { expect(ctx.currentStep).toBe(5) }) - /** - * The same bug in its destructive form, and the one the fix's own call site could still have had: - * `addPlan` computes `index = plans.length`, so after a removal that index belongs to a plan that - * is still loaded. Keyed by position, the new plan's snapshots overwrite the survivor's outright - * rather than merely being read in its place. - */ + // `addPlan` computes `index = plans.length`, which after a removal is a position another plan + // still holds. it('does not overwrite a surviving plan when a new one is added after a removal', () => { const { ctx, world } = mount() addPlans(ctx, [2, 3, 7]) @@ -151,7 +127,6 @@ describe('removing a plan', () => { expect(ctx.activePlanIndex).toBeNull() expect(ctx.totalSteps).toBe(0) expect(ctx.plans.map((p) => p.name)).toEqual(['plan-0']) - // The removed plan's geometry goes with it rather than being left in the world. expect(drawnFrames(world)).toEqual([]) }) @@ -166,16 +141,8 @@ describe('removing a plan', () => { expect(ctx.totalSteps).toBe(2) }) - /** - * An index naming no plan now returns before anything else happens. That is not only tidiness: - * the id lookup needs the entry, and the shift below used to run unconditionally, so a negative - * or fractional index moved the active plan onto its neighbour without removing anything. Those - * are only reachable from outside, `removePlan` being public API through `./plugins`, but they - * are the same class of bug as the one this fixes. - */ - // The active plan is the last one so that a negative or fractional index would satisfy the - // `activePlanIndex > index` shift. Held at index 1 instead, both compare false and the case - // would pass whether or not the guard exists. + // The active plan is last on purpose: only from there does a negative or fractional index + // satisfy the `activePlanIndex > index` shift, so held at 1 the case would pass without a guard. it.each([ ['out of range', 7], ['negative', -1], @@ -194,11 +161,6 @@ describe('removing a plan', () => { }) describe('plan identity', () => { - /** - * Ids, not names, are what the store and the panel's `{#each}` key on. Names are only deduplicated - * on the upload path, so `addPlan` and the `plans` prop can both produce a collision, and a - * duplicate `{#each}` key throws in production builds as well as in dev. - */ it('gives two plans with the same name distinct ids', () => { const { ctx } = mount() ctx.addPlan('same.json', 'content-a', planSnapshots('plan-a', 2)) @@ -207,19 +169,14 @@ describe('plan identity', () => { const [first, second] = ctx.plans expect(first!.id).not.toBe(second!.id) - // And they keep their own snapshots, which is the whole point of the id. ctx.selectPlan(0) expect(ctx.totalSteps).toBe(2) ctx.selectPlan(1) expect(ctx.totalSteps).toBe(5) }) - /** - * Every other test hands `addPlan` precomputed snapshots, which is the hosted path. Without a - * `resolvePlanSnapshots` the plan is parsed here instead, and that branch is the one that writes - * the store itself and rewrites `plans[index]` through a spread. The spread has to carry the id - * across, or the plan is left pointing at snapshots it can no longer find. - */ + // The only case on the parse path: every other test hands `addPlan` precomputed snapshots, so + // nothing else reaches the `plans[index]` spread that has to carry the id across. it('keys a plan it parsed itself the same way, without landing on a live plan', () => { const { ctx, world } = mount() // A removal first, so the parsed plan's position and its id genuinely differ. Added straight @@ -232,8 +189,6 @@ describe('plan identity', () => { expect(ctx.plans[1]!.status).toBe('ready') expect(ctx.totalSteps).toBe(2) - // plan-1 now sits at the position the parsed plan was written from. Its snapshots have to be - // untouched, which is the same corruption as the one above, on the branch that parses. ctx.selectPlan(0) expect(ctx.totalSteps).toBe(3) @@ -266,12 +221,6 @@ describe('scrubbing', () => { expect(drawnFrames(world)).toEqual([]) }) - /** - * Reconcile runs `updateMetadata` on every step, which resets `Opacity` to its default and drops - * `Invisible` / `ShowAxesHelper`. Without the capture-and-restore around it, scrubbing wipes - * whatever the user set from the Details panel or the tree, one frame at a time. That is a - * regression this module has already had once, and nothing was holding it. - */ it('keeps display edits made while scrubbing', () => { const { ctx, world } = mount() addPlans(ctx, [4]) @@ -292,18 +241,8 @@ describe('scrubbing', () => { }) describe('display defaults', () => { - /** - * `setOrAddColor` used to be a local reimplementation of `setOrAddTrait`; the spawn path now - * calls the shared helper directly. Plan transforms carry no color metadata, so this is the - * "entity doesn't have the trait yet" branch `setOrAddTrait` exists for — the one koota's own - * `entity.set` would write into an unallocated store slot and lose silently, since `has()` - * would stay false and no query would ever see it. - * - * Needs a `physicalObject` on the transform: `drawTransform` only pushes `traits.Geometry` - * (here, `Sphere`) when one is present, and `applyStep` only colors entities that got real - * geometry rather than the bare `ReferenceFrame` marker — `planSnapshots` above omits it, so it - * can't be reused for this one. - */ + // Needs its own `physicalObject`: `applyStep` colors only entities that got real geometry, and + // `planSnapshots` above spawns bare `ReferenceFrame` markers. it('colors a freshly spawned plan entity even though Color is always absent on spawn', () => { const { ctx, world } = mount() ctx.addPlan('plan-0', 'content-0', [ diff --git a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts index 7e201b13c..dc749f04c 100644 --- a/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts +++ b/src/lib/plugins/MotionPlanReplayer/useMotionPlanReplayer.svelte.ts @@ -15,17 +15,6 @@ import * as planRelations from './relations' const PLAN_COLOR = { r: 0, g: 0.47, b: 1 } const PLAN_OPACITY = 0.6 -// Plan transforms carry no color metadata, so `Color` is always absent on spawn; `Opacity` only -// happens to be present because `drawTransform` adds it unconditionally. Neither can be assumed -// present, so both go through `setOrAddTrait` (`$lib/ecs`) rather than a raw `entity.set`. -// -// koota's `entity.set` on a trait this entity lacks does not throw here: it writes the trait's -// store slot without touching the entity's mask, so `has()` stays false and the write is silently -// lost to every query that reads it. `set` only throws when the trait was never registered on the -// *world* at all (`TypeError: Cannot read properties of undefined (reading 'store')`) — a -// different precondition than "this entity doesn't have it", and not one plan entities can hit, -// since other entities register `Color`/`Opacity` on this world well before a plan ever loads. - export interface PlanEntry { name: string content: string @@ -33,10 +22,7 @@ export interface PlanEntry { // Only primitives here — proto objects (Snapshot[]) live outside $state to avoid Svelte 5 deep proxy interface PlanState { - /** - * Survives the reindexing that `removePlan` does to `plans`, which a position cannot. Everything - * held outside `plans` is keyed by this rather than by where the plan currently sits. - */ + /** Survives the reindexing `removePlan` does to `plans`, which a position does not. */ id: number name: string content: string @@ -65,9 +51,8 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { const world = useWorld() const relationships = useRelationships() - // Proto objects stored here — never inside $state to avoid Svelte 5 deep proxy. - // Keyed by `PlanState.id`: keyed by position, removing any plan ahead of the active one left every - // later entry pointing at its neighbour's snapshots. + // Proto objects stored here — never inside $state to avoid Svelte 5 deep proxy + // Keyed by `PlanState.id`, not by position in `plans`. const snapshotStore = new Map() let nextPlanId = 0 @@ -225,8 +210,6 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { if (activePlanIndex === index) clearActivePlan() snapshotStore.delete(removed.id) plans = plans.filter((_, i) => i !== index) - // Every remaining plan keeps its snapshots because they were never keyed by position; only - // `activePlanIndex`, which is one, has to follow the shift. if (activePlanIndex !== null && activePlanIndex > index) { activePlanIndex = activePlanIndex - 1 }