fix(segmentation): scope SegmentLabelTool hover to the current slice for contours - #2770
Conversation
|
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 with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR tightens segment-label hover behavior to the displayed slice, updates the example seeding flow, adds regression coverage, and makes two small viewport-related code changes in type formatting and VOI selection. ChangesSegment label hover scope
Viewport cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (1)
utils/ExampleRunner/example-runner-cli.js (1)
296-309: 🩺 Stability & Availability | 🔵 TrivialQuote the
webpackConfigPathargument to prevent shell splitting on spaces.When
shell: trueis enabled, Node concatenates the command and arguments into a single string and executes it via the system shell. TherspackBinis correctly quoted to handle spaces, butwebpackConfigPath(derived frompath.join(__dirname, ...)) is passed unquoted. If the installation directory contains spaces (e.g.,Program Files), the shell will split the argument at the space, causing rspack to fail to locate the configuration file.🔧 Quote the config path argument
const rspackArgs = [ 'serve', '--host', '0.0.0.0', ...(options.https ? ['--https'] : []), '--config', - webpackConfigPath, + `"${webpackConfigPath}"`, ];🤖 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 `@utils/ExampleRunner/example-runner-cli.js` around lines 296 - 309, The rspack launch in example-runner-cli.js is not quoting the webpackConfigPath argument while shell: true is used, so paths with spaces can be split by the shell. Update the spawnSync call near the rspackArgs construction so the config path is passed in a shell-safe quoted form, matching the existing handling of rspackBin. Keep the fix localized to the rspack serve invocation and ensure webpackConfigPath is treated as a single argument even when the install path contains spaces.
🤖 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 `@tests/segmentLabelHover.spec.ts`:
- Around line 40-52: In getHoverPositions, add a null guard for
viewport.boundingBox() before reading width/height so hidden elements don’t
cause a dereference failure, and then validate the computed segment1 and
segment2Projection coordinates against the returned bounding box dimensions
before returning them. Update the logic in getHoverPositions to either clamp or
reject out-of-bounds hover points so the hover test only uses coordinates that
are inside the viewport.
---
Nitpick comments:
In `@utils/ExampleRunner/example-runner-cli.js`:
- Around line 296-309: The rspack launch in example-runner-cli.js is not quoting
the webpackConfigPath argument while shell: true is used, so paths with spaces
can be split by the shell. Update the spawnSync call near the rspackArgs
construction so the config path is passed in a shell-safe quoted form, matching
the existing handling of rspackBin. Keep the fix localized to the rspack serve
invocation and ensure webpackConfigPath is treated as a single argument even
when the install path contains spaces.
🪄 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: 5ee1bcce-b425-4bb3-933b-715928a13a70
📒 Files selected for processing (5)
packages/tools/examples/segmentLabel/index.tspackages/tools/src/utilities/segmentation/getSegmentIndexAtWorldPoint.tsplaywright.config.tstests/segmentLabelHover.spec.tsutils/ExampleRunner/example-runner-cli.js
|
The technically correct version of this is to show an annotation if the source annotation slice thickness as originally created is within the destination slice range. That is, if the annotation was created on slice S of thickness S_T and normal S_N with a point on S_P, then it should be displayed on destination plane D with the same sub-variables if: |
|
Can't you use the isPlaneViewable or isReferenceViewable instead? this is the same logic we use for rendering, i'm not sure about this custom logic |
Yes I can. Thanks so much. |
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/tools/annotation/LivewireContourTool.ts`:
- Around line 235-243: The `voiRange` fallback in `LivewireContourTool` is still
being forced to `Types.VOIRange` even when both
`getDisplaySetPresentation(dataId)?.voiRange` and `getDefaultVOIRange(dataId)`
return `undefined`. Remove the trailing no-op `?? undefined`/unsafe cast, keep
`voiRange` as optional, and add a guard before calling
`LivewireScissors.createInstanceFromRawPixelData` so the scissors instance is
only created when a real VOI range is available.
- Around line 233-243: The VOI lookup in LivewireContourTool’s source-viewport
branch can still resolve to undefined, but
LivewireScissors.createInstanceFromRawPixelData expects voiRange to be present.
Update the logic around csUtils.viewportSupportsDisplaySetPresentation,
getDisplaySetPresentation, and getDefaultVOIRange to either guard against a
missing VOI range before creating the scissors or provide an explicit safe
default, and remove the final cast that masks the undefined case.
🪄 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: d1b2fe92-aa7e-4a7a-a877-202c2dcca37c
📒 Files selected for processing (2)
packages/core/src/types/IGenericViewport.tspackages/tools/src/tools/annotation/LivewireContourTool.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/types/IGenericViewport.ts
| voiRange = ((dataId | ||
| ? ( | ||
| sourceViewport.getDisplaySetPresentation(dataId) as | ||
| | { voiRange?: Types.VOIRange } | ||
| | undefined | ||
| )?.voiRange | ||
| : undefined) ?? sourceViewport.getDefaultVOIRange(dataId)) ?? | ||
| : undefined) ?? | ||
| sourceViewport.getDefaultVOIRange(dataId) ?? | ||
| undefined) as Types.VOIRange; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Final as Types.VOIRange cast can mask an actually-undefined value.
Both getDisplaySetPresentation(dataId)?.voiRange and getDefaultVOIRange(dataId) can legitimately resolve to undefined (per the PlanarDataPresentation/getDefaultVOIRange signatures). The trailing ?? undefined is a no-op, and the outer as Types.VOIRange cast then forces the type to non-optional even when the value is genuinely undefined. This voiRange is passed straight into LivewireScissors.createInstanceFromRawPixelData(scalarData, width, height, voiRange) without a guard, so if both sources are absent, downstream code silently receives undefined typed as VOIRange.
Suggested simplification
- voiRange = ((dataId
- ? (
- sourceViewport.getDisplaySetPresentation(dataId) as
- | { voiRange?: Types.VOIRange }
- | undefined
- )?.voiRange
- : undefined) ??
- sourceViewport.getDefaultVOIRange(dataId) ??
- undefined) as Types.VOIRange;
+ voiRange =
+ (dataId
+ ? sourceViewport.getDisplaySetPresentation(dataId)?.voiRange
+ : undefined) ?? sourceViewport.getDefaultVOIRange(dataId);Consider also handling the case where voiRange remains undefined before constructing the scissors instance.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| voiRange = ((dataId | |
| ? ( | |
| sourceViewport.getDisplaySetPresentation(dataId) as | |
| | { voiRange?: Types.VOIRange } | |
| | undefined | |
| )?.voiRange | |
| : undefined) ?? sourceViewport.getDefaultVOIRange(dataId)) ?? | |
| : undefined) ?? | |
| sourceViewport.getDefaultVOIRange(dataId) ?? | |
| undefined) as Types.VOIRange; | |
| voiRange = | |
| (dataId | |
| ? sourceViewport.getDisplaySetPresentation(dataId)?.voiRange | |
| : undefined) ?? sourceViewport.getDefaultVOIRange(dataId); |
🤖 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/LivewireContourTool.ts` around lines 235
- 243, The `voiRange` fallback in `LivewireContourTool` is still being forced to
`Types.VOIRange` even when both `getDisplaySetPresentation(dataId)?.voiRange`
and `getDefaultVOIRange(dataId)` return `undefined`. Remove the trailing no-op
`?? undefined`/unsafe cast, keep `voiRange` as optional, and add a guard before
calling `LivewireScissors.createInstanceFromRawPixelData` so the scissors
instance is only created when a real VOI range is available.
There was a problem hiding this comment.
I didn't actually change this file so leaving it as is.
There was a problem hiding this comment.
@jbocce, understood — since this reshuffle is just a formatting/rebase artifact and the underlying voiRange fallback behavior predates this PR, I won't push for a fix here. Let me know if you'd like a follow-up issue opened to track the potential undefined VOI range masking separately; otherwise I'll leave it as is.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Context
See OHIF/Viewers#6080
Changes & Results
Skip contours that are not on the currently displayed slice. The distance between the contour plane and the camera focal point, measured along the view plane normal, must be within half the slice spacing.
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
System:
OS: Windows 11 10.0.26200
CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12700H
Memory: 8.83 GB / 31.68 GB
Binaries:
Node: 24.15.0 - C:\Users\joebo\AppData\Local\fnm_multishells\50148_1782198425052\node.EXE
Yarn: 1.22.22 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 11.12.1 - C:\Users\joebo\AppData\Local\fnm_multishells\50148_1782198425052\npm.CMD
pnpm: 11.5.2 - C:\Users\joebo\AppData\Local\pnpm\bin\pnpm.CMD
bun: 1.2.23 - C:\Users\joebo.bun\bin\bun.EXE
Browsers:
Chrome: 149.0.7827.115
Edge: Chromium (149.0.4022.80)
Internet Explorer: 11.0.26100.8115
Summary by CodeRabbit
New Features
Bug Fixes
Tests