-
Notifications
You must be signed in to change notification settings - Fork 327
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
feat: added display area to viewport #280
feat: added display area to viewport #280
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
826d73e
to
76897bf
Compare
daf7c4a
to
a5472cf
Compare
Let me know what you think, then I can review another round |
Hey Alireza, thanks for the review!
|
This is too confusing for an api. Why it switches the definition from top left (in 1, 1) to bottom right in (0,0)? |
After debugging, I found the issue with the canvasToWorld. It seems reset camera is called on |
a5472cf
to
c52a539
Compare
* fix: Re IDC #2761 fix loading of segmentations * fix: 🐛 use metadataProvider option instead of cornerstone.metaData in Segmentation_4X
b7a39e3
to
147f775
Compare
52f3d5f
to
0cb7814
Compare
@sedghi @wayfarer3130 the PR is ready for review |
@@ -426,6 +447,7 @@ enum Events { | |||
CAMERA_MODIFIED = 'CORNERSTONE_CAMERA_MODIFIED', | |||
|
|||
CAMERA_RESET = 'CORNERSTONE_CAMERA_RESET', | |||
DISPLAY_AREA_MODIFIED = 'CORNERSTONE_DISPLAY_AREA_MODIFIED', |
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.
I don't think this should have it's own even - there should be a way to get properties changed, or this should trigger camera modified events. @sedghi - any thoughts about the event? Other properties don't have their own events.
@@ -1078,6 +1103,11 @@ interface IViewport { | |||
reset(immediate: boolean): void; | |||
setActors(actors: Array<ActorEntry>): void; | |||
setCamera(cameraInterface: ICamera, storeAsInitialCamera?: boolean): void; | |||
setDisplayArea( |
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.
When there are more than one option, the options should go into a typed options property so that the number of arguments doesn't change.
@@ -104,6 +106,161 @@ addButtonToToolbar({ | |||
}, | |||
}); | |||
|
|||
addButtonToToolbar({ |
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.
I really like these examples - it makes it clear how this works
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.
thanks!
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.
There are a few issues to update here.
|
||
if (imageArea) { | ||
const { areaX, areaY } = imageArea; | ||
const zoom = Math.min(this.getZoom() / areaX, this.getZoom() / areaY); |
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.
This is not the correct calculation - the zoom level is:
const zoomHorizontal = canvas.height / (areaX * imageWidthInPixels)
const zoomVertical = canvas.width / (areaX * imageWidthInPixels)
then as an absolute zoom amount, you want:
setZoom(getZoom()/Math.min(zoomHorizontal, zoomVertical))
assuming that zoom 1 fills the smallest of horizontal and vertical spacing.
/** The camera that is defined for resetting displayArea to ensure absolute displayArea | ||
* settings | ||
*/ | ||
private fitToCanvasCamera: ICamera; |
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.
I wonder if this is a better option than initialCamera? I might get rid of initialCamera in favour of fitToCanvasCamera, and also get rid of the reset initial camera stuff - I think it actually makes more sense.
0cb7814
to
49b5448
Compare
@wayfarer3130, let me know what you think of these changes! |
Could we add some documentation on what is what exactly with the examples you have on the demo (Top, etc.) in here as well please? https://www.cornerstonejs.org/docs/concepts/cornerstone-core/viewports |
Sure I can add some documentation on this |
49b5448
to
adca884
Compare
|
||
In order to zoom into the image 200%, we would set the `imageArea` to [0.5, 0.5]. | ||
|
||
Panning is controlled by a provided `imagePoint` and a provided `canvasPoint`. You can imagine the canvas as a sheet of white paper and the image as another sheet of paper like a chest x-ray. Mark a point in the canvas paper with a pen and then mark another point on your chest x-ray image. Now try to "pan" your image so the point so the `imagePoint` matches the |
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.
Great docs indeed
Adds initial display area to viewport.
imageArea
takes aimageX
andimageY
which should be a number (0,1] which determines the % of image to display.imageFocalPoint
takes afocalX
andfocalY
which determines the camera focal point.focalX=1
andfocalY=1
means the image is in the bottom right corner offscreen.focalX=0
andfocalY=0
refers to the upper left corner offscreen andfocalX=0.5
andfocalY=0.5
is the default center image. These numbers represent the XY translation amount as a % of the image width/heightThe displayArea will save the stored camera and functions properly for stack viewport. The display area can also be programmatically set with or without saving the stored camera. One use case for programmatic display area setting is to automatically zoom in on a ROI.
Implementation note: setPan is not available despite the existence of imageData and vtkRenderer. It seems the off screen renderer requires some initial imageData render before the
displayToWorld
function is available to the viewer instance. Since the setPan function requirescanvasToWorld
which requiresdisplayToWorld
we do not usesetPan
in the implementation of setDisplayArea. Instead we use the imageData.indexToWorld function is used instead to calculate the delta vector which the camera should move. This delta vector is doubled so the image can translate completely out of the viewport rather than just 50% (since radius is calculated to fit image).