fix: annotation rendering engine on viewport removal (#303)

* feat: add hasBeenDestroyed to viewport and fix bugs

* fix: reset annotation render on remove element

* new api

* apply review comments

common/reviews/api/core.api.md

@@ -1279,6 +1279,8 @@ interface IViewport {
     // (undocumented)
     id: string;
     // (undocumented)
+    isDisabled: boolean;
+    // (undocumented)
     options: ViewportInputOptions;
     // (undocumented)
     removeAllActors(): void;
@@ -2042,6 +2044,8 @@ export class Viewport implements IViewport {
     // (undocumented)
     protected initialCamera: ICamera;
     // (undocumented)
+    isDisabled: boolean;
+    // (undocumented)
     _isInBounds(point: Point3, bounds: number[]): boolean;
     // (undocumented)
     options: ViewportInputOptions;

common/reviews/api/streaming-image-volume-loader.api.md

@@ -905,6 +905,7 @@ interface IViewport {
     getRenderingEngine(): any;
     getZoom(): number;
     id: string;
+    isDisabled: boolean;
     options: ViewportInputOptions;
     removeAllActors(): void;
     render(): void;

common/reviews/api/tools.api.md

@@ -2456,6 +2456,7 @@ interface IViewport {
     getRenderingEngine(): any;
     getZoom(): number;
     id: string;
+    isDisabled: boolean;
     options: ViewportInputOptions;
     removeAllActors(): void;
     render(): void;

packages/core/src/RenderingEngine/RenderingEngine.ts

@@ -210,6 +210,7 @@ class RenderingEngine implements IRenderingEngine {
 
     // 5. Remove the requested viewport from the rendering engine
     this._removeViewport(viewportId);
+    viewport.isDisabled = true;
 
     // 6. Avoid rendering for the disabled viewport
     this._needsRender.delete(viewportId);

packages/core/src/RenderingEngine/Viewport.ts

@@ -48,6 +48,7 @@ class Viewport implements IViewport {
   protected flipHorizontal = false;
   protected flipVertical = false;
   protected rotation = 0;
+  public isDisabled: boolean;
 
   /** sx of viewport on the offscreen canvas */
   sx: number;
@@ -95,6 +96,7 @@ class Viewport implements IViewport {
       ? props.defaultOptions.suppressEvents
       : false;
     this.options = _cloneDeep(props.defaultOptions);
+    this.isDisabled = false;
   }
 
   getFrameOfReferenceUID: () => string;

packages/core/src/RenderingEngine/VolumeViewport.ts

@@ -107,6 +107,12 @@ class VolumeViewport extends Viewport implements IVolumeViewport {
     const volumeNewImageCleanUpBound = volumeNewImageCleanUp.bind(this);
 
     function volumeNewImageHandler(cameraEvent) {
+      const { viewportId } = cameraEvent.detail;
+
+      if (viewportId !== this.id || this.isDisabled) {
+        return;
+      }
+
       const viewportImageData = this.getImageData();
 
       if (!viewportImageData) {

packages/core/src/types/IViewport.ts

@@ -35,6 +35,8 @@ interface IViewport {
   options: ViewportInputOptions;
   /** Suppress events */
   suppressEvents: boolean;
+  /** if the viewport has been disabled */
+  isDisabled: boolean;
   /** frameOfReferenceUID the viewport's default actor is rendering */
   getFrameOfReferenceUID: () => string;
   /** method to convert canvas to world coordinates */

packages/tools/src/tools/CrosshairsTool.ts

@@ -265,6 +265,33 @@ class CrosshairsTool extends AnnotationTool {
     const viewportsInfo = this._getViewportsInfo();
 
     this._unsubscribeToViewportNewVolumeSet(viewportsInfo);
+
+    // Crosshairs annotations in the state
+    // has no value when the tool is disabled
+    // since viewports can change (zoom, pan, scroll)
+    // between disabled and enabled/active states.
+    // so we just remove the annotations from the state
+    viewportsInfo.forEach(({ renderingEngineId, viewportId }) => {
+      const enabledElement = getEnabledElementByIds(
+        viewportId,
+        renderingEngineId
+      );
+
+      if (!enabledElement) {
+        return;
+      }
+
+      const { viewport } = enabledElement;
+      const { element } = viewport;
+
+      const annotations = getAnnotations(element, this.getToolName());
+
+      if (annotations?.length) {
+        annotations.forEach((annotation) => {
+          removeAnnotation(annotation.annotationUID, element);
+        });
+      }
+    });
   }
 
   /**

packages/tools/src/utilities/getAnnotationNearPoint.ts

@@ -107,7 +107,10 @@ function findAnnotationNearPointByTool(
     const { element } = enabledElement.viewport;
     for (const annotation of annotations) {
       const referencedImageId = annotation.metadata?.referencedImageId;
-      if (currentId && referencedImageId && currentId !== referencedImageId) {
+      if (
+        (currentId && referencedImageId && currentId !== referencedImageId) ||
+        !tool.isPointNearTool
+      ) {
         continue;
       }
 

packages/tools/src/utilities/segmentation/triggerSegmentationRender.ts

@@ -140,6 +140,11 @@ class SegmentationRenderingEngine {
 
       const toolGroup = getToolGroupForViewport(viewportId, renderingEngineId);
 
+      if (!toolGroup) {
+        console.warn('toolGroup has been destroyed');
+        return;
+      }
+
       const eventDetail: SegmentationRenderedEventDetail = {
         toolGroupId: toolGroup.id,
         viewportId,

packages/tools/src/utilities/triggerAnnotationRender.ts

@@ -53,10 +53,11 @@ class AnnotationRenderingEngine {
     // delete element from needsRender if element exist
     this._needsRender.delete(element);
 
-    // Reset the request animation frame if no enabled elements
-    if (this._viewportElements.size === 0) {
-      this._reset();
-    }
+    // I don' think there is any disadvantage to canceling the animation frame
+    // and resetting the flags on viewport's element removal, since the removeVIewportElement
+    // might be as a result of reEnabling the element (in re-enable we disable first), hence the need to render the
+    // new one while removing the old one
+    this._reset();
   }
 
   /**
@@ -105,6 +106,16 @@ class AnnotationRenderingEngine {
     }
   };
 
+  private _setAllViewportsToBeRenderedNextFrame() {
+    const elements = [...this._viewportElements.values()];
+
+    elements.forEach((element) => {
+      this._needsRender.add(element);
+    });
+
+    this._renderFlaggedViewports();
+  }
+
   private _setViewportsToBeRenderedNextFrame(elements: HTMLDivElement[]) {
     const elementsEnabled = [...this._viewportElements.values()];
 
@@ -205,6 +216,8 @@ class AnnotationRenderingEngine {
     this._needsRender.clear();
     this._animationFrameSet = false;
     this._animationFrameHandle = null;
+
+    this._setAllViewportsToBeRenderedNextFrame();
   }
 }