From 4172d0427b6642dfa666c67c0d9e7b0947d56b05 Mon Sep 17 00:00:00 2001 From: Joe Boccanfuso <109477394+jbocce@users.noreply.github.com> Date: Fri, 12 May 2023 16:24:00 -0400 Subject: [PATCH] fix(Measurements): in MeasurementService keep a separate set of unmapped measurements (#3385) * fix(Measurements): return {} instead of undefined for getSOPInstanceAttributes * PR feedback: - separated measurements from non-acquisition plane - reverted to previous behaviour of returning undefined in getSOPInstanceAttributes * Added unit tests for unmapped measurements. * PR feedback: added comments to the unmappedMeasurement used in the unit tests. --- .../utils/getSOPInstanceAttributes.js | 47 +-------- .../MeasurementService.test.js | 95 +++++++++++++++++++ .../MeasurementService/MeasurementService.ts | 12 ++- 3 files changed, 108 insertions(+), 46 deletions(-) diff --git a/extensions/cornerstone/src/utils/measurementServiceMappings/utils/getSOPInstanceAttributes.js b/extensions/cornerstone/src/utils/measurementServiceMappings/utils/getSOPInstanceAttributes.js index c0707dd4f8c..dd6992b831b 100644 --- a/extensions/cornerstone/src/utils/measurementServiceMappings/utils/getSOPInstanceAttributes.js +++ b/extensions/cornerstone/src/utils/measurementServiceMappings/utils/getSOPInstanceAttributes.js @@ -3,39 +3,16 @@ import * as cornerstone from '@cornerstonejs/core'; /** * It checks if the imageId is provided then it uses it to query * the metadata and get the SOPInstanceUID, SeriesInstanceUID and StudyInstanceUID. - * If the imageId is not provided then it uses the sceneUID to get the viewports - * inside the scene and then it checks each viewport to find the one that has - * acquisition plane view, and uses the currentImageId of the viewport to - * query the metadata and get UIDs. + * If the imageId is not provided then undefined is returned. * @param {string} imageId The image id of the referenced image - * @param {string} sceneUID The scene UID of the measurement tool * @returns */ -export default function getSOPInstanceAttributes( - imageId, - cornerstoneViewportService = undefined, - viewportId = undefined -) { +export default function getSOPInstanceAttributes(imageId) { if (imageId) { return _getUIDFromImageID(imageId); } // Todo: implement for volume viewports and use the referencedSeriesInstanceUID - - // if no imageId => measurement is not in the acquisition plane - // const metadata = getUIDFromScene(cornerstoneViewportService, viewportId); - - // if (!metadata) { - // throw new Error('Not viewport with imageId found'); - // } - - // // Since the series and study UID is derived from another viewport in the - // // same scene, we cannot include the SOPInstanceUID - // return { - // SOPInstanceUID: null, - // SeriesInstanceUID: metadata.SeriesInstanceUID, - // StudyInstanceUID: metadata.StudyInstanceUID, - // }; } function _getUIDFromImageID(imageId) { @@ -48,23 +25,3 @@ function _getUIDFromImageID(imageId) { frameNumber: instance.frameNumber || 1, }; } - -// function getUIDFromScene(cornerstoneViewportService) { -// const renderingEngine = cornerstoneViewportService.getRenderingEngine(); -// const scene = renderingEngine.getScene(sceneUID); - -// const viewportUIDs = scene.getViewportIds(); - -// if (viewportUIDs.length === 0) { -// throw new Error('No viewport found in scene'); -// } - -// for (let i = 0; i < viewportUIDs.length; i++) { -// const vp = renderingEngine.getViewport(viewportUIDs[i]); -// const imageId = vp.getCurrentImageId(); - -// if (imageId) { -// return _getUIDFromImageID(imageId); -// } -// } -// } diff --git a/platform/core/src/services/MeasurementService/MeasurementService.test.js b/platform/core/src/services/MeasurementService/MeasurementService.test.js index 75113e37142..78c13c83703 100644 --- a/platform/core/src/services/MeasurementService/MeasurementService.test.js +++ b/platform/core/src/services/MeasurementService/MeasurementService.test.js @@ -8,13 +8,16 @@ jest.mock('../../log', () => ({ })); describe('MeasurementService.js', () => { + const unmappedMeasurementUID = 'unmappedMeasurementUId'; let measurementService; let measurement; + let unmappedMeasurement; let source; let annotationType; let matchingCriteria; let toSourceSchema; let toMeasurement; + let toMeasurementThrowsError; let annotation; beforeEach(() => { @@ -40,6 +43,25 @@ describe('MeasurementService.js', () => { ], source: source, }; + // A measurement with various metadata missing (e.g. referenced SOPInstanceUID) that + // would not typically get mapped my the MeasurementService possibly because it was + // made in a non-acquisition plane of a volume. + unmappedMeasurement = { + uid: unmappedMeasurementUID, + SOPInstanceUID: undefined, + FrameOfReferenceUID: undefined, + referenceSeriesUID: undefined, + label: 'Label', + description: 'Description', + unit: 'mm', + area: 123, + type: measurementService.VALUE_TYPES.POLYLINE, + points: [ + { x: 1, y: 2 }, + { x: 1, y: 2 }, + ], + source: source, + }; toSourceSchema = () => annotation; toMeasurement = () => { if (Object.keys(measurement).includes('invalidProperty')) { @@ -48,6 +70,9 @@ describe('MeasurementService.js', () => { return measurement; }; + toMeasurementThrowsError = () => { + throw new Error('Unmapped measurement.'); + }; matchingCriteria = { valueType: measurementService.VALUE_TYPES.POLYLINE, points: 2, @@ -429,5 +454,75 @@ describe('MeasurementService.js', () => { expect(updateCallbackWasCalled).toBe(false); }); + + it('subscribers do NOT receive add unmapped measurements event', () => { + measurementService.addMapping( + source, + annotationType, + matchingCriteria, + toSourceSchema, + toMeasurementThrowsError + ); + + const { MEASUREMENT_ADDED } = measurementService.EVENTS; + let addCallbackWasCalled = false; + + /* Subscribe to add event */ + measurementService.subscribe( + MEASUREMENT_ADDED, + () => (addCallbackWasCalled = true) + ); + + /* Add new measurement - two calls needed for the start and the other for the completed*/ + // expect exceptions for unmapped measurements + expect(() => { + source.annotationToMeasurement(annotationType, unmappedMeasurement); + }).toThrow(); + + expect(() => { + source.annotationToMeasurement(annotationType, { + unmappedMeasurementUID, + ...unmappedMeasurement, + }); + }).toThrow(); + + expect(addCallbackWasCalled).toBe(false); + }); + + it('subscribers do receive remove unmapped measurements event', () => { + measurementService.addMapping( + source, + annotationType, + matchingCriteria, + toSourceSchema, + toMeasurementThrowsError + ); + + const { MEASUREMENT_REMOVED } = measurementService.EVENTS; + let removeCallbackWasCalled = false; + + /* Subscribe to add event */ + measurementService.subscribe( + MEASUREMENT_REMOVED, + () => (removeCallbackWasCalled = true) + ); + + /* Add new measurement - two calls needed for the start and the other for the completed*/ + // expect exceptions for unmapped measurements + expect(() => { + source.annotationToMeasurement(annotationType, unmappedMeasurement); + }).toThrow(); + + expect(() => { + source.annotationToMeasurement(annotationType, { + unmappedMeasurementUID, + ...unmappedMeasurement, + }); + }).toThrow(); + + measurementService.remove(unmappedMeasurementUID); + + expect(removeCallbackWasCalled).toBe(true); + }); }); }); diff --git a/platform/core/src/services/MeasurementService/MeasurementService.ts b/platform/core/src/services/MeasurementService/MeasurementService.ts index b3c0b3138f3..bd1ac8b8436 100644 --- a/platform/core/src/services/MeasurementService/MeasurementService.ts +++ b/platform/core/src/services/MeasurementService/MeasurementService.ts @@ -112,6 +112,7 @@ class MeasurementService extends PubSubService { public readonly VALUE_TYPES = VALUE_TYPES; private measurements = new Map(); + private unmappedMeasurements = new Set(); constructor() { super(EVENTS); @@ -532,6 +533,8 @@ class MeasurementService extends PubSubService { measurement = toMeasurementSchema(sourceAnnotationDetail); measurement.source = source; } catch (error) { + this.unmappedMeasurements.add(sourceAnnotationDetail.uid); + console.log('Failed to map', error); throw new Error( `Failed to map '${sourceInfo}' measurement for annotationType ${annotationType}: ${error.message}` @@ -594,11 +597,16 @@ class MeasurementService extends PubSubService { * @param {MeasurementSource} source The measurement source instance */ remove(measurementUID, source, eventDetails) { - if (!measurementUID || !this.measurements.has(measurementUID)) { + if ( + !measurementUID || + (!this.measurements.has(measurementUID) && + !this.unmappedMeasurements.has(measurementUID)) + ) { log.warn(`No uid provided, or unable to find measurement by uid.`); return; } + this.unmappedMeasurements.delete(measurementUID); this.measurements.delete(measurementUID); this._broadcastEvent(this.EVENTS.MEASUREMENT_REMOVED, { source, @@ -608,6 +616,8 @@ class MeasurementService extends PubSubService { } clearMeasurements() { + this.unmappedMeasurements.clear(); + // Make a copy of the measurements const measurements = [...this.measurements.values()]; this.measurements.clear();