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

displaying images (series) with different sizes causes the image to be clipped #304

Open
Zaid-Safadi opened this issue Sep 9, 2018 · 22 comments
Assignees

Comments

@Zaid-Safadi
Copy link
Contributor

When a series with different image's size is displayed in cornerstone (stack scroll behavior), the subsequent images might get clipped (if the first image is smaller).

For example, if a series with 3 images (128x128, 256x256, 512x512) is used to render images using cornerstone.displayImage (stack scroll) the 2nd and 3rd images will be clipped to 128x128.

The reason for this is when setting the first image, the displayedArea which is part of the viewport is initialized with the same image size:

displayedArea: {
tlhc: {
x: 1,
y: 1
},
brhc: {
x: image.columns,
y: image.rows
},

Subsequent calls to displayImage will use the original viewport and merge any new properties:

// Merge viewport
if (viewport) {
for (const attrname in viewport) {
if (viewport[attrname] !== null) {
enabledElement.viewport[attrname] = viewport[attrname];
}
}
}

Workaround
The user provide a viewport parameter in the displayImage method what includes the new image displayedArea initialized

@Zaid-Safadi
Copy link
Contributor Author

Possible solution
In the displayImage method, check if old viewport.displayedArea has the default old image values (image size...) then assume the user didn't change this and the new image size should be applied to the new viewport.displayedArea.

If the user wants to actually apply a displayedArea then it might be safe to assume a viewport parameter with a dsplayedArea will be passed to the displayImage method.

@chafey
Copy link
Collaborator

chafey commented Sep 10, 2018

Your solution is a bit too implicit for me - I would prefer a more explicit "fitToWindow" or "calculateDefaultViewportSettings" flag somewhere.

@Zaid-Safadi
Copy link
Contributor Author

Thanks for your input @chafey, I agree that the solution has assumptions/AI that I am not a big fan of as well but I am trying to avoid the need to update every component to be aware of what flag it should pass when calling displayImage.

For example, in cornerstoneTools the stack scroll will need to know now if the user like to pass this flag or not, as well as every place in OHIF Viewer.

A cleaner option to solve this might be not to initialize the displayedArea in the first place, then update the rendering code to calculate the default area every time if not explicitly set. Only issue with this (beside thorough testing) that it will break user code that depends on the displayedArea being always available -which might not be a big issue as this property has been only recently introduced.

Any thoughts?

@diaztula
Copy link

diaztula commented Nov 13, 2018

Hello,
Thanks for pointing out this, I'm having this problem too (several images within the same series with different sizes). I noted that in version 2.1.0 the problem is not present. I rolled back to version 2.1.0 but now I'm having problems with colored doppler US.
Cheers

@vdnsnkv
Copy link

vdnsnkv commented Nov 27, 2018

Hello!
I've been having this problem since I've updated to 2.2.7.
I've put together a simple example of this (based on changeImage example), with Image#2 four times larger than Image#1:
https://vdnsnkv.github.io/changeImage/

Also, I would really appreciate some help on how to get around this with version 2.2.7.

@Zaid-Safadi
Copy link
Contributor Author

Zaid-Safadi commented Nov 28, 2018

@diaztula/ @vdnsnkv , here is a temp fix you can use until this is resolved:
You can subscript to the cornerstonenewimage:
element.addEventListener('cornerstonenewimage', onNewImage);

Then in that event update the viewport:

eventData.viewport.displayedArea.brhc.x = eventData.image.width;
eventData.viewport.displayedArea.brhc.y = eventData.image.height;

@diaztula
Copy link

Thanks @Zaid-Safadi !

Cheers

@dannyrb
Copy link
Member

dannyrb commented Mar 24, 2019

@Zaid-Safadi, has this work-around been sufficient? Do you feel we need to address this in some way with a pull request to modify behavior, or would a note in the documentation for this do the trick?

@Zaid-Safadi
Copy link
Contributor Author

I think we need to fix this in the library some way or the other. Do you have any thoughts?

@JustinTQ
Copy link

I also have this problem. Can you tell me how to solve it? Thank you very much.

@dannyrb
Copy link
Member

dannyrb commented Apr 28, 2019

@JustinTQ: #304 (comment)

I don't believe we've landed on an official solution. This is something I feel comfortable leaving for a viewer to implement, but it appears to be a common issue. I'm trying to think of all the scenarios where we would want to change this value.

I've only encountered this issue when a series contains a pilot image. We manually reset the viewport when loading a new series, so I would only see this between individual images in the same series/stack. Most series/stack want to be able to leverage CINE, and automatically resizing by default could be problematic.

I like @Zaid-Safadi's workaround solution, but could see it as a toggle on/off tool/setting for one or more enabledElements. I'm not sure how we would go about implementing something that fits the bulk of use cases in core without accessing tool data.

@Zaid-Safadi
Copy link
Contributor Author

Zaid-Safadi commented Jan 6, 2020

Anyone can poke holes with this solution?

  • Add a flag on the displayedArea property called "mode" with two values "Default" and "Custom"
  • If "Default" (which is the default value) the code ignores the values set in the displayedArea and just assumes the full image size
  • if "Custom" the code does what it does now and uses the values set in the "displayedArea" to calculate the displayed image size

This means that the code by default will work as if the "displayedArea" doesn't even exists -before introducing the bug- and if the user wants to use the "displayedArea" then set the flag to Custom and provide the right values to the code

@b-wils
Copy link

b-wils commented May 7, 2020

The solution from @Zaid-Safadi got me most of the way there, but the scale was off. Calling fitToWindow after setting displayedArea did the trick.

@kevinleedrum
Copy link

We just ran into this same problem due to a 512x512 localizer image being the first image in a stack of 888x888 derived MPR images. I had a workaround similar to @Zaid-Safadi, but his is much cleaner. 👍

@kevinleedrum
Copy link

Also, IMO, whether or not images are scaled proportionately while changing images vs. maintaining a consistent zoom level should be left up to the implementation via the cornerstonenewimage handler.

However, I do think reusing the same displayedArea for all images in a stack is a bug. Either the displayedArea should always be reset to the default viewport's values, or the displayedArea should be removed from the viewport and handled elsewhere somehow.

@Zaid-Safadi
Copy link
Contributor Author

I narrowed this down to 3 options and have an implementation for solution 1 below which I plan to submit a PR for, before I go ahead though, I wanted to put my reasoning and see if anyone has constructive thoughts:

Solution 1: By default, don't create a "displayedArea" in the viewport object and let user provides one if needed. otherwise, the code will use the full image dimensions

Pros:

  • Cleanest (IMO), someone explicitly setting the displayedArea knows what they are doing and can reset it for new images if displayedArea should be different.

Cons:

  • Any code assuming the viewport.displayedArea is created by default and accessing directly (such as applying my workaround above) will break.

Solution 2: Add a flag to the displayedArea.apply which is 'false' by default, unless this flag is set to 'true' the code will ignore the displayedArea and use the image dimensions

Pros:

  • Doesn't cause existing code accessing the displayedArea directly to break

Cons:

  • Breaks existing code setting the displayedArea without updating the new flag to 'true'

Solution 3: Add a flag to the 'displayedArea.applyToAllImages' which is 'false' by default. If flag is 'false' the displayedArea will be reset in the "displayImage" (unless one is provided)

Pros:

  • Doesn't break existing code accessing the displayedArea directly
  • Doesn't break existing code setting the 'displayedArea' for the first image

Cons:

  • May not fix the problem for code that always get existing viewport and set it in the update image (cornerstoneTools Stack?)
  • Break the code logic for someone assuming the 'displayedArea' set on the viewport will work on all the stack images

@leonardorame
Copy link

Hi, I stumbled upon a similar issue. In a CR image stack there are images with different WW/WC, when I scroll through it all images are displayed using the viewport of the 1nst on the stack.

@chafey
Copy link
Collaborator

chafey commented Feb 26, 2021 via email

@leonardorame
Copy link

leonardorame commented Feb 26, 2021

Yes, I also think the correct behavior is that. But in my case the image's voi are all set to the 1nst image of the stack even if the user doesn't change anything.

I'm using the stack tool of CornerstoneTools, maybe the issue is caused by it.

I noticed something weird. I attached an NEW_IMAGE event handler and found the viewport's voi returned by cornerstone.getDefaultViewportForImage is always the same, even when looking at the image's dicom header I can see them are different.

@leonardorame
Copy link

Here's a screenshot of the issue, as you can see the WW/WC of the left image is different thant the right one:

image

Now I also see I cannot change the window level manually and that, I think, is because the images have a voi lut defined.

BTW, the screenshot below is taken from Weasis, and is displayed correctly.

image

@leonardorame
Copy link

If I assign undefined to viewport.voiLUT the images are displayed exactly as in Weasis.

image.

I'll post a question in CornerstoneWADOImageLoader asking for a way to globally ignore voiLUT, or should I ask that here?.

@kevinleedrum
Copy link

@leonardorame See cornerstonejs/cornerstoneTools#209. I think it may be what you're describing.

swederik pushed a commit that referenced this issue Sep 13, 2021
#462)

* fix: displaying images (series) with different sizes causes the image to be clipped #304

* add example to display multiple images with different sizes
swederik added a commit that referenced this issue Sep 13, 2021
…#462) (#465)

* fix: displaying images (series) with different sizes causes the image… (#462)

* fix: displaying images (series) with different sizes causes the image to be clipped #304

* add example to display multiple images with different sizes

* fix createViewport test

Co-authored-by: swederik <erik.sweed@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants