fix CircleROI tool for wsi #1940

mbellehumeur · 2025-03-26T17:02:27Z

Context

CircleROI and other ROI tools were not generating stats for wsi viewport.

Changes & Results

This PR fixes the CircleROI, RectangleROI, EllipseROI and FreehandROI tool for whole slide (wsi) viewports by skipping statistics calculation that require the voxel manager.
Key changes include:

Changing wsiViewport so that getScalarData returns an empty CanvasScalarData object instead of null.

Wrapping the voxel manager-based points collection of each tool in a condition that bypasses the process when the scalarData is empty.

netlify · 2025-03-26T17:02:45Z

❌ Deploy Preview for cornerstone-3d-docs failed. Why did it fail? →

Name	Link
🔨 Latest commit	`c6db595`
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/67e56a3b2aff190007682d7a

wayfarer3130 · 2025-03-26T17:10:00Z

packages/tools/src/tools/annotation/CircleROITool.ts

@@ -919,7 +919,10 @@ class CircleROITool extends AnnotationTool {
      // Check if one of the indexes are inside the volume, this then gives us
      // Some area to do stats over.

-      if (this._isInsideVolume(pos1Index, pos2Index, dimensions)) {
+      if (
+        this._isInsideVolume(pos1Index, pos2Index, dimensions) ||


Can you make the isInsideVolume work for WSI as well? I'd rather not have a viewport.type==='whileSlide' anywhere inside the tools.

It might be necessary to improve the WSIViewport with better mapping functions, or perhaps the transformWorldToIndex mapping functions.

yes, wsiviewport worldToCanvas returns negative values.

I can make it work by changing wsiviewport 'getTransform' but I dont know what else it impacts.

That impacts too much right now - @pedrokohler is working on a fix for that separately, so lets leave this as is for now.

packages/tools/src/tools/annotation/CircleROITool.ts

Copilot

Pull Request Overview

This PR fixes the CircleROI tool so that it properly handles whole slide (wsi) viewports by skipping stats that require the voxel manager. Key changes include:

Extending the conditional check to include viewport type "wholeSlide" for stats calculation.
Wrapping the voxel manager-based points collection in a condition that bypasses the process for whole slide viewports.

Comments suppressed due to low confidence (1)

packages/tools/src/tools/annotation/CircleROITool.ts:924

[nitpick] Consider replacing the magic string 'wholeSlide' with a named constant to improve maintainability.

viewport.type === 'wholeSlide'

…and improve handling of finite values in statistics

…pe and improve null checks

…lar data access

…mations

wayfarer3130 · 2025-04-17T17:51:24Z

Can you ensure the tests pas[s - I think those are real issues

…arData length for pointsInShape calculation

…sInShape calculation

…nd Elliptical ROI tools in the tools list

…ornerstone3D into fix-circleroi-wsi

wayfarer3130 · 2025-04-22T11:46:25Z

packages/core/src/RenderingEngine/WSIViewport.ts

@@ -244,7 +254,7 @@ class WSIViewport extends Viewport {
      getDirection: () => metadata.direction,
      getDimensions: () => metadata.dimensions,
      getRange: () => [0, 255],
-      getScalarData: () => this.getScalarData(),


Can you add a viewReference to the getter as an optional? I know we don't pass it in yet, but we need to start thinking about it as the getScalarData for whole slide imaging won't be able to return the entire scalar data.

wayfarer3130 · 2025-04-22T11:47:59Z

packages/core/src/RenderingEngine/WSIViewport.ts

+    // Return an empty CanvasScalarData object
+    const emptyData = new Uint8ClampedArray() as CanvasScalarData;
+    emptyData.getRange = () => [0, 255];
+    emptyData.frameNumber = -1;


Can you add an emptyData.region that is a rectangle of the enclosing region, and a scale parameter? The region should be zero sized for now, and the scale can be 1, but again I'd like to start thinking about the future.

wayfarer3130 · 2025-04-22T11:48:58Z

packages/tools/src/tools/annotation/CircleROITool.ts

@@ -1028,6 +1030,14 @@ class CircleROITool extends AnnotationTool {
  };

  _isInsideVolume = (index1, index2, dimensions) => {
+    // for wsiViewport
+    if (index1[1] < 0) {


Can you put this fix into the WSIViewport itself?

wayfarer3130 · 2025-04-22T11:50:24Z

packages/core/src/RenderingEngine/WSIViewport.ts

@@ -615,12 +627,16 @@ class WSIViewport extends Viewport {
  protected canvasToIndex = (canvasPos: Point2): Point2 => {
    const transform = this.getTransform();
    transform.invert();
-    return transform.transformPoint(canvasPos);
+    return transform.transformPoint(


Can you use Math.abs on the result here - I think that will get us close to the right value, or on initial setup, figure out which attribute is negative and store a negative multiplier for it.

wayfarer3130 · 2025-04-22T11:51:21Z

packages/core/src/RenderingEngine/WSIViewport.ts

  };

  protected indexToCanvas = (indexPos: Point2): Point2 => {
    const transform = this.getTransform();
-    return transform.transformPoint(indexPos);
+    return transform


Will need to convert back to negative values, so if you on initial setup store an index of which values are negative and multiple by that vector you should get the right sign.

packages/tools/src/tools/annotation/EllipticalROITool.ts

packages/tools/src/tools/annotation/PlanarFreehandROITool.ts

wayfarer3130

Couple of small remaining changes, hopefully merge this today.

…mber

…tion

…in _getScalarData method

sedghi · 2025-05-28T16:10:18Z

packages/tools/src/tools/base/AnnotationTool.ts

+   * @param n - array of numbers or a simple number
+   * @returns True if n or the first element of n is finite and not NaN
+   */
+  public static isNumber(n: number[] | number): boolean {


there is already a isNumber in packages/core/src/utilities/isEqual.ts , adapt that for your use

sedghi · 2025-05-28T16:12:35Z

packages/tools/src/tools/annotation/RectangleROITool.ts

@@ -968,6 +968,14 @@ class RectangleROITool extends AnnotationTool {
  };

  _isInsideVolume = (index1, index2, dimensions) => {
+    // for wsiViewport
+    if (index1[1] < 0) {


This doesn't look good. Could you please check if the viewport type is WSI before proceeding? I imagine this might disrupt other types of viewports.

sedghi · 2025-05-28T16:12:42Z

packages/tools/src/tools/annotation/CircleROITool.ts

+    // for wsiViewport
+    if (index1[1] < 0) {
+      index1[1] = -index1[1];
+    }
+    if (index2[1] < 0) {
+      index2[1] = -index2[1];


This doesn't look good. Could you please check if the viewport type is WSI before proceeding? I imagine this might disrupt other types of viewports.

sedghi

please see my comments

check viewport type for voxel stats

b9c9f50

wayfarer3130 reviewed Mar 26, 2025

View reviewed changes

packages/tools/src/tools/annotation/CircleROITool.ts Outdated Show resolved Hide resolved

sedghi requested a review from Copilot March 26, 2025 17:48

Copilot AI reviewed Mar 26, 2025

View reviewed changes

mbellehumeur added 8 commits March 27, 2025 13:52

check imageData.getScalar() instead of viewport type

ca3fdb8

This will be fixed in wsiviewport worldToCanvas func

3c6e74a

Fix for isInsideModule?

0356835

revert

c6db595

Fix wsiviewport deviceScaling

f8b94a0

Refactor ROI tools to check for scalar data before processing points …

64a7b95

…and improve handling of finite values in statistics

Refactor WSIViewport to return typed scalar data

7f07ac4

Remove unnecessary initialization of scalarData in getScalarData method

bebdbd6

wayfarer3130 approved these changes Apr 16, 2025

View reviewed changes

mbellehumeur added 3 commits April 17, 2025 12:40

Refactor scalar data handling in ROI tools to use CanvasScalarData ty…

9f286a5

…pe and improve null checks

Refactor PlanarFreehandROITool to use getScalarValueFromWorld for sca…

57c88c5

…lar data access

Remove commented-out code in WSIViewport for canvas position transfor…

63336f6

…mations

wayfarer3130 approved these changes Apr 17, 2025

View reviewed changes

Fixed closing bracket not to exclude stats calculation

5ded038

mbellehumeur and others added 8 commits April 20, 2025 18:55

Refactor WSIViewport to replace getScalarData with _getScalarData

99f2d2b

Refactor ROI tools to check voxelManager length instead of image.scal…

048f9dd

…arData length for pointsInShape calculation

Refactor ROI tools to use imageData.getScalarValueFromWorld for point…

00bf564

…sInShape calculation

Refactor ROI tools to use voxelManager for pointsInShape calculation

65d8149

Refactor WSIViewport to replace getScalarData with _getScalarData method

b8fb6b2

Refactor WSI Annotation Tools example to include Circle, Rectangle, a…

73a6d4b

…nd Elliptical ROI tools in the tools list

fix: Display/hide area for color images correctly

6d21c80

Merge branch 'fix-circleroi-wsi' of https://github.com/mbellehumeur/c…

88b5bfd

…ornerstone3D into fix-circleroi-wsi

wayfarer3130 reviewed Apr 22, 2025

View reviewed changes

packages/tools/src/tools/annotation/EllipticalROITool.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Apr 22, 2025

View reviewed changes

packages/tools/src/tools/annotation/PlanarFreehandROITool.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Apr 22, 2025

View reviewed changes

mbellehumeur added 5 commits May 28, 2025 15:59

fix: Update number validation in ROI tools to use AnnotationTool.isNu…

613279f

…mber

fix: Update canvasToIndex method to return Point3 instead of Point2

5fb2331

fix: Initialize WSIUtilFunctions and update indexToCanvas transformation

1fb803a

fix: Remove unused CanvasScalarData type and WSIUtilFunctions declara…

474fb89

…tion

fix: Define CanvasScalarData type and initialize with default values …

cd18632

…in _getScalarData method

sedghi reviewed May 28, 2025

View reviewed changes

fix CircleROI tool for wsi #1940

Are you sure you want to change the base?

fix CircleROI tool for wsi #1940

Conversation

mbellehumeur commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes & Results

Uh oh!

netlify bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Deploy Preview for cornerstone-3d-docs failed. Why did it fail? →

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

wayfarer3130 commented Apr 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wayfarer3130 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedghi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mbellehumeur commented Mar 26, 2025 •

edited

Loading

netlify bot commented Mar 26, 2025 •

edited

Loading