From 6e951c46f5e2f243d4c722a5c0a8d16e161243da Mon Sep 17 00:00:00 2001 From: Matt Dawkins Date: Fri, 25 Sep 2026 10:53:05 -0400 Subject: [PATCH 1/3] Fix web stereo review selection and camera configuration --- client/dive-common/apispec.ts | 3 + .../components/Review/ReviewDatasetsPanel.vue | 7 +- client/dive-common/use/useReview.spec.ts | 10 + client/dive-common/use/useReview.ts | 13 +- client/platform/web-girder/App.vue | 4 + .../web-girder/api/dataset.service.ts | 16 +- client/platform/web-girder/api/index.ts | 2 +- client/platform/web-girder/api/review.spec.ts | 255 ++++++++++++++++++ .../web-girder/api/scoring.service.ts | 12 + .../web-girder/api/scoringDatasetPicker.ts | 12 +- .../ScoringDatasetPickerDialog.spec.ts | 69 +++++ .../components/ScoringDatasetPickerDialog.vue | 21 +- docs/Review.md | 14 +- 13 files changed, 418 insertions(+), 20 deletions(-) create mode 100644 client/platform/web-girder/api/review.spec.ts create mode 100644 client/platform/web-girder/components/ScoringDatasetPickerDialog.spec.ts diff --git a/client/dive-common/apispec.ts b/client/dive-common/apispec.ts index 43c146267..fd4d0058a 100644 --- a/client/dive-common/apispec.ts +++ b/client/dive-common/apispec.ts @@ -480,6 +480,9 @@ interface Api { * dataset list the review page offers. */ listScoringDatasets?(): Promise; + /** Review includes whole stereo/multicamera sequences, unlike scoring. */ + listReviewDatasets?(): Promise; + pickReviewDataset?(excludeIds: string[]): Promise; /** * Open a platform dataset picker; returns null when the user cancels. * Shared by the scoring and review pages. diff --git a/client/dive-common/components/Review/ReviewDatasetsPanel.vue b/client/dive-common/components/Review/ReviewDatasetsPanel.vue index 54ad9ce05..1f26334c5 100644 --- a/client/dive-common/components/Review/ReviewDatasetsPanel.vue +++ b/client/dive-common/components/Review/ReviewDatasetsPanel.vue @@ -16,7 +16,8 @@ export default defineComponent({ const api = useApi(); const review = useReview(); const picking = ref(false); - const usePicker = computed(() => typeof api.pickScoringDataset === 'function'); + const pickDataset = api.pickReviewDataset ?? api.pickScoringDataset; + const usePicker = computed(() => typeof pickDataset === 'function'); const selectedIds = computed(() => review.datasets.value.map((d) => d.id)); @@ -35,10 +36,10 @@ export default defineComponent({ } async function openPicker() { - if (!api.pickScoringDataset || picking.value) return; + if (!pickDataset || picking.value) return; picking.value = true; try { - const picked = await api.pickScoringDataset(selectedIds.value); + const picked = await pickDataset(selectedIds.value); if (picked) await review.addDataset(picked.id, picked, { defer: true }); } finally { picking.value = false; diff --git a/client/dive-common/use/useReview.spec.ts b/client/dive-common/use/useReview.spec.ts index dc2f973e1..09fda65bd 100644 --- a/client/dive-common/use/useReview.spec.ts +++ b/client/dive-common/use/useReview.spec.ts @@ -53,6 +53,16 @@ function makeApi(tracksById: Record, overrides: Partial { + it('uses the review-specific dataset list when a platform separates it from scoring', async () => { + const listReviewDatasets = vi.fn(async () => [{ id: 'rig', name: 'Stereo', type: 'multi' }]); + const api = makeApi({}, { listReviewDatasets }); + const service = createReviewService({ api }); + await service.refreshAvailable(); + expect(service.available.value).toEqual([{ id: 'rig', name: 'Stereo', type: 'multi' }]); + expect(api.listScoringDatasets).not.toHaveBeenCalled(); + service.dispose(); + }); + it('uses the web tracks-only reader without loading unused annotation data', async () => { const loadReviewTracks = vi.fn(async () => [track(1, [['fish', 0.9]], [0])]); const api = makeApi({}, { loadReviewTracks }); diff --git a/client/dive-common/use/useReview.ts b/client/dive-common/use/useReview.ts index 15d268fe4..9def6a209 100644 --- a/client/dive-common/use/useReview.ts +++ b/client/dive-common/use/useReview.ts @@ -1,3 +1,4 @@ +import { orderedMultiCamCameraNames } from 'dive-common/multicamDisplay'; import { orderedHeadTail } from 'vue-media-annotator/headTail'; /** * State behind the Review page: the datasets under review (with their @@ -47,7 +48,7 @@ export interface ReviewGeometryEdit { export type ReviewApi = Pick; + | 'listScoringDatasets' | 'pickScoringDataset' | 'listReviewDatasets' | 'pickReviewDataset'>; export interface ReviewServiceDeps { api: ReviewApi; @@ -322,9 +323,10 @@ function createScopedReviewService(deps: ReviewServiceDeps): ReviewService { } async function refreshAvailable() { - if (!api.listScoringDatasets) return; + const listDatasets = api.listReviewDatasets ?? api.listScoringDatasets; + if (!listDatasets) return; try { - const result = await requests.run(() => api.listScoringDatasets!()); + const result = await requests.run(() => listDatasets()); if (!disposed) available.value = result; } catch (err) { fail(err, 'Could not list datasets'); @@ -372,7 +374,10 @@ function createScopedReviewService(deps: ReviewServiceDeps): ReviewService { if (!isCurrent()) return; if (config.type === 'multi') { // Load each camera separately while exposing the parent as one selected sequence. - const cameras = Object.keys(config.multiCamMedia?.cameras || {}); + const cameras = [...new Set([ + ...orderedMultiCamCameraNames(config.multiCamMedia), + ...Object.keys(config.multiCamMedia?.cameras || {}), + ])]; const parentName = entry(id)?.name || config.name; if (!cameras.length) throw new Error('This sequence has no cameras'); parentNames.set(id, parentName); diff --git a/client/platform/web-girder/App.vue b/client/platform/web-girder/App.vue index 835077398..9a35c3c86 100644 --- a/client/platform/web-girder/App.vue +++ b/client/platform/web-girder/App.vue @@ -50,9 +50,11 @@ import { deleteScoringResult, listScoringSources, listScoringDatasets, + listReviewDatasets, saveScoringExport, exportScoringPdf, pickScoringDataset, + pickReviewDataset, } from './api'; import ScoringDatasetPickerDialog from './components/ScoringDatasetPickerDialog.vue'; import { @@ -127,7 +129,9 @@ export default defineComponent({ deleteScoringResult, listScoringSources: unwrap(listScoringSources), listScoringDatasets, + listReviewDatasets, pickScoringDataset, + pickReviewDataset, saveScoringExport, exportScoringPdf, }); diff --git a/client/platform/web-girder/api/dataset.service.ts b/client/platform/web-girder/api/dataset.service.ts index 871a30971..c1c4390c1 100644 --- a/client/platform/web-girder/api/dataset.service.ts +++ b/client/platform/web-girder/api/dataset.service.ts @@ -107,11 +107,23 @@ function mergeDatasetConfig( */ async function loadDatasetConfig(datasetId: string): Promise { const { compositeId } = await resolveDatasetFolderId(datasetId); - const [metaStatic, media] = await Promise.all([ + const [metaStatic, media, parentConfig] = await Promise.all([ getDataset(datasetId), getDatasetMedia(datasetId), + compositeId + ? girderRest.get(`dive_dataset/${parentDatasetId(datasetId)}/configuration`) + : Promise.resolve(null), ]); - return mergeDatasetConfig(metaStatic.data, media.data, compositeId); + const config = mergeDatasetConfig(metaStatic.data, media.data, compositeId); + if (parentConfig) { + // The parent owns the shared hierarchy. In particular, an absent parent + // hierarchy must not revive obsolete edges stored on a camera folder. + config.typeHierarchy = parentConfig.data.typeHierarchy; + config.customTypeStyling = { + ...config.customTypeStyling, ...parentConfig.data.customTypeStyling, + }; + } + return config; } function clone({ diff --git a/client/platform/web-girder/api/index.ts b/client/platform/web-girder/api/index.ts index 9bcca84e4..cd1d78031 100644 --- a/client/platform/web-girder/api/index.ts +++ b/client/platform/web-girder/api/index.ts @@ -13,7 +13,7 @@ export * from './girder.service'; export * from './multicamResolve'; export * from './rpc.service'; export * from './scoring.service'; -export { pickScoringDataset } from './scoringDatasetPicker'; +export { pickScoringDataset, pickReviewDataset } from './scoringDatasetPicker'; export * from './waitForFolderDatasetReady'; export { default as watchPipelineJob, watchScoringJob } from './watchPipelineJob'; export * from './largeImage.service'; diff --git a/client/platform/web-girder/api/review.spec.ts b/client/platform/web-girder/api/review.spec.ts new file mode 100644 index 000000000..f687ba569 --- /dev/null +++ b/client/platform/web-girder/api/review.spec.ts @@ -0,0 +1,255 @@ +import { effectScope } from 'vue'; +import type { DatasetConfig, SaveDetectionsArgs } from 'dive-common/apispec'; +import type { TrackData } from 'vue-media-annotator/track'; +import { createReviewService, ReviewService } from 'dive-common/use/useReview'; +import { createFrameSource } from 'dive-common/review/frameSource'; +import girderRest from 'platform/web-girder/plugins/girder'; +import { loadDatasetConfig } from './dataset.service'; +import { loadReviewTracks, saveDetections } from './annotation.service'; +import { listReviewDatasets } from './scoring.service'; +import { clearMultiCamMetaCache } from './multicamResolve'; + +vi.mock('platform/web-girder/plugins/girder', () => ({ default: { get: vi.fn(), patch: vi.fn() } })); +vi.mock('dive-common/review/frameSource', () => ({ + createFrameSource: vi.fn(() => ({ frameCount: 3, getFrame: vi.fn(), dispose: vi.fn() })), +})); + +function track(frames: number[]): TrackData { + return { + id: 7, + begin: 0, + end: 2, + attributes: {}, + confidencePairs: [['fish', 0.9], ['animal', 0.3]], + features: frames.map((frame) => ({ frame, keyframe: true, bounds: [0, 0, 20, 20] })), + }; +} + +const media = (camera: string) => ({ + imageData: [0, 1, 2].map((frame) => ({ url: `/api/v1/${camera}/${frame}.jpg`, filename: `${frame}.jpg` })), +}); +const parentConfig = { + typeHierarchy: { fish: 'animal', shark: 'animal' }, + customTypeStyling: { fish: { color: '#123456' } }, +}; + +describe('web stereo review through the real platform adapters', () => { + let review: ReviewService; + let scope: ReturnType; + let tracks: Record; + let failCamera = ''; + beforeEach(() => { + vi.resetAllMocks(); + clearMultiCamMetaCache(); + tracks = { leftFolder: [track([0, 2])], rightFolder: [track([0, 1, 2])] }; + failCamera = ''; + vi.mocked(createFrameSource).mockImplementation(() => ({ + frameCount: 3, getFrame: vi.fn(), dispose: vi.fn(), + })); + vi.mocked(girderRest.get).mockImplementation(async (url, options) => { + if (url === 'folder/rig') { + return { + data: { + meta: { + multiCam: { + defaultDisplay: 'right', cameras: { left: { folderId: 'leftFolder' }, right: { folderId: 'rightFolder' } }, + }, + }, + }, + } as never; + } + if (url === 'dive_dataset/rig') { + return { + data: { + id: 'rig', + name: 'Stereo', + type: 'multi', + subType: 'stereo', + fps: 10, + multiCamMedia: { + // Deliberately different from object-key order (as with sorted server JSON). + cameraOrder: ['right', 'left'], + defaultDisplay: 'right', + cameras: { + left: { type: 'image-sequence', ...media('left'), videoUrl: '' }, + right: { type: 'image-sequence', ...media('right'), videoUrl: '' }, + }, + }, + ...parentConfig, + }, + } as never; + } + if (url === 'dive_dataset/rig/media') return { data: { imageData: [] } } as never; + if (url === 'dive_dataset/rig/configuration') return { data: parentConfig } as never; + if (url === 'dive_annotation/track') { + const folder = options?.params.folderId; + if (folder === failCamera) throw new Error('Camera access denied'); + return { data: JSON.parse(JSON.stringify(tracks[folder])) } as never; + } + const match = String(url).match(/^dive_dataset\/(leftFolder|rightFolder)(\/media)?$/); + if (match) { + return { + data: match[2] ? media(match[1]) : { + id: match[1], + name: match[1], + type: 'image-sequence', + fps: 10, + typeHierarchy: { fish: 'obsolete camera parent' }, + }, + } as never; + } + throw new Error(`Unexpected GET ${url}`); + }); + vi.mocked(girderRest.patch).mockImplementation(async (_url, body, options) => { + const folder = options?.params.folderId; + if (folder === failCamera) throw new Error('Write denied'); + const { upsert, delete: deleted } = (body as SaveDetectionsArgs).tracks; + tracks[folder] = tracks[folder].filter((item) => !deleted.includes(item.id)); + upsert.forEach((item: TrackData) => { + tracks[folder] = [...tracks[folder].filter((old) => old.id !== item.id), JSON.parse(JSON.stringify(item))]; + }); + return { data: {} } as never; + }); + scope = effectScope(); + review = scope.run(() => createReviewService({ + api: { + loadConfig: loadDatasetConfig, + peekConfig: loadDatasetConfig, + loadDetections: vi.fn(), + loadReviewTracks, + saveDetections, + }, + }))!; + }); + afterEach(() => { review.dispose(); scope.stop(); }); + + it('loads both cameras in display order, aligns sparse frames, and uses their own media', async () => { + await review.addDataset('rig'); + expect(review.datasets.value).toMatchObject([{ id: 'rig', status: 'ready', trackCount: 1 }]); + const [entry] = review.entries.value; + expect(entry.labels).toEqual(['right', 'left']); + expect(entry.items.map((item) => item.datasetId)).toEqual(['rig/right', 'rig/left']); + expect(entry.items.map((item) => item.frames.map((frame) => frame.frame))).toEqual([[0, 1, 2], [0, 1, 2]]); + expect(entry.items[1].frames[1].missing).toBe(true); + expect(review.parentOf('rig/right')).toBe('rig'); + expect(review.colorFor('fish')).toBe('#123456'); + const configs = vi.mocked(createFrameSource).mock.calls.map(([config]) => config); + expect(configs).toHaveLength(2); + configs.forEach((config) => { + const folder = config.id.endsWith('/left') ? 'leftFolder' : 'rightFolder'; + expect(config.imageData[0].url).toBe(`/api/v1/${folder}/0.jpg`); + expect(config.typeHierarchy).toEqual(parentConfig.typeHierarchy); + }); + }); + + it('keeps parent claims during type edits and persists edits/deletions to both camera folders', async () => { + await review.addDataset('rig'); + const [entry] = review.entries.value; + entry.items.forEach((item) => review.assignType(item, 'shark')); + const left = entry.items[1]; + review.addKeyframe(left, 1, [1, 2, 11, 12]); + review.updateGeometry(left, 0, { bounds: [2, 3, 12, 13] }); + await review.save(); + expect(review.pendingCount.value).toBe(0); + expect(tracks.leftFolder[0].confidencePairs).toEqual([['shark', 1], ['animal', 0.3]]); + expect(tracks.rightFolder[0].confidencePairs).toEqual(tracks.leftFolder[0].confidencePairs); + expect(tracks.leftFolder[0].features[1].bounds).toEqual([1, 2, 11, 12]); + expect(tracks.rightFolder[0].features[0].bounds).toEqual([0, 0, 20, 20]); + await review.reloadDataset('rig'); + expect(review.entries.value).toHaveLength(1); + review.entries.value[0].items.forEach((item) => review.deleteTrack(item)); + await review.save(); + expect(tracks).toEqual({ leftFolder: [], rightFolder: [] }); + expect(vi.mocked(girderRest.patch).mock.calls.every(([, , options]) => options?.params.folderId !== 'rig')).toBe(true); + }); + + it('loads each stereo video URL and frame rate without using the parent media', async () => { + const original = vi.mocked(girderRest.get).getMockImplementation()!; + vi.mocked(girderRest.get).mockImplementation(async (url, options) => { + const response = await original(url, options) as { data: DatasetConfig }; + if (/dive_dataset\/(leftFolder|rightFolder)$/.test(String(url))) { + return { + data: { + ...response.data, type: 'video', fps: 5, originalFps: 30, + }, + } as never; + } + if (/dive_dataset\/(leftFolder|rightFolder)\/media$/.test(String(url))) { + return { data: { imageData: [], video: { url: `${url}/movie.mp4` } } } as never; + } + return response; + }); + await review.addDataset('rig'); + expect(review.entries.value[0].items).toHaveLength(2); + const configs = vi.mocked(createFrameSource).mock.calls.map(([config]) => config); + expect(configs.map((config) => config.videoUrl).sort()).toEqual([ + 'dive_dataset/leftFolder/media/movie.mp4', 'dive_dataset/rightFolder/media/movie.mp4', + ]); + configs.forEach((config) => expect(config).toMatchObject({ type: 'video', fps: 5, originalFps: 30 })); + }); + + it('does not omit a camera when a legacy cameraOrder list is incomplete', async () => { + const original = vi.mocked(girderRest.get).getMockImplementation()!; + vi.mocked(girderRest.get).mockImplementation(async (url, options) => { + const response = await original(url, options) as { data: DatasetConfig }; + if (url === 'dive_dataset/rig') { + return { data: { ...response.data, multiCamMedia: { ...response.data.multiCamMedia, cameraOrder: ['right'] } } } as never; + } + return response; + }); + await review.addDataset('rig'); + expect(review.entries.value[0].labels).toEqual(['right', 'left']); + }); + + it('removes stale camera hierarchy when the parent has no hierarchy', async () => { + const original = vi.mocked(girderRest.get).getMockImplementation()!; + vi.mocked(girderRest.get).mockImplementation((url, options) => ( + url === 'dive_dataset/rig/configuration' + ? Promise.resolve({ data: {} } as never) : original(url, options) + )); + const config = await loadDatasetConfig('rig/left'); + expect(config.typeHierarchy).toBeUndefined(); + expect(config.imageData[0].url).toBe('/api/v1/leftFolder/0.jpg'); + }); + + it('retains only failed camera edits for retry', async () => { + await review.addDataset('rig'); + review.entries.value[0].items.forEach((item) => review.acceptType(item)); + failCamera = 'rightFolder'; + await review.save(); + expect(review.error.value).toBeTruthy(); + expect(review.pendingCount.value).toBe(1); + failCamera = ''; + await review.save(); + expect(review.pendingCount.value).toBe(0); + }); + + it('reports a denied camera rather than marking the whole rig ready', async () => { + failCamera = 'rightFolder'; + await review.addDataset('rig'); + expect(review.datasets.value[0].status).toBe('error'); + expect(review.datasets.value[0].error).toContain('Camera access denied'); + failCamera = ''; + await review.reloadDataset('rig'); + expect(review.datasets.value[0].status).toBe('ready'); + expect(review.entries.value[0].items).toHaveLength(2); + }); +}); + +it('lists stereo parents once for review without changing the scoring list', async () => { + vi.mocked(girderRest.get).mockResolvedValue({ + data: [ + { _id: 'rig', name: 'Stereo', meta: { type: 'multi' } }, + { + _id: 'leftFolder', parentId: 'rig', name: 'left', meta: { type: 'video' }, + }, + { + _id: 'rightFolder', parentId: 'rig', name: 'right', meta: { type: 'video' }, + }, + { _id: 'single', name: 'Single', meta: { type: 'video' } }, + ], + } as never); + await expect(listReviewDatasets()).resolves.toEqual([ + { id: 'rig', name: 'Stereo', type: 'multi' }, { id: 'single', name: 'Single', type: 'video' }, + ]); +}); diff --git a/client/platform/web-girder/api/scoring.service.ts b/client/platform/web-girder/api/scoring.service.ts index 0bdbfecca..6a0c4f395 100644 --- a/client/platform/web-girder/api/scoring.service.ts +++ b/client/platform/web-girder/api/scoring.service.ts @@ -70,6 +70,17 @@ async function listScoringDatasets(): Promise { })); } +/** Review operates on whole sequences; suppress their individual camera folders. */ +async function listReviewDatasets(): Promise { + const { data } = await getDatasetList(SCORING_DATASET_LIMIT, 0, 'name', 1); + const parents = new Set(data.filter((folder) => folder.meta?.type === 'multi').map((folder) => folder._id)); + return data.filter((folder) => !(folder.parentId && parents.has(folder.parentId))).map((folder) => ({ + id: folder._id, + name: folder.name, + type: typeof folder.meta?.type === 'string' ? folder.meta.type : undefined, + })); +} + export { runScoring, listScoringResults, @@ -77,6 +88,7 @@ export { deleteScoringResult, listScoringSources, listScoringDatasets, + listReviewDatasets, }; /** Browser download; the user's download settings decide the destination. */ diff --git a/client/platform/web-girder/api/scoringDatasetPicker.ts b/client/platform/web-girder/api/scoringDatasetPicker.ts index de007fda1..c1e234f6f 100644 --- a/client/platform/web-girder/api/scoringDatasetPicker.ts +++ b/client/platform/web-girder/api/scoringDatasetPicker.ts @@ -3,18 +3,20 @@ import { reactive } from 'vue'; export const scoringDatasetPickerState = reactive({ open: false, + purpose: 'scoring' as 'scoring' | 'review', excludeIds: [] as string[], }); let pendingResolve: ((value: ScoringDatasetSummary | null) => void) | null = null; -export function pickScoringDataset(excludeIds: string[]): Promise { +function pickDataset(excludeIds: string[], purpose: 'scoring' | 'review'): Promise { return new Promise((resolve) => { if (pendingResolve) { pendingResolve(null); } pendingResolve = resolve; scoringDatasetPickerState.excludeIds = [...excludeIds]; + scoringDatasetPickerState.purpose = purpose; scoringDatasetPickerState.open = true; }); } @@ -25,3 +27,11 @@ export function finishScoringDatasetPicker(dataset: ScoringDatasetSummary | null pendingResolve?.(dataset); pendingResolve = null; } + +export function pickScoringDataset(excludeIds: string[]) { + return pickDataset(excludeIds, 'scoring'); +} + +export function pickReviewDataset(excludeIds: string[]) { + return pickDataset(excludeIds, 'review'); +} diff --git a/client/platform/web-girder/components/ScoringDatasetPickerDialog.spec.ts b/client/platform/web-girder/components/ScoringDatasetPickerDialog.spec.ts new file mode 100644 index 000000000..dbe2347c9 --- /dev/null +++ b/client/platform/web-girder/components/ScoringDatasetPickerDialog.spec.ts @@ -0,0 +1,69 @@ +import { shallowMount } from '@vue/test-utils'; +import Vue, { ComponentOptions, CreateElement, nextTick } from 'vue'; +import Picker from './ScoringDatasetPickerDialog.vue'; +import { + pickReviewDataset, pickScoringDataset, finishScoringDatasetPicker, scoringDatasetPickerState, +} from '../api/scoringDatasetPicker'; + +vi.mock('@girder/components/src', () => ({ GirderFileManager: {} })); +vi.mock('platform/web-girder/plugins/girder', () => ({ useGirderRest: () => ({ user: { _id: 'user', login: 'reader' } }) })); +vi.mock('platform/web-girder/store/useLocation', () => ({ useLocation: () => ({ getLocation: () => null }) })); + +function mountPicker() { + const wrapper = shallowMount({ + ...(Picker as unknown as ComponentOptions), render: (h: CreateElement) => h('div'), + }); + return { wrapper, vm: wrapper.vm as unknown as InstanceType }; +} +const rig = { + _id: 'rig', _modelType: 'folder' as const, name: 'Stereo', meta: { annotate: true, type: 'multi' }, +}; + +beforeEach(() => finishScoringDatasetPicker(null)); +afterEach(() => finishScoringDatasetPicker(null)); + +it('allows selecting a complete stereo sequence for review', async () => { + const { vm, wrapper } = mountPicker(); + const result = pickReviewDataset([]); + await nextTick(); + vm.setLocation(rig); + expect(vm.canAdd).toBe(true); + expect(vm.invalidSelection).toBeNull(); + vm.confirm(); + await expect(result).resolves.toEqual({ id: 'rig', name: 'Stereo', type: 'multi' }); + wrapper.destroy(); +}); + +it('preserves scoring restrictions when switching back from review', async () => { + const { vm, wrapper } = mountPicker(); + const review = pickReviewDataset([]); + await nextTick(); + vm.setLocation(rig); + vm.cancel(); + await expect(review).resolves.toBeNull(); + await nextTick(); + const scoring = pickScoringDataset([]); + await nextTick(); + expect(scoringDatasetPickerState.purpose).toBe('scoring'); + vm.setLocation(rig); + expect(vm.canAdd).toBe(false); + vm.setLocation({ + ...rig, _id: 'left', name: 'left', meta: { annotate: true, type: 'video' }, + }); + expect(vm.canAdd).toBe(true); + vm.confirm(); + await expect(scoring).resolves.toEqual({ id: 'left', name: 'left', type: 'video' }); + wrapper.destroy(); +}); + +it('prevents duplicate parent selections and cancels when dismissed', async () => { + const { vm, wrapper } = mountPicker(); + const result = pickReviewDataset(['rig']); + await nextTick(); + vm.setLocation(rig); + expect(vm.canAdd).toBe(false); + expect(vm.invalidSelection).toContain('already'); + vm.onDialogInput(false); + await expect(result).resolves.toBeNull(); + wrapper.destroy(); +}); diff --git a/client/platform/web-girder/components/ScoringDatasetPickerDialog.vue b/client/platform/web-girder/components/ScoringDatasetPickerDialog.vue index 814ad0681..5416de694 100644 --- a/client/platform/web-girder/components/ScoringDatasetPickerDialog.vue +++ b/client/platform/web-girder/components/ScoringDatasetPickerDialog.vue @@ -21,10 +21,10 @@ type BrowserLocation = { meta?: { annotate?: boolean | string; type?: string }; }; -function isScoringDataset(loc: BrowserLocation): boolean { +function isSelectableDataset(loc: BrowserLocation): boolean { return loc._modelType === 'folder' && !!loc.meta?.annotate - && loc.meta?.type !== 'multi'; + && (scoringDatasetPickerState.purpose === 'review' || loc.meta?.type !== 'multi'); } function toSummary(loc: BrowserLocation): ScoringDatasetSummary { @@ -58,7 +58,7 @@ export default defineComponent({ selected.value = null; const browseLocation = getLocation(); location.value = browseLocation ?? defaultUserLocation(); - if (browseLocation && isGirderModel(browseLocation) && isScoringDataset(browseLocation)) { + if (browseLocation && isGirderModel(browseLocation) && isSelectableDataset(browseLocation)) { selected.value = browseLocation; } } @@ -70,7 +70,7 @@ export default defineComponent({ }); function setLocation(newLoc: BrowserLocation) { - if (isScoringDataset(newLoc)) { + if (isSelectableDataset(newLoc)) { selected.value = newLoc; return; } @@ -86,7 +86,7 @@ export default defineComponent({ const invalidSelection = computed(() => { if (!selected.value) return null; if (excluded.value) return 'This dataset is already in the list.'; - if (selected.value.meta?.type === 'multi') { + if (scoringDatasetPickerState.purpose === 'scoring' && selected.value.meta?.type === 'multi') { return 'Multicamera parent folders cannot be scored; choose a camera dataset instead.'; } if (!selected.value.meta?.annotate) { @@ -96,7 +96,7 @@ export default defineComponent({ }); const canAdd = computed( - () => !!selected.value && isScoringDataset(selected.value) && !excluded.value, + () => !!selected.value && isSelectableDataset(selected.value) && !excluded.value, ); function cancel() { @@ -141,8 +141,13 @@ export default defineComponent({ Choose a dataset - Browse to a DIVE dataset and select it to score. Multicamera parent folders - are not listed as scorable sequences. + + Date: Fri, 25 Sep 2026 11:07:21 -0400 Subject: [PATCH 2/3] Review complete stereo sequences when selecting web camera folders --- client/dive-common/apispec.ts | 2 ++ client/dive-common/use/useReview.ts | 14 ++++++++- client/platform/web-girder/App.vue | 2 ++ .../web-girder/api/multicamResolve.spec.ts | 29 +++++++++++++++++++ .../web-girder/api/multicamResolve.ts | 22 ++++++++++++++ client/platform/web-girder/api/review.spec.ts | 29 ++++++++++++++++++- docs/Review.md | 4 ++- 7 files changed, 99 insertions(+), 3 deletions(-) diff --git a/client/dive-common/apispec.ts b/client/dive-common/apispec.ts index fd4d0058a..11fcff4a5 100644 --- a/client/dive-common/apispec.ts +++ b/client/dive-common/apispec.ts @@ -480,6 +480,8 @@ interface Api { * dataset list the review page offers. */ listScoringDatasets?(): Promise; + /** Resolve a selected camera to its whole sequence before loading review. */ + resolveReviewDatasetId?(datasetId: string): Promise; /** Review includes whole stereo/multicamera sequences, unlike scoring. */ listReviewDatasets?(): Promise; pickReviewDataset?(excludeIds: string[]): Promise; diff --git a/client/dive-common/use/useReview.ts b/client/dive-common/use/useReview.ts index 9def6a209..3bdd68ff5 100644 --- a/client/dive-common/use/useReview.ts +++ b/client/dive-common/use/useReview.ts @@ -48,7 +48,8 @@ export interface ReviewGeometryEdit { export type ReviewApi = Pick; + | 'listScoringDatasets' | 'pickScoringDataset' | 'listReviewDatasets' | 'pickReviewDataset' + | 'resolveReviewDatasetId'>; export interface ReviewServiceDeps { api: ReviewApi; @@ -367,6 +368,17 @@ function createScopedReviewService(deps: ReviewServiceDeps): ReviewService { const isCurrent = () => loadTokens.get(id) === token && !!entry(id); loading.value = true; try { + // Normalize user selections, but keep expanded cameras separate internally + // so media, annotations, and writes continue using their own folders. + if (api.resolveReviewDatasetId && !memberships.has(id)) { + const resolvedId = await requests.run(() => api.resolveReviewDatasetId!(id)); + if (!isCurrent()) return; + if (resolvedId !== id) { + datasets.value = datasets.value.filter((d) => d.id !== id); + await addDataset(resolvedId); + return; + } + } const config = await requests.run(() => { if (!isCurrent()) throw new Error('Dataset removed'); return loadConfig(id); diff --git a/client/platform/web-girder/App.vue b/client/platform/web-girder/App.vue index 9a35c3c86..a2874863c 100644 --- a/client/platform/web-girder/App.vue +++ b/client/platform/web-girder/App.vue @@ -51,6 +51,7 @@ import { listScoringSources, listScoringDatasets, listReviewDatasets, + resolveReviewDatasetId, saveScoringExport, exportScoringPdf, pickScoringDataset, @@ -130,6 +131,7 @@ export default defineComponent({ listScoringSources: unwrap(listScoringSources), listScoringDatasets, listReviewDatasets, + resolveReviewDatasetId, pickScoringDataset, pickReviewDataset, saveScoringExport, diff --git a/client/platform/web-girder/api/multicamResolve.spec.ts b/client/platform/web-girder/api/multicamResolve.spec.ts index 478017b91..515e725e5 100644 --- a/client/platform/web-girder/api/multicamResolve.spec.ts +++ b/client/platform/web-girder/api/multicamResolve.spec.ts @@ -11,6 +11,7 @@ import { clearMultiCamMetaCache, parseCompositeDatasetId, resolveDatasetFolderId, + resolveReviewDatasetId, } from './multicamResolve'; describe('multicamResolve', () => { @@ -75,4 +76,32 @@ describe('multicamResolve', () => { 'Unknown camera "missing"', ); }); + + it('keeps a standalone dataset under an ordinary folder', async () => { + vi.spyOn(girderRest, 'get') + .mockResolvedValueOnce({ data: { parentId: 'ordinary', parentCollection: 'folder' } } as never) + .mockResolvedValueOnce({ data: { meta: {} } } as never); + expect(await resolveReviewDatasetId('standalone')).toBe('standalone'); + }); + + it('does not include unrelated datasets nested beneath a stereo parent', async () => { + vi.spyOn(girderRest, 'get') + .mockResolvedValueOnce({ data: { parentId: 'rig', parentCollection: 'folder' } } as never) + .mockResolvedValueOnce({ + data: { + meta: { + type: 'multi', multiCam: { cameras: { left: { folderId: 'left' } } }, + }, + }, + } as never); + expect(await resolveReviewDatasetId('unrelated')).toBe('unrelated'); + }); + + it('does not resolve a collection id as a folder', async () => { + const get = vi.spyOn(girderRest, 'get').mockResolvedValueOnce({ + data: { parentId: 'collection', parentCollection: 'collection' }, + } as never); + expect(await resolveReviewDatasetId('standalone')).toBe('standalone'); + expect(get).toHaveBeenCalledTimes(1); + }); }); diff --git a/client/platform/web-girder/api/multicamResolve.ts b/client/platform/web-girder/api/multicamResolve.ts index b62113707..c7a336878 100644 --- a/client/platform/web-girder/api/multicamResolve.ts +++ b/client/platform/web-girder/api/multicamResolve.ts @@ -56,3 +56,25 @@ export function clearMultiCamMetaCache(parentId?: string): void { multiCamMetaCache.clear(); } } + +/** Review a whole rig even when entered from a camera folder or viewer link. */ +export async function resolveReviewDatasetId(datasetId: string): Promise { + const { parentId, cameraName } = parseCompositeDatasetId(datasetId); + if (cameraName) return parentId; + const { data: folder } = await girderRest.get<{ + parentId?: string; + parentCollection?: string; + meta?: { type?: string }; + }>(`folder/${datasetId}`); + if (folder.meta?.type === 'multi' || folder.parentCollection !== 'folder' || !folder.parentId) { + return datasetId; + } + const { data: parent } = await girderRest.get<{ + meta?: { type?: string; multiCam?: MultiCamStorageMeta }; + }>(`folder/${folder.parentId}`); + const cameras = parent.meta?.multiCam?.cameras; + // Only registered cameras belong to the sequence. + return parent.meta?.type === 'multi' + && Object.values(cameras ?? {}).some((camera) => camera.folderId === datasetId) + ? folder.parentId : datasetId; +} diff --git a/client/platform/web-girder/api/review.spec.ts b/client/platform/web-girder/api/review.spec.ts index f687ba569..11aa2a750 100644 --- a/client/platform/web-girder/api/review.spec.ts +++ b/client/platform/web-girder/api/review.spec.ts @@ -7,7 +7,7 @@ import girderRest from 'platform/web-girder/plugins/girder'; import { loadDatasetConfig } from './dataset.service'; import { loadReviewTracks, saveDetections } from './annotation.service'; import { listReviewDatasets } from './scoring.service'; -import { clearMultiCamMetaCache } from './multicamResolve'; +import { clearMultiCamMetaCache, resolveReviewDatasetId } from './multicamResolve'; vi.mock('platform/web-girder/plugins/girder', () => ({ default: { get: vi.fn(), patch: vi.fn() } })); vi.mock('dive-common/review/frameSource', () => ({ @@ -47,10 +47,14 @@ describe('web stereo review through the real platform adapters', () => { frameCount: 3, getFrame: vi.fn(), dispose: vi.fn(), })); vi.mocked(girderRest.get).mockImplementation(async (url, options) => { + if (url === 'folder/leftFolder' || url === 'folder/rightFolder') { + return { data: { parentId: 'rig', parentCollection: 'folder', meta: { type: 'image-sequence' } } } as never; + } if (url === 'folder/rig') { return { data: { meta: { + type: 'multi', multiCam: { defaultDisplay: 'right', cameras: { left: { folderId: 'leftFolder' }, right: { folderId: 'rightFolder' } }, }, @@ -117,12 +121,35 @@ describe('web stereo review through the real platform adapters', () => { peekConfig: loadDatasetConfig, loadDetections: vi.fn(), loadReviewTracks, + resolveReviewDatasetId, saveDetections, }, }))!; }); afterEach(() => { review.dispose(); scope.stop(); }); + it.each(['leftFolder', 'rightFolder', 'rig/left', 'rig/right'])('pairs both cameras when review starts from %s', async (id) => { + await review.addDataset(id); + expect(review.datasets.value).toMatchObject([{ id: 'rig', status: 'ready' }]); + expect(review.entries.value).toHaveLength(1); + expect(review.entries.value[0].items.map((item) => item.datasetId)).toEqual(['rig/right', 'rig/left']); + }); + + it('deduplicates camera and parent selections into one stereo sequence', async () => { + await review.addDatasets(['leftFolder', 'rig', 'rightFolder']); + expect(review.datasets.value).toMatchObject([{ id: 'rig', status: 'ready' }]); + expect(review.entries.value).toHaveLength(1); + expect(vi.mocked(createFrameSource)).toHaveBeenCalledTimes(2); + }); + + it('expands a queued camera selection when review results are requested', async () => { + await review.addDataset('leftFolder', undefined, { defer: true }); + expect(vi.mocked(girderRest.get)).not.toHaveBeenCalled(); + await review.loadQueued(); + expect(review.entries.value).toHaveLength(1); + expect(review.entries.value[0].labels).toEqual(['right', 'left']); + }); + it('loads both cameras in display order, aligns sparse frames, and uses their own media', async () => { await review.addDataset('rig'); expect(review.datasets.value).toMatchObject([{ id: 'rig', status: 'ready', trackCount: 1 }]); diff --git a/docs/Review.md b/docs/Review.md index 30c77c63a..3f39dfd00 100644 --- a/docs/Review.md +++ b/docs/Review.md @@ -117,7 +117,9 @@ Every result is also an annotation in waiting. Typing a type under a chip, or ed ### Stereo review on the web -Select the stereo parent sequence in the library or in Review → Datasets → Browse. +Select the stereo sequence in the library or in Review → Datasets → Browse. +Selecting an individual camera folder or entering review from a camera link also +loads the whole sequence, so both sides appear together. The review picker accepts whole stereo/multicamera sequences; scoring still requires a single camera. Cameras appear together as one entry per track, in the sequence's configured camera order, with synchronized frame cycling and zoom/pan. From 4db37b64478c7a701517a46f006ef0b20a77d13e Mon Sep 17 00:00:00 2001 From: Bryon Lewis Date: Fri, 25 Sep 2026 12:54:02 -0400 Subject: [PATCH 3/3] prevent showing sub cameras as their own datasets in the picker --- client/dive-common/use/useReview.spec.ts | 34 +++++++++++++++++++ client/dive-common/use/useReview.ts | 26 +++++++++++--- client/platform/web-girder/api/review.spec.ts | 11 +++++- 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/client/dive-common/use/useReview.spec.ts b/client/dive-common/use/useReview.spec.ts index 09fda65bd..6dc1b2732 100644 --- a/client/dive-common/use/useReview.spec.ts +++ b/client/dive-common/use/useReview.spec.ts @@ -101,6 +101,40 @@ describe('createReviewService', () => { expect(api.loadDetections).toHaveBeenCalledTimes(1); }); + it('queues the resolved parent for a deferred camera pick and skips duplicates', async () => { + const resolveReviewDatasetId = vi.fn(async (id: string) => (id === 'leftFolder' ? 'rig' : id)); + const api = makeApi({ + 'rig/left': [track(1, [['fish', 1]], [0])], + 'rig/right': [track(1, [['fish', 1]], [0])], + }, { + resolveReviewDatasetId, + loadConfig: vi.fn(async (id: string) => (id === 'rig' + ? config('rig', { + type: 'multi', + name: 'Stereo', + multiCamMedia: { + defaultDisplay: 'left', + cameras: { + left: { type: 'image-sequence', imageData: [{ url: 'l.jpg', filename: 'l.jpg' }], videoUrl: '' }, + right: { type: 'image-sequence', imageData: [{ url: 'r.jpg', filename: 'r.jpg' }], videoUrl: '' }, + }, + }, + }) + : config(id))), + }); + const service = createReviewService({ api }); + await service.addDataset('leftFolder', { id: 'leftFolder', name: 'left' }, { defer: true }); + expect(resolveReviewDatasetId).toHaveBeenCalledWith('leftFolder'); + expect(service.datasets.value).toMatchObject([{ id: 'rig', status: 'queued' }]); + expect(api.loadConfig).not.toHaveBeenCalled(); + await service.loadQueued(); + expect(service.datasets.value).toMatchObject([{ id: 'rig', status: 'ready' }]); + // Deferred browse of a camera folder must not sit beside the loaded rig. + await service.addDataset('leftFolder', { id: 'leftFolder', name: 'left' }, { defer: true }); + expect(service.datasets.value).toEqual([expect.objectContaining({ id: 'rig', status: 'ready' })]); + service.dispose(); + }); + it('loads datasets, prefers peekConfig, and builds items for a query', async () => { const peekConfig = vi.fn(async (id: string) => config(id)); const api = makeApi({ diff --git a/client/dive-common/use/useReview.ts b/client/dive-common/use/useReview.ts index 3bdd68ff5..fea365578 100644 --- a/client/dive-common/use/useReview.ts +++ b/client/dive-common/use/useReview.ts @@ -446,20 +446,36 @@ function createScopedReviewService(deps: ReviewServiceDeps): ReviewService { /** * Add a dataset; with `defer` it only joins the list and loads on the * next `loadQueued`, so picking many datasets costs nothing until the - * results are actually wanted. + * results are actually wanted. Deferred picks still resolve camera folders + * to their sequence so a browse pick cannot sit beside an already-loaded rig. */ async function addDataset(id: string, summary?: ScoringDatasetSummary, options: { defer?: boolean } = {}) { if (disposed || !id || entry(id) || selectedDatasets.value.some((dataset) => dataset.id === id)) return; + let selectedId = id; + let selectedSummary = summary; + if (options.defer && api.resolveReviewDatasetId) { + try { + const resolvedId = await api.resolveReviewDatasetId(id); + if (resolvedId !== id) { + selectedId = resolvedId; + // Drop the camera-folder summary; the parent owns the sequence name. + selectedSummary = undefined; + } + } catch { + // Keep the original id; load() will surface the error. + } + } + if (entry(selectedId) || selectedDatasets.value.some((dataset) => dataset.id === selectedId)) return; datasets.value = [...datasets.value, { - id, - name: summary?.name || datasetName(id), - type: summary?.type, + id: selectedId, + name: selectedSummary?.name || datasetName(selectedId), + type: selectedSummary?.type, status: options.defer ? 'queued' : 'loading', trackCount: 0, croppable: false, }]; if (options.defer) return; - await load(id); + await load(selectedId); } /** Load every queued dataset; annotations are read and the query rerun as each arrives. */ diff --git a/client/platform/web-girder/api/review.spec.ts b/client/platform/web-girder/api/review.spec.ts index 11aa2a750..de13de2ae 100644 --- a/client/platform/web-girder/api/review.spec.ts +++ b/client/platform/web-girder/api/review.spec.ts @@ -144,12 +144,21 @@ describe('web stereo review through the real platform adapters', () => { it('expands a queued camera selection when review results are requested', async () => { await review.addDataset('leftFolder', undefined, { defer: true }); - expect(vi.mocked(girderRest.get)).not.toHaveBeenCalled(); + // Deferred camera picks resolve to the parent id, but skip config/tracks until loadQueued. + expect(review.datasets.value).toMatchObject([{ id: 'rig', status: 'queued' }]); + expect(vi.mocked(girderRest.get).mock.calls.every(([url]) => String(url).startsWith('folder/'))).toBe(true); await review.loadQueued(); expect(review.entries.value).toHaveLength(1); expect(review.entries.value[0].labels).toEqual(['right', 'left']); }); + it('ignores a deferred camera pick when its stereo sequence is already selected', async () => { + await review.addDataset('rig'); + await review.addDataset('leftFolder', { id: 'leftFolder', name: 'left' }, { defer: true }); + expect(review.datasets.value).toMatchObject([{ id: 'rig', status: 'ready' }]); + expect(review.datasets.value).toHaveLength(1); + }); + it('loads both cameras in display order, aligns sparse frames, and uses their own media', async () => { await review.addDataset('rig'); expect(review.datasets.value).toMatchObject([{ id: 'rig', status: 'ready', trackCount: 1 }]);