Incorrect volume mapper bounds

sedghi commented 3 years ago

High-level description

Volume mapper bounds are incorrect

Steps to reproduce

This is a bug for rendering the first and last slice.

It is visible on any sample data (headsq.vti etc). Also, I have created a synthetic dicom data set with long stripes in each slice along the y-axis, which change position for each slice (illustration below). In addition, if we manually change the intensity values of the first and last slice in headsq.vti, we can see the cut at the edges.

Expected behavior

Bounds should be to the edge of the volume.

Environment

vtk.js: 17.xx, 18.xx
OS: MacOS
Browser: Chrome 90.0.4430.212

Code Sandbox link

https://codesandbox.io/s/volume-rendering-bounds-exp9h?file=/src/index.js

headsq.vti with the changed intensity value for the first and last slice

As seen, the slice thickness is not consistent and the last slice is thinner (as if bounds are forcing it to get cut)
If interpolation is nearest, the last slice is gone

With linear interpolation

With nearest interpolation

This becomes more clear if we intentionally increase the slice thickness (below screenshot is from local running of demo, not in sandbox)

Test dicom files can be downloaded from my dropbox (9kb)-10 slices and can be dropped into paraview glance for volume rendering as well

Synthetic data illustration

Paraview glance volume rendering on the left, with the slice rendering on the right

sedghi commented 3 years ago

I have tried 3D Slicer for volume rendering and I'm getting different renderings depending on CPU, GPU or Multi-volume.

I recorded my screen and you can see it here

The data I used can be find here (same as before)

https://user-images.githubusercontent.com/7490180/119839031-da9b8680-bed1-11eb-8ab5-a8e768d8d00f.mp4

sedghi commented 3 years ago

Dropping the nrrd into the paraview glance + turning on the slice mappers shows some accumulated error

Taking a look into the OpenGL volumeMapper here. @floryst Is there any reason vsize definition is not consistent in the code?

In replaceShaderValues we have

const ext = model.currentInput.getExtent();
const spc = model.currentInput.getSpacing(); 
const vsize = new Float64Array(3);
vec3.set(
  vsize, 
 (ext[1] - ext[0]) * spc[0],
 (ext[3] - ext[2]) * spc[1],
 (ext[5] - ext[4]) * spc[2]);
const maxSamples =      vec3.length(vsize) / model.renderable.getSampleDistance();

In getNeedToRebuildShaders we have:

const ext = model.currentInput.getExtent();
const spc = model.currentInput.getSpacing(); 
const vsize = new Float64Array(3);
vec3.set(
  vsize, 
 (ext[1] - ext[0]) * spc[0],
 (ext[3] - ext[2]) * spc[1],
 (ext[5] - ext[4]) * spc[2]);
const maxSamples =  vec3.length(vsize) / model.renderable.getSampleDistance();

Lastly, in setCameraShaderParameters we have:

const ext = model.currentInput.getExtent();
const spc = model.currentInput.getSpacing(); 
const vsize = new Float64Array(3);
vec3.set(
  vsize, 
 (ext[1] - ext[0] + 1) * spc[0],
 (ext[3] - ext[2] + 1) * spc[1],
 (ext[5] - ext[4] + 1) * spc[2]);
...
...
vec3.set(vctoijk, 1.0, 1.0, 1.0);
vec3.divide(vctoijk, vctoijk, vsize);
program.setUniform3f('vVCToIJK', vctoijk[0], vctoijk[1], vctoijk[2]);

The last one makes a wrong vVCToIJK matrix as a result which is used in the shader.

Removing the +1s from the last vsize definition, results in a correct rendering.

Before: chrome-capture (8)

After: chrome-capture (7)

Attachements:

agirault commented 3 years ago

@sedghi thank you very much for the thorough bug report! That's very appreciated.

@martinken looks like the +1 was introduced here to fix some other bug: https://github.com/Kitware/vtk-js/commit/dbd4dac3f28208da8ad4592bebc6bc087a2b942e. Is there a test or reference to the bug in question somewhere, to make sure reverting wouldn't just break something else?

Also noting that we should be careful of using the ImageMapper as the ground truth for bounds because of #1553

sedghi commented 3 years ago

So the vsize and the +1 that is introduced might be the problem here, Something that is also wrong and might be the actual bug is the location of the volume in space

Data: 10 slices, each slice has a bar with value of 255 (explanation above)

If you visualize the data in 3D slicer (vtk c++) and the first and last voxel center pos (two spheres) we get:

I understand volume rendering does not render half of the first and last voxel (8 full squares and 2 half ones), but looking at the vtk.js example I created codeSandbox here we can see that only 9 squares are showing up, but more importantly is how volume is rendered between the sphere corners which should be similar to 3D slicer view (8 full and 2 half ones)

My point is, vtk c++ and vtk js should be able to give a consistent rendering even if it is missing half of voxel from top and bottom (which I guess is next step after solving this inconsistency)

agirault commented 3 years ago

vtk c++ and vtk js should be able to give a consistent rendering

What do you get in Paraview? Slicer has a layer over the vtkImageData to add things like orientation, and might be actually doing some work to counter the #1553 issue (also seen in paraview here)

agirault commented 3 years ago

Hi @sedghi, @swederik.

We discussed the state of your change last week: as mentioned in the PR, we envision more changes to do things properly and consistently with the image data, mapper, and volume mapper. The changes we plan to do involve:

Changing the convention for bounds and origin in ImageData (still using point data, but basically treated as cell data)
- origin will be the center of the first pixel
- bounds will encapsulate all of the representation, to properly fit spacing (i.e.: a single pixel will have bounds, outer of the represented image)
Update the volume mapper to adjust to this new convention (different from your fix)
Update the image mapper to adjust to this new convention

Given our current funding and available staffing, we've planned to have someone working on this realistically mid-October. We hope that'll work for you and that distributing your change in a beta release is satisfactory in the meantime. Of course, you're more welcome to contribute to some of those changes yourself if you're looking to get things in master in a shorter timeline.

Kitware / vtk-js