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

Improve screenshot options and capturing #18

Closed
3 tasks
aashish24 opened this issue Jun 4, 2021 · 4 comments · Fixed by #79
Closed
3 tasks

Improve screenshot options and capturing #18

aashish24 opened this issue Jun 4, 2021 · 4 comments · Fixed by #79
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@aashish24
Copy link

aashish24 commented Jun 4, 2021

  • Enable a option (via admin interface) to set the height of the screenshot. Width of the screenshot will be computed from the aspect ratio.
  • The screenshot needs to capture only the data outline (without the background). Hence the size needs correspond to the data bounds and not to the viewport. Below is the message from Jim from Stanford.
  • The screen shot issue is somewhat fixed. 512x512 max proportions seems fine. The problem we seem is that there is too much black, and the actual MRI image is very small and low resolution in comparison to the image size embedded in the email.
    Image sample from 4K browser

The browser size and host resolution seems to also still play a variable, as when I took snapshots using my iPad, the scan better fills the 512x512 box, however the image resolution seems low the size.
Image Sample from iPad Pro 12.9"

@aashish24
Copy link
Author

Follow on of #4

@jimklo
Copy link
Collaborator

jimklo commented Jun 4, 2021

@aashish24 regarding:

Enable a option (via admin interface) to set the width of the screenshot. Height of the screenshot will be computed from the aspect ratio.

I would suggest supplying both a height and a width constraints. It should be possible to compute which way to scale based upon the the aspect ratio of the image.

ie:

  • 4:3 aspect ratio ( > 1 ) would use width constraint
  • 3:4 aspect ratio ( < 1 ) would use height constraint

This way we might want to set height constraint to 512, but width constraint to 800.

If you only want to set only one, choose height in lieu of width. Vertical space for most viewers is more limited than horizontal, Constraining the width could cause portrait oriented scan to resize much taller than desired, however a landscape image might be much smaller than desired.

For Kilian's perspective, when reviewing screenshots via email, the vertical space is most important as he needs to be able to see the notes with the image without vertical scrolling. So in this case of a 4:3 viewport... having an image with a fixed height of 512 px (resulting in 683 px wide) would be preferred over fixing the width at 512, and ending up with an image thats 384 px tall.

@aashish24
Copy link
Author

Thanks, these are excellent thoughts. I think from the simplicity point of view, I am in favor of just one so height would be the best pick as per your comment. Also, please note that I am hoping that we can get rid of most of the background space which will require camera to change its parameters. Would that be okay?

@aashish24
Copy link
Author

I have sent an email to the vtk.js developers team to this end and now waiting for their evaluation.

@aashish24 aashish24 added blocked bug Something isn't working enhancement New feature or request labels Jun 16, 2021
@aashish24 aashish24 removed the blocked label Jul 8, 2021
@aashish24 aashish24 added this to the Sprint August 1-15 milestone Jul 29, 2021
@dchiquito dchiquito linked a pull request Jul 29, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants