cornerstonejs / cornerstone

JavaScript library to display interactive medical images including but not limited to DICOM
https://docs.cornerstonejs.org/
MIT License
2.06k stars 598 forks source link

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

Open Zaid-Safadi opened 6 years ago

Zaid-Safadi commented 6 years ago

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: https://github.com/cornerstonejs/cornerstone/blob/12da9ed267499345ef192d9e5f214cc79122ffc3/src/internal/getDefaultViewport.js#L102-L110

Subsequent calls to displayImage will use the original viewport and merge any new properties: https://github.com/cornerstonejs/cornerstone/blob/12da9ed267499345ef192d9e5f214cc79122ffc3/src/displayImage.js#L41-L48

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

Zaid-Safadi commented 6 years ago

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 commented 6 years ago

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

Zaid-Safadi commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

Thanks @Zaid-Safadi !

Cheers

dannyrb commented 5 years ago

@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 commented 5 years ago

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

JustinTQ commented 5 years ago

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

dannyrb commented 5 years ago

@JustinTQ: https://github.com/cornerstonejs/cornerstone/issues/304#issuecomment-442268445

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 commented 4 years ago

Anyone can poke holes with this solution?

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

Cons:


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:

Cons:


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:

Cons:

leonardorame commented 3 years ago

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 commented 3 years ago

I thought the behavior originally was for stacks to use the image frame specific WW/WC UNLESS the WW/WC had manually been changed. This is the correct default behavior for most use cases as the WW/WC can and does change for images in a stack (MR and PET in particular tend do this - not CT though)

On Fri, Feb 26, 2021 at 5:25 AM Leonardo M. Ramé notifications@github.com wrote:

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cornerstonejs/cornerstone/issues/304#issuecomment-786589050, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJVXWR5HEE2UQC46FY42Q3TA6AL5ANCNFSM4FUBRHFQ .

leonardorame commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@leonardorame See https://github.com/cornerstonejs/cornerstoneTools/issues/209. I think it may be what you're describing.