Skip to content
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

Conversation

Ouwen
Copy link
Contributor

@Ouwen Ouwen commented Nov 4, 2022

Adds initial display area to viewport.

imageArea takes a imageX and imageY which should be a number (0,1] which determines the % of image to display.
imageFocalPoint takes a focalX and focalY which determines the camera focal point. focalX=1 and focalY=1 means the image is in the bottom right corner offscreen. focalX=0 and focalY=0 refers to the upper left corner offscreen and focalX=0.5 and focalY=0.5 is the default center image. These numbers represent the XY translation amount as a % of the image width/height

The 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 requires canvasToWorld which requires displayToWorld we do not use setPan 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).

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 3df7e0d
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/6436161cfec5310008ca1f95
😎 Deploy Preview https://deploy-preview-280--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Ouwen Ouwen force-pushed the gradienthealth/added_initial_display_area branch 2 times, most recently from 826d73e to 76897bf Compare November 4, 2022 09:20
@Ouwen Ouwen force-pushed the gradienthealth/added_initial_display_area branch 2 times, most recently from daf7c4a to a5472cf Compare November 6, 2022 00:35
@sedghi
Copy link
Member

sedghi commented Nov 9, 2022

  1. focalX=1 and focalY=1 means the image is in the bottom right corner offscreen. what do you mean? which part of image is in the right corner? center of the image is in the bottom right?

  2. Is there any reason you are not using just a flat object for displayedArea? {imageX, imageY, focalX, focalY}

  3. Having focal point at the surface api mean that we are assuming the user knows the focal point is the center of the image, can we rename it to something else from focalX to something else (like offsetX or etc.)? Also I guess imageX does not represent that is %. We used to have topLeftHandCorner but I guess the benefit of % is more.

Let me know what you think, then I can review another round

@Ouwen
Copy link
Contributor Author

Ouwen commented Nov 9, 2022

Hey Alireza, thanks for the review!

  1. focalX=1, focalY=1 will set the top left corner of the image at the bottom right of the screen,and the image will not be visible on canvas. focalX=0, focalY=0 will set the bottom right corner of the image to the top left of the screen, and the image will not be visible on canvas.

  2. We can make it flat

  3. Yeah there might be a better name possible, effectively it is what % the center of the image will be on screen. Ideally we can use the pan functionality, but it seems like at constructor time the vtkoffsetrenderer is not initialized.

@sedghi
Copy link
Member

sedghi commented Nov 9, 2022

focalX=1, focalY=1 will set the top left corner of the image at the bottom right of the screen,and the image will not be visible on canvas. focalX=0, focalY=0 will set the bottom right corner of the image to the top left of the screen, and the image will not be visible on canvas.

This is too confusing for an api. Why it switches the definition from top left (in 1, 1) to bottom right in (0,0)?

@Ouwen
Copy link
Contributor Author

Ouwen commented Nov 14, 2022

After debugging, I found the issue with the canvasToWorld. It seems reset camera is called on addVtkjsDrivenViewport
Then another reset camera is called on addActors since vtkImageData is not present. On the third call which reuses the vtkImageData object in stackviewport, is canvasToWorld functional

@Ouwen Ouwen force-pushed the gradienthealth/added_initial_display_area branch from a5472cf to c52a539 Compare November 19, 2022 05:33
wayfarer3130 pushed a commit that referenced this pull request Jan 20, 2023
* fix: Re IDC #2761 fix loading of segmentations

* fix: 🐛 use metadataProvider option instead of cornerstone.metaData in Segmentation_4X
@Ouwen Ouwen force-pushed the gradienthealth/added_initial_display_area branch 2 times, most recently from b7a39e3 to 147f775 Compare February 4, 2023 05:44
@Ouwen Ouwen force-pushed the gradienthealth/added_initial_display_area branch 2 times, most recently from 52f3d5f to 0cb7814 Compare March 18, 2023 02:13
@Ouwen
Copy link
Contributor Author

Ouwen commented Mar 18, 2023

@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',
Copy link
Collaborator

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(
Copy link
Collaborator

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({
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

Copy link
Collaborator

@wayfarer3130 wayfarer3130 left a 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);
Copy link
Collaborator

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;
Copy link
Collaborator

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.

packages/core/src/RenderingEngine/Viewport.ts Show resolved Hide resolved
packages/core/src/enums/Events.ts Show resolved Hide resolved
packages/core/src/types/displayArea.ts Outdated Show resolved Hide resolved
@Ouwen Ouwen force-pushed the gradienthealth/added_initial_display_area branch from 0cb7814 to 49b5448 Compare March 29, 2023 21:18
@Ouwen
Copy link
Contributor Author

Ouwen commented Mar 29, 2023

@wayfarer3130, let me know what you think of these changes!

@sedghi
Copy link
Member

sedghi commented Apr 4, 2023

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

@Ouwen
Copy link
Contributor Author

Ouwen commented Apr 11, 2023

Sure I can add some documentation on this

@Ouwen Ouwen force-pushed the gradienthealth/added_initial_display_area branch from 49b5448 to adca884 Compare April 11, 2023 23:56

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
Copy link
Member

Choose a reason for hiding this comment

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

Great docs indeed

@wayfarer3130 wayfarer3130 merged commit ec64803 into cornerstonejs:main Apr 12, 2023
10 checks passed
@wayfarer3130 wayfarer3130 deleted the gradienthealth/added_initial_display_area branch April 12, 2023 19:05
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.

None yet

3 participants