fix(viewports): The display of linked viewports during drag and drop has a race

extensions/cornerstone-dicom-seg/src/utils/_hydrateSEG.ts

@@ -31,8 +31,6 @@ async function _hydrateSEGDisplaySet({
     displaySetInstanceUID
   );
 
-  viewportGridService.setDisplaySetsForViewports(updatedViewports);
-
   // Todo: fix this after we have a better way for stack viewport segmentations
 
   // check every viewport in the viewports to see if the displaySetInstanceUID
@@ -50,7 +48,7 @@ async function _hydrateSEGDisplaySet({
     );
 
     if (shouldDisplaySeg) {
-      viewportGridService.setDisplaySetsForViewport({
+      updatedViewports.push({
         viewportIndex: index,
         displaySetInstanceUIDs: viewport.displaySetInstanceUIDs,
         viewportOptions: {
@@ -62,6 +60,9 @@ async function _hydrateSEGDisplaySet({
     }
   });
 
+  // Do the entire update at once
+  viewportGridService.setDisplaySetsForViewports(updatedViewports);
+
   return true;
 }
 

extensions/cornerstone/src/Viewport/OHIFCornerstoneViewport.tsx

@@ -10,7 +10,6 @@ import {
   utilities as csUtils,
   CONSTANTS,
 } from '@cornerstonejs/core';
-import { Services } from '@ohif/core';
 
 import { setEnabledElement } from '../state';
 
@@ -23,9 +22,6 @@ import {
 import getSOPInstanceAttributes from '../utils/measurementServiceMappings/utils/getSOPInstanceAttributes';
 import { CinePlayer, useCine, useViewportGrid } from '@ohif/ui';
 
-import { CornerstoneViewportService } from '../services/ViewportService/CornerstoneViewportService';
-import Presentation from '../types/Presentation';
-
 const STACK = 'stack';
 
 function areEqual(prevProps, nextProps) {
@@ -407,7 +403,6 @@ const OHIFCornerstoneViewport = React.memo(props => {
         lutPresentation:
           lutPresentationStore[presentationIds?.lutPresentationId],
       };
-      console.log('Using presentations', presentations);
 
       cornerstoneViewportService.setViewportData(
         viewportIndex,

extensions/cornerstone/src/getHangingProtocolModule.ts

@@ -11,7 +11,7 @@ const mpr: Types.HangingProtocol.Protocol = {
   // Unknown number of priors referenced - so just match any study
   numberOfPriorsReferenced: 0,
   protocolMatchingRules: [],
-  imageLoadStrategy: 'interleaveTopToBottom',
+  imageLoadStrategy: 'nth',
   callbacks: {
     // Switches out of MPR mode when the layout change button is used
     onLayoutChange: [
@@ -303,11 +303,11 @@ const mprAnd3DVolumeViewport = {
 function getHangingProtocolModule() {
   return [
     {
-      id: 'mpr',
+      name: 'mpr',
       protocol: mpr,
     },
     {
-      id: mprAnd3DVolumeViewport.id,
+      name: mprAnd3DVolumeViewport.id,
       protocol: mprAnd3DVolumeViewport,
     },
   ];

extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts

@@ -155,9 +155,14 @@ class CornerstoneViewportService extends PubSubService
   /**
    * Disables the viewport inside the renderingEngine, if no viewport is left
    * it destroys the renderingEngine.
+   *
+   * This is called when the element goes away entirely - with new viewportId's
+   * created for every new viewport, this will be called whenever the set of
+   * viewports is changed, but NOT when the viewport position changes only.
+   *
    * @param viewportIndex
    */
-  public disableElement(viewportIndex: number) {
+  public disableElement(viewportIndex: number): void {
     const viewportInfo = this.viewportsInfo.get(viewportIndex);
     if (!viewportInfo) {
       return;
@@ -254,12 +259,6 @@ class CornerstoneViewportService extends PubSubService
     viewportInfo.setDisplaySetOptions(displaySetOptions);
     viewportInfo.setViewportData(viewportData);
 
-    this._broadcastEvent(this.EVENTS.VIEWPORT_DATA_CHANGED, {
-      viewportData,
-      viewportIndex,
-      viewportId,
-    });
-
     const element = viewportInfo.getElement();
     const type = viewportInfo.getViewportType();
     const background = viewportInfo.getBackground();
@@ -283,6 +282,15 @@ class CornerstoneViewportService extends PubSubService
 
     const viewport = renderingEngine.getViewport(viewportId);
     this._setDisplaySets(viewport, viewportData, viewportInfo, presentations);
+
+    // The broadcast event here ensures that listeners have a valid, up to date
+    // viewport to access.  Doing it too early can result in exceptions or
+    // invalid data.
+    this._broadcastEvent(this.EVENTS.VIEWPORT_DATA_CHANGED, {
+      viewportData,
+      viewportIndex,
+      viewportId,
+    });
   }
 
   public getCornerstoneViewport(
@@ -386,11 +394,7 @@ class CornerstoneViewportService extends PubSubService
       }
     }
 
-    // There is a bug in CS3D that the setStack does not
-    // navigate to the desired image.
-    viewport.setStack(imageIds, 0).then(() => {
-      // The scroll, however, works fine in CS3D
-      viewport.scroll(initialImageIndexToUse);
+    viewport.setStack(imageIds, initialImageIndexToUse).then(() => {
       viewport.setProperties(properties);
       const camera = presentations.positionPresentation?.camera;
       if (camera) viewport.setCamera(camera);
@@ -621,6 +625,10 @@ class CornerstoneViewportService extends PubSubService
 
     const viewportInfo = this.getViewportInfo(viewport.id);
 
+    if (!viewportInfo) {
+      console.warn('Viewport info not defined for', viewport.id);
+    }
+
     const toolGroup = toolGroupService.getToolGroupForViewport(viewport.id);
     csToolsUtils.segmentation.triggerSegmentationRender(toolGroup.id);
 

extensions/cornerstone/src/utils/nthLoader.ts

@@ -18,7 +18,6 @@ const viewportIdVolumeInputArrayMap = new Map<string, unknown[]>();
 export default function interleaveNthLoader({
   data: { viewportId, volumeInputArray },
   displaySetsMatchDetails,
-  viewportMatchDetails: matchDetails,
 }) {
   viewportIdVolumeInputArrayMap.set(viewportId, volumeInputArray);
 
@@ -29,6 +28,7 @@ export default function interleaveNthLoader({
     const volume = cache.getVolume(volumeId);
 
     if (!volume) {
+      console.log("interleaveNthLoader::No volume, can't load it");
       return;
     }
 
@@ -39,33 +39,6 @@ export default function interleaveNthLoader({
     }
   }
 
-  /**
-   * The following is checking if all the viewports that were matched in the HP has been
-   * successfully created their cornerstone viewport or not. Todo: This can be
-   * improved by not checking it, and as soon as the matched DisplaySets have their
-   * volume loaded, we start the loading, but that comes at the cost of viewports
-   * not being created yet (e.g., in a 10 viewport ptCT fusion, when one ct viewport and one
-   * pt viewport are created we have a guarantee that the volumes are created in the cache
-   * but the rest of the viewports (fusion, mip etc.) are not created yet. So
-   * we can't initiate setting the volumes for those viewports. One solution can be
-   * to add an event when a viewport is created (not enabled element event) and then
-   * listen to it and as the other viewports are created we can set the volumes for them
-   * since volumes are already started loading.
-   */
-  if (matchDetails.size !== viewportIdVolumeInputArrayMap.size) {
-    return;
-  }
-
-  // Check if all the matched volumes are loaded
-  for (const [_, details] of displaySetsMatchDetails.entries()) {
-    const { SeriesInstanceUID } = details;
-
-    // HangingProtocol has matched, but don't have all the volumes created yet, so return
-    if (!Array.from(volumeIdMapsToLoad.values()).includes(SeriesInstanceUID)) {
-      return;
-    }
-  }
-
   const volumeIds = Array.from(volumeIdMapsToLoad.keys()).slice();
   // get volumes from cache
   const volumes = volumeIds.map(volumeId => {

extensions/default/src/getHangingProtocolModule.js

@@ -290,7 +290,7 @@ const defaultProtocol = {
 function getHangingProtocolModule() {
   return [
     {
-      id: defaultProtocol.id,
+      name: defaultProtocol.id,
       protocol: defaultProtocol,
     },
   ];

extensions/test-extension/src/hp/index.ts

@@ -2,7 +2,7 @@ import hpMN from './hpMN';
 
 const hangingProtocols = [
   {
-    id: '@ohif/hp-extension.mn',
+    name: '@ohif/hp-extension.mn',
     protocol: hpMN,
   },
 ];

extensions/tmtv/src/getHangingProtocolModule.js

@@ -323,7 +323,7 @@ const ptCT = {
 function getHangingProtocolModule() {
   return [
     {
-      id: ptCT.id,
+      name: ptCT.id,
       protocol: ptCT,
     },
   ];

platform/core/src/extensions/ExtensionManager.test.js

@@ -206,40 +206,40 @@ describe('ExtensionManager.ts', () => {
       const extension = {
         id: 'hello-world',
         getViewportModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getSopClassHandlerModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getPanelModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getToolbarModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getCommandsModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getLayoutTemplateModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getDataSourcesModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getHangingProtocolModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getContextModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getUtilityModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getCustomizationModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getStateSyncModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
       };
 

platform/core/src/extensions/ExtensionManager.ts

@@ -283,6 +283,11 @@ export default class ExtensionManager {
             // Default for most extension points,
             // Just adds each entry ready for consumption by mode.
             extensionModule.forEach(element => {
+              if (!element.name) {
+                throw new Error(
+                  `Extension ID ${extensionId} module ${moduleType} element has no name`
+                );
+              }
               const id = `${extensionId}.${moduleType}.${element.name}`;
               element.id = id;
               this.modulesMap[id] = element;
@@ -366,10 +371,10 @@ export default class ExtensionManager {
 
   _initHangingProtocolsModule = (extensionModule, extensionId) => {
     const { hangingProtocolService } = this._servicesManager.services;
-    extensionModule.forEach(({ id, protocol }) => {
+    extensionModule.forEach(({ name, protocol }) => {
       if (protocol) {
         // Only auto-register if protocol specified, otherwise let mode register
-        hangingProtocolService.addProtocol(id, protocol);
+        hangingProtocolService.addProtocol(name, protocol);
       }
     });
   };

platform/core/src/services/ViewportGridService/ViewportGridService.ts

@@ -26,7 +26,6 @@ class ViewportGridService extends PubSubService {
   public setServiceImplementation({
     getState: getStateImplementation,
     setActiveViewportIndex: setActiveViewportIndexImplementation,
-    setDisplaySetsForViewport: setDisplaySetsForViewportImplementation,
     setDisplaySetsForViewports: setDisplaySetsForViewportsImplementation,
     setLayout: setLayoutImplementation,
     reset: resetImplementation,
@@ -40,9 +39,6 @@ class ViewportGridService extends PubSubService {
     if (setActiveViewportIndexImplementation) {
       this.serviceImplementation._setActiveViewportIndex = setActiveViewportIndexImplementation;
     }
-    if (setDisplaySetsForViewportImplementation) {
-      this.serviceImplementation._setDisplaySetsForViewport = setDisplaySetsForViewportImplementation;
-    }
     if (setDisplaySetsForViewportsImplementation) {
       this.serviceImplementation._setDisplaySetsForViewports = setDisplaySetsForViewportsImplementation;
     }
@@ -77,22 +73,13 @@ class ViewportGridService extends PubSubService {
     return this.serviceImplementation._getState();
   }
 
-  public setDisplaySetsForViewport({
-    viewportIndex,
-    displaySetInstanceUIDs,
-    viewportOptions,
-    displaySetOptions,
-  }) {
-    this.serviceImplementation._setDisplaySetsForViewport({
-      viewportIndex,
-      displaySetInstanceUIDs,
-      viewportOptions,
-      displaySetOptions,
-    });
+  public setDisplaySetsForViewport(props) {
+    // Just update a single viewport, but use the multi-viewport update for it.
+    this.serviceImplementation._setDisplaySetsForViewports([props]);
   }
 
-  public setDisplaySetsForViewports(viewports) {
-    this.serviceImplementation._setDisplaySetsForViewports(viewports);
+  public setDisplaySetsForViewports(props) {
+    this.serviceImplementation._setDisplaySetsForViewports(props);
   }
 
   /**