fix (tools): freehand roi in volume viewport for oblique data#2744
Open
arul-trenser wants to merge 3 commits into
Open
fix (tools): freehand roi in volume viewport for oblique data#2744arul-trenser wants to merge 3 commits into
arul-trenser wants to merge 3 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
Author
|
@wayfarer3130 This PR replaces #2619. While we are able to draw the freehand ROI in the oblique view, the stats calculation for non-acquisition viewports in MPR mode was incorrect. This PR fixes those issues as well. Kindly review the changes. Thanks! |
| [canvasMaxXPadded, cy] as Types.Point2 | ||
| ); | ||
|
|
||
| if (!intersections || intersections.length === 0) { |
Collaborator
There was a problem hiding this comment.
This entire logic seems like it should be repeated for every volume type, just the actual method used to create the AABB intersections would differ depending on the actual object. That would then handle oblique ellipse etc.
Collaborator
There was a problem hiding this comment.
Actually, it would be the overall intersections iterator that would need to be generated. You could then put it onto the stats calculator or the voxel manager as helper methods. That way the storage of thigns like the set of points iterated over could be standardized, and decided whether it is required or not.
wayfarer3130
requested changes
May 26, 2026
wayfarer3130
left a comment
wayfarer3130
left a comment
Collaborator
There was a problem hiding this comment.
Generally really like this change, but would like it to be made a general solution for any area measurement, in terms of the intersectionIterator
arul-trenser
force-pushed
the
fix-freehand-mpr-layout-oblique
branch
from
4ab923e to
8a9563e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds oblique-volume spacing support and changes closed freehand ROI stats sampling to use canvas-space rasterization before projecting samples back to voxel space. ChangesOblique Volume Freehand ROI Stats
Sequence Diagram(s)sequenceDiagram
participant FreehandROITool
participant getIntersectionIterator
participant sampleVoxelsFromCanvas
participant viewport
participant imageData
FreehandROITool->>getIntersectionIterator: build canvas-space ROI samples
FreehandROITool->>sampleVoxelsFromCanvas: sample voxels from iterator output
sampleVoxelsFromCanvas->>viewport: canvasToWorld and worldToIndex
sampleVoxelsFromCanvas->>imageData: bounds-check sampled voxels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/tools/src/tools/annotation/PlanarFreehandROITool.ts (1)
1016-1028: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
samplefor the stats callback.The object passed to
statsCallbackduplicatessamplefield-for-field; passsampledirectly to avoid the redundant allocation and keep the two in sync.♻️ Proposed tweak
pointsInShape.push(sample); - this.configuration.statsCalculator.statsCallback({ - value: value as number, - pointLPS: worldPoint as Types.Point3, - pointIJK: ijkPoint, - }); + this.configuration.statsCalculator.statsCallback(sample);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/annotation/PlanarFreehandROITool.ts` around lines 1016 - 1028, The code in the statsCallback call creates a duplicate object with the exact same structure as the sample variable that was just defined. Replace the object literal passed to this.configuration.statsCalculator.statsCallback() with the sample variable directly to eliminate the redundant allocation and maintain consistency between the two uses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/src/tools/annotation/PlanarFreehandROITool.ts`:
- Around line 974-1028: The loop iterating through canvas pixels from xEnter to
xExit uses a step size of 1, which undersamples voxels when zoomed out since
each canvas pixel can span multiple voxels in world space. Calculate the
voxel-to-canvas spacing by transforming two adjacent canvas points (e.g., cx and
cx+1) to world space and then to index space, then determine the IJK distance
between them. If this distance is greater than 1 voxel, reduce the canvas step
size or add sub-sampling to ensure all voxels within each canvas pixel are
visited. Modify the loop increment in the for statement and/or add logic to
handle fractional canvas-pixel steps so that voxel sampling density remains
independent of zoom level while preserving the oblique-view correctness.
In
`@packages/tools/src/utilities/math/polyline/getSubPixelSpacingAndXYDirections.ts`:
- Around line 99-107: The _computeEffectiveSpacing function requires a
normalized direction vector according to its JSDoc, but viewUp is being passed
raw without normalization when computing ySpacing and yDir. Apply the same
normalization pattern that is already used for viewRight at line 52 to normalize
viewUp before passing it to _computeEffectiveSpacing in the ySpacing computation
block, and use the normalized viewUp vector for the yDir assignment as well to
ensure consistent unit-length behavior in the spacing calculations.
---
Nitpick comments:
In `@packages/tools/src/tools/annotation/PlanarFreehandROITool.ts`:
- Around line 1016-1028: The code in the statsCallback call creates a duplicate
object with the exact same structure as the sample variable that was just
defined. Replace the object literal passed to
this.configuration.statsCalculator.statsCallback() with the sample variable
directly to eliminate the redundant allocation and maintain consistency between
the two uses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 29fbc263-4437-43c7-bee6-e7c1eda0470f
📒 Files selected for processing (2)
packages/tools/src/tools/annotation/PlanarFreehandROITool.tspackages/tools/src/utilities/math/polyline/getSubPixelSpacingAndXYDirections.ts
5 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/src/utilities/math/polyline/getIntersectionIterator.ts`:
- Around line 44-49: The scanline sampling in getIntersectionIterator is
snapping intersection bounds to integer pixels too early, which can skip valid
samples when canvasStep is fractional. Update the loop that computes xEnter and
xExit from intersections so it preserves the raw intersection span and only
aligns iteration to the sampling step, ensuring narrow spans still yield points.
Keep the fix localized to getIntersectionIterator and its intersection-pair
traversal logic.
In `@packages/tools/src/utilities/sampleVoxelsFromCanvas.ts`:
- Line 82: The voxel sampling logic in sampleVoxelsFromCanvas is reading
voxelManager from viewport.getImageData(), which can mismatch the selected
target image. Update the code to use the voxel manager from the passed-in
imageData for the current targetId throughout the sampling flow, and ensure any
IJK-to-value lookups in sampleVoxelsFromCanvas are paired with that target’s
voxel manager rather than the viewport’s current image data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebccfcff-25a2-41a5-8b63-df3f798054cb
📒 Files selected for processing (8)
packages/tools/src/index.tspackages/tools/src/tools/annotation/PlanarFreehandROITool.tspackages/tools/src/utilities/math/polyline/computeEffectiveVoxelSpacing.tspackages/tools/src/utilities/math/polyline/getIntersectionIterator.tspackages/tools/src/utilities/math/polyline/getSubPixelSpacingAndXYDirections.tspackages/tools/src/utilities/math/polyline/index.tspackages/tools/src/utilities/sampleVoxelsFromCanvas.tspackages/tools/test/utilities/getIntersectionIterator.jest.js
✅ Files skipped from review due to trivial changes (3)
- packages/tools/src/utilities/math/polyline/index.ts
- packages/tools/test/utilities/getIntersectionIterator.jest.js
- packages/tools/src/utilities/math/polyline/computeEffectiveVoxelSpacing.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/src/utilities/sampleVoxelsFromCanvas.ts`:
- Around line 71-77: The sampleVoxelsFromCanvas helper now requires voxelManager
but the PlanarFreehandROITool call site is still not passing it, which can leave
getAtIJKPoint unusable. Update the sampleVoxelsFromCanvas signature to type
voxelManager explicitly, then thread the same voxelManager argument through the
PlanarFreehandROITool invocation so the helper always receives a valid manager.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b89359fe-48bd-4244-bcaa-d1427caa6d26
📒 Files selected for processing (3)
packages/tools/src/utilities/math/polyline/getIntersectionIterator.tspackages/tools/src/utilities/sampleVoxelsFromCanvas.tspackages/tools/test/utilities/getIntersectionIterator.jest.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tools/test/utilities/getIntersectionIterator.jest.js
- packages/tools/src/utilities/math/polyline/getIntersectionIterator.ts
arul-trenser
force-pushed
the
fix-freehand-mpr-layout-oblique
branch
from
30dbc71 to
9f34282
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/tools/src/tools/annotation/PlanarFreehandROITool.ts (1)
945-951: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDe-duplicate rounded IJK samples before updating stats.
sampleVoxelsFromCanvasinvokesstatsCallbackfor every canvas sample after rounding to IJK, so sub-pixel sampling or zoomed-in views can count the same voxel multiple times. That makes ROI stats viewport/zoom weighted instead of voxel weighted; filter bypointIJKbefore calling the stats calculator and before storingpointsInShape.🐛 Possible call-site fix
- const pointsInShape = sampleVoxelsFromCanvas({ + const pointsInShape = []; + const visitedIJK = new Set<string>(); + + sampleVoxelsFromCanvas({ iterator: pixelIterator, imageData, viewport, voxelManager, - statsCallback: this.configuration.statsCalculator.statsCallback, + statsCallback: (sample) => { + const key = sample.pointIJK.join(','); + + if (visitedIJK.has(key)) { + return; + } + + visitedIJK.add(key); + pointsInShape.push(sample); + this.configuration.statsCalculator.statsCallback(sample); + }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/annotation/PlanarFreehandROITool.ts` around lines 945 - 951, The ROI sampling in PlanarFreehandROITool is counting duplicate rounded IJK voxels because sampleVoxelsFromCanvas emits stats for every canvas sample. Update the call site around pointsInShape so duplicates are filtered by pointIJK before invoking this.configuration.statsCalculator.statsCallback and before saving pointsInShape, keeping stats voxel-weighted rather than viewport-weighted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/tools/src/tools/annotation/PlanarFreehandROITool.ts`:
- Around line 945-951: The ROI sampling in PlanarFreehandROITool is counting
duplicate rounded IJK voxels because sampleVoxelsFromCanvas emits stats for
every canvas sample. Update the call site around pointsInShape so duplicates are
filtered by pointIJK before invoking
this.configuration.statsCalculator.statsCallback and before saving
pointsInShape, keeping stats voxel-weighted rather than viewport-weighted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef5c4512-87f7-4807-9f96-817fd0269f1a
📒 Files selected for processing (8)
packages/tools/src/index.tspackages/tools/src/tools/annotation/PlanarFreehandROITool.tspackages/tools/src/utilities/math/polyline/computeEffectiveVoxelSpacing.tspackages/tools/src/utilities/math/polyline/getIntersectionIterator.tspackages/tools/src/utilities/math/polyline/getSubPixelSpacingAndXYDirections.tspackages/tools/src/utilities/math/polyline/index.tspackages/tools/src/utilities/sampleVoxelsFromCanvas.tspackages/tools/test/utilities/getIntersectionIterator.jest.js
✅ Files skipped from review due to trivial changes (1)
- packages/tools/src/utilities/math/polyline/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/tools/src/utilities/math/polyline/computeEffectiveVoxelSpacing.ts
- packages/tools/src/index.ts
- packages/tools/src/utilities/sampleVoxelsFromCanvas.ts
- packages/tools/test/utilities/getIntersectionIterator.jest.js
- packages/tools/src/utilities/math/polyline/getIntersectionIterator.ts
- packages/tools/src/utilities/math/polyline/getSubPixelSpacingAndXYDirections.ts
Contributor
Author
|
@wayfarer3130 I've updated the code based on your feedback. Could you please take another look when you have a moment? Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
This PR resolves an issue where the freehand ROI tool was not functioning correctly within volume viewports displaying oblique data. The PR is incorporated by FlyWheel.io.
Changes & Results
Before:
PR-before.mp4
After:
PR-after.mp4
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
New Features
Tests