In Volume shader for iso surface ommit point that color has alpha equal to 0

[Czaki]

Description

when creating an iso surface omit points that are mapped to transparent color.

Type of change

• [x] Bug-fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] This change requires a documentation update

References

closes #5199

How has this been tested?

• [ ] example: the test suite for my feature covers cases x, y, and z
• [ ] example: all tests pass with my change
• [ ] example: I check if my changes works with both PySide and PyQt backends as there are small differences between the two Qt bindings.

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
• [ ] If I included new strings, I have used trans. to make them localizable. For more information see our translations guide.

[codecov[bot]]

Codecov Report

Merging #5227 (287f031) into main (a1ddd47) will increase coverage by 1.53%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5227      +/-   ##
==========================================
+ Coverage   87.27%   88.80%   +1.53%     
==========================================
  Files         574      574              
  Lines       48921    48921              
==========================================
+ Hits        42694    43446     +752     
+ Misses       6227     5475     -752     

Impacted Files	Coverage Δ
napari/_vispy/visuals/volume.py	100.00% <ø> (ø)
napari/_qt/qt_viewer.py	78.68% <0.00%> (+0.40%)	:arrow_up:
napari/components/viewer_model.py	96.56% <0.00%> (+0.45%)	:arrow_up:
napari/settings/_base.py	91.98% <0.00%> (+0.47%)	:arrow_up:
napari/utils/theme.py	89.79% <0.00%> (+0.68%)	:arrow_up:
napari/_vispy/layers/image.py	96.59% <0.00%> (+0.68%)	:arrow_up:
napari/utils/info.py	83.33% <0.00%> (+1.11%)	:arrow_up:
napari/components/experimental/monitor/_monitor.py	34.52% <0.00%> (+1.19%)	:arrow_up:
napari/_qt/qt_event_loop.py	82.31% <0.00%> (+1.36%)	:arrow_up:
napari/utils/progress.py	100.00% <0.00%> (+2.00%)	:arrow_up:
... and 33 more

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

[psobolewskiPhD]

Just checked, on my end: https://github.com/napari/napari/issues/5199 is fixed. Meanwhile, the issues with surfaces from here: https://github.com/napari/napari/pull/5154 remain fixed. Likewise, the original test example for #4935 is also fine:

Great catch btw, I even understood the Label code before you went with the shader route...

[brisvag]

For context: this is technically not the correct fix, because it simply assumes that anything that results as a completely transparent label should be ditched (even non-background ones)... Now, why someone would want a transparent label and still care about it being rendered, I don't know :P

I don't know why this fix even works in the first place... I'll check again tomorrow!

[andy-sweet]

I don't know why this fix even works in the first place... I'll check again tomorrow!

If you work it out, please suggest an explanatory comment in the shader!

[perlman]

This is definitely confusing. I don't understand why floatNotEqual(color.g, categorical_bg_value) would be returning true for label 0.

Theoretically the use of categorical_bg_value would let us change what the background color is.

But, I have to say, if it ever maps to transparent, why draw it? (As @brisvag said, this seems like the wrong fix, but I'm not sure I can argue with it.)

[Czaki]

I don't know why this fix even works in the first place... I'll check again tomorrow!

and I do not understand where could be confusion.

Maybe here: https://github.com/napari/napari/blob/a1ddd475ef60319bf3d4e4087dd3c418a86f2483/napari/layers/labels/labels.py#L918-L930 ?

[perlman]

Oooh -- is this for categorical labels only?

Then this makes sense, as their label values won't actually be zero.

[Czaki]

This is definitely confusing. I don't understand why floatNotEqual(color.g, categorical_bg_value) would be returning true for label 0.

because it is hardcoded as 0, not 0.5 / num_of_colors as you define in your PR.

https://github.com/napari/napari/blob/a1ddd475ef60319bf3d4e4087dd3c418a86f2483/napari/_vispy/visuals/volume.py#L25

Oooh -- is this for categorical labels only?

yes

[perlman]

Reason number 259 that the data should needs to be passed in raw form to the GPU and mapped there. Sigh.

I'm ok with this fix, but it's really only a bandaid. We need to think of a cleaner way for going between {source data space} -> {mapped data space} -> {color}. As you point out, I changed the value to 0.5, but I'm not yet following why that comparison needs to happen here.

[Czaki]

As you point out, I changed the value to 0.5, but I'm not yet following why that comparison needs to happen here.

As the problem happens only for the iso surface mode it may be connected with the discover border.

[brisvag]

what confuses me is that I tried a whole bunch of changes to basically get to the pain where the correct value is set (instead of being a constant) to categorical_bg_value. I'm sure it's correct cause I was printing both the data and the value itself just before setting them to the uniforms in the shader... And yet floatEqual failed. I certainly did something wrong, but I got so stuck that at this point I thing someone else is more likely to find the bug than me xD

But yeah, I agree with @perlman: let's get this fix in and fix the bigger picture later.

[Czaki]

I'm sure it's correct cause I was printing both the data and the value itself just before setting them to the uniforms in the shader... And yet floatEqual failed. I certainly did something wrong, but I got so stuck that at this point I thing someone else is more likely to find the bug than me xD

could you share the code to print such values in opengl?

[brisvag]

could you share the code to print such values in opengl?

Sorry, maybe I was unclear: I'm printing the value just before setting (in python). So yeah, I'm not 100% sure everything goes as planned after :/ I tried mapping to a color, and it looked the same to me, but the comparison still fails.

[Czaki]

Sorry, maybe I was unclear: I'm printing the value just before setting (in python). So yeah, I'm not 100% sure everything goes as planned after :/ I tried mapping to a color, and it looked the same to me, but the comparison still fails.

ensure that you cast them to same base (float32 or float64). From my experience, this is a common source of problems with comparison.

[brisvag]

I'm fairly sure this is handled by vispy... EDIT: yes, and they are all float32 in the first place :/

[brisvag]

Ok, narrowing it down: the setting is working correctly; the values that appear to be incorrect is the actual value from sampling the texture. The reason floatEqual fails is because the values are actually different, and only pass the check if I change the tolerance to 1e-2 from 1e-8, which is probably undesirable.

[brisvag]

So basically, in my test code, the data should contain the following mapped values:

[0.125 0.625 0.875]

With mappings being:

In [53]: layer._label_color_index
Out[53]: {1: 0.875, 2: 0.625, 0: 0.125, None: 0.375}

First of all, I'm not sure what the None is supposed to do differently from the 0. 0 was the auto-added background label, since my direct mapping was:

layer.color = {1: [1,0,0,1], 2: [0,1,0,1]}

Now, I would expect the data to contain only the above values. Instead, if I sample where 0 is supposed to be, which would result in 0.125, what I get is close to 0.1254903. It shouldn't be an interpolation issue, since that's set to nearest.

[Czaki]

for 0 the default color is transparent and for none the default color is black. So objects that does not have set color are painted with black.

Now, I would expect the data to contain only the above values. Instead, if I sample where 0 is supposed to be, which would result in 0.125, what I get is close to 0.1254903. It shouldn't be an interpolation issue, since that's set to nearest.

and this is why the solution with checking transparency works and checking that it is close to the background color does not work. as the difference is on levels 1e-3.

[brisvag]

for 0 the default color is transparent and for none the default color is black. So objects that does not have set color are painted with black.

Oh, I see: so None is for labels different from background, but for which there is no entry in the direct dict.

and this is why the solution with checking transparency works and checking that it is close to the background color does not work. as the difference is on levels 1e-3.

Yup; now, what's weird is: how did that number get messed up by about 1e-3?

[Czaki]

Yup; now, what's weird is: how did that number get messed up by about 1e-3?

Is there any interpolation in volume data? Looking at artificial data it looks like there is some interpolation:

import numpy as np
from skimage.morphology import diamond	

d = diamond(10)

d1 = np.zeros((3,) + d.shape, dtype=np.uint8)

d1[1] = d

viewer.add_labels(d1)

[brisvag]

Interpolation is nearest, so no. The jagged edges you see are not due to interpolation, but to the size of the ray casting step being relatively big compared to the size of the data voxels.

[Czaki]

Interpolation is nearest, so no. The jagged edges you see are not due to interpolation, but to the size of the ray casting step being relatively big compared to the size of the data voxels.

where it is set?

[andy-sweet]

I will merge this after 24 hours unless anyone objects. Or someone else should feel free to merge it after that time instead.

[brisvag]

where it is set?

https://github.com/napari/napari/blob/8196109bcb04d9c6d40d600d0e0a3e8ee946d8a7/napari/layers/labels/labels.py#L252-L253

[Czaki]

hm. This looks like connected with my and @perlman problem with colors that were assigned improperly when try to fix colormap

[andy-sweet]

@Czaki : just double checking - is this good to merge from your perspective?

[Czaki]

yes