Skip to content

fix(Volume viewport): Image shift in mpr when measurement is loaded.#2539

Merged
wayfarer3130 merged 6 commits intocornerstonejs:mainfrom
nithin-trenser:fix/mpr-image-shift
Jan 30, 2026
Merged

fix(Volume viewport): Image shift in mpr when measurement is loaded.#2539
wayfarer3130 merged 6 commits intocornerstonejs:mainfrom
nithin-trenser:fix/mpr-image-shift

Conversation

@nithin-trenser
Copy link
Copy Markdown
Contributor

@nithin-trenser nithin-trenser commented Jan 7, 2026

Context

This PR resolves image shifting in MPR viewports when loading or selecting measurements. This issue was observed with multiframe datasets.

Fixes: OHIF/Viewers#5684

This PR has been incorporated by FlyWheel.io

Changes & Results

The setViewPlane() method was updated to compute a view plane that is the correct distance but centered on the same spot as it was previously. This does not resolve the underlying issue, but is a correct fix for newly loaded SRs that can change the focal position relative to the image.

Before:

mpr-image-shift-bug.mp4

After:

fix-mpr-image-shift-bug.mp4

Testing

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS: Windows 11 10.0.22631
  • Node version: 20.17.0
  • Browser: Chrome: 143.0.7499.170

@nithin-trenser
Copy link
Copy Markdown
Contributor Author

Fix Summary

The issue occurred because setViewReference() was called without viewPlaneNormal, which triggered fallback logic that invokes setViewPlane().

setViewPlane() then recursively calls setViewReference() while passing viewPlaneNormal, but omits referenceImageId. As a result, subsequent logic in setViewReference() that depends on referenceImageId fails due to the value being undefined.

The fix ensures referenceImageId is passed to setViewPlane(), so it is correctly forwarded during the recursive setViewReference() call.

@sedghi could you please review this PR ?

@@ -807,6 +807,7 @@ abstract class BaseVolumeViewport extends Viewport {
FrameOfReferenceUID,
cameraFocalPoint: point,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed solution only works if one has a referenced image id. I believe the correct behaviour is to use the existing focal point from the camera and modify it with the difference between the current view plane normal and the specified value.
That is:
get the view plane normal/focal point from the camera right now
const deltaFocal = vec3.subtract(vec3.create(),point, focalPoint);
const alongNormal = vec3.dot(deltaFocal,viewPlaneNormal)
const deltaNormal = vec3.scaleAndAdd(vec3.create(), focalPoint, viewPlaneNormal, alongNormal)
then use deltaNormal as the new point.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can test the difference between solutions by saving annotations in a non-acquisition orientation and then seeing the jumping for those ones.

@nithin-trenser
Copy link
Copy Markdown
Contributor Author

@wayfarer3130 The code has been refactored as per your instructions. Could you please verify the changes?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes image shifting in MPR (Multi-Planar Reconstruction) viewports when measurements are loaded or selected with multiframe datasets. The fix modifies the setViewPlane() method to project the target point onto the view plane normal direction, maintaining the current in-plane position while adjusting only the depth.

Changes:

  • Updated setViewPlane() method to calculate a new focal point that preserves the lateral position while adjusting depth to match the measurement point
  • Added vector calculations to project the point along the view plane normal direction

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +809 to +814
const deltaNormal = vec3.scaleAndAdd(
vec3.create(),
focalPoint,
viewPlaneNormal,
alongNormal
) as Point3;
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name "deltaNormal" is misleading. This variable represents the new focal point projected onto the view plane normal direction, not a delta or change value. A more descriptive name would be "newFocalPoint" or "projectedFocalPoint" to accurately reflect what the variable contains. The calculation adds the projected distance along the normal to the current focal point, resulting in a new position, not a delta.

Copilot uses AI. Check for mistakes.
Comment on lines +806 to +814
const { focalPoint, viewPlaneNormal } = this.getCamera();
const deltaFocal = vec3.subtract(vec3.create(), point, focalPoint);
const alongNormal = vec3.dot(deltaFocal, viewPlaneNormal);
const deltaNormal = vec3.scaleAndAdd(
vec3.create(),
focalPoint,
viewPlaneNormal,
alongNormal
) as Point3;
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new focal point computation logic lacks inline comments explaining the mathematical approach. Adding comments would help future maintainers understand that this calculates the projection of the target point onto the current view plane normal direction, maintaining the same in-plane position while adjusting the distance along the normal. Consider adding a comment like: "Project the target point onto the view plane normal to maintain in-plane position while adjusting depth".

Copilot uses AI. Check for mistakes.
@wayfarer3130 wayfarer3130 merged commit cd9a217 into cornerstonejs:main Jan 30, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Image shift in MPR viewports when selecting measurements.

4 participants