Volume rendering updates for isosurface and attenuated MIP #5215

aganders3 · 2022-10-14T00:13:37Z

Description

This is meant to fix some ugly volume rendering issues with certain data types when using iso or attenuated_mip modes. In my investigation it seemed to mostly be a result of certain data types not being normalized in the GPU memory when using texture_format = 'auto'.

Here's what it can look like now with 16 bit signed ints (from a directory of DICOM files):

Screen.Recording.2022-10-13.at.3.12.58.PM.mov

With these changes, this is how it behaves:

Screen.Recording.2022-10-13.at.8.11.07.PM.mov

Type of change

Bug-fix (non-breaking change which fixes an issue)

References

Discussion in Zulip
Visual Human Project CT Data

How has this been tested?

Tested with some of the sample data that comes with Napari (e.g. Brain (3D)) and some CT data from the Visual Human Project. I would appreciate others testing this as it seems like "tuning" parameters and I don't have a huge collection of relevant data. Suggestions from the Zulip stream were split into separate commits for easy testing.

Any input on possible automated tests is also appreciated.

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

codecov · 2022-10-14T00:34:02Z

Codecov Report

Merging #5215 (af27d4b) into main (cef660e) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

❗ Current head af27d4b differs from pull request most recent head cb28d03. Consider uploading reports for the commit cb28d03 to get more accurate results

@@            Coverage Diff             @@
##             main    #5215      +/-   ##
==========================================
- Coverage   89.00%   88.83%   -0.17%     
==========================================
  Files         578      579       +1     
  Lines       48938    49125     +187     
==========================================
+ Hits        43557    43642      +85     
- Misses       5381     5483     +102

Impacted Files	Coverage Δ
napari/components/viewer_model.py	`96.56% <ø> (ø)`
napari/view_layers.py	`100.00% <ø> (ø)`
napari/_qt/layer_controls/qt_image_controls.py	`98.04% <100.00%> (+0.06%)`	⬆️
napari/_vispy/layers/image.py	`96.68% <100.00%> (+0.09%)`	⬆️
napari/_vispy/visuals/volume.py	`100.00% <100.00%> (ø)`
napari/layers/image/_tests/test_image.py	`100.00% <100.00%> (ø)`
napari/layers/image/image.py	`95.92% <100.00%> (+0.27%)`	⬆️
napari/_qt/_qapp_model/qactions/__init__.py	`90.90% <0.00%> (-0.40%)`	⬇️
napari/_app_model/constants/_menus.py	`95.23% <0.00%> (-0.22%)`	⬇️
napari/_app_model/constants/_commands.py	`98.07% <0.00%> (-0.08%)`	⬇️
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

brisvag · 2022-10-14T09:02:21Z

Nice, thanks for the PR! I have a few comments:

Is there really a reason to not do this for every data type? I feel like we sould just always set based on the clim. Just use self.node._texture.clim_normalized and it will always work no matter the internal texture format :)
Isosurface level is an important value for some use cases: for example, interactively deciding what threshold to use when creating a mask for a cryoEM map is something I use this for. With these changes, the value of layer.iso_threshold is different from the actual binarization value... so maybe for this, we should actually only do it for uints?

Either way I would be ok with merging this as is as well :)

aganders3 · 2022-10-14T17:29:27Z

Thanks! Great thoughts. I will iterate on this.

Point 1 makes complete sense - I will play with this. I think I just didn't understand the clim_normalized property. Note however that for the attenuated mip it's actually using the contrast_limits_range. I will humbly admit that I don't really know why this seems to produce good results.

For point 2 I definitely understand the motivation. I guess the goal would be to surface the actual binarization value the user might want regardless of data type?

brisvag · 2022-10-17T07:22:39Z

Point 1 makes complete sense - I will play with this. I think I just didn't understand the clim_normalized property. Note however that for the attenuated mip it's actually using the contrast_limits_range. I will humbly admit that I don't really know why this seems to produce good results.

I see... Yeah, I'm not sure what's the best solution here. In theory, if range is used, changing the contrast limit within that range should not affect the isosurface threshold, but only the limits. By that I mean, if you use the contrast limits range as isosurface limits, you get this:

clim_range: 0-0.2-----0.9-1
iso:        0------0.7----1

This should show anything between 0.7 and 0.9. But if you use the contrast_limits themselves, then:

clim: 0-0.2-----0.9-1
iso:  0.2-----0.7-0.9

I think the range is more intuitive... which means we need to somehow get the normalized range (which I suppose is already happening to set the slider? And if not, we got another thing to solve :P)

For point 2 I definitely understand the motivation. I guess the goal would be to surface the actual binarization value the user might want regardless of data type?

