-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Fix: Jump to measurement from measurement panel after rotating viewport #5090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Jump to measurement from measurement panel after rotating viewport #5090
Conversation
✅ Deploy Preview for ohif-dev canceled.
|
|
@sedghi Could you please review and merge this PR? |
jbocce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nithin-trenser please update your branch and resolve the conflicts. Otherwise it looks good! Thanks for your contribution.
|
@nithin-trenser, We are planning a code freeze at the end of this week. So please make updates soon to have this include in the release. Thanks for you contribution. |
|
@jbocce Conflicts resolved and committed. Could you please re-review and approve? |
sedghi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
platform/app should not use cornerstoneViewportService
|
Upon revisiting the issue, it was observed that the implementation of |
dd66c09 to
df40ffb
Compare
|
On further analysis found the below issues
Observed Behavior
maxium_call_stack_error.mp4Note
working-scenario-jump-mpr.mp4Issue 1: Measurement jump occurs in the active viewport Root Cause:
When the viewport is rotated using the crosshair, navigation without orientation change is no longer possible. As a result, the logic falls back to selecting the active viewport that can change orientation, which is incorrectly chosen as the best fit. Fix Issue 2: Maximum call stack exceeded error when crosshair is enabled Root Cause
After applying the orientation, Fix I have: Created a PR in CS3D cornerstonejs/cornerstone3D#2519 to fix the maximum call stack exceeded issue. @sedghi Could you please review both this PR and the corresponding CS3D PR? |
|
@wayfarer3130 is reviewing these changes, will check and then merge if it looks good. I was noticing this hang as well, so I'm glad to have this in for the next release. |
extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts
Outdated
Show resolved
Hide resolved
extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts
Outdated
Show resolved
Hide resolved
wayfarer3130
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two relatively small changes - extract the sort by closest orientation viewport logic so it can be re-used in #5630 and use the planeRestriction instead of the view plane normal, as the planeRestriction object inside the metadata is more consistently created. Note you may have the second inPlaneVector absent (or even both of them).
The logic is
alignmentScore = Math.abs(vec3.dot(inPlaneVector1,viewPlaneNormal) + Math.abs(vec3.dot(inPlaneVector2,viewPlaneNormal))
where the two dot products default to 0 when the in plane vector is absent.
Context
Changes & Results
The issue was resolved by resetting the viewport to the plane where the measurement is located and highlighting it when a measurement is selected from the measurement panel.
jump-to-measurement-cross-hair.mp4
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment