fix: more generic viewport push for parity with legacy - #2766
Merged
Conversation
…lice-count
Generic Viewport ("next") migration - Waves 0 and 1 of the OHIF native-next
enablement (see GENERIC_VIEWPORT_MIGRATION_PLAN).
Wave 0 - foundation:
- CS-5: GenericViewport.getCurrentMode() + PlanarViewport content-true
stack/volume/empty classification from the active source binding; add
protected getSourceBinding().
- CS-17: isGenericViewport() narrowing guard plus getViewportContentMode,
viewportIsInVolumeMode and viewportIsInStackMode capability utilities,
exported from utilities.
- CS-16: new IGenericViewport and ViewportContentMode types. The
IEnabledElement.viewport union widening is intentionally deferred to the
tools-camera wave (CS-8/CS-10) to avoid cascading narrowing changes across
the tools annotation/render path; documented inline.
Wave 1 - planar stack:
- CS-1: setDisplaySetPresentation now emits VOI_MODIFIED / COLORMAP_MODIFIED via
a mergeDataPresentation -> notifyDataPresentationModified hook on the native
path only, so the legacy compatibility adapter (which applies colormap through
setDataPresentationState directly) does not double-fire.
- CS-6: volume-backed slice changes now also emit VOLUME_NEW_IMAGE so volume
scroll indicators, MPR slice sync and segmentation slice tracking react.
- CS-7: native PlanarViewport.getNumberOfSlices() so csUtils.jumpToSlice and
image-slice synchronizers work on PLANAR_NEXT (which is not a StackViewport).
CS-4 (data-inferred stack/volume render path) already holds via
PlanarRenderPathDecisionService/DefaultPlanarDataProvider, no change needed.
Verified: core tsc clean, oxlint clean, 106 jest unit tests pass (97 existing
planar/compat + 9 new viewportContentMode), genericViewport Playwright suite
124 passed / 2 skipped / 0 failed on chromium.
…tDisplaySets
Generic Viewport ("next") migration - Wave 2 (planar volume / MPR),
cornerstone-core part.
- CS-15: add PlanarRenderPathDecisionService.canRender(), a non-throwing
companion to select(), and PlanarViewport.canRenderOrientation(orientation,
dataId?) so callers (e.g. OHIF MPR/orientation controls) can pre-check whether
a reformatted orientation is renderable for the bound data instead of relying
on select() throwing on an unsupported request.
- CS-3: PlanarViewport.setDisplaySets() now preserves externally-managed
segmentation overlays when the source is re-set to the same display set
(removeReplaceableData replaces the blanket removeAllData). A hydrated labelmap
overlay, mounted out-of-band by the segmentation display tool, no longer blanks
when the source is re-applied (e.g. when an app re-runs its viewport-data
sync). A source change still clears everything so a stale segmentation cannot
leak across datasets.
Verified: core tsc clean, oxlint clean, 89 planar/compat jest tests pass,
genericViewport Playwright suite 124 passed / 2 skipped / 0 failed on chromium.
…hronizers
Generic Viewport ("next") migration - Wave 2 (CS-11).
- voiSyncCallback: apply VOI / invert / colormap to direct Generic ("next")
viewports via setDisplaySetPresentation (guarded by utilities.isGenericViewport)
instead of throwing "Viewport type not supported". Reacts to the VOI_MODIFIED /
COLORMAP_MODIFIED events now emitted by the native presentation path.
- imageSliceSyncCallback: detect volume-backed slice content on a native
PlanarViewport via utilities.viewportIsInVolumeMode (it is not a VolumeViewport
instance), so the volume slice-index reversal is applied correctly.
Verified: core tsc clean, tools tsc clean, oxlint clean, genericViewport
Playwright suite 124 passed / 2 skipped / 0 failed on chromium.
Generic Viewport ("next") migration - Wave 3 (fusion), CS-19.
- Add PlanarSetDataOptions.forceCPU so an individual display set / overlay can be
pinned to the CPU render path regardless of global rendering configuration or
byte-size thresholds (forwarded as webGLAvailable:false to the render-path
decision). Left undefined by default, so global config + thresholds continue to
decide.
CS-4 (multi-volume fusion mount via setDisplaySets source/overlay roles) and
CS-22 (per-overlay colormap with a multi-point opacity transfer function via
setDisplaySetPresentation, ColormapPublic.opacity: OpacityMapping[] | number) are
already implemented in the native architecture and exercised by the
genericMultiVolumeAPI example + Playwright snapshot, so no change was needed there.
Verified: core tsc clean, oxlint clean, genericViewport Playwright suite 124
passed / 2 skipped / 0 failed on chromium.
…CS-8)
Generic Viewport ("next") migration - contour sub-track of segmentation (Wave 4),
part of CS-8 (tools must not assume a legacy ICamera).
Contour rendering and contour-segmentation hydration read
viewport.getCamera().viewPlaneNormal, which is absent on a direct PLANAR_NEXT
viewport (TypeError), so RTSTRUCT / contour overlays could not render natively.
Route these reads through the existing getViewportICamera bridge, which derives
the camera from getResolvedView().toICamera() and falls back to the legacy
getCamera() on classic viewports. This is backward-compatible (identical camera
on legacy viewports) and enables contour rendering on native generic viewports,
matching how the annotation base tools already resolve their camera.
Native viewports intentionally do not expose getCamera/setCamera (enforced by
the "does not expose view-presentation methods on Planar Next" test), so the
bridge - not a promoted native getCamera - is the correct path.
Verified: tools tsc clean, oxlint clean (only pre-existing warnings),
genericViewport (124) + contourRendering + contourRenderingTiled Playwright
specs: 126 passed / 2 skipped / 0 failed on chromium.
ReferenceLinesTool reads the source/target viewport camera (viewUp, viewPlaneNormal, focalPoint) to draw the lines. On a direct PLANAR_NEXT viewport getCamera() is absent (TypeError), so reference lines could not render natively. Route the three reads through the getViewportICamera bridge (resolved-view derived camera with a legacy getCamera() fallback). ReferenceLinesTool is read-only, so this fully enables it on native generic viewports while remaining byte-identical on legacy viewports. Verified: tools tsc clean, oxlint clean (only pre-existing warnings), genericViewport (124) + MPRReformat Playwright specs: 126 passed / 2 skipped / 0 failed on chromium.
cameraSyncCallback wrote tViewport.setCamera(camera), which throws on a direct
PLANAR_NEXT viewport. When the target is a Generic ("next") viewport, copy the
source viewport's spatial view reference (setViewReference) instead - the native
analog for cross-viewport camera-position synchronization. Legacy viewports keep
the setCamera path unchanged (guarded by utilities.isGenericViewport).
Verified: tools tsc clean, oxlint clean, genericViewport 124 passed / 2 skipped /
0 failed on chromium.
The legacy Viewport exposes getImageActor(volumeId), which VOI/colorbar tooling (e.g. tools/utilities/voi/colorbar/ViewportColorbar) depends on to read an actor's image/VOI range. PlanarViewport (PLANAR_NEXT) extends GenericViewport, which lacks it, so constructing a ViewportColorbar against a native viewport threw 'viewport.getImageActor is not a function'. Add a getImageActor mirroring the legacy implementation: resolve the actor entry from getActors() (by volumeId or the first entry) and return it when it is an image actor, otherwise null. CPU render paths with no VTK actor return null, and existing callers already handle null by falling back to a default range.
playClip's _createCinePlayContext routed by instanceof StackViewport/ VolumeViewport/VideoViewport and threw 'Unknown viewport type' for anything else. Direct PLANAR_NEXT (next) viewports match none of those, so starting cine on a native viewport threw. Add a capability branch (utilities.isGenericViewport) before the throw that builds a generic cine play context: numScrollSteps/currentStepIndex from the semantic getNumberOfSlices/getSliceIndex, frameTimeVector enabled only in stack mode (acquired multi-frame; volume reformat cine uses a fixed rate), and scroll via csUtils.scroll with the same waitForRendered gate as the stack context (guarded so a missing viewportStatus does not stall). The legacy instanceof branches are unchanged, and stopClip already no-ops safely for native (it only clears the interval), so the stop path needed no change. Verified live in OHIF on a 295-slice native stack: playClip no longer throws, cine auto-advances (sliceIndex 0->24 at 10fps) via both the raw utility and the OHIF cineService, and stopClip halts it; legacy (flag off) cine is byte-identical.
A native VolumeViewport3D renders its VTK volume directly to the shared on-screen canvas (.cornerstone-canvas) and, unlike PlanarViewport, has no separate CPU canvas. When the same element previously hosted a CPU PlanarViewport, that viewport set the shared canvas to display:none (showing its cpuCanvas instead). On switching the element to a 3D viewport, the canvas stayed hidden, so the volume rendering was produced (the engine composited it) but never shown — a black viewport. Ensure the 3D viewport's canvas is visible (display:'') in the constructor. Legacy 3D viewports never hid the canvas, so they are unaffected. Verified live in OHIF: native VOLUME_3D_NEXT volume rendering now displays (previously black); legacy VOLUME_3D unchanged.
CrosshairsTool and ReferenceCursors drove cross-viewport MPR navigation through
getCamera/setCamera/resetCamera and slab APIs that direct PLANAR_NEXT (next)
viewports do not expose, so crosshairs threw on native MPR.
Reads: every viewport.getCamera() read (22 in CrosshairsTool, 3 in ReferenceCursors)
now goes through getViewportICamera(viewport) - byte-identical for legacy (it falls
back to getCamera) and resolved-view-derived for native. Mirrors ReferenceLinesTool.
Writes (guarded by isGenericViewport, legacy path unchanged):
- setToolCenter / applyDeltaShift: pure along-normal scroll -> setViewReference
({cameraFocalPoint}) on native (snaps to slice); ReferenceCursors plane move too.
- resetCrosshairs: resetViewState (pan/zoom/orientation/flip) on native (no slab /
slice-recenter); legacy keeps resetCamera + resetSlabThickness.
- ROTATE, SLAB, auto-pan: gated off on native (no validated free-oblique orientation
write, no slab API, no world-space pan); handles are inert rather than throwing.
Native-safe reads: getSlabThickness -> MINIMUM_SLAB_THICKNESS, _checkIfViewports-
RenderingSameScene compares view-reference dataId (no getAllVolumeIds), and a
getDisplayedCanvasSize helper uses the element size for native (viewport.canvas is the
hidden cornerstone-canvas with clientWidth/Height 0, which collapsed the reference
lines).
Verified live in OHIF MPR 3-up, both lanes: reference lines render with correct
geometry, crosshairs jump scrolls the other viewports, console clean; rotate/slab/
auto-pan no-op on native; legacy MPR crosshairs unchanged.
The 3D volume-rendering interaction tools (TrackballRotate, VolumeRotate, OrientationController, VolumeCropping, VolumeCroppingControl) drove the VR camera through getCamera/setCamera/resetCamera, which native VolumeViewport3D does not expose by design (it uses getViewState/setViewState/resetViewState, asserted by viewportProjectionService.jest). So native 3D VR rendered but could not be rotated, cropped, or re-oriented. Add a setViewportCamera write bridge (+ resetViewportCamera) - the write counterpart of getViewportICamera: legacy uses setCamera/resetCamera; native applies the same camera fields (position/focalPoint/viewUp/viewPlaneNormal/parallelScale) via setViewState -> applyVolume3DCamera on the vtk camera, and resets via resetViewState. Migrate all five tools: getCamera() -> getViewportICamera(viewport) (byte-identical for legacy), setCamera(...) -> setViewportCamera(viewport, ...), resetCamera(opts) -> resetViewportCamera(viewport, opts). getVtkActiveCamera (already native-safe) is untouched. No viewport API change, so the jest assertion that VolumeViewport3D lacks resetCamera/getViewPresentation still holds. Verified live in OHIF: TrackballRotate rotates a native VOLUME_3D_NEXT viewport (the volume visibly reorients, camera position updates), 0 errors; legacy 3D camera writes unchanged (guarded).
WindowLevelRegionTool, ScaleOverlayTool, ETDRSGridTool, and OverlayGridTool read viewport.getCamera() (viewPlaneNormal/viewUp/focalPoint) to place their overlays/ annotations, which throws on direct PLANAR_NEXT (next) viewports. These are read-only (no setCamera), so swap each getCamera() to getViewportICamera(viewport): byte-identical for legacy (it falls back to getCamera) and resolved-view-derived for native. Mirrors the ReferenceLinesTool/CrosshairsTool read migration.
MagnifyTool hard-gated to StackViewport and read several source-viewport APIs that direct PLANAR_NEXT (next) viewports do not expose (getProperties, getViewPresentation, getCamera.parallelScale), so magnify threw on a native source. - Relax the StackViewport gate to also accept csUtils.isGenericViewport viewports. - _getReferencedImageId: native stack-mode viewport returns getCurrentImageId(). - Source appearance for the loupe: native VOI/LUT via getDisplaySetPresentation (getSourceDataId), and rotation/flip via the projection-aware getViewportPresentation helper (legacy still uses getProperties/getViewPresentation). - Magnification base: native has no parallelScale, so fall back to the loupe viewport's own (fit) parallelScale. The loupe is always a legacy StackViewport (its setStack/getCamera/setProperties are unchanged), so only the SOURCE reads needed native handling; the legacy source path is byte-identical (guarded by isGenericViewport). Typecheck clean; native methods verified (PlanarViewport.getCurrentImageId/getDisplaySetPresentation/getSourceDataId). AdvancedMagnifyTool needs deeper native source-type dispatch and is a follow-up.
A single example that exercises everything migrated to the native next API: planar acquisition rendering, volume MPR (axial/sagittal/coronal PlanarViewport), native 3D volume rendering with presets, plus the Crosshairs, Cine, Reference Cursors, Magnify, Scale Overlay and Window Level tools - all running on GenericViewports.
getReferenceLineDraggableRotatable was returning false, which gates the crosshairs reference-line grab, the center gap, and the drag-to-translate reslice in the tool's hit detection (_pointNearTool) and drag handler. The result was static lines with no gap that could not reslice the other MPR viewports. Return true so the crosshairs translate/jump works on the native PLANAR_NEXT viewports (rotation drag remains a no-op on native by design - the tool gates OPERATION.ROTATE off for GenericViewports).
…ports Native PLANAR_NEXT sources lack getCamera/getProperties/getViewPresentation and, crucially, setStack/addVolumes are absent on the native class, so the existing _isStackViewport/_isVolumeViewport detection misses native entirely and the loupe was mounted with no data. - Add a third dispatch lane: when the source is a generic (next) viewport, back the loupe with a legacy STACK viewport (mirrors MagnifyTool) and feed it via the native getImageIds()/getCurrentImageIdIndex(). - Bridge the source reads: orientation via getViewportICamera, VOI/LUT via getDisplaySetPresentation(getSourceDataId()), flip via getViewportPresentation, referenced image id via getCurrentImageId (the base getReferencedImageId would parse the frameOfReference UID as a bogus image id on native). - Reconstruct the source parallelScale geometrically in _convertZoomFactorToParallelScale, since native has no getCamera and its on-screen canvas is hidden (offsetWidth === 0). Legacy viewports take the original code paths unchanged (isGenericViewport is false for them), so flag-off behavior is byte-identical. Validated end-to-end in the browser on both native and legacy: loupe creates, clones the stack, transfers VOI, renders magnified content, and the glass/circle/cursor coincide.
Native PlanarViewport had no calibrateSpacing, so viewportSupportsStackCalibration
was false and the tools calibrateImageSpacing utility skipped native viewports
entirely (silent no-op) — CalibrationLine never took effect on the next path. Two
gaps, both fixed:
1. PlanarViewport.calibrateSpacing(imageId): the native counterpart of
StackViewport.calibrateSpacing. Re-renders and emits IMAGE_SPACING_CALIBRATED so
annotation tools invalidate cached stats and recompute lengths. Defining it makes
viewportSupportsStackCalibration true, so calibrateImageSpacing routes here.
2. buildPlanarImageData merged the USER calibration (calibratedPixelSpacing provider)
on top of the DICOM calibration module, mirroring legacy StackViewport.getImageData
({ ...csImage.calibration, ...this.calibration }). getImageDataMetadata reads only
the DICOM 'calibrationModule', so without this native getImageData().calibration
ignored CalibrationLine and getCalibratedLengthUnitsAndScale never picked up the
scale.
Validated live in OHIF on a native PLANAR_NEXT CT: calibrateImageSpacing(imageId, re,
2.5) -> getImageData().calibration {} -> {type:'User',scale:2.5}; the length tool's
getCalibratedLengthUnitsAndScale scale 1.848 -> 2.5; IMAGE_SPACING_CALIBRATED fires;
console clean. Additive/conditional (no user calibration => behavior unchanged), so
the legacy path and existing genericViewport behavior are byte-identical.
…dering feat(PlanarViewport): establish stacking context for viewport element
… better debugging context
|
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:
📝 WalkthroughWalkthroughIntroduces ChangesGeneric Viewport Integration
Labelmap Layer Storage Kind Refactor
Sequence Diagram(s)sequenceDiagram
participant App
participant PlanarViewport
participant PlanarRenderPathDecisionService
participant CpuImageSliceRenderPath
participant planarImageRendering
App->>PlanarViewport: setDisplaySets(entries, options)
PlanarViewport->>PlanarViewport: removeReplaceableData(entries)
PlanarViewport->>PlanarViewport: addDisplaySetInternal(entry, options)
PlanarViewport->>PlanarRenderPathDecisionService: select(dataSet, {webGLAvailable, forceCPU})
PlanarRenderPathDecisionService-->>PlanarViewport: renderPath (cpu/gpu)
PlanarViewport->>CpuImageSliceRenderPath: addData(payload)
CpuImageSliceRenderPath->>CpuImageSliceRenderPath: markCpuImagePreScaled(image)
CpuImageSliceRenderPath->>planarImageRendering: applyPlanarImagePresentation({voiLUTFunction})
CpuImageSliceRenderPath-->>PlanarViewport: rendered
PlanarViewport->>PlanarViewport: triggerCameraModifiedEvent()
sequenceDiagram
participant Tool
participant getViewportICamera
participant setViewportCamera
participant GenericViewport
participant LegacyViewport
Tool->>getViewportICamera: getViewportICamera(viewport)
getViewportICamera-->>Tool: {focalPoint, viewPlaneNormal, viewUp, ...}
Tool->>setViewportCamera: setViewportCamera(viewport, camera)
setViewportCamera->>setViewportCamera: isGenericViewport(viewport)?
alt generic viewport
setViewportCamera->>GenericViewport: setViewState(camera)
else legacy viewport
setViewportCamera->>LegacyViewport: setCamera(camera)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
sedghi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/core/src/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.ts (1)
247-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrigger new-image event on max-slice updates in VTK path too.
At Line 247, the guard ignores
maxImageIdIndexChanged. That can leavenumberOfSlicesstale for consumers when only the max index changes.Suggested fix
- if (triggerImageEvent && imageIdIndexChanged) { + if (triggerImageEvent && (imageIdIndexChanged || maxImageIdIndexChanged)) { triggerPlanarVolumeNewImage(ctx, { camera, acquisitionOrientation: rendering.acquisitionOrientation, imageIds: rendering.imageIds, imageIdIndex: rendering.currentImageIdIndex, maxImageIdIndex: rendering.maxImageIdIndex, }); }🤖 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/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.ts` around lines 247 - 254, The guard condition for triggering the planar volume new image event only checks imageIdIndexChanged but ignores maxImageIdIndexChanged, which causes the numberOfSlices to become stale when only the maximum index changes. Modify the if condition at line 247 to include maxImageIdIndexChanged in the guard, so the triggerPlanarVolumeNewImage call is executed whenever either imageIdIndexChanged or maxImageIdIndexChanged is true, ensuring consumers receive the updated slice information in both cases.packages/core/src/RenderingEngine/GenericViewport/Planar/CpuVolumeSliceRenderPath.ts (1)
536-543:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
VOLUME_NEW_IMAGEwhen max slice count changes.At Line 536, event emission is gated only by
imageIdIndexChanged. IfmaxImageIdIndexChangedoccurs alone, listeners miss the updatednumberOfSliceseven though the payload now carries it.Suggested fix
- if (imageIdIndexChanged) { + if (imageIdIndexChanged || maxImageIdIndexChanged) { triggerPlanarVolumeNewImage(ctx, { camera, acquisitionOrientation: rendering.acquisitionOrientation, imageIds: rendering.imageIds, imageIdIndex: rendering.currentImageIdIndex, maxImageIdIndex: rendering.maxImageIdIndex, }); }🤖 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/RenderingEngine/GenericViewport/Planar/CpuVolumeSliceRenderPath.ts` around lines 536 - 543, The triggerPlanarVolumeNewImage event emission is gated only by the imageIdIndexChanged condition, which means listeners will miss updates when maxImageIdIndexChanged occurs independently. Update the if condition to emit the event when either imageIdIndexChanged OR maxImageIdIndexChanged is true, ensuring that listeners receive the updated numberOfSlices payload whenever the max slice count changes.packages/tools/src/tools/CrosshairsTool.ts (1)
369-423:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSuppress camera-event reentrancy during generic reset.
The generic reset path calls
resetViewStatewithout an event-suppression guard, while the legacy path usessuppressEvents: true. DuringonResetCamera, this can triggeronCameraModifiedmid-reset and cause redundant recompute/render work.Proposed patch
resetCrosshairs = () => { + const previousIgnoreFiredEvents = this._ignoreFiredEvents; + this._ignoreFiredEvents = true; + try { const viewportsInfo = this._getViewportsInfo(); for (const viewportInfo of viewportsInfo) { const { viewportId, renderingEngineId } = viewportInfo; @@ } this._computeToolCenter(viewportsInfo); + } finally { + this._ignoreFiredEvents = previousIgnoreFiredEvents; + } };🤖 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/CrosshairsTool.ts` around lines 369 - 423, The resetCrosshairs method has inconsistent event suppression between the generic and legacy viewport paths. The generic viewport branch (inside the `if (csUtils.isGenericViewport(viewport))` block) calls `resetViewState` without suppressing events, while the legacy path passes `suppressEvents: true` to `resetCamera`. Add the suppressEvents option to the `resetViewState` call in the generic viewport path to prevent reentrancy issues where onResetCamera triggers onCameraModified during reset.packages/tools/src/tools/displayTools/Contour/contourHandler/handleContourSegmentation.ts (1)
58-67:⚠️ Potential issue | 🔴 CriticalReplace unsafe StackViewport cast with viewport-agnostic image reference resolution.
Line 63 uses
getClosestImageIdForStackViewport(viewport as StackViewport, ...)despite the function parameter acceptingStackViewport | Types.IVolumeViewport. WhenhandleContourSegmentationis called with an IVolumeViewport (e.g., from PLANAR_NEXT rendering), this cast bypasses the viewport contract and produces missing or incorrect image references.Use the viewport type-checking pattern from
annotationHydration.ts(lines 59–74): conditionally callgetClosestImageIdForStackViewportfor StackViewport orutilities.getClosestImageIdfor volume viewports.🤖 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/displayTools/Contour/contourHandler/handleContourSegmentation.ts` around lines 58 - 67, The unsafe StackViewport cast in the getClosestImageIdForStackViewport call on line 63 bypasses type safety and causes incorrect behavior when called with IVolumeViewport instances. Replace the unsafe cast with a viewport type-checking pattern that conditionally calls getClosestImageIdForStackViewport when the viewport is a StackViewport, or calls utilities.getClosestImageId when the viewport is an IVolumeViewport. Check the viewport type before the forEach loop using instanceof or a similar type-guard pattern, then use the appropriate function based on the viewport type to resolve the image reference correctly.packages/tools/src/tools/MagnifyTool.ts (1)
50-63:⚠️ Potential issue | 🟠 MajorWiden
_getReferencedImageIdto the admitted viewport contract.
preMouseDownCallbacknow permits generic viewports via the guard at lines 75-78, but_getReferencedImageIdstill declares a narrower parameter type and returns onlystringdespite the generic lookup being optional. The method must acceptTypes.IViewportand returnstring | undefinedto align the TypeScript signature with the implementation's actual behavior and the caller's expectations at line 84.Proposed fix
_getReferencedImageId( - viewport: Types.IStackViewport | Types.IVolumeViewport - ): string { + viewport: Types.IViewport + ): string | undefined { const targetId = this.getTargetId(viewport); - let referencedImageId; + let referencedImageId: string | undefined;🤖 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/MagnifyTool.ts` around lines 50 - 63, The `_getReferencedImageId` method has a parameter type that is too narrow and a return type that does not match its actual behavior. Change the parameter type from `Types.IStackViewport | Types.IVolumeViewport` to the broader `Types.IViewport` to accept generic viewports (aligning with the guard check at lines 75-78 in preMouseDownCallback). Additionally, change the return type from `string` to `string | undefined` because the method uses optional chaining with `getCurrentImageId?.()` for generic viewports, which can return undefined, and this must match the caller's expectations at line 84.
🧹 Nitpick comments (1)
packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportTypes.ts (1)
76-83: ⚡ Quick winAlign the
forceCPUdocs with the new image-path decision.
shouldUseCPUForImagenow ignores image byte-size thresholds and only useswebGLAvailable === falseor the global CPU setting, so this comment still over-promises that the “CPU thresholds above” participate whenforceCPUis omitted.Suggested wording
- * configuration, WebGL availability, and the CPU thresholds above. + * configuration and WebGL availability. Size-based CPU thresholds apply only + * to volume-backed render paths.🤖 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/RenderingEngine/GenericViewport/Planar/PlanarViewportTypes.ts` around lines 76 - 83, Update the JSDoc comment for the forceCPU property to accurately reflect the current image-path decision logic. Remove the reference to "CPU thresholds above" from the comment since shouldUseCPUForImage no longer considers image byte-size thresholds. The updated documentation should clarify that when forceCPU is omitted, the render path is decided only by global rendering configuration and WebGL availability, not by CPU thresholds.
🤖 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/core/src/RenderingEngine/GenericViewport/Planar/planarImageEvents.ts`:
- Around line 66-68: The imageIndex property in the VOLUME_NEW_IMAGE event
triggered in the triggerEvent call is using a null coalescing operator to
default to 0 when params.imageIdIndex is unknown, which incorrectly snaps
volume-aware listeners to the first slice. Fix this by removing the default
fallback so that imageIndex is only set to a defined value when
params.imageIdIndex actually has a known value, allowing listeners to properly
handle unknown indices instead of treating them as the first slice.
In `@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts`:
- Around line 763-765: The getNumberOfSlices() method currently returns 1 for an
empty viewport when it should return 0. This happens because when getImageIds()
is empty (length is 0), getMaxImageIdIndex() falls back to 0, and Math.max(0, 1)
returns 1. Fix this by adding an early return statement that checks if
getImageIds().length is 0 and returns 0 immediately, before executing the
Math.max calculation. This ensures empty viewports correctly report zero slices
for navigation and prefetch operations.
- Around line 692-714: The code retrieves displaySetId and its presentation at
the beginning but ignores the displaySetId when emitting VOI_MODIFIED and
COLORMAP_MODIFIED events. Instead of using the global volumeId from
this.getVolumeId(), replace all volumeId references in both event triggerEvent
calls with the volume ID specific to the displaySetId being modified. Determine
the correct method to get the display-set-specific volume ID (likely
this.getVolumeId(displaySetId) or extracting it from the presentation object)
and use that in place of the current this.getVolumeId() calls to ensure the
events carry the correct actor reference for the modified display set.
In `@packages/core/src/utilities/isPTPrescaledWithSUV.ts`:
- Line 4: The return statement in the isPTPrescaledWithSUV function currently
returns a truthy/falsy value from the && operator instead of an explicit
boolean. Modify the return statement to explicitly coerce the result to a strict
boolean value using the !! operator or the Boolean() function, ensuring the
predicate returns true or false rather than potentially returning the suvbw
value or false when the conditions are evaluated.
In `@packages/tools/examples/genericViewportShowcase/index.ts`:
- Around line 252-260: The `onSelectedValueChange` callback handler and similar
toolbar handlers at lines 279, 317, 339, and 357 assume that
ToolGroupManager.getToolGroup(acqToolGroupId) returns a valid tool group
instance. If a user clicks before the `run()` function completes initialization,
the toolGroup could be null or undefined, causing errors when calling methods
like setToolPassive() and setToolActive(). Add guard checks at the beginning of
each affected handler callback to verify that the toolGroup exists and is
properly initialized before attempting to call methods on it. If the toolGroup
is not yet available, return early from the handler to prevent null reference
errors.
- Line 549: The `run()` function call on line 549 is not handling promise
rejections, which causes initialization and network failures to be silently
ignored. Add a `.catch()` handler to the `run()` call to properly catch any
rejected promises and log the error details. This ensures that bootstrap
failures are visible and can be diagnosed instead of being silently dropped.
- Around line 96-103: The code retrieves the content element using
document.getElementById but does not check if it exists before calling
appendChild on it. Add a defensive null check after the getElementById call to
verify that the content element was found in the DOM before attempting to append
the grid to it, and either skip the grid creation or provide an appropriate
fallback behavior if the content container is missing.
In `@packages/tools/src/tools/MagnifyTool.ts`:
- Around line 140-144: The viewportProperties obtained from
getDisplaySetPresentation() contain the property voiLUTFunction (lowercase), but
the legacy StackViewport's setProperties() method expects VOILUTFunction
(uppercase). Before using viewportProperties with setProperties() in the
MagnifyTool.ts magnify viewport, add a mapping function that transforms the
generic property name voiLUTFunction to the legacy property name VOILUTFunction.
Apply the same mapping transformation in AdvancedMagnifyTool.ts where similar
property handling occurs around lines 1763-1785.
In `@packages/tools/src/tools/ReferenceCursors.ts`:
- Around line 523-528: The updateViewportImage method signature declares a
parameter type of IStackViewport or IVolumeViewport, but the implementation
includes a branch for generic viewports (the isGenericViewport check) that
forces an unnecessary cast at line 528. Update the updateViewportImage method
signature to accept Types.IViewport as the viewport parameter type instead of
the union of IStackViewport and IVolumeViewport. This broader type accurately
reflects the method's actual contract since all viewport types derive from
IViewport, and it will eliminate the workaround cast currently needed to handle
the generic viewport case.
In `@packages/tools/src/tools/VolumeCroppingControlTool.ts`:
- Around line 685-688: The condition checking enabledElement.viewport.getCamera
is a legacy gate preventing orientation computation on generic/native viewports,
leaving orientation null and causing annotation filtering failures. Remove the
getCamera check from the conditional at line 685 by deleting the &&
enabledElement.viewport.getCamera clause, keeping only the
enabledElement.viewport check so that the orientation assignment with
getOrientationFromNormal and getViewportICamera executes for all viewport types.
---
Outside diff comments:
In
`@packages/core/src/RenderingEngine/GenericViewport/Planar/CpuVolumeSliceRenderPath.ts`:
- Around line 536-543: The triggerPlanarVolumeNewImage event emission is gated
only by the imageIdIndexChanged condition, which means listeners will miss
updates when maxImageIdIndexChanged occurs independently. Update the if
condition to emit the event when either imageIdIndexChanged OR
maxImageIdIndexChanged is true, ensuring that listeners receive the updated
numberOfSlices payload whenever the max slice count changes.
In
`@packages/core/src/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.ts`:
- Around line 247-254: The guard condition for triggering the planar volume new
image event only checks imageIdIndexChanged but ignores maxImageIdIndexChanged,
which causes the numberOfSlices to become stale when only the maximum index
changes. Modify the if condition at line 247 to include maxImageIdIndexChanged
in the guard, so the triggerPlanarVolumeNewImage call is executed whenever
either imageIdIndexChanged or maxImageIdIndexChanged is true, ensuring consumers
receive the updated slice information in both cases.
In `@packages/tools/src/tools/CrosshairsTool.ts`:
- Around line 369-423: The resetCrosshairs method has inconsistent event
suppression between the generic and legacy viewport paths. The generic viewport
branch (inside the `if (csUtils.isGenericViewport(viewport))` block) calls
`resetViewState` without suppressing events, while the legacy path passes
`suppressEvents: true` to `resetCamera`. Add the suppressEvents option to the
`resetViewState` call in the generic viewport path to prevent reentrancy issues
where onResetCamera triggers onCameraModified during reset.
In
`@packages/tools/src/tools/displayTools/Contour/contourHandler/handleContourSegmentation.ts`:
- Around line 58-67: The unsafe StackViewport cast in the
getClosestImageIdForStackViewport call on line 63 bypasses type safety and
causes incorrect behavior when called with IVolumeViewport instances. Replace
the unsafe cast with a viewport type-checking pattern that conditionally calls
getClosestImageIdForStackViewport when the viewport is a StackViewport, or calls
utilities.getClosestImageId when the viewport is an IVolumeViewport. Check the
viewport type before the forEach loop using instanceof or a similar type-guard
pattern, then use the appropriate function based on the viewport type to resolve
the image reference correctly.
In `@packages/tools/src/tools/MagnifyTool.ts`:
- Around line 50-63: The `_getReferencedImageId` method has a parameter type
that is too narrow and a return type that does not match its actual behavior.
Change the parameter type from `Types.IStackViewport | Types.IVolumeViewport` to
the broader `Types.IViewport` to accept generic viewports (aligning with the
guard check at lines 75-78 in preMouseDownCallback). Additionally, change the
return type from `string` to `string | undefined` because the method uses
optional chaining with `getCurrentImageId?.()` for generic viewports, which can
return undefined, and this must match the caller's expectations at line 84.
---
Nitpick comments:
In
`@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportTypes.ts`:
- Around line 76-83: Update the JSDoc comment for the forceCPU property to
accurately reflect the current image-path decision logic. Remove the reference
to "CPU thresholds above" from the comment since shouldUseCPUForImage no longer
considers image byte-size thresholds. The updated documentation should clarify
that when forceCPU is omitted, the render path is decided only by global
rendering configuration and WebGL availability, not by CPU thresholds.
🪄 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: 88ca1336-12a4-4a7d-a37b-0132a6a8bbc3
📒 Files selected for processing (44)
packages/core/src/RenderingEngine/ContextPoolRenderingEngine.tspackages/core/src/RenderingEngine/GenericViewport/GenericViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/CpuImageSliceRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/CpuVolumeSliceRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarRenderPathDecisionService.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportTypes.tspackages/core/src/RenderingEngine/GenericViewport/Planar/VtkImageMapperRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarImageEvents.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/viewport3D.tspackages/core/src/RenderingEngine/helpers/planarImageRendering.tspackages/core/src/RenderingEngine/helpers/stats/RenderModesPanel.tspackages/core/src/RenderingEngine/helpers/stats/StatsOverlay.tspackages/core/src/types/IEnabledElement.tspackages/core/src/types/IGenericViewport.tspackages/core/src/types/index.tspackages/core/src/utilities/index.tspackages/core/src/utilities/isPTPrescaledWithSUV.tspackages/core/src/utilities/viewportCapabilities.tspackages/core/test/viewportContentMode.jest.jspackages/tools/examples/genericViewportShowcase/index.tspackages/tools/src/synchronizers/callbacks/cameraSyncCallback.tspackages/tools/src/synchronizers/callbacks/imageSliceSyncCallback.tspackages/tools/src/synchronizers/callbacks/voiSyncCallback.tspackages/tools/src/tools/AdvancedMagnifyTool.tspackages/tools/src/tools/CrosshairsTool.tspackages/tools/src/tools/MagnifyTool.tspackages/tools/src/tools/OrientationControllerTool.tspackages/tools/src/tools/OverlayGridTool.tspackages/tools/src/tools/ReferenceCursors.tspackages/tools/src/tools/ReferenceLinesTool.tspackages/tools/src/tools/ScaleOverlayTool.tspackages/tools/src/tools/TrackballRotateTool.tspackages/tools/src/tools/VolumeCroppingControlTool.tspackages/tools/src/tools/VolumeCroppingTool.tspackages/tools/src/tools/VolumeRotateTool.tspackages/tools/src/tools/WindowLevelRegionTool.tspackages/tools/src/tools/annotation/ETDRSGridTool.tspackages/tools/src/tools/displayTools/Contour/contourDisplay.tspackages/tools/src/tools/displayTools/Contour/contourHandler/handleContourSegmentation.tspackages/tools/src/utilities/cine/playClip.tspackages/tools/src/utilities/setViewportCamera.tspackages/tools/src/utilities/stackPrefetch/stackPrefetchUtils.ts
…support - planarImageEvents: only emit VOLUME_NEW_IMAGE when the slice index is known instead of synthesizing index 0 - PlanarViewport: emit VOI/COLORMAP events with the modified display set's own volumeId; getNumberOfSlices returns 0 for an empty viewport - isPTPrescaledWithSUV: return a strict boolean - tools: extract shared generic-viewport helpers (slab thickness, jumpToFocalPoint, native source properties) and bridge voiLUTFunction casing so the magnify loupe honors a sigmoid VOI - viewport3D: honor reset options in resetViewState - ReferenceCursors: widen updateViewportImage to IViewport and drop the cast - VolumeCroppingControlTool: resolve orientation on native viewports - sync/cine/prefetch: capability-based guards for generic viewports
The callback now consults utilities.viewportIsInVolumeMode for native generic viewports; the test's @cornerstonejs/core mock lacked it, throwing "viewportIsInVolumeMode is not a function". The fixtures are plain stack/volume mocks (no getCurrentMode), so it reports non-volume mode and detection falls through to the existing renderMode check.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tools/src/tools/MagnifyTool.ts (1)
74-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message is now misleading.
The guard now accepts both
StackViewportand generic viewports, but the error message still says "MagnifyTool only works on StackViewports". Consider updating to reflect the expanded support.Proposed fix
if ( !(viewport instanceof StackViewport) && !csUtils.isGenericViewport(viewport) ) { - throw new Error('MagnifyTool only works on StackViewports'); + throw new Error('MagnifyTool only works on Stack or native planar viewports'); }🤖 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/MagnifyTool.ts` around lines 74 - 79, The error message in the viewport validation guard in MagnifyTool is now inaccurate because the condition has been expanded to accept both StackViewport instances and generic viewports (checked via csUtils.isGenericViewport). Update the error message string to reflect that MagnifyTool now works on both StackViewports and generic viewports, ensuring the message accurately describes the actual viewport requirements enforced by the guard condition.
🤖 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.
Outside diff comments:
In `@packages/tools/src/tools/MagnifyTool.ts`:
- Around line 74-79: The error message in the viewport validation guard in
MagnifyTool is now inaccurate because the condition has been expanded to accept
both StackViewport instances and generic viewports (checked via
csUtils.isGenericViewport). Update the error message string to reflect that
MagnifyTool now works on both StackViewports and generic viewports, ensuring the
message accurately describes the actual viewport requirements enforced by the
guard condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4510b371-296b-4848-9462-11fa735ad824
📒 Files selected for processing (14)
packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarImageEvents.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/viewport3D.tspackages/core/src/utilities/isPTPrescaledWithSUV.tspackages/tools/src/synchronizers/callbacks/cameraSyncCallback.tspackages/tools/src/tools/AdvancedMagnifyTool.tspackages/tools/src/tools/CrosshairsTool.tspackages/tools/src/tools/MagnifyTool.tspackages/tools/src/tools/ReferenceCursors.tspackages/tools/src/tools/VolumeCroppingControlTool.tspackages/tools/src/utilities/cine/playClip.tspackages/tools/src/utilities/genericViewportToolHelpers.tspackages/tools/src/utilities/index.tspackages/tools/src/utilities/stackPrefetch/stackPrefetchUtils.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/utilities/isPTPrescaledWithSUV.ts
- packages/tools/src/synchronizers/callbacks/cameraSyncCallback.ts
- packages/tools/src/utilities/stackPrefetch/stackPrefetchUtils.ts
- packages/tools/src/utilities/cine/playClip.ts
- packages/tools/src/tools/VolumeCroppingControlTool.ts
- packages/tools/src/tools/CrosshairsTool.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts
…action The prior next-viewport fix bridged the VOI read/write via the display-set presentation, but extractWindowLevelRegionToolData still threw 'Viewport not supported' for native PLANAR_NEXT viewports (neither StackViewport nor VolumeViewport), so the region tool failed before reaching that bridge. Add a generic-viewport branch that reads the displayed slice from the cornerstone cache via getCurrentImageId(), returning the same shape the stack extractor produces (scalarData, width/height, min/max, rows/columns, color). Also add getCurrentImageId() to IGenericViewport so the new branch narrows via isGenericViewport instead of an inline cast.
…ray log The storeScalarData parameter was documented as caching the expanded scalar data but only console.logged a debug string and discarded it. Implement the documented behavior (retain on this.scalarData) and remove the stray log. No current caller passes true, so the default path is unchanged.
…icViewport narrowing Eliminates the recurring inline intersection-cast anti-pattern on viewports across the next-viewport accessor cluster by fixing the root cause: the methods the call sites probed are now declared on IGenericViewport (or a focused capability guard) so sites narrow via isGenericViewport instead of casting and degrading silently on rename. Interface/guards: - IGenericViewport: add setViewReference + getResolvedView (both universal via base GenericViewport) and the 2-arg setDisplaySetPresentation overload. - viewportCapabilities: add viewportSupportsDisplaySetPresentation guard + DisplaySetPresentationViewport type for the PlanarViewport-only getSourceDataId / getDefaultVOIRange (focused guard, not weakening IGenericViewport). Sites converted (no behavior change, tsc + runtime verified): - getSubPixelSpacingAndXYDirections -> getViewportContentMode util - CrosshairsTool resetViewState -> direct call after isGenericViewport - ViewportColorbar -> setDisplaySetPresentation/render after narrowing - genericViewportToolHelpers (jumpToFocalPoint, getNativeSourceProperties) - getViewportICamera (the getCamera bridge) -> getResolvedView via narrowing - WindowLevelRegionTool / LivewireContourTool -> viewportSupportsDisplaySetPresentation Remaining cast sites (rendering-engine lifecycle hooks, actor-accessor bundles, setViewportCamera's multi-method zoom path, debug fields) deferred as a focused follow-up; documented in the task.
…arity Native PLANAR_NEXT viewports applied a freshly-mounted display set's default VOI to the actor but emitted no VOI_MODIFIED event: addDisplaySetInternal goes through setDefaultDataPresentation -> setDataPresentationState, which (unlike the public mergeDataPresentation path) does not notify. Legacy Stack/Volume viewports emit VOI_MODIFIED at load, so OHIF consumers that key off that event (colorbar, VOI synchronizers, window-level sliders) went stale on a display-set change for next viewports. Emit the effective VOI (explicit override or computed default) per binding after the data is mounted, mirroring the existing resetDisplaySetPresentation precedent. Fires once per binding (source and each overlay) with that binding's own volumeId resolved internally; guarded so nothing is emitted when no range is available, and suppressed under suppressEvents.
…gacy
The native image render path (getDefaultImageVOIRange) used only the image
DICOM window center/width, so a prescaled PT (SUV) series rendered as a stack
got a far wider default window than legacy. Legacy StackViewport overrides the
WC/WW with a 0-5 range for prescaled PT (_getPTPreScaledRange); the volume path
(handlePreScaledVolume) already did the same. Only the native image path was
missing it, so e.g. a PET AC series showed W:11 L:5 instead of legacy W:6 L:3.
Return {lower:0, upper:5} when isPTPrescaledWithSUV(image), reusing the existing
helper (keyed off the loader-set preScale.scaled/suvbw fields, not image.isPreScaled
which the native path never sets). Covers GPU and CPU image paths, mount and scroll.
… volume mount On a stack->MPR (volume) mount the scrollbar source (rendering.currentImageIdIndex) was seeded from the raw initialImageIdIndex (0) and emitted via VOLUME_NEW_IMAGE before the camera projection ran, while the camera renders the centered/reference slice. The scrollbar reported the first slice while a different slice was shown, and scroll math relative to the stale index returned the old carried slice (perceived as a prepended first slice). Reconcile currentImageIdIndex/maxImageIdIndex with the camera-projected slice (resolvePlanarRenderPathProjection) before emitting the first VOLUME_NEW_IMAGE, so the scrollbar matches the rendered slice at mount — a single source of truth, mirroring legacy getSliceIndex (camera-derived).
…arity) getActiveImageIdIndex returned the stored rendering.currentImageIdIndex scalar, which tracks the camera on render but can lag a post-mount camera carry (e.g. the layout-selector MPR protocol restoring the prior stack slice via setViewReference). That decoupled the scrollbar/scroll index from the rendered slice: the camera showed the carried slice while the index stayed at the mount value, so scrolling moved relative to the wrong base. Derive the index from the resolved view (the current camera/projection) for volume-slice content, mirroring legacy VolumeViewport.getSliceIndex (camera focal point as the single source of truth); fall back to the stored scalar otherwise. Validated: normal scroll unchanged, camera carry now reflected in the index.
createInitialVolumeSliceState pinned the ACQUISITION orientation to planarData.initialImageIdIndex, which defaults to 0 when no slice is carried (e.g. the MPR hanging protocol supplies none). So a stack->MPR mounted the acquisition (axial) viewport at slice 0 with the scrollbar at the top, while the reformatted (sagittal/coronal) orientations and the legacy VolumeViewport both center the volume. Map a defaulted 0 to 'center' (|| undefined) while still honoring a real carried slice (> 0). Validated on 3001: mpr-axial now mounts at the center (147/295, matching legacy) with index/viewReference/resolved view all consistent, and scroll tracks correctly.
# Conflicts: # packages/dicomImageLoader/src/imageLoader/getScalingParameters.ts
… unit tests The mount-time resolvePlanarRenderPathProjection call in the volume render paths' addData invoked ctx.viewport.isCurrentDataId before the viewport is fully wired (failing planarCpuVolumeRenderPath.jest.js: 'isCurrentDataId is not a function'). It was redundant: the acquisition-centering fix sets the initial viewState and getActiveImageIdIndex derives the index from the resolved view, so the scrollbar already tracks the rendered slice without seeding the index at addData. Remove it from both VtkVolumeSliceRenderPath and CpuVolumeSliceRenderPath.
| } | ||
|
|
||
| triggerPlanarNewImage(ctx); | ||
| // A volume-backed slice change must also emit VOLUME_NEW_IMAGE so that |
Collaborator
There was a problem hiding this comment.
Probably we should deprecate both volume and stack new image in favour of viewport new image, but keep on firing the old ones for a while.
Member
Author
There was a problem hiding this comment.
Agreed, that's the right long-term direction: a single VIEWPORT_NEW_IMAGE that supersedes STACK_NEW_IMAGE / VOLUME_NEW_IMAGE, with both legacy events kept firing through a deprecation window so existing consumers (the OHIF scrollbar, synchronizers, stack prefetch, etc.) keep working. I'd rather land that as its own focused PR than fold it into this one since it touches event consumers across core, tools, and downstream. Tracking it as a follow-up.
wayfarer3130
left a comment
wayfarer3130
left a comment
Collaborator
There was a problem hiding this comment.
Automated review pass (correctness-focused). These are findings not already covered by the existing CodeRabbit review/a5492df fixes. Each was verified against the current head. Cleanup/duplication nits (parallelScale reconstruction across the magnify tools, dataId→volumeId actor resolution shared between getViewportModality/isViewportPreScaled) are omitted here to keep signal high.
Comment on lines
+471
to
+473
| const incomingComponents = image.color ? 3 : 1; | ||
|
|
||
| return scalars.getNumberOfComponents() === incomingComponents; |
Collaborator
There was a problem hiding this comment.
Correctness (perf) — canReuseImageDataForImage assumes 3 components for every color image, so RGBA images never reuse their texture.
image.color ? 3 : 1 yields 3 for a 4-component RGBA image, while the existing vtkImageData reports 4 components, so this equality always fails and a fresh vtkImageData/texture is allocated on every scroll — reintroducing the one-frame black flash this function exists to prevent for RGBA ultrasound / secondary-capture stacks.
Derive the component count from the image (e.g. image.rgba ? 4 : image.color ? 3 : 1, or from the actual pixel-data layout) instead of hard-coding 3 for all color.
(The getDataType() vs constructor.name compare a few lines up is fine — VTK's data-type strings match the TypedArray constructor names.)
Member
Author
There was a problem hiding this comment.
Leaving this as image.color ? 3 : 1 (comment clarified in c939187). In this render path a color image's vtkImageData is always built with 3 components: getImageDataMetadata derives numberOfComponents from the photometric interpretation (RGB/YBR -> 3), and updateVTKImageDataWithCornerstoneImage down-converts an RGBA (4-component) source to RGB in place. So the post-transform component count is 3 for color and 1 otherwise, and color ? 3 : 1 matches what the imageData is actually built with.
Using image.rgba (or a literal 4) would mis-report a freshly loaded RGBA frame (rgba still true, numberOfComponents unset) as 4, never match the 3-component imageData, and force a fresh vtkImageData on every scroll - reintroducing the one-frame black flash this reuse path exists to prevent. Verified on a 94-frame RGB ultrasound: the scalars are 3-component and reuse stays on the fast path.
Comment on lines
+441
to
+451
| if (voiRange) { | ||
| this.notifyDataPresentationModified(dataId, { voiRange }); | ||
| } | ||
|
|
||
| if (role === 'source') { | ||
| // Emit an initial CAMERA_MODIFIED for the source binding only, mirroring | ||
| // legacy StackViewport's first-image triggerCameraEvent. Overlays and | ||
| // orientation markers paint their first frame off this event. Trigger the | ||
| // event directly rather than routing through modified() to avoid a | ||
| // redundant render (addLoadedData already rendered). | ||
| this.triggerCameraModifiedEvent(this.getCameraForEvent()); |
Collaborator
There was a problem hiding this comment.
Correctness — mount-time VOI_MODIFIED/CAMERA_MODIFIED can reset or echo across synchronizer groups.
addLoadedData now fires notifyDataPresentationModified (→ VOI_MODIFIED) per binding and triggerCameraModifiedEvent for the source on every mount. For a PLANAR_NEXT viewport in a camera or VOI sync group, mounting/replacing one display set drives the sync callback, which can copy this freshly-centered camera / default VOI onto the other viewports in the group (overwriting a slice/zoom the user already set), or feed a VOI_MODIFIED echo back into the synchronizer.
The per-display-set volumeId scoping here is already correct (fixed in a5492df); the remaining concern is the unconditional mount-time emission itself. Consider suppressing these during the mount/replace path, or guarding the synchronizer against self-originated mount events.
Member
Author
There was a problem hiding this comment.
I believe this matches legacy behavior rather than introducing a new reset/echo: legacy StackViewport.setStack emits VOI_MODIFIED on mount (via setVOI) and the first-image CAMERA_MODIFIED, and the native emission goes through notifyDataPresentationModified, which honors suppressEvents the same way. The VOI/camera synchronizers already have echo-suppression, and the per-binding volumeId scoping was fixed in a5492df.
So I'd prefer not to blanket-suppress the mount emission, since OHIF relies on it to refresh the W/L readout, colorbar, and VOI/camera sync on a display-set change. I'll verify the specific synced-group scenario (load a series into one pane of a synced group and confirm it doesn't reset the others' slice/VOI any differently than legacy) and narrow the emission only if there's a real divergence. Happy to take a concrete repro if you have one.
Collaborator
|
Generally I'm happy with the PR, but some repeated code bothered me, and also some of hte prescaling handling. I worked with claude to generate an analysis of that which is below: How much of the PR is prescaling CpuImageSliceRenderPath.ts — markCpuImagePreScaled (sets image.isPreScaled = preScale?.scaled) called at two render sites, so the CPUFallback skips the modality LUT. Would a single Uint16 raw→display LUT simplify it?
GPU path isn't an indexed LUT. Volumes and the new PlanarViewport VTK image mapper use a vtkColorTransferFunction over a continuous scalar range, not a 64K-entry index array. You'd fold the affine modality transform into the TF domain instead — fine for affine, but it's a different mechanism, so "one Uint16 LUT everywhere" isn't achievable; you'd have two composing strategies. The bigger win — the one that kills most of this PR's special-casing regardless of bake-vs-LUT — is a single scaling descriptor on the image/viewport: { rescaleSlope, rescaleIntercept, suvbw, modality, units } exposed uniformly by both StackViewport and PlanarViewport. Rendering reads it to build the composite LUT or TF; tools read it to convert raw→modality. That alone collapses markCpuImagePreScaled, the WindowLevelTool dual-detection, the isViewportPreScaled/getViewportModality per-family branches, and the scattered isPTPrescaledWithSUV checks into one source of truth — which is ~80% of the prescale pain in this PR. Whether you then also drop baking in favor of always-LUT is a separable, smaller decision. If it's useful I can sketch what that descriptor + the two render-side consumers (CPU composite LUT, GPU TF folding) would look like, scoped to where this PR already touches. |
getScalarData(true) caches the expanded scalar data (e.g. an RLE decode) so later reads skip re-expansion. A subsequent voxel write went to the backing store but not the cache, so getScalarData() could return stale data. Track the cached expansion with a flag and drop it on setAtIJK/setAtIndex writes; a real backing store (constructor / setScalarData / _updateScalarData) is left untouched, so this is a one-boolean-check no-op on the hot path. Addresses review feedback on VoxelManager.ts.
…nd on reformatted planes - WindowLevelRegionTool: when a presentation (native) viewport has no resolvable source data id, apply the computed VOI via the single-arg setDisplaySetPresentation (targets the current binding) instead of silently dropping it. - extractWindowLevelRegionToolData: a volume-mode native viewport on a reformatted (sagittal/coronal/oblique) plane has no single backing image; degrade gracefully (warn + return undefined, with a caller guard) instead of throwing and aborting the tool interaction. Full reformatted-slice support is a follow-up. Addresses review feedback on WindowLevelRegionTool.ts and extractWindowLevelRegionToolData.ts.
…eometry - Stop defaulting initialImageIdIndex to 0 in DefaultPlanarDataProvider; carry 'no slice requested' as undefined through PlanarPayload so the volume acquisition view centers, while an explicit index (including 0) is honored instead of being collapsed to center. Render-path seeds default the placeholder index with ?? 0. - Refresh a reused vtkImageData's origin/direction/spacing on the scroll path (updateVTKImageDataGeometryFromImage). Multi-frame stacks (e.g. ultrasound cine) place each frame at a distinct world Z and the camera follows that plane on scroll; without this the reused actor stayed on the first frame's plane while the camera moved away, leaving the viewport black from the second frame onward. - Clarify canReuseImageDataForImage: a color image's vtkImageData is always 3-component here (metadata + RGBA->RGB down-conversion), so color?3:1 is the correct post-transform count; document why not to use image.rgba / 4. Addresses review feedback on PlanarViewport.ts (slice 0) and VtkImageMapperRenderPath.ts.
…port guard message Accept any IViewport in _getReferencedImageId (called with native generic viewports) and return string | undefined to match its actual behavior. Update the guard error to reflect that the tool now supports native planar viewports in addition to StackViewports.
The notifyDataPresentationModified hook referenced the old DataId type name, which was renamed to DisplaySetId during the DataSet to DisplaySet terminology change merged from main. This broke the ESM build (TS2304), failing the docusaurus-build and OHIF downstream CI jobs.
…modality and scaling state
… stop stack cache corruption on scroll createVTKImageDataFromImage wrapped the source image's voxelManager scalar buffer by reference. The reuse-in-place scroll path (updateVTKImageData- WithCornerstoneImage -> scalarData.set) then overwrote that buffer with the next frame's pixels, corrupting the first image's cached pixel data. Scrolling back to a previously-viewed stack slice rendered the wrong (previous) slice's pixels. Invisible in grayscale (adjacent slices look near-identical) but surfaced as a ~52% pixel diff under a high-contrast colormap (HSV), failing the compat Playwright stackProperties previous-image snapshots. Copy the scalars so the actor's vtkImageData owns a private buffer (mirroring legacy StackViewport, whose actor buffer is independent of the image cache). The no-flash texture-reuse optimization is preserved - only the private buffer's contents are updated in place.
…isters on the self-hosted runner The legacy Playwright lengthTool snapshot captured a half-created measurement (~0.552mm instead of ~138mm): on the loaded self-hosted runner the stepped mousemoves were dropped before the active handle left the start point, so the annotation was finalized with a near-zero length. Increase the post-mousedown handler-attach wait (50->150ms) and the per-step settle (10->30ms) so each move registers under runner load. End point and local behavior unchanged (length tests still pass locally); validated on the runner via CI.
This was referenced Jul 3, 2026
Merged
3 tasks
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.
OHIF_REF: ohifohifnextapi
Context
This PR teaches the legacy
toolspackage and parts ofcoreto operate against the new native GenericViewport ("next") rendering lane, so existing tools and synchronizers work across both the legacy and native viewport families.Changes & Results
Core
isGenericViewport,viewportIsInVolumeMode,getCurrentMode) plus aviewportCapabilitiesutility, replacinginstanceofchecks.voiLUTFunctionplumbed through the planar render paths and presentation props.forceCPUoption for planar render-path selection.setDisplaySets.RenderModesPanel/StatsOverlaynow includeviewportTypefor debugging context.getImageActoradded toPlanarViewport; keep the on-screen canvas visible forVolumeViewport3D.Core — VOI / slice-index parity (recent)
VOI_MODIFIEDwhen a display set is mounted (its default VOI), mirroring legacy stack/volume viewports so OHIF's window-level readout, colorbar, and VOI synchronizers update on a display-set change.0–5VOI on the native image path, matching legacyStackViewport— previously the raw DICOM window center/width was used, rendering PET far too wide.VolumeViewport.getSliceIndex); the scrollbar index is reconciled with the rendered slice on volume mount; and the acquisition orientation centers the volume on MPR mount instead of pinning to the first slice.Tools
getViewportICamera/setViewportCamera/resetViewportCamera) andisGenericViewport-gated branches so the following support native (next) viewports: Crosshairs, Magnify, AdvancedMagnify, ReferenceLines, ReferenceCursors, cineplayClip, 3D VR tools (Trackball/VolumeRotate/VolumeCropping), overlay/region 2D tools, and contour segmentation rendering (CS-8).Examples
genericViewportShowcaseexample demonstrating the native viewport with interactive crosshairs.Testing
viewportContentModejest coverage added.genericViewportShowcaseexample and exercise the supported tools (crosshairs jump, magnify, reference lines, cine) on native viewports.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
forceCPU).threshold; refined render-mode decisions and stats UI (viewport type).