Yes, I find this quite important; I think all of this is pointing in the direction of changing the isosurface limits for the slider, rather than changing the valye programmatically once it's set.

aganders3 · 2022-10-18T23:04:41Z

Thanks for the review and comments. I took another crack at this if you have time to look again.

Now the iso_threshold slider is working I believe as you described and it reflects the actual threshold. This feels intuitive to me when using it.

I also think the attenuated_mip is working better now by modifying the shader to handle the scaling. This also solved a more subtle problem where negative image values could actually cause background values to be amplified instead of attenuated.

aganders3 · 2022-10-18T23:28:20Z

napari/_vispy/visuals/volume.py

+        vec3 max_loc_tex = vec3(0.0);  // Location where the maximum value was encountered
+        """,
+    in_loop="""
+        scaled_sumval += (val - clim.x) / (clim.y - clim.x);


This is the relevant change to the shader, otherwise it's just copied in from vispy. Previously it was just sumval = sumval + val;.

Great! We should probably upstream this to vispy as well, so we can drop the vendoring here.

Now that I think about it, does this mean we can/should fix the isosurface shader in the same way, instead of doing stuff in python?

Good question that I also didn't think about until now for some reason. I actually haven't looked at the iso shader so it's worth checking it out.

Yeah taking a look at the in_loop portion of the isosurface shader, it seems like it's assuming something about the scale of the data with this line:

if (val > u_threshold-0.2) { ...

I'll try some similar changes there - it would simplify the code in napari and unify the iso and attenuated_mip implementations.

Actually it seems this might not work. Only the contrast limits–not the range–are passed to the shader, so I can't get the same scaling in the shader without more changes to vispy (and I'm not sure making those changes would make sense).

I see... Yeah maybe this is overcomplicating things unnecessarily :P

brisvag

Looks great! Only thing I suggest is to switch everything to using range rather than the clim itself.

napari/_qt/layer_controls/qt_image_controls.py

napari/_vispy/layers/image.py

brisvag · 2022-10-19T09:17:29Z

napari/_vispy/visuals/volume.py

+        vec3 max_loc_tex = vec3(0.0);  // Location where the maximum value was encountered
+        """,
+    in_loop="""
+        scaled_sumval += (val - clim.x) / (clim.y - clim.x);


Great! We should probably upstream this to vispy as well, so we can drop the vendoring here.

brisvag

Changes look great, though I'm not sure I know what those test failures mean... the pyside6 one maybe will be fixed by #5244, but the headless test looks odd.

I left a comment suggesting a change, but I'm happy with the current state as well if that is not possible/too hard.

brisvag · 2022-10-20T07:27:03Z

napari/_vispy/visuals/volume.py

+        vec3 max_loc_tex = vec3(0.0);  // Location where the maximum value was encountered
+        """,
+    in_loop="""
+        scaled_sumval += (val - clim.x) / (clim.y - clim.x);


Now that I think about it, does this mean we can/should fix the isosurface shader in the same way, instead of doing stuff in python?

aganders3 · 2022-10-20T15:34:19Z

Hm - I'll also look into the headless failure when I have a chance.

Thanks for all your time on this!

aganders3 · 2022-10-20T19:32:28Z

I can't reproduce the headless test failure (here's a successful run in my fork) - it might just be bad luck as it seems to be a failure to triangulate some points that are randomly generated when setting up a test.

…d value

…contrast * Address review comments

aganders3 · 2022-11-02T01:13:57Z

This is back to green after rebasing on main. I consider this ready to go but will be happy to make further updates if there are more suggestions.

brisvag · 2022-11-02T16:09:48Z

it seems to be a failure to triangulate some points that are randomly generated

Ah, this is a known old issue... Well, we don't need to solve it here :)

As far as I'm concerned, this can be merged! Will leave it open for a day or so to see if we get other feedback.

brisvag · 2022-11-04T12:47:15Z

Merged! Thanks a lot @aganders3, also for upstreaming what possible to vispy :)

aganders3 · 2022-11-04T16:50:24Z

Thanks for all your time reviewing and providing feedback!

github-actions bot assigned aganders3 Oct 14, 2022

aganders3 force-pushed the volume-render-updates branch from f36b517 to 00121ce Compare October 18, 2022 22:44

github-actions bot added qt Relates to qt tests Something related to our tests labels Oct 18, 2022

aganders3 commented Oct 18, 2022

View reviewed changes

brisvag requested changes Oct 19, 2022

View reviewed changes

aganders3 force-pushed the volume-render-updates branch from deb25b3 to 066173c Compare October 19, 2022 17:19

brisvag approved these changes Oct 20, 2022

View reviewed changes

aganders3 force-pushed the volume-render-updates branch 2 times, most recently from dc55ee1 to af27d4b Compare October 27, 2022 14:44

aganders3 mentioned this pull request Oct 27, 2022

Scale values when calculating attenuation in attenuated_mip shader vispy/vispy#2417

Merged

aganders3 added 6 commits November 2, 2022 00:30

Set isosurface threshold accounting for contrast limits

1e7a4e3

Set mip attenuation accounting for contrast limits (range)

86fb066

Update attenuation and iso_threshold when contrast limits are updated

1ddbcea

Change iso_threshold slider to hold actual threshold instead of scale…

0ea94b1

…d value

Move attenuated_mip adjustments to shader

ea3687a

Change iso_threshold limits to reflect contrast range instead of set …

cb28d03

…contrast * Address review comments

aganders3 force-pushed the volume-render-updates branch from af27d4b to cb28d03 Compare November 2, 2022 00:30

brisvag merged commit 1cc90a7 into napari:main Nov 4, 2022

aganders3 deleted the volume-render-updates branch November 4, 2022 16:50

brisvag added a commit to brisvag/napari that referenced this pull request Nov 8, 2022

revert part of napari#5215 that was pushed to vispy

df7727c

brisvag mentioned this pull request Nov 8, 2022

Vispy 0.12: per-point symbol and several bugfixes #5312

Merged

4 tasks

Czaki mentioned this pull request Jun 9, 2023

v0.4.18 - release process #5911

Closed

Czaki added this to the 0.4.18 milestone Jun 15, 2023

Czaki added the bugfix PR with bugfix label Jun 15, 2023

Czaki pushed a commit that referenced this pull request Jun 16, 2023

Volume rendering updates for isosurface and attenuated MIP (#5215)

9e18cca

Czaki pushed a commit that referenced this pull request Jun 17, 2023

Volume rendering updates for isosurface and attenuated MIP (#5215)

517d464

Czaki pushed a commit that referenced this pull request Jun 18, 2023

Volume rendering updates for isosurface and attenuated MIP (#5215)

27cb9fe

Czaki pushed a commit that referenced this pull request Jun 19, 2023

Volume rendering updates for isosurface and attenuated MIP (#5215)

1958c48

Czaki pushed a commit that referenced this pull request Jun 21, 2023

Volume rendering updates for isosurface and attenuated MIP (#5215)

3af17cb

Czaki pushed a commit that referenced this pull request Jun 21, 2023

Volume rendering updates for isosurface and attenuated MIP (#5215)

5ebd460

Czaki pushed a commit that referenced this pull request Jun 21, 2023

Volume rendering updates for isosurface and attenuated MIP (#5215)

97ad0e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Volume rendering updates for isosurface and attenuated MIP #5215

Volume rendering updates for isosurface and attenuated MIP #5215

aganders3 commented Oct 14, 2022

codecov bot commented Oct 14, 2022 •

edited

Loading

brisvag commented Oct 14, 2022

aganders3 commented Oct 14, 2022

brisvag commented Oct 17, 2022

aganders3 commented Oct 18, 2022

aganders3 Oct 18, 2022 •

edited

Loading

brisvag Oct 19, 2022

brisvag Oct 20, 2022

aganders3 Oct 20, 2022

aganders3 Oct 20, 2022

aganders3 Oct 20, 2022

brisvag Oct 21, 2022

brisvag left a comment

brisvag Oct 19, 2022

brisvag left a comment

brisvag Oct 20, 2022

aganders3 commented Oct 20, 2022

aganders3 commented Oct 20, 2022 •

edited

Loading

aganders3 commented Nov 2, 2022

brisvag commented Nov 2, 2022

brisvag commented Nov 4, 2022

aganders3 commented Nov 4, 2022

Volume rendering updates for isosurface and attenuated MIP #5215

Volume rendering updates for isosurface and attenuated MIP #5215

Conversation

aganders3 commented Oct 14, 2022

Description

Type of change

References

How has this been tested?

Final checklist:

codecov bot commented Oct 14, 2022 • edited Loading

Codecov Report

brisvag commented Oct 14, 2022

aganders3 commented Oct 14, 2022

brisvag commented Oct 17, 2022

aganders3 commented Oct 18, 2022

aganders3 Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 commented Oct 20, 2022

aganders3 commented Oct 20, 2022 • edited Loading

aganders3 commented Nov 2, 2022

brisvag commented Nov 2, 2022

brisvag commented Nov 4, 2022

aganders3 commented Nov 4, 2022

codecov bot commented Oct 14, 2022 •

edited

Loading

aganders3 Oct 18, 2022 •

edited

Loading

aganders3 commented Oct 20, 2022 •

edited

Loading