feat(tools): new world crosshairs and intersect planes#2781
Conversation
… navigation
- Introduced support for rendering the world crosshair as intersecting lines in Generic 3D viewports.
- Added configuration options for 3D rendering, including line length and visibility settings.
- Implemented navigation functionality to align the viewport slice with a specified world point for both volume-backed slices and image stacks.
- Enhanced the WorldCrosshairTool to auto-initialize the crosshair point based on the current slice center.
- Updated the rendering logic to accommodate new marker styles ('crosshair' and 'point') and improved off-slice display options.
- Refactored viewport navigation logic to streamline interactions and improve usability.
- Updated example descriptions to reflect new features and clarify tool functionalities.
|
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:
📝 WalkthroughWalkthroughThis PR adds spatial geometry utilities, persistent world-crosshair and slice-intersection tools, slab-intensity picking, extensive tests and exports, multiple demos, and a guard for unsupported stack scrolling. ChangesWorld Crosshair and Slice Intersection Feature
Demo examples and metadata
Core Scroll Guard
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WorldCrosshairTool
participant PlanarViewport
participant WorldCrosshairLines3D
participant RenderingEngine
User->>WorldCrosshairTool: setWorldPoint()
WorldCrosshairTool->>PlanarViewport: navigate linked viewports
WorldCrosshairTool->>WorldCrosshairLines3D: update 3D crosshair lines
WorldCrosshairLines3D->>RenderingEngine: render viewport
PlanarViewport->>RenderingEngine: render linked viewports
sequenceDiagram
participant User
participant SliceIntersectionTool
participant TargetViewport
participant GroupMemberViewport
participant RenderingEngine
User->>SliceIntersectionTool: drag intersection line
SliceIntersectionTool->>TargetViewport: compute clipped intersection lines
SliceIntersectionTool->>GroupMemberViewport: translate, rotate, or change slab
SliceIntersectionTool->>RenderingEngine: render affected viewports
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
🧹 Nitpick comments (7)
packages/core/src/utilities/scroll.ts (1)
49-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a debug log for the silently-ignored path.
The guard silently no-ops when a stack-like viewport lacks
getCurrentImageIdIndex/scrollmethods. This is a deliberate behavior change (previously such a call would throw), but silent ignoring can make misconfigurations (e.g., an unexpected viewport type routed here) hard to diagnose.♻️ Optional: add a debug-level warning
if ( typeof (viewport as IStackViewport).getCurrentImageIdIndex !== 'function' || typeof (viewport as IStackViewport).scroll !== 'function' ) { // Viewports without index-based scrolling (e.g. Generic 3D volume // rendering) ignore scroll requests instead of throwing. + console.debug( + 'Scroll::Viewport does not support index-based scrolling, ignoring request' + ); return; }🤖 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/core/src/utilities/scroll.ts` around lines 49 - 58, The scroll guard in scroll() now silently ignores viewports missing getCurrentImageIdIndex or scroll, which can hide unexpected routing or misconfiguration. Add a debug-level log in this early-return path that records the viewport type/state and makes it clear scroll was intentionally skipped. Keep the existing no-throw behavior, but make the silent branch diagnosable by logging from the scroll utility before returning.packages/tools/src/utilities/spatial/distancePointToPlane.ts (1)
10-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider guarding against a zero-length
plane.normal.
vec3.normalizeon a zero vector yieldsNaNcomponents, which would silently propagate throughprojectPointToPlane(no downstream finite-check) since it also relies on this function.🛡️ Optional guard for degenerate normals
export default function distancePointToPlane( point: Types.Point3, plane: Plane ): number { + const normalLengthSquared = vec3.squaredLength(plane.normal); + if (normalLengthSquared === 0) { + return NaN; + } const normal = vec3.normalize(vec3.create(), plane.normal); const toPoint = vec3.subtract(vec3.create(), point, plane.point); return vec3.dot(toPoint, normal); }🤖 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/utilities/spatial/distancePointToPlane.ts` around lines 10 - 18, Guard `distancePointToPlane` against a degenerate `plane.normal` before calling `vec3.normalize`, since normalizing a zero vector will produce `NaN` and propagate into `projectPointToPlane`. Update the `distancePointToPlane` function to detect a zero-length normal and handle it explicitly, either by throwing a clear error or returning a safe fallback value, while keeping the existing dot-product logic for valid normals.packages/tools/src/tools/worldCrosshair/WorldCrosshairLines3D.ts (1)
44-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReallocates VTK points/cell-array on every update, including during interactive drag.
setCrosshairLinePointsalways creates a brand-newvtkPointsandvtkCellArrayinstance, even in the "update existing entry" path (Line 122). SinceupdateWorldCrosshairLines3Dis invoked on everysetWorldPointcall, this runs on every mouse-move tick while dragging the world crosshair withliveUpdateOnShiftMouseMoveenabled, causing avoidable per-frame allocations/GC churn in the 3D viewport's render loop.Consider mutating the existing points buffer in place (
entry.polyData.getPoints().setPoint(...)) and only recreating points/cells once, on first creation.♻️ Suggested approach
function setCrosshairLinePoints( polyData: ReturnType<typeof vtkPolyData.newInstance>, worldPoint: Types.Point3, halfLength: number ): void { const [x, y, z] = worldPoint; - const points = vtkPoints.newInstance(); - points.setNumberOfPoints(6); + let points = polyData.getPoints(); + if (!points || points.getNumberOfPoints() !== 6) { + points = vtkPoints.newInstance(); + points.setNumberOfPoints(6); + polyData.setPoints(points); + polyData.setLines( + vtkCellArray.newInstance({ values: [2, 0, 1, 2, 2, 3, 2, 4, 5] }) + ); + } points.setPoint(0, x - halfLength, y, z); points.setPoint(1, x + halfLength, y, z); points.setPoint(2, x, y - halfLength, z); points.setPoint(3, x, y + halfLength, z); points.setPoint(4, x, y, z - halfLength); points.setPoint(5, x, y, z + halfLength); - - const lines = vtkCellArray.newInstance({ - values: [2, 0, 1, 2, 2, 3, 2, 4, 5], - }); - - polyData.setPoints(points); - polyData.setLines(lines); + points.modified(); polyData.modified(); }Also applies to: 121-123
🤖 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/worldCrosshair/WorldCrosshairLines3D.ts` around lines 44 - 66, The crosshair update path in setCrosshairLinePoints is allocating new vtkPoints and vtkCellArray objects on every call, including the updateExistingEntry flow in updateWorldCrosshairLines3D. Update the existing entry.polyData points in place via the current vtkPoints instance instead of recreating buffers each mouse move, and only create the points/cell array once when the crosshair entry is first initialized.packages/tools/src/tools/WorldCrosshairTool.ts (2)
1185-1256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink-policy branching duplicated between
_getLinkedViewportsand_isViewportLinked.Both methods re-implement the same
explicit/frameOfReferenceUID/toolGrouppolicy switch. A future change to one policy's semantics risks only being applied to one of the two call sites (e.g._isViewportLinkedis used per-render for hit-testing/rendering while_getLinkedViewportsdrives jumping/render fan-out).Consider deriving
_isViewportLinked(viewport)from_getLinkedViewports().some(v => v.id === viewport.id), or factoring the policy predicate into a shared private method taking a single viewport.🤖 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/WorldCrosshairTool.ts` around lines 1185 - 1256, The link-policy logic is duplicated between _getLinkedViewports and _isViewportLinked in WorldCrosshairTool, so update the implementation to use one shared source of truth. Factor the explicit/frameOfReferenceUID/toolGroup decision into a single private helper, or have _isViewportLinked derive its result from _getLinkedViewports(), and keep both methods aligned through that shared logic. Make sure the shared path still preserves the source viewport special case and the planar/known-viewport checks.
841-859: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated marker geometry logic between
renderAnnotationand_getMarkerCanvasPosition.Both methods independently recompute the plane, distance-to-plane, off-slice tolerance, projected point and canvas point. Any future change to this geometry (e.g. a new off-slice rule) needs to be kept in sync in two places.
Consider extracting a shared private helper (e.g.
_computeMarkerGeometry(viewport): { canvasPoint, distanceMm, inSlice } | null) used by both call sites.Also applies to: 1272-1314
🤖 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/WorldCrosshairTool.ts` around lines 841 - 859, The marker geometry calculation in WorldCrosshairTool is duplicated between renderAnnotation and _getMarkerCanvasPosition, so extract the shared plane/distance/tolerance/projection logic into a private helper such as _computeMarkerGeometry(viewport) that returns the canvas point, distanceMm, and inSlice state. Update both call sites to use this helper and keep all off-slice and canvas validation behavior centralized so future geometry changes only need to be made once.packages/tools/src/tools/SliceIntersectionTool.ts (2)
1019-1077: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPotential NaN slab-handle canvas positions when the clipped line is degenerate.
_computeSlabHandlesfalls back tovec2.normalize(vec2.create(), vec2.subtract(vec2.create(), end, start))when the slab is collapsed. Ifstartandendare equal or nearly equal (a near-zero-length clipped canvas line, e.g. near-parallel planes),vec2.normalizedivides by a ~0 length, producingNaNhandle coordinates that would then be drawn into the SVG.A short guard (skip handle placement, or default to a fixed direction, when
vec2.length(vec2.subtract(...)) < epsilon) would avoid this edge case.🤖 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/SliceIntersectionTool.ts` around lines 1019 - 1077, _computeSlabHandles in SliceIntersectionTool can generate NaN canvas positions when the clipped line is degenerate and vec2.normalize is called on a near-zero vector. Add a length/epsilon guard before computing the perpendicular fallback from start/end: if the line segment is too short, either skip handle placement or use a stable default direction instead of normalizing. Keep the fix localized to _computeSlabHandles and ensure both the plus/minus handle paths return finite canvas points.
206-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoundary/intermediate lines share the "slab" field name and SVG id with actual slab-thickness lines.
_appendBoundaryGeometrypushes volume boundary/intermediate slice segments into the samelineInfo.slabLineSegmentsarray that_appendSlabGeometryuses for slab-thickness boundaries, andrenderAnnotationdraws all of them with the sameslab-${groupId}-${index}id and dashed style. These are conceptually different features (showSlabThicknessvs.showBoundaryLines/showIntermediateLines) that happen to share styling today; reusing the same field/id makes it harder to later give them independent styling or to debug which lines are rendering from the DOM.Consider a more neutral field name (e.g.
dashedLineSegments) or separate arrays per concept.Also applies to: 844-935
🤖 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/SliceIntersectionTool.ts` around lines 206 - 226, The boundary/intermediate slice segments are being stored and rendered through the same slab-specific field and SVG id path as true slab-thickness geometry, which conflates two different concepts. Update SliceIntersectionTool’s RenderedIntersectionLine and the related _appendBoundaryGeometry, _appendSlabGeometry, and renderAnnotation flow to use a more neutral shared name like dashedLineSegments, or split the arrays by purpose, and generate distinct SVG ids for boundary/intermediate vs slab-thickness lines so they can be styled and debugged independently.
🤖 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/examples/worldCrosshair/index.ts`:
- Around line 71-77: The worldCrosshair demo instructions currently claim that a
plain click sets the reference point, but WorldCrosshairTool defaults clickToSet
to false so that behavior is not enabled. Update the WorldCrosshairTool setup in
the worldCrosshair example to pass clickToSet: true, or revise the instructions
text to remove the click-based wording; use the WorldCrosshairTool and
instructions block to locate the change.
In `@packages/tools/examples/worldCrosshairAndSliceIntersections/index.ts`:
- Around line 70-75: The demo instructions currently imply a plain click sets
the reference point, but `WorldCrosshairTool` is configured with `clickToSet:
false` by default, so empty-space clicks only work with Shift unless
`clickToSet` is enabled. Update the text in the `instructions` block to match
the actual behavior, or change the `WorldCrosshairTool` setup to pass `{
clickToSet: true }` if you want plain clicks to work.
In `@packages/tools/src/tools/worldCrosshair/WorldCrosshairLines3D.ts`:
- Line 34: The WorldCrosshairLines3D cache can retain stale 3D crosshair entries
when a viewport disappears before onSetToolDisabled() runs. Update the cleanup
logic around linesByKey, _get3DViewports(), and the disabled-tool flow so
missing/destroyed viewports are pruned too, and consider wiring ELEMENT_DISABLED
to remove any LinesEntry no longer backed by an active viewport. Make sure the
cache deletes dead actors/polyData for all keys, not just currently returned
viewports.
In `@packages/tools/src/tools/WorldCrosshairTool.ts`:
- Around line 1052-1102: The centered jump path in
WorldCrosshairTool._jumpViewportToPoint ignores the return value from
navigatePlanarViewportToPoint, unlike the slice-only branch. Update the centered
branch to only set anchorWorld, anchorCanvas, and call viewport.render() when
navigation succeeds; otherwise return early so failed navigation on invalid
viewports does not leave the viewport pinned to an incorrect point.
In `@packages/tools/src/utilities/spatial/translateViewportAlongNormal.ts`:
- Around line 25-30: Wrap the getViewportICamera call inside
translateViewportAlongNormal in a try/catch, following the same defensive
pattern used by getViewportPlane, because it can throw when the viewport camera
is unavailable or not yet rendered. If the call fails, return false from
translateViewportAlongNormal so SliceIntersectionTool._applyTranslate can
continue processing the remaining viewports in its forEach loop without aborting
the interaction.
---
Nitpick comments:
In `@packages/core/src/utilities/scroll.ts`:
- Around line 49-58: The scroll guard in scroll() now silently ignores viewports
missing getCurrentImageIdIndex or scroll, which can hide unexpected routing or
misconfiguration. Add a debug-level log in this early-return path that records
the viewport type/state and makes it clear scroll was intentionally skipped.
Keep the existing no-throw behavior, but make the silent branch diagnosable by
logging from the scroll utility before returning.
In `@packages/tools/src/tools/SliceIntersectionTool.ts`:
- Around line 1019-1077: _computeSlabHandles in SliceIntersectionTool can
generate NaN canvas positions when the clipped line is degenerate and
vec2.normalize is called on a near-zero vector. Add a length/epsilon guard
before computing the perpendicular fallback from start/end: if the line segment
is too short, either skip handle placement or use a stable default direction
instead of normalizing. Keep the fix localized to _computeSlabHandles and ensure
both the plus/minus handle paths return finite canvas points.
- Around line 206-226: The boundary/intermediate slice segments are being stored
and rendered through the same slab-specific field and SVG id path as true
slab-thickness geometry, which conflates two different concepts. Update
SliceIntersectionTool’s RenderedIntersectionLine and the related
_appendBoundaryGeometry, _appendSlabGeometry, and renderAnnotation flow to use a
more neutral shared name like dashedLineSegments, or split the arrays by
purpose, and generate distinct SVG ids for boundary/intermediate vs
slab-thickness lines so they can be styled and debugged independently.
In `@packages/tools/src/tools/worldCrosshair/WorldCrosshairLines3D.ts`:
- Around line 44-66: The crosshair update path in setCrosshairLinePoints is
allocating new vtkPoints and vtkCellArray objects on every call, including the
updateExistingEntry flow in updateWorldCrosshairLines3D. Update the existing
entry.polyData points in place via the current vtkPoints instance instead of
recreating buffers each mouse move, and only create the points/cell array once
when the crosshair entry is first initialized.
In `@packages/tools/src/tools/WorldCrosshairTool.ts`:
- Around line 1185-1256: The link-policy logic is duplicated between
_getLinkedViewports and _isViewportLinked in WorldCrosshairTool, so update the
implementation to use one shared source of truth. Factor the
explicit/frameOfReferenceUID/toolGroup decision into a single private helper, or
have _isViewportLinked derive its result from _getLinkedViewports(), and keep
both methods aligned through that shared logic. Make sure the shared path still
preserves the source viewport special case and the planar/known-viewport checks.
- Around line 841-859: The marker geometry calculation in WorldCrosshairTool is
duplicated between renderAnnotation and _getMarkerCanvasPosition, so extract the
shared plane/distance/tolerance/projection logic into a private helper such as
_computeMarkerGeometry(viewport) that returns the canvas point, distanceMm, and
inSlice state. Update both call sites to use this helper and keep all off-slice
and canvas validation behavior centralized so future geometry changes only need
to be made once.
In `@packages/tools/src/utilities/spatial/distancePointToPlane.ts`:
- Around line 10-18: Guard `distancePointToPlane` against a degenerate
`plane.normal` before calling `vec3.normalize`, since normalizing a zero vector
will produce `NaN` and propagate into `projectPointToPlane`. Update the
`distancePointToPlane` function to detect a zero-length normal and handle it
explicitly, either by throwing a clear error or returning a safe fallback value,
while keeping the existing dot-product logic for valid normals.
🪄 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: cacbfc96-6b86-44be-8927-6512ddb74504
📒 Files selected for processing (29)
packages/core/src/utilities/scroll.tspackages/tools/examples/sliceIntersections/index.tspackages/tools/examples/tenViewportReferencePoint/index.tspackages/tools/examples/worldCrosshair/index.tspackages/tools/examples/worldCrosshairAndSliceIntersections/index.tspackages/tools/examples/worldCrosshairSliceIntersectionsPetCt/index.tspackages/tools/src/enums/Events.tspackages/tools/src/index.tspackages/tools/src/tools/SliceIntersectionTool.spec.tspackages/tools/src/tools/SliceIntersectionTool.tspackages/tools/src/tools/WorldCrosshairTool.spec.tspackages/tools/src/tools/WorldCrosshairTool.tspackages/tools/src/tools/index.tspackages/tools/src/tools/worldCrosshair/WorldCrosshairLines3D.tspackages/tools/src/utilities/genericViewportToolHelpers.tspackages/tools/src/utilities/index.tspackages/tools/src/utilities/spatial/areViewportsSpatiallyLinked.tspackages/tools/src/utilities/spatial/clipWorldLineToViewportCanvas.tspackages/tools/src/utilities/spatial/distancePointToPlane.tspackages/tools/src/utilities/spatial/getDisplayedCanvasSize.tspackages/tools/src/utilities/spatial/getViewportPlane.tspackages/tools/src/utilities/spatial/index.tspackages/tools/src/utilities/spatial/intersectPlanes.tspackages/tools/src/utilities/spatial/projectPointToPlane.tspackages/tools/src/utilities/spatial/rotateViewportAroundWorldPoint.tspackages/tools/src/utilities/spatial/spatial.spec.tspackages/tools/src/utilities/spatial/translateViewportAlongNormal.tspackages/tools/src/utilities/spatial/types.tsutils/ExampleRunner/example-info.json
| polyData: ReturnType<typeof vtkPolyData.newInstance>; | ||
| }; | ||
|
|
||
| const linesByKey = new Map<string, LinesEntry>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and surrounding symbols first.
ast-grep outline packages/tools/src/tools/worldCrosshair/WorldCrosshairLines3D.ts --view expanded
# Read the file with line numbers, focusing on the cache and cleanup logic.
sed -n '1,240p' packages/tools/src/tools/worldCrosshair/WorldCrosshairLines3D.ts | cat -n
# Find related cleanup / disable lifecycle references in the tool.
rg -n "removeWorldCrosshairLines3D|onSetToolDisabled|ELEMENT_DISABLED|getEnabledElementByViewportId|linesByKey|WorldCrosshairLines3D" packages/tools/src/tools/worldCrosshair -SRepository: cornerstonejs/cornerstone3D
Length of output: 7338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tool lifecycle and any viewport-destroy cleanup hooks.
ast-grep outline packages/tools/src/tools/WorldCrosshairTool.ts --view expanded || true
ast-grep outline packages/tools/src/tools/worldCrosshair --view expanded
rg -n "removeWorldCrosshairLines3D|updateWorldCrosshairLines3D|onSetToolDisabled|ELEMENT_DISABLED|disableElement|viewport.*destroy|destroy.*viewport|getEnabledElementByViewportId|addEventListener|removeEventListener" packages/tools/src -S
# Read the main tool file if present.
fd -a "WorldCrosshairTool.ts" packages/tools/srcRepository: cornerstonejs/cornerstone3D
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant lifecycle methods and 3D viewport enumeration.
sed -n '461,500p' packages/tools/src/tools/WorldCrosshairTool.ts | cat -n
sed -n '1111,1155p' packages/tools/src/tools/WorldCrosshairTool.ts | cat -n
# Check whether there is any cleanup tied to element-disable or viewport-destroy events in this tool.
rg -n "ELEMENT_DISABLED|disableElement|removeWorldCrosshairLines3D|_get3DViewports|onSetToolDisabled" packages/tools/src/tools/WorldCrosshairTool.ts packages/tools/src/tools/worldCrosshair -SRepository: cornerstonejs/cornerstone3D
Length of output: 4124
Remove stale 3D crosshair cache entries when a viewport is disabled
linesByKey is only cleared for viewports returned by _get3DViewports(), so if a 3D viewport is destroyed before onSetToolDisabled() runs, its LinesEntry can stay pinned in this module. Hooking ELEMENT_DISABLED (or otherwise pruning missing viewports) would prevent the cache from retaining dead actors/polyData and leaking GPU resources over repeated study/engine lifecycles.
🤖 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/worldCrosshair/WorldCrosshairLines3D.ts` at line 34,
The WorldCrosshairLines3D cache can retain stale 3D crosshair entries when a
viewport disappears before onSetToolDisabled() runs. Update the cleanup logic
around linesByKey, _get3DViewports(), and the disabled-tool flow so
missing/destroyed viewports are pruned too, and consider wiring ELEMENT_DISABLED
to remove any LinesEntry no longer backed by an active viewport. Make sure the
cache deletes dead actors/polyData for all keys, not just currently returned
viewports.
…structions The demos instruct users to click (empty space) to set the reference point, but WorldCrosshairTool defaults clickToSet to false, so a plain click did nothing without Shift. Pass clickToSet: true so the documented behavior works. In the combined demo, line-near clicks are still grabbed by SliceIntersectionTool before addNewAnnotation fires. Addresses CodeRabbit review threads on both example files.
…fails The centered jump branch called navigatePlanarViewportToPoint but ignored its return value, then unconditionally pinned anchorWorld/anchorCanvas and rendered. When navigation fails (e.g. a stack viewport with no valid closest image) this anchored the viewport to a point it never navigated to, producing a visibly wrong pan. Early-return on failure, matching the slice-only branch. Addresses CodeRabbit review thread.
…hrow getViewportICamera can throw for viewports without a valid/rendered camera. Wrap it in try/catch returning false, mirroring the sibling getViewportPlane, so this exported utility is robust to a bad viewport regardless of caller. Addresses CodeRabbit review thread.
sedghi
changed the title
…ts to hottest voxel
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tools/src/utilities/pickIntensityPointInSlab.spec.ts (1)
87-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the sample-cap and missing-plane branches.
The suite thoroughly covers max/min picking, slab bounds, and non-applicable modes, but doesn't exercise the
MAX_SAMPLEScap (large slab thickness relative to spacing) or thegetViewportPlanereturning falsy. Both are real code paths inpickIntensityPointInSlab.ts(Lines 143-153, 133-135) that affect correctness for full-volume MIP demos likeworldCrosshairMip.🤖 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/utilities/pickIntensityPointInSlab.spec.ts` around lines 87 - 169, Add tests in pickIntensityPointInSlab.spec.ts to cover the two untested branches in pickIntensityPointInSlab/getSlabIntensityPickContext: one case should use a very large slabThickness to force the MAX_SAMPLES cap path and assert the picked point still resolves correctly, and another should make getViewportPlane return a falsy value and verify pickIntensityPointInSlab returns null. Use the existing helpers like createFakeMipViewport, stubCachedVolume, and pickIntensityPointInSlab so the new specs sit alongside the current max/min and non-applicable mode coverage.
🤖 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.
Nitpick comments:
In `@packages/tools/src/utilities/pickIntensityPointInSlab.spec.ts`:
- Around line 87-169: Add tests in pickIntensityPointInSlab.spec.ts to cover the
two untested branches in pickIntensityPointInSlab/getSlabIntensityPickContext:
one case should use a very large slabThickness to force the MAX_SAMPLES cap path
and assert the picked point still resolves correctly, and another should make
getViewportPlane return a falsy value and verify pickIntensityPointInSlab
returns null. Use the existing helpers like createFakeMipViewport,
stubCachedVolume, and pickIntensityPointInSlab so the new specs sit alongside
the current max/min and non-applicable mode coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a09ea91-9999-4a60-b824-a8fb2d604372
📒 Files selected for processing (7)
packages/tools/examples/worldCrosshairMip/index.tspackages/tools/src/tools/WorldCrosshairTool.spec.tspackages/tools/src/tools/WorldCrosshairTool.tspackages/tools/src/utilities/index.tspackages/tools/src/utilities/pickIntensityPointInSlab.spec.tspackages/tools/src/utilities/pickIntensityPointInSlab.tsutils/ExampleRunner/example-info.json
✅ Files skipped from review due to trivial changes (1)
- utils/ExampleRunner/example-info.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tools/src/tools/WorldCrosshairTool.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
|
|
||
| CROSSHAIR_TOOL_CENTER_CHANGED = 'CORNERSTONE_TOOLS_CROSSHAIR_TOOL_CENTER_CHANGED', | ||
|
|
||
| /** |
There was a problem hiding this comment.
These feel like a lot of event types that are fairly specialized. Is there some way to just get events on tool changed by tool type, and have them have a tool type changed?
Alternatively, do we need to eventually get a way to use zustand to allow filtering for specific behaviour changes?
| } | ||
| }; | ||
|
|
||
| _activateModify = (element: HTMLDivElement): void => { |
There was a problem hiding this comment.
These functions seems like they are really standard for a lot of tools - any chance they could just be declared in the parent class and then inheritted?
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
there are a few comments inline, plus:
Correctness
Unguarded getViewportICamera calls. getViewportPlane.ts:30-35 and translateViewportAlongNormal.ts:28-33 deliberately wrap getViewportICamera in try/catch because it can throw on an unrendered viewport. But WorldCrosshairTool.ts:1005 (_maybeAutoInitialize) and WorldCrosshairTool.ts:1081 (_jumpViewportToPoint) call it directly. _maybeAutoInitialize runs at the very top of renderAnnotation and also from onSetToolEnabled while iterating possibly-unrendered enabled viewports — a throw there would escape the enable/render path. Low real-world risk (guarded by _isPlanarViewport), but for consistency it should use the same defensive pattern.
setWorldPoint skips _syncAnnotation when the point is unchanged but the FoR changed. WorldCrosshairTool.ts:355-374 — if a caller passes a new frameOfReferenceUID with a ~identical point, _state.frameOfReferenceUID updates but the annotation isn't regrouped. A genuine edge case, essentially harmless, worth being aware of.
Performance
SliceIntersectionTool recomputes plane groups and viewport lookups heavily per render pass. renderAnnotation → _computeLinesForTarget → _getPlaneGroups() runs for each target viewport, and _getPlaneGroups() calls _getViewportById() per viewport, which rebuilds the whole list via _getToolGroupViewports() (fresh getEnabledElementByIds + getResolvedView/camera resolve for every viewport). That's roughly O(V³) enabled-element/camera resolves per full render pass. On the 10-viewport example during a drag or scroll this is a lot of repeated work. Recommend memoizing the plane-group computation and a viewport-id→viewport map for the duration of a single render pass (or per animation frame). See SliceIntersectionTool.ts:492-552 and SliceIntersectionTool.ts:1956-1965.
pickIntensityPointInSlab runs on every shift-mouse-move when liveUpdateOnShiftMouseMove is on (up to 2001 volume samples per move). The MAX_SAMPLES cap makes this bounded and probably fine, but it's worth confirming it stays smooth on large PET slabs.
Consistency / Documentation
"two lines" vs. three. WorldCrosshairTool.ts:165-167 and the PR description say the 3D point renders as "two world-space intersecting lines," but WorldCrosshairLines3D.ts:44-66 draws three (one per patient axis). Update the comment.
rotateViewportAroundWorldPoint is dead for these tools. It returns false for generic viewports (rotateViewportAroundWorldPoint.ts:27-29) — the only viewport type these tools support — which is exactly why SliceIntersectionTool._applyRotate reimplements rotation inline via setViewReference. Shipping a public utility that no-ops on the supported surface is a minor smell; consider documenting it as legacy-only or reconciling with the inline path.
slabLineSegments reused for boundary lines. _appendBoundaryGeometry pushes boundary/intermediate lines into lineInfo.slabLineSegments (SliceIntersectionTool.ts:924-934). Harmless (they render identically) but the field name is now misleading.
that has at lesat a few things worth addressing.
…tersection tools - Hoist the standard _activateModify/_deactivateModify modify-listener wiring into AnnotationTool as protected members; WorldCrosshairTool and SliceIntersectionTool inherit them, and the identical private copies in LivewireContourTool and SplineROITool are removed - Guard the direct getViewportICamera calls in _maybeAutoInitialize and _jumpViewportToPoint against unrendered viewports (same pattern as getViewportPlane) - setWorldPoint: regroup the annotation when the frameOfReferenceUID changes even if the point itself is unchanged - SliceIntersectionTool: cache plane groups and viewport-id lookups per synchronous render/interaction pass (cleared in a microtask), removing the repeated per-target enabled-element and camera resolves - Keep boundary/intermediate slice lines in their own boundaryLineSegments field instead of overloading slabLineSegments - Fix the 3D marker doc comments (three axis lines, not two) and document rotateViewportAroundWorldPoint as legacy-viewports-only
2 participants
Context
This PR adds two independent, world-space navigation/annotation tools built exclusively for the Generic ("next") viewport architecture (
PLANAR_NEXT, plusVOLUME_3D_NEXTfor the 3D crosshair). Both operate through the native view-state API (getResolvedView/setViewReference/setViewState); legacy stack/volume viewports and their compatibility adapters are intentionally ignored. The two tools share nothing and can be enabled together or separately.Tools
1.
WorldCrosshairTool— "Reference Point"Stores and renders one persistent world-space point of interest per tool group. The user picks an anatomical/world point and the tool keeps it fixed while viewports scroll, pan, zoom or rotate — the point is never derived from camera state (the tool deliberately ignores
CAMERA_MODIFIED/ view-state resets).markerStyle).projectedWithDistance|projected|hide).VOLUME_3D_NEXTviewports as world-space intersecting lines (viaWorldCrosshairLines3D).frameOfReferenceUID|toolGroup|explicit) control which viewports show / jump to the point.sliceOnly(navigate along the normal, keep pan) orcentered(also pin the point to canvas center).clickToSetenables plain-click; optional live update on Shift-move. Programmatic API:setWorldPoint/clearWorldPoint/jumpLinkedViewportsToWorldPoint.WORLD_CROSSHAIR_POINT_CHANGED,WORLD_CROSSHAIR_POINT_CLEARED,WORLD_CROSSHAIR_JUMPED_TO_POINT.It renders no intersection lines and never rotates/translates planes or touches slab thickness — that belongs to the other tool.
2.
SliceIntersectionTool— "Slice Intersections"Renders and manipulates slice plane intersection lines. It thinks in planes, not viewports: viewports showing the same plane family over the same frame of reference (e.g. CT axial + PT axial + a 2D axial stack) form one plane group, rendered as a single line.
SLICE_INTERSECTION_LINE_SELECTED,SLICE_INTERSECTION_MANIPULATION_STARTED,SLICE_INTERSECTION_MANIPULATION_ENDED,SLICE_INTERSECTION_SLAB_THICKNESS_CHANGED.Stores no persistent world point and does not render in 3D viewports.
3.
WorldCrosshairLines3D(helper)3D rendering extension used by
WorldCrosshairToolto draw the reference point in Generic 3D (VOLUME_3D_NEXT) viewports as three world-space axis-aligned lines crossing at the point, attached through the viewport'sgetRenderer()hook. Holds no tool state.Supporting spatial utilities (
packages/tools/src/utilities/spatial)Shared, architecture-agnostic geometry helpers, unit-tested in
spatial.spec.ts:getViewportPlanenullfor unrendered / non-image viewports.intersectPlanesnullif parallel).distancePointToPlaneprojectPointToPlaneclipWorldLineToViewportCanvasnull).translateViewportAlongNormalrotateViewportAroundWorldPointareViewportsSpatiallyLinkedgetDisplayedCanvasSizePlus
genericViewportToolHelpers(navigatePlanarViewportToPoint,getNativeSourceProperties) for driving generic viewports by view reference.Examples
Other changes
core/utilities/scroll.ts: scroll now safely ignores viewports without index-based scrolling (e.g. Generic 3D volume rendering) instead of throwing.enums/Events.ts(listed above).Testing
WorldCrosshairTool.spec.ts,SliceIntersectionTool.spec.ts,spatial.spec.ts— run withyarn test:unit(Jest).Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit