-
Notifications
You must be signed in to change notification settings - Fork 334
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 for elliptical roi #479
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
const rotation = Math.abs(viewport.getRotation() - data.initialRotation); | ||
let canvasCorners; | ||
|
||
if (rotation == 90 || rotation == 270) { |
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.
Can you check the save/restore in the adapters please?
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.
Is this what you are referring to?
would you like me to follow this approach here too?
I'm not sure what you mean by save/restore
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.
No, I want you to run OHIF, create some markup with different initial rotations, then save it, and see if it still displays the same object after it loads back in. You may need to do something special in the adapter to make sure it gets saved/restored correctly.
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.
Yeah it doesn’t save correctly, I’ll work on this and update you.
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.
It works as expected now, before saving, If its already rotated, I convert so the rotation is 0, so when its restored we won't need the rotation value since it will always be 0.
FYI the DICOMSRDisplayTool in OHIF needs to change so it renders properly, only the cornerstone native tool renders properly now.
It renders fine in my GIF because I modified the DICOMSRDisplayTool locally to account for the rotation the same way I did it for the native cornerstone tool. so this below has to be added to the DICOMSRDisplayTool
const rotation = viewport.getRotation();
canvasCoordinates = ellipsePointsWorld.map(p =>
viewport.worldToCanvas(p)
);
let canvasCorners;
if (rotation == 90 || rotation == 270) {
canvasCorners = <Array<Types.Point2>>(
utilities.math.ellipse.getCanvasEllipseCorners([
canvasCoordinates[2],
canvasCoordinates[3],
canvasCoordinates[0],
canvasCoordinates[1],
])
);
} else {
canvasCorners = <Array<Types.Point2>>(
utilities.math.ellipse.getCanvasEllipseCorners(canvasCoordinates)
);
}
Thank u
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.
Tested, and seems to work consistently now. The fixes for saving look good.
This fixes the elliptical roi tool not rendering properly when rotation happens, I added an initial rotation attribute to help determine where the bottom/left/top/right attributes are located in the canvasCoordinates array.
[bottom, top, left, right] would become [left, right, bottom, top] when rotated
this function expects the order to be [bottom. top, left. right], so we need to re-order the canvas coordinates for it if there's a rotation, we can't pass it the array as it is when it's rotated.
this is a more simple fix than the one I proposed previously in my other pull request, let me know if there's a better approach of doing this.