fix(planar): anchor imageId-addressed slices at their exact IJK world center#2800
Merged
Conversation
… center The planar volume slice basis positions a slice by walking min + index * spacing along the viewPlaneNormal, which counts slices in CAMERA (scroll) order. For the acquisition orientation the camera normal is the negated scan axis, so an index expressed in the volume's imageId list ordering - which runs along the scan axis - lands on the mirrored slice when fed into that walk (and drifts off slice centers on oblique volumes, whose corner projections span the voxel bounding box). Two write paths fed imageId-list indices into the camera-order walk: - PlanarViewReferenceController resolved referencedImageId (and plane points snapped via getClosestImageId) to an index in the volume's imageId list, then navigated through getVolumeSliceWorldPointForImageIdIndex. setViewReference with a referencedImageId therefore navigated to the mirror of the requested slice on volume-backed planar viewports. - PlanarViewport.createInitialVolumeSliceState fed the payload's initialImageIdIndex (which addresses payload.imageIds) into the basis, so an explicitly carried initial slice opened mirrored. Both now anchor at the slice's exact IJK center via a shared getVolumeImageIdIndexWorldPoint helper (volumes are constructed so imageIds[k] IS IJK slice k). Numeric sliceIndex references keep the camera-order domain and the existing opposite-normal flip. The test harness previously placed volume imageIds at mirrored world positions (z = numSlices - 1 - index) while indexToWorld mapped k -> z=k, modeling a volume whose imageId list runs against its own k axis - something createAndCacheVolume never produces - which made the camera-order walk coincide with the imageId ordering and masked the mirror. The mock now follows the real invariant, and the two read-side expectations that relied on the coincidence now assert the truthful pairing (camera-order sliceIndex alongside the actually displayed imageId).
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughVolume slice references now resolve image-list indices to exact world-space slice centers when possible. Explicit initial slice indices use the same mapping, with fallback to existing index-based behavior. Tests cover flipped volumes, clamping, missing image data, and controller navigation. ChangesVolume slice targeting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlanarViewReferenceController
participant resolveVolumeReferenceSliceTarget
participant getVolumeImageIdIndexWorldPoint
participant PlanarViewport
PlanarViewReferenceController->>resolveVolumeReferenceSliceTarget: resolve volume slice reference
resolveVolumeReferenceSliceTarget->>getVolumeImageIdIndexWorldPoint: calculate image-index world center
getVolumeImageIdIndexWorldPoint-->>resolveVolumeReferenceSliceTarget: return world point or undefined
resolveVolumeReferenceSliceTarget-->>PlanarViewReferenceController: return slice target
PlanarViewReferenceController->>PlanarViewport: apply world point or image index
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/PlanarViewReferenceController.ts`:
- Around line 474-481: Update the sliceWorldPoint resolution in the relevant
controller method to prioritize the resolved sliceTarget anchor
(sliceWorldPoint, then imageIdIndex-derived point) before falling back to
normalizedViewRef.cameraFocalPoint. This ensures
resolveVolumeReferenceSliceTarget results determine the exact slice center for
combined references.
🪄 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: 6058541c-35a3-4500-b927-b37b00d6dc3e
📒 Files selected for processing (5)
packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewReferenceController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarSliceBasis.tspackages/core/test/planarSliceBasis.jest.jspackages/core/test/planarViewReference.jest.js
Comment on lines
474
to
481
| const sliceWorldPoint = | ||
| normalizedViewRef.cameraFocalPoint ?? | ||
| (typeof nextImageIdIndex === 'number' | ||
| ? this.host.getVolumeSliceWorldPointForImageIdIndex(nextImageIdIndex) | ||
| sliceTarget?.sliceWorldPoint ?? | ||
| (typeof sliceTarget?.imageIdIndex === 'number' | ||
| ? this.host.getVolumeSliceWorldPointForImageIdIndex( | ||
| sliceTarget.imageIdIndex | ||
| ) | ||
| : undefined); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prioritize the resolved slice target over the raw focal point.
Line 475 currently wins whenever a reference carries cameraFocalPoint, so imageId/plane-point targets computed by resolveVolumeReferenceSliceTarget are discarded rather than snapping to the exact IJK slice center. This leaves combined references on an arbitrary focal point instead of the intended slice anchor.
Proposed fix
const sliceWorldPoint =
- normalizedViewRef.cameraFocalPoint ??
sliceTarget?.sliceWorldPoint ??
(typeof sliceTarget?.imageIdIndex === 'number'
? this.host.getVolumeSliceWorldPointForImageIdIndex(
sliceTarget.imageIdIndex
)
- : undefined);
+ : undefined) ??
+ normalizedViewRef.cameraFocalPoint;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const sliceWorldPoint = | |
| normalizedViewRef.cameraFocalPoint ?? | |
| (typeof nextImageIdIndex === 'number' | |
| ? this.host.getVolumeSliceWorldPointForImageIdIndex(nextImageIdIndex) | |
| sliceTarget?.sliceWorldPoint ?? | |
| (typeof sliceTarget?.imageIdIndex === 'number' | |
| ? this.host.getVolumeSliceWorldPointForImageIdIndex( | |
| sliceTarget.imageIdIndex | |
| ) | |
| : undefined); | |
| const sliceWorldPoint = | |
| sliceTarget?.sliceWorldPoint ?? | |
| (typeof sliceTarget?.imageIdIndex === 'number' | |
| ? this.host.getVolumeSliceWorldPointForImageIdIndex( | |
| sliceTarget.imageIdIndex | |
| ) | |
| : undefined) ?? | |
| normalizedViewRef.cameraFocalPoint; |
🤖 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/PlanarViewReferenceController.ts`
around lines 474 - 481, Update the sliceWorldPoint resolution in the relevant
controller method to prioritize the resolved sliceTarget anchor
(sliceWorldPoint, then imageIdIndex-derived point) before falling back to
normalizedViewRef.cameraFocalPoint. This ensures
resolveVolumeReferenceSliceTarget results determine the exact slice center for
combined references.
…itial slice The compat lane's initial volume slice was one slice off the legacy center: getInitialVolumeImageIdIndex returns floor(count / 2) to match legacy resetCamera's getVolumeCenterIJK snap, but the camera-order basis walk landed that index on its mirror - IJK floor((count - 1) / 2) for even slice counts. The 30-slice MPR reformat volume therefore opened on IJK 14 instead of the legacy IJK 15, and the old baselines encode that off-center slice. With imageId-list indices now anchored at their exact IJK centers the compat mount opens on IJK 15, matching legacy resetCamera; baselines are promoted from the CI runner's actuals.
Same legacy-centered initial slice as the sibling baselines; this snapshot was previously unreached because the earlier assertion in the test failed first. Promoted from the CI runner's actual.
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
Follow-up to #2799 — that fix delivered the payload's
initialImageIdIndexin the volume's imageId ordering, but two write paths still interpret imageId-list indices in the slice basis's CAMERA (scroll) ordering, which for the acquisition orientation runs along the NEGATED scan axis. An imageId-list index fed into the camera-order walk (min + index * spacingalong the viewPlaneNormal) therefore navigates to the mirrored slice, and on oblique volumes additionally drifts off slice centers (the walk's corner projections span the voxel bounding box, not slice centers).Observed in OHIF (next viewports): opening a SEG jumps to
firstSegmentedSliceImageIdviasetViewReference({ referencedImageId })— the viewport landed on slice 18/30 instead of 13/30 (mirror indices: 12 + 17 = 29). Reproducible live with one line:viewport.setViewReference({ referencedImageId: imageIds[12] })landed on the plane ofimageIds[17].Changes & Results
getVolumeImageIdIndexWorldPoint(imageVolume, k): the exact world center of IJK slice k viaindexToWorld— the ordering-safe anchor for an index expressed in the volume's imageId list (volumes are constructed soimageIds[k]IS IJK slice k).PlanarViewReferenceController:referencedImageIdreferences (and plane points snapped viagetClosestImageId) now resolve to that exact world center instead of routing an imageId-list index through the camera-ordergetVolumeSliceWorldPointForImageIdIndex. NumericsliceIndexreferences keep the camera-order domain and the existing opposite-normal flip — scroll/getSliceIndex semantics are untouched.PlanarViewport.createInitialVolumeSliceState: an explicitly carriedinitialImageIdIndex(addressespayload.imageIds) is anchored the same way; the undefined-index centering path is unchanged.imageIds[k]at worldz = numSlices - 1 - kwhileindexToWorldmappedk -> z = k— modeling a volume whose imageId list runs against its own k axis, whichcreateAndCacheVolumenever produces. That coincidence made the camera-order walk line up with the imageId ordering in tests and masked the mirror. The mock now follows the real invariant; two read-side expectations that relied on the coincidence now assert the truthful pairing (camera-ordersliceIndexalongside the actually displayed imageId), and new tests pin the round-trip (setViewReference(imageId)→ that image is current) and the helper's geometry (including a flipped-Z volume and clamping).Testing
npx jest(packages/core): 31 suites, 534 passed.setViewReference({ referencedImageId })round-trips to the exact requested image.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Summary by CodeRabbit
New Features
Bug Fixes
Tests