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.

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
+ * If the imageId is not provided then undefined is returned.
- * 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.
  * @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(
+export default function getSOPInstanceAttributes(imageId) {
-  imageId,
-  cornerstoneViewportService = undefined,
-  viewportId = undefined
-) {
   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);
-//     }
-//   }
-// }

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);
+    });
   });
 });

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();