fix(planar): remap initialImageIdIndex to the volume imageId ordering#2799
Merged
Conversation
The volume-slice branch of DefaultPlanarDataProvider builds its payload from imageVolume.imageIds, which createAndCacheVolume sorts by position along the scan axis - commonly the reverse of the registered dataSet ordering the caller computed initialImageIdIndex against. The payload then addressed the mirrored slice (e.g. requesting index 12 of 30 landed on 17). Remap the index through the requested imageId when the volume reorders, so the initial slice is the one the caller asked for. The image branch keeps the dataSet ordering and is unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesPlanar data provider indexing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/DefaultPlanarDataProvider.ts`:
- Around line 68-75: When remapping the initial image index in the volume setup
logic, add a console.warn for the remappedIndex === -1 case before falling back,
including the requested imageId and relevant indices/array context to diagnose
imageId mismatches. Update the block around volumeInitialImageIdIndex and
preserve the existing successful-remap behavior.
🪄 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: cd2e044f-7e27-41dc-a633-f3f47cd15553
📒 Files selected for processing (2)
packages/core/src/RenderingEngine/GenericViewport/Planar/DefaultPlanarDataProvider.tspackages/core/test/defaultPlanarDataProvider.jest.js
sedghi
added a commit
to OHIF/Viewers
that referenced
this pull request
Jul 10, 2026
Picks up the planar initial-slice remap fix (cornerstonejs/cornerstone3D#2799): opening a SEG display set now lands on the first segmented slice instead of its mirror when the cached volume reverses the imageId ordering.
2 tasks
wayfarer3130
added a commit
to OHIF/Viewers
that referenced
this pull request
Jul 11, 2026
* feat(core): add appConfig.useNextViewports flag (Generic Viewport M0 step 1)
Opt-in flag to drive viewports through the DIRECT native cornerstone3D
GenericViewport ("next") API surface (PLANAR_NEXT / VOLUME_3D_NEXT, setDisplaySets,
setDisplaySetPresentation, setViewState, view references) instead of the legacy
stack/volume methods.
Distinct from (and overrides) useGenericViewport, which only routes legacy
viewport types through cornerstone compatibility adapters. This flag does NOT set
cornerstone rendering.useGenericViewport; it is read by getCornerstoneViewportType
and the CornerstoneViewportService backend split (subsequent M0 steps). Defaults
false; the legacy path stays byte-identical. Opt-in only.
* feat(cornerstone): map viewport types to native *_NEXT under useNextViewports (M0 step 2)
getCornerstoneViewportType gains an optional useNextViewports param. When set,
stack/volume/orthographic collapse to PLANAR_NEXT (render path inferred from data
shape), and volume3d/video/wholeslide/ecg map to their VOLUME_3D_NEXT / VIDEO_NEXT /
WHOLE_SLIDE_NEXT / ECG_NEXT types. Defaults false → legacy mapping byte-identical;
no caller passes true yet (wired via appConfig in the service split, step 3).
Tests: +6 cases (21 total) covering the *_NEXT mapping, displaySet override, the
invalid-type throw, and that the legacy mapping is unchanged when the flag is off.
* feat(cornerstone): native-next stack mount behind useNextViewports (M0 step 3 foundation)
Wires the useNextViewports flag through and mounts stack viewports natively:
- nextViewports.ts: module accessor; init.tsx captures appConfig.useNextViewports.
- getCornerstoneViewportType: defaults the flag from the accessor, and now passes
native (*_NEXT) types through idempotently (a viewport's stored cs type is
re-fed into the mapper; legacy types were already idempotent, native were not).
- CornerstoneViewportService._setDisplaySets: route native generic viewports by
data shape (StackData vs VolumeData), since PLANAR_NEXT is one type for both.
- _setStackViewport: native branch mounts via genericViewportDataSetMetadataProvider.add
+ setDisplaySets, applies VOI/colormap via setDisplaySetPresentation and
displayArea/rotation/flip via setViewState (no legacy setStack/setProperties/setCamera).
Validated in a running OHIF (linked cornerstone 5.0.8): with the flag on, the
viewer creates a native PlanarViewport (window.cornerstone...getViewports()[0] ->
'PlanarViewport :: type=planarNext'); the prior 'Invalid viewport type: planarNext'
is resolved. Flag OFF is byte-identical (native branches gated by isGenericViewport).
KNOWN WIP (next): flag-ON full render is blocked by the presentation-read seam —
peripheral consumers (useViewportRendering, overlays, colorbar, resize) still call
legacy getProperties/getViewPresentation/getCamera on the viewport. Stack render
completes once those are routed through getDisplaySetPresentation/viewportProjection.
* feat(cornerstone): render native Generic (next) stack viewport behind useNextViewports
Makes the flag-on native PLANAR_NEXT stack render end-to-end in OHIF:
- CornerstoneCacheService: resolve the stack-vs-volume data-builder from the
legacy mapping, since native types collapse that distinction into PLANAR_NEXT.
Without this the stack fell through to the 'other' builder and imageIds were
never populated (PlanarViewport threw 'No registered planar dataset metadata').
- ImageOverlayViewerTool: skip overlay rendering when the viewport has no
resolvable view reference yet (native returns falsy until data is bound),
instead of letting getTargetId() throw and kill the route during enable.
- Add getViewportPresentation helpers (getViewportProperties / getViewportCameraState)
bridging legacy getProperties/getCamera and native getDisplaySetPresentation/
getViewState; apply at the toolbar property evaluator, VOI-range init, and the
position/LUT presentation snapshots so reads no longer throw on native viewports.
Validated in a running OHIF (flag on): PlanarViewport(planarNext), 295 slices,
image renders, zero console errors, scroll + setImageIdIndex navigate correctly.
Legacy (flag off) behavior is unchanged. Native volume/MPR is a later increment.
* feat(cornerstone): render native Generic (next) volume/MPR behind useNextViewports
Mounts volumes on a direct PLANAR_NEXT viewport for volume/MPR rendering:
- CornerstoneViewportService.setVolumesForViewport: native branch -> new
_setNativeVolumeDisplaySets. Each base volume is registered with its
already-cached volumeId and bound via setDisplaySets at the viewport's
orientation (first = source, others = overlay); VOI/colormap/invert applied
per-binding via setDisplaySetPresentation(dataId, props). Skips the legacy
setVolumes/setProperties/setPresentations surface a PLANAR_NEXT viewport does
not expose. Cornerstone reuses the OHIF-cached volume (getVolumeId returns the
passed volumeId) and selects the image vs reformatted-volume render path from
the requested orientation.
- useViewportRendering colormap resolver: read via getViewportProperties for
native viewports (getDisplaySetPresentation) instead of getProperties/getActors,
which threw 'Error getting viewport colormap' on native volume.
Validated in a running OHIF (flag on): axial/sagittal/coronal all render in
volume mode, scroll navigates the volume, round-trip volume<->stack switches
getCurrentMode cleanly, zero console errors. Legacy (flag off) unchanged.
* feat(cornerstone): allow setViewportOrientation on native volume viewports
The setViewportOrientation command guarded on isOrthographicViewportType, which
is false for a native PLANAR_NEXT viewport (it reports type=planarNext even when
rendering MPR). Add the content-mode capability guard (csUtils.viewportIsInVolumeMode)
so the MPR orientation toolbar works on native viewports; PlanarViewport.setOrientation
already exists. Legacy behavior is unchanged (viewportIsInVolumeMode is false for
legacy viewports, so the existing isOrthographicViewportType branch still gates them).
Also flips useNextViewports on in the dev default config for local testing of the
native path (NOT for merge; see TODO_BEFORE_MERGE).
* feat: Add temporary support for native GenericViewport ("next") migration
- Introduced a dev-only configuration flag to toggle the native viewport backend.
- Added a toolbar button to switch between legacy and native viewports for debugging.
- Implemented logic to handle image slice data and viewport type detection for native viewports.
- Enhanced viewport service to derive default VOI window/level from DICOM metadata.
- Added utility functions for managing localStorage overrides for viewport settings.
- Marked all temporary changes with comments for easy identification and removal before merging.
* Add plan viewer HTML page for migration master plan display
* feat(cornerstone): fork viewport presentation read/write into the backend (§4.3)
Extends the legacyBackend/nextBackend seam so presentation read/write is forked,
not inline-guarded in the service:
- IViewportBackend gains getPositionPresentation / setPositionPresentation /
setLutPresentation. LegacyViewportBackend keeps the exact legacy logic
(getViewPresentation / setProperties / setViewPresentation) byte-identical;
NextViewportBackend uses the native surface (getViewReference + setViewReference;
setDisplaySetPresentation for VOI/colormap/invert). WITH_ORIENTATION is inlined in
the backends to avoid a backend->service value-import cycle.
- CornerstoneViewportService._getPositionPresentation / _setLutPresentation /
_setPositionPresentation now delegate to this.backend. _getLutPresentation stays
shared (already native-aware via the getViewportProperties bridge).
Effect: setPresentations is now native-safe — it previously threw on a PLANAR_NEXT
viewport (setProperties / setViewPresentation), so the native mount skipped it; the
native path now round-trips presentation cleanly. Native pan/zoom persistence
(viewPresentation is still undefined on native) is a later increment.
Validated both lanes: native PlanarViewport renders + storePresentation/getPresentations/
setPresentations round-trip with no crash; legacy StackViewport byte-identical
round-trip; zero console errors each.
* feat(cornerstone): persist native viewport pan/zoom/rotation/flip via the backend
A PLANAR_NEXT viewport has no getViewPresentation/setViewPresentation, so pan/zoom
previously did not survive navigation/resize/layout on the native path. NextViewportBackend
now snapshots the pan/zoom subset of the semantic view state and restores it:
- getPositionPresentation: snapshots the PlanarViewState pan/zoom fields (displayArea,
anchorWorld, anchorCanvas, scale, scaleMode, rotation, flipHorizontal, flipVertical) via
getViewState() (already deep-cloned + serializable), stored in viewPresentation. Slice and
orientation are intentionally excluded (they ride on the view reference).
- setPositionPresentation: applies the view reference first (slice/orientation), then a partial
setViewState patch with only the pan/zoom subset — the merge preserves slice/orientation, so
the view reference is never clobbered. Stale displayArea is cleared when live anchor/scale
pan/zoom is restored. anchorCanvas is canvas-fractional, so it survives resize without drift.
- _setStackViewport native branch now restores the persisted positionPresentation on mount
(position-only; LUT already applied inline), so a returning stack recovers its camera.
Validated on native: zoom -> snapshot (scale captured) -> reset -> restore (scale back),
slice unchanged, zero errors. Legacy path unchanged. Volume/MPR mount pan/zoom restore is a
follow-up (its native mount helper does not yet thread presentations).
* feat(cornerstone): restore native volume/MPR pan/zoom on mount
Extends native pan/zoom persistence to the volume/MPR mount: the native branch of
setVolumesForViewport now restores the persisted positionPresentation (view reference
+ pan/zoom via the backend) after _setNativeVolumeDisplaySets, mirroring the stack
mount. Position-only (LUT applied per-binding above), native-safe.
Validated on a native sagittal MPR: zoom -> snapshot (scale 1.7) -> reset -> restore
(scale back to 1.7) with orientation (sagittal) and slice (256) preserved; zero errors.
* fix(next): bridge invert/flip/window-level commands for native viewports
Add setViewportProperties/setViewportCameraState write bridges alongside the
existing read bridges, and route invertViewport, flipViewportHorizontal,
flipViewportVertical and setViewportWindowLevel through them.
Direct PLANAR_NEXT viewports have no getCamera/setCamera/getProperties/
setProperties, so these four commands threw on native. The bridges dispatch on
isGenericViewport: native reads/writes via getViewState/setViewState (flip) and
getDisplaySetPresentation/setDisplaySetPresentation (invert/voiRange) on the
active binding; legacy falls through to the identical getCamera/setCamera/
getProperties/setProperties calls, so flag-off stays byte-identical.
Verified live on a native stack viewport: all four now apply (invert
undefined->true, flipH/flipV false->true, voiRange retargets) instead of
throwing.
* fix(next): apply setViewportColormap on native viewports
setViewportColormap was fully guarded by isStackViewportType/
isOrthographicViewportType, both of which report false for native PLANAR_NEXT
viewports, so the command was a silent no-op on native (returned ok but applied
nothing). Add a native branch that applies the colormap via the
setViewportProperties bridge (setDisplaySetPresentation) on the active binding,
honoring the immediate render flag, before the legacy guards.
Verified live: HSV colormap now applies and renders on a native stack viewport.
* fix(next): make ColorbarService native-safe
ColorbarService called getActors/getProperties/setProperties directly, all of
which throw on direct PLANAR_NEXT (next) viewports, so toggling a colorbar threw
on native. Replace the getActors content gate with a viewportHasContent helper
(getCurrentMode for native, getActors for legacy), and route the property
read/write through the getViewportProperties/setViewportProperties bridges.
Legacy keeps the identical getActors/getProperties/setProperties calls.
Verified live on native: addColorbar no longer throws, hasColorbar becomes true,
and the colormap applies via setDisplaySetPresentation.
* fix(next): guard per-volume histogram WL panel for native viewports
getWindowLevelsData drives the per-volume histogram WL panel via
getAllVolumeIds/getProperties, which direct PLANAR_NEXT (next) viewports do not
expose (they throw). Add an early guard that returns no rows when getAllVolumeIds
is absent, so the panel degrades to 'No window level data available' instead of
erroring on the interval/event refresh. Legacy stack viewports also lack
getAllVolumeIds and were never passed here, so this is a no-op for them; the
native stack/volume WL path is driven by setViewportWindowLevel.
* fix(next): make resetViewport/scaleViewport/rotate commands native-safe
These three command paths assumed legacy camera APIs that direct PLANAR_NEXT
(next) viewports do not expose:
- resetViewport called viewport.resetCamera() (absent on native -> threw). Native
branch uses resetViewState() (resets pan/zoom/rotation/orientation/flip;
navigation/slice preserved). resetProperties stays optional-chained.
- scaleViewport (scaleUpViewport/scaleDownViewport) was guarded by
isStackViewportType, which is false for native, so the zoom buttons were a
silent no-op. Native branch uses getZoom/setZoom; parallelScale and zoom are
inversely related so it divides by scaleFactor to match legacy direction.
- _rotateViewport (rotateViewportCW/CCW/CWSet) used getViewPresentation/
setViewPresentation (absent on native). Native branch reads rotation/flip via
the getViewportCameraState bridge (getViewState) and writes the new rotation
via setViewportCameraState (setViewState), preserving the flip-parity logic of
the 'set' mode.
Verified live on native stack: reset no longer throws and resets zoom; zoom in
1->1.111; CW/CCW/Set rotation applies (90/180/90/90). Legacy unchanged.
* fix(next): guard jumpToMeasurement camera-centering for native viewports
jumpToMeasurement re-centers the camera when a measurement is off-screen via
isMeasurementWithinViewport (calls viewport.getCamera()) + getCamera/setCamera.
Native PLANAR_NEXT viewports have neither, so jumping to a measurement threw
'viewport.getCamera is not a function' at the gate before the centering block.
Short-circuit the centering on native (isGenericViewport) so it is skipped;
setViewReference above already navigated to the measurement's slice, so the
measurement is still reached - only in-plane re-centering is deferred.
TODO(next): port in-plane centering via the camera bridge + setViewState pan.
Verified live: native viewport has no getCamera (getCamera() throws) and
isGenericViewport is true, so the throwing branch is now skipped; setViewReference
remains available for slice navigation. Legacy unchanged.
* fix(next): make getViewportAlignmentData + updateViewport native-safe
Two CornerstoneViewportService methods called getCamera()/setCamera(), which
direct PLANAR_NEXT (next) viewports lack:
- getViewportAlignmentData looped every viewport reading getCamera().viewPlaneNormal
(reached from findNavigationCompatibleViewportId on a cross-orientation
jumpToMeasurement). Native reads viewPlaneNormal from getViewReference() instead
(both backends populate it); legacy keeps getCamera (byte-identical).
- updateViewport (metadata-invalidation re-mount, keepCamera) read getCamera()
unconditionally and had no native branch in its stack/volume if/else, so it threw
and would not re-mount native data. Add a native branch that snapshots/restores the
camera via the view-state bridges and routes the re-mount through _setDisplaySets
(backend.dispatchMount, which dispatches by data shape). Legacy path unchanged.
Verified live on native: getViewportAlignmentData returns data (no throw);
updateViewport(keepCamera) re-mounts, restores zoom (1.4/1.5), keeps the image actor
and renders, with zero errors.
* refactor(next): move viewport interaction ops into a Legacy/Next operations backend
The commandsModule carried inline native-vs-legacy branches for every viewport
interaction/appearance command. Extract them into a dedicated operations backend
that mirrors the existing IViewportBackend twin pattern:
- IViewportOperations: the interface (flip/invert/rotate/reset/scaleBy/
setWindowLevel/setColormap/getViewPlaneNormal/centerOnMeasurement + 3D VR ops)
- LegacyViewportOperations: legacy lane via direct legacy APIs (getCamera/
setProperties/getViewPresentation/resetCamera/actors), lifted verbatim
- NextViewportOperations: native lane via the presentation/camera-state bridges +
native semantic API (getViewState/resetViewState/getViewReference/setZoom); the
3D VR ops warn-once and no-op behind a CS-14 gate (native VR not supported yet)
- viewportOperations: per-viewport dispatcher (isGenericViewport ? next : legacy)
commandsModule's 13 interaction commands become one-line delegations (the file
shrinks ~239 lines) and CornerstoneViewportService.getViewportAlignmentData uses
viewportOperations.getViewPlaneNormal.
Dispatch is per-viewport (not flag-selected like the lifecycle IViewportBackend)
because operations run on already-created, self-describing viewports and a session
can mix lanes; this preserves the previous inline isGenericViewport branching
exactly. Render() stays in the command (per-command render timing preserved).
Validated live both lanes: native (flag on) applies all ops (invert/flip/rotate/
zoom/WL/colormap; reset is camera-only) and legacy (flag off) is byte-identical
(parallelScale*0.9 zoom, getViewPresentation rotation, resetProperties+resetCamera),
both with a clean console. Adversarial review found no byte-identity/runtime defects.
Segmentation untouched.
* feat(next): render native 3D volume rendering + enable its VR operations
Make native VOLUME_3D_NEXT viewports actually render volume rendering and wire up
the 3D VR operations:
- CornerstoneViewportService._setNativeVolumeDisplaySets: a 3D viewport (cornerstone
type VOLUME_3D_NEXT) now mounts with setDisplaySets({ options: { renderMode:
'vtkVolume3d' } }) instead of the planar { orientation, role }, and applies the
display-set's volume-rendering preset to the volume actor via csUtils.applyPreset
(the bare native VolumeViewport3D has no setProperties). colormap (a planar LUT
concept) is skipped for 3D. The dataId registration is unchanged (the volume3d data
provider reads imageIds/volumeId and ignores the stored kind).
- NextViewportOperations: the four VR ops are no longer CS-14 no-ops. setPreset
applies the preset to the volume actor via applyPreset; setVolumeRenderingQuality/
shiftVolumeOpacityPoints/setVolumeLighting operate on the vtk volume actor through
getActors (which native VolumeViewport3D exposes), so they reuse the legacy
actor-based implementations. Pairs with the cornerstone fix that keeps the 3D
viewport's canvas visible.
Verified live: native 3D VR renders (CT-Bone/CT-Cardiac presets) and all four VR
commands apply on a native VOLUME_3D_NEXT viewport; legacy VOLUME_3D unchanged.
* docs(next): refresh migration plan to HEAD (2026-06-19 audit)
* fix(next): target fusion colormap at the overlay binding
NextViewportOperations.setColormap dropped params.displaySetInstanceUID and
always wrote to the active source binding via getSourceDataId(), so a PT/CT
fusion colormap landed on the CT source instead of the PT overlay. Thread the
displaySetInstanceUID through to setViewportProperties so it targets the right
native binding (OHIF maps each display set 1:1 onto its bare dataId); falls back
to the source when no id is given (single-volume / plain stack colormap).
Also document DataIdRegistry.dataIdFor's 'overlay' suffix as reserved for the
same-UID source/overlay case (derived labelmap overlays, M4) rather than fusion,
whose distinct-UID overlays are already collision-free under the bare id.
* fix(next): make residual native-unsafe viewport sites safe
Sweeps the OHIF-side sites a native PLANAR_NEXT viewport reaches that still
called legacy-only APIs (getProperties/getCamera) or branched on the collapsed
viewportType:
- CornerstoneCacheService: persist the legacy stack/volume decision as
viewportData.dataShapeType (createViewportData) and branch on it in
invalidateViewportData instead of viewportType. Native collapses stack+volume
onto PLANAR_NEXT, so a native stack previously fell through to the VOLUME
rebuild and re-mounted as volume data on metadata invalidation. Falls back to
viewportType for legacy/older data (byte-identical off-path).
- ViewportOrientationMarkers: gate the synthetic-IOP default-cosine check on
dataShapeType, not viewportType==='stack' (dead on native -> guard was skipped).
- CornerstoneViewportDownloadForm: the capture viewport is the source's type, so a
native source threw on getProperties/setStack/setProperties. Add a native capture
path that re-mounts the source's already-registered dataId via setDisplaySets and
copies presentation + view state through the bridges (legacy path byte-identical).
- tmtv ROI-threshold: read the slice focal point via a new getViewportFocalPoint
bridge (native getViewReference().cameraFocalPoint vs legacy getCamera().focalPoint)
instead of getCamera(), which is absent on native.
New bridge getViewportFocalPoint added to getViewportPresentation.ts and exported
from @ohif/extension-cornerstone. Validated live: native stack renders, console
clean, viewportData.dataShapeType='stack' while viewportType='planarNext',
orientation markers render.
* feat(next): mount native video/WSI/ECG viewports
Under useNextViewports, NextViewportBackend.dispatchMount routed ALL viewports by
data shape (volume vs stack), so VIDEO_NEXT/WHOLE_SLIDE_NEXT/ECG_NEXT constructed
as native classes but mis-ran _setStackViewport's stack-specific prefetch/VOI/
kind:'planar' logic and never reached their dedicated mounts; ECG additionally
called the absent setEcg.
- NextViewportBackend.dispatchMount: route the non-planar families by viewport
type to _setEcgViewport / _setOtherViewport (mirrors the legacy backend's type
dispatch); planar stack/volume still routes by data shape.
- _setEcgViewport: native branch registers {kind:'ecg', sourceDataId} and mounts
via the generic setDisplaySets API (native ECG has no setEcg).
- _setOtherViewport: native branch registers {kind:'video', sourceDataId} or, for
WSI, {kind:'wsi', imageIds, options:{webClient}} with the client resolved from
WADO_WEB_CLIENT metadata exactly as the legacy WSI adapter does, then mounts via
setDisplaySets + setViewReference.
- DataIdPayload widened to a family-specific union (planar/video/ecg/wsi).
All registration goes through the ref-counted DataIdRegistry (§4.7). OHIF-only —
the cornerstone native classes already support setDisplaySets (per the
genericVideo/genericEcg/genericWsi examples). Validated: native planar render
unaffected by the dispatch change (console clean); video/WSI/ECG mounts follow the
canonical cornerstone examples but are not yet live-validated (no such study on the
dev dicomweb).
* chore(next): guard the useNextViewports flag-read allowlist (M7 prep)
Adds .scripts/check-next-viewports-flag-reads.mjs (wired as `yarn
next:check-flag-reads`) enforcing migration plan §4.2: the flag may be read only
in the sanctioned seam — getCornerstoneViewportType (type selection),
CornerstoneViewportService (backend selection), nextViewports.ts (the accessor),
init.tsx (the one appConfig.useNextViewports read), and the TEMP dev toggle in
getToolbarModule.tsx. Any other isNextViewportsEnabled()/appConfig.useNextViewports
read under extensions/cornerstone/src fails the check, so the legacy off-path
cannot drift. (Comment-only mentions are ignored; tests are exempt.)
The earlier audit framed the '2 sanctioned reads' contract as already violated by a
'backend trio', but those files only MENTION the flag in doc comments — the actual
runtime read surface is the sanctioned set above, so the rule is enforceable as
written. TODO_BEFORE_MERGE.md updated: the guard is permanent (not a dev revert),
and removing the dev toggle must also drop its allowlist entry.
Does NOT perform the destructive M7 reverts (config default flip, toggle button):
those are premature while segmentation/M4 is unmigrated and would disable the
in-browser test loop.
* docs(next): mark CS-12 native calibration done; refresh CS-20/M6 status
* docs(next): re-verify migration status at HEAD; correct stale prose
Re-audited every milestone (M0-M7) and CS blocker against HEAD source via a
multi-agent audit + adversarial verification pass. Five commits landed after the
last full prose refresh (05e0df0) and only d5d03d8 touched the doc, so the
Implementation status section was materially stale. Corrections:
- M2: fusion colormap keying is FIXED (7b61e08), not 'unsound'
- M3: four 'native-unsafe throws' are FIXED (a19bd78); the per-volume WL panel
is guarded (d282026), not throwing - feature port, not a crash
- M6: video/WSI/ECG ARE mounted natively (ca746e2)
- M7: flag-read allowlist IS built (b5784ca), just not wired into CI
- CS-21: stated trigger is unreachable; narrowed to single-point SCOORD3D, and
the open code is PlanarViewReferenceController.ts (not planarViewReference.ts)
Adds a consolidated, verified remaining-work punch-list and a corrections
subsection. The one real native crash that remains is the M4 segmentation OHIF
half (convertStackToVolumeViewport throws AND promotes to legacy ORTHOGRAPHIC).
* Refactor SegmentationService to support dual backends for segmentation handling
- Introduced ISegmentationBackend interface to define methods for segmentation backends.
- Implemented LegacySegmentationBackend for existing behavior with stack/volume promotion.
- Implemented NextSegmentationBackend for native GenericViewport handling without promotion.
- Updated SegmentationService to utilize the appropriate backend based on viewport type.
- Removed legacy viewport handling logic from SegmentationService and delegated to backends.
- Enhanced segmentation data assembly to support overlapping segments in the Next backend.
- Updated CornerstoneViewportService to ensure proper restoration of segmentation presentations.
- Added support for preserving additional query parameters in the application.
* temp
* Refactor CornerstoneViewportService to optimize setDisplaySets handling for 3D volumes
* d
* Refactor viewport handling and opacity management for native volume rendering
* fix(WindowLevel): re-sync fusion tab to foreground default after async resolve
The effect only adopted the foreground (PT) default when activeDisplaySetUID was
falsy, so if foregroundDisplaySets was empty at mount the tab seeded to the CT
fallback and stayed pinned to CT once PT resolved. Track explicit user selection
and re-sync to the foreground default until the user picks a tab.
* chore(next): remove WIP migration plan artifacts from the branch
Delete the planning docs and plan-viewer pages that were committed during
development (migration plans, blueprint, TODO_BEFORE_MERGE, plan-viewer.html).
They are not part of the shipped viewer and only attract review noise.
* fix(next): address review findings (guards + correctness)
Apply CodeRabbit review comments on the migration code:
- commandsModule: guard missing viewport before setWindowLevel/render
- WindowLevel: drop stale activeDisplaySetUID when the viewport's display sets change
- SegmentationService: hard guard when segmentation lookup fails before backend classify
- LegacyViewportOperations: guard getActors()[0] before actor-chain calls
- CornerstoneViewportService: guard empty native stack imageIds; don't route generic
overlay-only mounts to legacy setVolumes; stop overwriting tracked display sets with
base-volume-only ids (drops SEG/RT/fusion overlay UIDs)
- getCornerstoneViewportType: list orthographic/volume3d in the invalid-type error
- nextViewports: skip reload (warn) when the toggle can't be persisted
- tmtv: guard missing focal point before mutating ROI annotation coordinates
- check-next-viewports-flag-reads: also catch bracket/destructured flag reads
* fix(overlay): show instance number on next viewports
The viewport overlay's getInstanceNumber switched on viewportData.viewportType,
which for next viewports is the native PLANAR_NEXT type (the stack/volume shape
is persisted separately as dataShapeType). The switch matched no case, so the
instance number was null and the overlay showed only the slice index/count.
Switch on (dataShapeType ?? viewportType) so next stack/volume viewports take
the correct branch (legacy is unaffected). Also make _getInstanceNumberFromVolume
read the view-plane normal via getViewReference for native viewports, which
expose no getCamera, so routing next volume viewports through it cannot throw.
* fix(overlay): refresh window level on series change for next viewports
The overlay's WW/WL comes from useViewportRendering, whose init effect reads the
VOI via getViewportProperties. On series change the effect re-runs, but native
(next) viewports expose only explicit VOI overrides through
getDisplaySetPresentation; a freshly shown series has none, so properties.voiRange
was undefined, setVoiRange was skipped, and the overlay kept showing the previous
series' window level. Legacy getProperties always returns the applied VOI, so only
native viewports were affected.
Fall back to the viewport's computed default VOI (getDefaultVOIRange) for generic
viewports when no override is stored, matching the LivewireContourTool and
WindowLevelTool bridges. Legacy behavior is unchanged.
* fix(scrollbar): seed slice state on orientation change for next viewports
The progress scrollbar seeded its slice state (imageIndex/numberOfSlices) only
when viewportData changed. Native (next) viewports keep the same viewportData
across a stack->volume transition or an orientation change, and the slice-
navigation event does not fire until the first scroll, so the scrollbar was
missing on the initial slice (or stale with a wrong slice count) until the user
scrolled once.
Re-seed the slice state from the live viewport on CAMERA_MODIFIED (which native
viewports emit on orientation/geometry changes, as the sibling full-mode hook
already relies on). A guard skips redundant state updates so pure pan/zoom does
not churn React state.
* refactor(next): call resetDisplaySetPresentation on reset
Follow the cs3D rename of the native viewport's presentation-reset method from
resetProperties to resetDisplaySetPresentation (the next viewport API uses
get/set DisplaySetPresentation, not get/set Properties). Behavior unchanged.
* fix(segmentation): make border/outline thickness slider integer-only
The Border (outline width) slider for labelmap and RTSTRUCT used step=0.1,
allowing fractional outline widths. Outline thickness is a pixel width and
should be a whole number, so use step=1 and round the committed value. The
fill/opacity sliders keep their fractional step.
* fix(crosshairs): guard resetCrosshairs against unregistered Crosshairs tool
resetCrosshairs (run by Reset Viewport) called toolGroup.getToolInstance('Crosshairs')
for every tool group; getToolInstance logs 'Crosshairs is not registered with this
toolGroup' when the tool is absent, and a next viewport's default tool group does not
include Crosshairs, so Reset Viewport logged a spurious warning. Guard the lookup with
toolGroup.hasTool('Crosshairs') and skip tool groups that lack it; also guard against a
missing tool group for the viewport.
* fix(next-fusion): promote source to volume slice when a data overlay is added
A data overlay (fusion) on a next (PLANAR_NEXT) viewport rendered the source as a
vtkImage stack while the overlay was a vtkVolumeSlice, producing a broken/unstable
fusion (geometry mismatch, intermittent across slices/scroll). Two causes:
1. CornerstoneCacheService.createViewportData built stack data when the fusion's
primary display set resolved to a stack shape, so the source never got a
volumeId. Force a volume (orthographic) shape when there are 2+ reconstructable
image display sets (a data fusion must be volume; legacy already did this, and
non-reconstructable SEG/RT overlays are excluded).
2. dataIdRegistry.register used first-writer-wins, so re-registering the source
(originally a vtkImage stack, no volumeId) with its fusion volumeId was dropped,
leaving its dataset volumeId-less -> the render-path decision kept it vtkImage.
Update the provider when a payload promotes a dataId to volume-backed.
Validated via agent-browser: adding a PT overlay onto a CT source now mounts both
as vtkVolumeSlice (mode=volume), the fusion is anatomically coherent and stable
across scroll, with no console errors.
* fix(next-rtss): keep referenced CT in stack mode on RTSTRUCT hydrate
RTSTRUCT (contour) hydration on a native PLANAR_NEXT viewport re-mounted the
referenced CT as a volume slice, which is the slow path the perf AC forbids.
Spike proved cs3d already renders contour segmentations on a stack/vtkImage
PLANAR_NEXT viewport (via the annotation + isReferenceViewable path), and that
stack-mode contour scroll is fast (~0.15ms/scroll, no metadata storm) once the
canvas-dimension layout-thrash fix is in place. The only remaining issue was the
hydrate re-mount promoting the CT to volume.
Pin the referenced viewport to 'stack' for RTSTRUCT hydration on next viewports:
hydrateSecondaryDisplaySet passes viewportType:'stack' (scoped to RTSTRUCT +
isNextViewportsEnabled), and loadSegmentationDisplaySetsForViewport applies it as
a per-mount viewportOptions override. SEG and legacy keep their current behavior.
Verified at runtime: after hydrate the viewport stays vtkImage/stack, contours
render across slices, scroll is 0.16ms with 0 metaData.get calls, CT VOI intact.
* fix(next-fusion): match legacy initial data-overlay opacity (~40%) on next viewports
The data-overlay add path passes a nominal colormap opacity of 0.9. Legacy
volume rendering attenuates that to ~40% effective via ray-cast opacity-unit-
distance correction, but native PLANAR_NEXT viewports composite the overlay as
a flat 2D image-slice blend with no such attenuation, so the same 0.9 rendered
at ~80-90%. Override the initial overlay opacity to 0.4 for next viewports
(mirrors the TMTV fusion NEXT_FUSION_PT_OPACITY), gated on isGenericViewport and
a numeric opacity so SEG/RTSTRUCT overlays and the legacy path are unaffected.
* fix(next-fusion): preserve fusion on orientation change in next viewports
The orientation corner menu branched on viewportType === ORTHOGRAPHIC to decide
in-place reorient vs viewport recreation. Native next viewports always report
planarNext, so a fusion already in volume mode wrongly took the recreation path,
which passes empty displaySetOptions and drops the PET overlay colormap/opacity
(rendering PET only). Branch instead on whether the live viewport is already in
volume mode (isOrthographicViewportType || utilities.viewportIsInVolumeMode):
volume-mode viewports reorient in place (setViewportOrientation, which preserves
all bindings and their presentation); only a genuine stack->volume conversion
recreates. Legacy ortho/stack behavior is unchanged.
* fix(next-seg): preserve base image window level through SEG hydration
Hydrating a SEG re-mounted the referenced image and restored a stale, computed
VOI from the LUT presentation store, brightening the base image (e.g. an MR went
from its DICOM WC/WW default to a volume min/max default). During the SEG-load
intermediate mount the base image briefly carries a computed default VOI; the
native read bridge returned it with no isComputedVOI marker, so cleanProperties
never stripped it and it was persisted then replayed over the correct default.
Legacy StackViewport tags computed VOIs (isComputedVOI) so they are stripped.
Mirror that: in getViewportProperties, stamp isComputedVOI on a native binding's
VOI when it matches the binding's getDefaultVOIRange, so the LUT capture strips
it. Harmless when a genuine user VOI equals the default (stripping falls back to
the same value).
* fix(next-mpr): re-seed slice scrollbar after post-mount camera carry
On a stack->volume/MPR transition the slice carry (e.g. layout-selector MPR HP
restoring the prior slice) moves the camera and fires its slice events
synchronously during the mount, before the scrollbar effect attaches its
listeners and around its initial seed -- so the scrollbar latched the mount-time
index. Re-seed once on the next frame after mount+carry settle; the pushSliceData
guard makes it a no-op when nothing changed (no churn).
* chore(deps): bump @cornerstonejs/* to 5.1.2
* chore: empty commit
* fix(next): use published genericViewportDisplaySetMetadataProvider export
The next viewport backend imported genericViewportDataSetMetadataProvider
from @cornerstonejs/core, a symbol that only existed in the local custom
cornerstone worktree (linked via symlink). Published cornerstone exports it
as genericViewportDisplaySetMetadataProvider (same add/remove/get/clear API).
CI always builds against published packages, so the production rspack build
failed (ESModulesLinkingError -> Netlify red) and the cypress dev-server
build showed a full-screen error overlay that blocked all clicks (PR_CHECKS
red). Renaming to the published symbol fixes both.
* test(e2e): extend Scoord3dProbe jump screenshot retry window
The jump-to-measurement screenshot was the only failing assertion (pre/post
hydration pass). It uses waitVolumeLoad:false, so on slower CI the dynamic
tfl_dyn_fast_tra series can still be progressively loading when the shot is
taken, giving a partial probe value (52.0 vs baseline 78.0) and shifted VOI.
Raise checkForScreenshot attempts 10->20 and delay 1250->2000ms (~11s -> ~40s)
to let the volume settle before failing.
* fix(next): address review findings (scaleBy 3D crash, fusion W/L target, off-path gate, seg export perf)
- NextViewportOperations.scaleBy: guard getZoom/setZoom so the zoom hotkey
no-ops on a native VolumeViewport3D instead of throwing (matches legacy).
- Native setWindowLevel: forward displaySetInstanceUID so PT/CT fusion W/L
targets the intended binding (mirrors setColormap) instead of always source.
- CornerstoneCacheService: scope the reconstructable-fusion STACK->ORTHOGRAPHIC
promotion to PLANAR_NEXT so the legacy (flag-off) path stays byte-identical.
- dicom-seg buildLabelmap3D: precompute a referencedImageId->index Map to drop
the multi-layer export from O(slices^2) to O(slices).
* fix(next): address CodeRabbit review (dispatch discriminator, seg return contract, error msg)
- NextViewportBackend.dispatchMount: route stack/volume on the persisted
dataShapeType contract instead of the lazily-populated 'volume' field probe.
- SegmentationService.attemptStackToVolumeConversion: return false explicitly on
the frame-of-reference-mismatch path to honor the Promise<boolean> contract.
- getCornerstoneViewportType: list orthographic/volume3d in the legacy-path
invalid-type error (both are valid and handled above the throw).
* test(next): update getCornerstoneViewportType invalid-type assertion
Match the legacy-path error message now listing orthographic/volume3d (b16129e).
* chore(next): replace dev toggle + flag-read guard with URL opt-in
- Remove the .scripts/check-next-viewports-flag-reads.mjs CI guard and its
package.json script entry.
- Drop the TEMP ToggleNextViewport toolbar button across all modes plus the
toggleNextViewports command and getToolbarModule evaluator (revert mode files
to their master state; keep the real native-path toolbar fixes).
- Remove the localStorage override / toggleNextViewportsAndReload from
nextViewports.ts; resolveNextViewportsEnabled now honors a ?useNextViewports
URL query param (true/1/empty enable) over appConfig.
- Stop forcing useNextViewports:true in default.js (opt-in via URL/appConfig).
- Preserve useNextViewports across navigation (preserveQueryParameters).
* fix(tmtv): keep legacy fusion PT opacity ramp; flatten only on next path
Flattening the fusion PT opacity to a scalar 0.9 in hpViewports changed the
legacy TMTV fusion rendering (the ramp keeps low PT values transparent so the CT
shows through). Restore the legacy ramp in hpViewports and instead replace it
with the flat native scalar (0.4) inside getHangingProtocolModule only when
useNextViewports is on, so legacy is unchanged and native still gets a flat blend.
* redo
* fix(next): avoid top-level dicomWebUtils destructure crash on boot
getSopClassHandlerModule destructured transferDenaturalizedDataset /
fixMultiValueKeys from dicomWebUtils at module-eval time. This module and
@ohif/extension-default form a circular import, so dicomWebUtils can be
undefined at eval time depending on bundler module order; the top-level
destructure then throws (Cannot destructure property ... of dicomWebUtils
as it is undefined) and crashes app boot before the Layout renders, which
surfaced as the Playwright globalSetup warmup timing out on [data-cy=Layout].
Access the utils lazily at call time inside getDICOMwebMetadata instead.
* fix(next): guard remount no-op path and defer legacy camera snapshot
- CornerstoneViewportService.updateViewport: backend.remount() is typed
Promise<void> | undefined and returns undefined for viewport families with
no re-mount path; guard before .then() so those families no longer throw.
- LegacyViewportBackend.remount: take the camera snapshot inside the volume
branch (the only consumer) instead of before the family checks, so families
without a camera surface no-op safely and the stack path skips a dead call.
Addresses CodeRabbit review findings on PR #6101.
* feat(next): add ?cpu=true URL opt-in to force the CPU render path
Mirrors the ?useNextViewports opt-in: a cpu URL query param overrides
appConfig.useCPURendering per-session (?cpu, =true, or =1 enable it).
Wired through cornerstone.setUseCPURendering in init, whose global flag is
consulted by the GenericViewport PlanarRenderPathDecisionService for both
the image (CPU_IMAGE) and volume (CPU_VOLUME) paths, so under
useNextViewports a single ?cpu=true forces the next viewport onto CPU.
Extracted the shared query-param parsing into resolveBooleanUrlOptIn.
* feat(cornerstone): select render backends via viewportRendering param
Replaces the boolean cpu URL param with viewportRendering=cpu|webgl|auto
(or any backend registered via cornerstone registerRenderBackend, e.g. a
webgpu backend), mapped to the cornerstone render-backend registry. A
per-viewport-type override (e.g. orthographic.viewportRendering=cpu) is
passed as the per-mount renderBackend option on native planar mounts.
The global value also drives the legacy useCPURendering flag so legacy
viewports follow the same selection, letting a session force GPU when
the deployed config defaults to CPU.
* fix(dicom-seg): store overlapping segmentations as binary SEG
A LABELMAP SEG frame stores a single label per voxel, so the labelmap
encoder cannot represent overlapping segments (the later layer wins).
When the export produces multiple overlapping layers and the resolved
store mode is labelmap, switch that store to the binary SEG encoding,
which writes overlapping segments as separate frames referencing the
same source slice.
* fix(cornerstone): apply review fixes to viewport backends
- Anchor the cached-volume lookup in NextViewportAdapter to the
loaderSchema:displaySetInstanceUID id shape instead of a substring
match, so a derived volume id embedding the same UID cannot resolve.
- Keep the viewing orientation on fitViewportToWindow (scaleBy 0) for
native planar viewports, matching legacy resetCamera semantics.
- Release legacy WSI metadata-provider registrations through the same
ref-counted DataIdRegistry the native backend uses, so entries are
removed on viewport disable and service destroy.
* test(cornerstone): use real volumeId shape in adapter contract test
The anchored cached-volume lookup rejects ids that merely embed the
display set UID, so the mock now uses the real
volumeLoaderSchema:displaySetInstanceUID shape and asserts an embedded
UID id does not match.
* docs(config): document useNextViewports and viewportRendering in default config
Gives deployments an obvious place to opt into the native Generic
Viewport path and configure its render backend. Both stay off/auto by
default; the explicit false preserves the opt-in contract.
* refactor(config): group next viewport settings under genericViewports
Replaces the two top-level app config keys (useNextViewports,
viewportRendering) with one genericViewports object:
{ enabled, viewportRendering }. The URL params are unchanged.
* chore(deps): bump cornerstone packages to 5.4.15
Picks up the planar initial-slice remap fix (cornerstonejs/cornerstone3D#2799):
opening a SEG display set now lands on the first segmented slice instead of
its mirror when the cached volume reverses the imageId ordering.
* test(segmentation): verify overlapping SEG rendering
* fix(sr): make SCOORD3D hydration deterministic
---------
Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
When a planar Generic Viewport mounts a dataset on the volume-slice render path,
DefaultPlanarDataProvider.loadbuilds the payload fromimageVolume.imageIds.createAndCacheVolumesorts those imageIds by position along the scan axis, which can reorder — commonly exactly reverse — them relative to the registered dataSet ordering the caller computedinitialImageIdIndexagainst. The index is then interpreted in the volume ordering and the viewport opens on the mirrored slice.Observed in OHIF (next viewports): opening a SEG whose first segmented slice is index 12 of 30 lands on slice 18 instead of 13 (12 and 17 are mirror indices of a reversed 30-slice stack). The SEG overlay promotes the source series to the volume-slice path, which is why plain stack mounts don't show it.
Changes & Results
DefaultPlanarDataProvider: when the volume branch returnsimageVolume.imageIdsand an explicitinitialImageIdIndexwas registered, remap the index through the requested imageId (imageIds.indexOf(dataSet.imageIds[index])) so the payload index addresses the slice the caller asked for in the payload's own ordering.undefined(no slice requested) is preserved so the acquisition view still centers; the plain-image branch keeps the dataSet ordering and is unchanged.defaultPlanarDataProvider.jest.jscovering: reversed-order remap, same-order passthrough, undefined preservation, and the image branch.Testing
npx jest test/defaultPlanarDataProvider.jest.js(4 passed)npx jest test/planarOrchestration.jest.js test/planarViewReference.jest.js test/planarRenderBackend.jest.js test/planarViewportState.jest.js(122 passed)Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Summary by CodeRabbit
imageIdsordering differs from the dataset’s ordering by remapping the requested starting index.imageIdisn’t present in the volume.imageIdwarning/fallback, undefined initial slice handling, and image-based rendering.