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

BUG: use alpha to calculate RGBA lighting #43

Merged
merged 3 commits into from Oct 10, 2023

Conversation

pieper
Copy link
Member

@pieper pieper commented Oct 6, 2023

Implements the feature and fixes discussed in this thread:

https://discourse.slicer.org/t/volume-rendering-colorized-with-segmentation/26689/6

Basically allows RGBA volumes to be shaded according to their alpha component so that something like a CT or microCT can define an rgb value at spatial regions, while getting opacity and gradient information from the alpha channel (e.g. the Hounsfield unit).

Also always uses the 0 channel for defining the lighting parameters. This part is WIP since it may be more appropriate to set the alpha channel lighting parameters in this scenario. Currently, at least in 3D Slicer, only the 0 compnent's lighting parameters are being set, so they are being used here.

Implements the feature and fixes discussed in this thread:

https://discourse.slicer.org/t/volume-rendering-colorized-with-segmentation/26689/6

Basically allows RGBA volumes to be shaded according to their alpha
component so that something like a CT or microCT can define an
rgb value at spatial regions, while getting opacity and gradient
information from the alpha channel (e.g. the Hounsfield unit).

Also always uses the 0 channel for defining the lighting parameters.
This part is WIP since it may be more appropriate to set the alpha
channel lighting parameters in this scenario.  Currently, at least in
3D Slicer, only the 0 compnent's lighting parameters are being set, so
they are being used here.
@pieper
Copy link
Member Author

pieper commented Oct 6, 2023

See also: lassoan@c372d71

@pieper pieper marked this pull request as draft October 6, 2023 22:37
In the vtkOpenGLGPUVolumeRayCastMapper when the components are
dependent (that is, RGBA), only a single value is set in the lighting
arrays (in_ambient, in_diffuse, ...).  So in the shader code it must
also use the zeroth element of the lighting arrays to calculate
the final lit colors.
@pieper pieper requested a review from lassoan October 7, 2023 21:18
@pieper
Copy link
Member Author

pieper commented Oct 7, 2023

@lassoan I think this looks better than hard coding those 0 values.

I tracked down that when it's copying lighting info to the shader code it only copies numberOfSamplers of them even if the others are set:

https://github.com/Slicer/VTK/blob/slicer-v9.2.20230607-1ff325c54/Rendering/VolumeOpenGL2/vtkOpenGLGPUVolumeRayCastMapper.cxx#L858

When the components are dependent (rgba) it only has one sampler.

https://github.com/Slicer/VTK/blob/slicer-v9.2.20230607-1ff325c54/Rendering/VolumeOpenGL2/vtkOpenGLGPUVolumeRayCastMapper.cxx#L3892

So with this proposed change the component will be zero in the case of dependent components, but will be the component number in the case of independent components.

Can you have a look and see if you agree with the logic?

If so we make the an upstream MR for VTK and then cherry pick the commit back here.

@lassoan
Copy link

lassoan commented Oct 7, 2023

Regex can be slow and seems that it might be a bit of an overkill. Could we add something like int sampler_index = 0 or int sampler_index = component (depending on components are dependent or not) instead of string manipulation?

@jcfr
Copy link
Member

jcfr commented Oct 8, 2023

@pieper Thanks for working on this.

Cc: @sankhesh @LucasGandel

@lassoan
Copy link

lassoan commented Oct 8, 2023

FYI, I've implemented "Colorize volume" module in Slicer that creates really nice RGBA volumes. To make edges smooth, I've developed a masked median filter that can slightly expand the labels and it is very quick, robust, and works well when there are multiple labels in the neighborhood.

For example, CTLiver sample data set segmented with TotalSegmentator.

Without masking (segmentation is only used for coloring and slightly decreasing opacity outside segments):

SlicerCapture.mp4

With masking (region outside segments are blanked out):

SlicerCapture4.mp4

What is remarkable that this allows visualization of the texture from the original CT, see for example the muscle fibers of gluteus maximus.

shaderStr += std::string("\
\nvec4 computeColor(vec4 scalar, float opacity)\
\n {\
\n return clamp(computeLighting(vec4(scalar.xyz, opacity), 3, 0.0), 0.0, 1.0);\

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we consider that using alpha for shading is what makes more sense for every use cases? Then this part should be contributed back to VTK ideally.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes, if there's an alpha channel you almost always want to use that for lighting, shading, and opacity calculations. I don't know of any use case where you wouldn't want that. And yes, this should go in VTK once we finalize it.

@pieper
Copy link
Member Author

pieper commented Oct 9, 2023

Regex can be slow and seems that it might be a bit of an overkill. Could we add something like int sampler_index = 0 or int sampler_index = component (depending on components are dependent or not) instead of string manipulation?

Since the whole shader system in VTK is based on string replacements this seems consistent. And this is not performance critical code since the shaders are cached and meant to be rarely recomputed.

But yes, using a variable would be an option.

Copy link

@lassoan lassoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel that regex is an overkill (std::string::replace could be used that might be implemented more efficiently), but I don't want to hold up this PR. Implementation details will be reviewed anyway when it is merged to upstream VTK.

@lassoan
Copy link

lassoan commented Oct 10, 2023

@pieper please have a look, squash, and mark it ready if you think it is ready to be merged. Thank you! It would be nice if the fix could get into Slicer/VTK today.

@pieper pieper marked this pull request as ready for review October 10, 2023 17:57
@pieper pieper merged commit 3e45140 into Slicer:slicer-v9.2.20230607-1ff325c54 Oct 10, 2023
@pieper pieper deleted the fix-rgba-gpu-vr branch October 10, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants