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/MotionPlanReplayerUI.svelte b/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte index b59740e34..60669bc70 100644 --- a/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte +++ b/src/lib/plugins/MotionPlanReplayer/MotionPlanReplayerUI.svelte @@ -97,7 +97,9 @@ {/if} - {#each ctx.plans as plan, i (plan.name)} + + {#each ctx.plans as plan, i (plan.id)} {@const isActive = ctx.activePlanIndex === i}
{ + const MockFloatingPanel = await import('./__fixtures__/MockFloatingPanel.svelte') + return { default: MockFloatingPanel.default } +}) + +// 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 } +}) + +// useToast requires a `provideToast` ancestor; nothing here checks toast content. +vi.mock('@viamrobotics/prime-core', async (importOriginal) => ({ + ...(await importOriginal()), + useToast: () => vi.fn(), +})) + +describe('MotionPlanReplayerUI', () => { + // 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: [ + { name: 'same.json', content: 'content-a' }, + { name: 'same.json', content: 'content-b' }, + ], + }, + }) + + 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__/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__/ReplayerHarness.svelte b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte new file mode 100644 index 000000000..d66618921 --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerHarness.svelte @@ -0,0 +1,31 @@ + 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..446923aae --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/__fixtures__/ReplayerUIHarness.svelte @@ -0,0 +1,27 @@ + + + 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..5ba8e2a80 --- /dev/null +++ b/src/lib/plugins/MotionPlanReplayer/__tests__/useMotionPlanReplayer.svelte.spec.ts @@ -0,0 +1,268 @@ +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 { 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' + +import type { MotionPlanReplayerContext } from '../useMotionPlanReplayer.svelte' + +import gantryPlan from './__fixtures__/gantry-plan.json?raw' +import ReplayerHarness from './__fixtures__/ReplayerHarness.svelte' + +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 +} + +/** + * 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 }, + () => + new Snapshot({ + transforms: [ + new Transform({ + referenceFrame: `${plan}-frame`, + poseInObserverFrame: new PoseInFrame({ referenceFrame: 'world' }), + // 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`)), + }), + ], + }) + ) + +const addPlans = (ctx: MotionPlanReplayerContext, lengths: number[]) => { + for (const [i, length] of lengths.entries()) { + ctx.addPlan(`plan-${i}`, `content-${i}`, planSnapshots(`plan-${i}`, length)) + } +} + +/** + * 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 + .query(traits.Name) + .map((entity: Entity) => entity.get(traits.Name)) + .filter((name): name is string => typeof name === 'string' && name.endsWith('-frame')) + .toSorted() + +const drawnEntity = (world: World): Entity => + world.query(traits.Name).find((entity) => entity.get(traits.Name)?.endsWith('-frame'))! + +describe('removing a plan', () => { + it('leaves the active plan reading its own snapshots, not its neighbour’s', () => { + const { ctx, world } = 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) + 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, world } = 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) + expect(drawnFrames(world)).toEqual(['plan-1-frame']) + ctx.setStep(5) + expect(ctx.currentStep).toBe(5) + }) + + // `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]) + + 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, world } = 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']) + expect(drawnFrames(world)).toEqual([]) + }) + + 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) + }) + + // 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], + ['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(index) + + 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', () => { + 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) + + ctx.selectPlan(0) + expect(ctx.totalSteps).toBe(2) + ctx.selectPlan(1) + expect(ctx.totalSteps).toBe(5) + }) + + // 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 + // 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) + + 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([]) + }) + + 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) + }) +}) + +describe('display defaults', () => { + // 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', [ + 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 ac77119f4..dc749f04c 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,21 +15,6 @@ 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. -// 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)) -} - export interface PlanEntry { name: string content: string @@ -37,6 +22,8 @@ export interface PlanEntry { // Only primitives here — proto objects (Snapshot[]) live outside $state to avoid Svelte 5 deep proxy interface PlanState { + /** Survives the reindexing `removePlan` does to `plans`, which a position does not. */ + id: number name: string content: string status: 'idle' | 'ready' | 'error' | 'no-trajectory' @@ -65,10 +52,14 @@ export const provideMotionPlanReplayer = (initialPlans?: PlanEntry[]) => { const relationships = useRelationships() // 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 + let plans = $state( (initialPlans ?? []).map((e) => ({ + id: nextPlanId++, name: e.name, content: e.content, status: 'idle' as const, @@ -126,14 +117,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) @@ -146,7 +138,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 +148,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 +165,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 +184,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,8 +204,11 @@ 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) if (activePlanIndex !== null && activePlanIndex > index) { activePlanIndex = activePlanIndex - 1