Open allanbarklie opened 1 year ago
Thanks, I will look at this.
@sedghi Thanks for progressing this issue - I've tested the linked pull request and I have some feedback.
I tested the fix in an application we have that can switch between several rendering implementations including both the old cornerstone and cornerstone3D.
The remaining error is much more subtle, mainly a very small zoom change that causes ~1 pixel of cropping around a 512x512 image and also a slight upscaling of the image.
I took a look at the code and tried a few things.
In the file cornerstone3D/packages/core/src/RenderingEngine/Viewport.ts there is a section that tweaks the bounding box just before it is used as part of the initial scaling calculation.
// Todo: remove this, this is just for tests passing
if (imageData) {
const spc = imageData.getSpacing();
bounds[0] = bounds[0] + spc[0] / 2;
bounds[1] = bounds[1] - spc[0] / 2;
bounds[2] = bounds[2] + spc[1] / 2;
bounds[3] = bounds[3] - spc[1] / 2;
bounds[4] = bounds[4] + spc[2] / 2;
bounds[5] = bounds[5] - spc[2] / 2;
}
This looked interesting so I commented it out.
// Todo: remove this, this is just for tests passing
/* if (imageData) {
const spc = imageData.getSpacing();
bounds[0] = bounds[0] + spc[0] / 2;
bounds[1] = bounds[1] - spc[0] / 2;
bounds[2] = bounds[2] + spc[1] / 2;
bounds[3] = bounds[3] - spc[1] / 2;
bounds[4] = bounds[4] + spc[2] / 2;
bounds[5] = bounds[5] - spc[2] / 2;
}*/
This corrected the behaviour and the image now seemed to be correct and a match for other renderers including the old cornerstone. This looks to be worth understanding and fixing.
As I was looking around I spotted a couple of other changes that are more speculative. They didn't have noticable effect but I thought I would mention them as well.
In the same area of code there is a line:
const middleIJK = dimensions.map((d) => Math.floor(d / 2));
I wonder if this would be better as:
const middleIJK = dimensions.map((d) => (d-1)/2);
I don't see a reason to floor the values as they are passed on to a function that does a floating point conversion. I think the values need to be shifted by 1 to go from absolute sizes to data space. So in my version the centre of something with dimensions 512, 512, 1 would be 255.5, 255.5, 0, representing the centre of 0...511, 0...511, 0.
I also saw the line:
const distance = 1.1 * radius;
This is a different occurence of 1.1 from the one removed in the pull request.
Following the reference
https://github.com/Kitware/vtk-js/blob/5ec65e9d9ad06ef4d6450df75cb1f7f15ed11501/Sources/Rendering/Core/Renderer/index.js
I wonder of this line would be better as:
//assumes viewing angle of 90 degrees
const distance = radius * Math.SQRT2;
@allanbarklie I think your suggestions makes sense. I saw for the bounds manipulation it has a comment that says only to make tests pass, which is ok i will remove and re-capture diff images.
For the const middleIJK = dimensions.map((d) => Math.floor(d / 2));
we were talking with @wayfarer3130 and we agree there should not be any flooring happening, but at the same time we think d/2 is correct and does not need to be changed to d-1 / 2
for the following example, it makes more sense to me d/2
Any thoughts?
Also would love to hear why you think const distance = radius * Math.SQRT2;
is better
@sedghi
Thanks for checking back
Sounds good on the test stuff and the floor removal.
I'll try and explain my other points a bit better and include whatever caveats I may have relied on or guesses I might have made.
My understanding is that we initially get the values for d by asking for dimensions. These dimensions are not in the same coordinate space as we need later. Much like in many languages an array will return a size or length that is not a valid index for that array, I think the dimensions have a plus 1 difference from the space we need.
If we have a single 8x8 image we would get a dimension result where: (x,y,z) is (8,8,1)
However we might repesent such an image in a view or on a canvas more like:
0 1 2 3 4 5 6 7 1 2 3 4 5 6 7
Where we define the origin to be in one corner and the highest valid index for either x or y is 7. Caveat - I couldn't find a standalone definition of the coordinate spaces used in the codebase so there is some guess work here.
We want to find the centre of this image - which would be (3.5,3.5) Extending the same pattern to 3d would give us (3.5,3.5,0)
This is the value we get if we use (d-1)/ 2.
If we use d/2 we get (4,4,0.5).
Extending this to a 8x8x8 cube we would get: (d-1)/ 2 --> (3.5.3.5,3.5)
d/2 --> (4,4,4)
This is the thinking that I went through - I also did some debugging that showed that dimensions were returned in that form.
The commit https://github.com/cornerstonejs/cornerstone3D/pull/743/commits/2da8c4eee8396f8d12772b24bf5467b35fe181ac adds the comment: // parallel scale should be equal to the radius to make the image // fit the window see https://github.com/Kitware/vtk-js/blob/5ec65e9d9ad06ef4d6450df75cb1f7f15ed11501/Sources/Rendering/Core/Renderer/index.js
If we follow that link we can see a method: publicAPI.resetCamera
which includes the lines: const angle = vtkMath.radiansFromDegrees(model.activeCamera.getViewAngle()); const parallelScale = radius; const distance = radius / Math.sin(angle * 0.5);
In our case we have: viewAngle: 90,
So my thinking was: 90 degrees = PI/2 const distance = radius / Math.sin(PI/2 0.5); const distance = radius / Math.sin(PI/4); const distance = radius / ((SQRT2)/2) const distance = radius 2/(SQRT2) const distance = radius * SQRT2
Caveat: I've quickly assumed that the distance calculation in that link is valid for the thing we are trying to do - I haven't checked that assumption in any detail.
I hope this helps - I don't know the codebase well so these are really my best quick guesses - they may be missing some context.
Hi @sedghi ,
This issue still appears to exist in the latest version of Cornerstone3D. We have created a minimal demo to show this behaviour, please see this example: https://jsfiddle.net/92ez5to8/
It can also be seen in the basic stack live example: https://www.cornerstonejs.org/live-examples/stackbasic
In both examples with default settings, there is a clear border all the way around the image that can be seen in the viewport, showing that the image is not fitting to the view.
The behaviour has also been documented (and fixed at the time!) in this previously closed PR: https://github.com/cornerstonejs/cornerstone3D/pull/743
I suspect since then, some changes in the Viewport have introduced an insetImageMultiplier which is producing the same effect as before with a default inset multiplier of 1.1 . It appears the inset cannot be set in the Viewport API?
Could you please look into this?
For some context, this is important for our application as we wish to know precisely where the data is positioned on screen, for instance to be able to accurately overlay measurements on top of it. If the design of Cornerstone will not change this default behaviour so data fits view exactly, a possible workaround for us is to know exactly how the default camera/scale is being set so we know how to compensate.
Thanks in advance!
@canon-cmre-jess-mcintosh Sure I will look, it is a great to see how you run cornerstone in jsfiddle pretty cool
Hi @sedghi I noticed that if after creating the viewport we do the following then the data seems to fit view exactly: viewport.setDisplayArea({ imageArea: [1, 1], storeAsInitialCamera: true }); By the way the following docs claim that "By default, the viewport will fit the dicom image to the screen." Maybe that's fair enough? They don't specify a fit that causes image to fill the viewport exactly. https://www.cornerstonejs.org/docs/concepts/cornerstone-core/viewports#initial-display-area
That is the expected method to set the display area to scale to fit to fill to show exactly 100% width or height.
Describe the Bug
Applications written using the framework do not display 2D images at the expected default zoom. Instead images appear to be zoomed out by a small amount. This is different behaviour than the old cornerstone framework.
For example a 2D stack view using StackViewPort does not fit a 512x512 image exactly to a 512x512 viewport.
(This report started as a slack discussion with @sedghi)
Steps to Reproduce
The current behavior
The rendering of the image data does not fill the 512x512 view. The displayed image contains a rendering of the data within an invisible bounding box that is <512x512 All pixels outside of this bounding box are rendered at the background color. The image looks like an image that has been slightly zoomed out.
The expected behavior
The rendering of the data should completely fill the 512x512 view. Pixels on the edge of the image data should correlate to pixels on the edge of the displayed view. In a simple set up where devicepixelratio =1 (e.g. 96dpi display and default 100% brower zoom) there should be a notional 1:1 mapping of pixel data from the DICOM to pixels displayed in the view (ignoring anything the OS/graphics driver/display do at a lower level).
OS
Windows 10
Node version
20.5.0
Browser
chrome 115.0.5790.171