Add cutoff to mip and minip volume projection for fragment discard #2308

brisvag · 2022-03-04T15:47:52Z

This PR takes out some code from the solution to the issues in #2305.

Specifically, see this comment and this other comment.

In practice, the new attributes allow to set a lower bound to MIP projection (and higher bound to MINIP) beyond which fragments are discarded.

almarklein · 2022-03-09T09:16:14Z

What is the actual use-case for this?

These cutoffs feel to me as ideally being inf/-inf (I wonder if its safe to actually use these values in a shader), and making them settable feels odd to me.

Maybe these use-cases could be solved differently, e.g. by setting clim and using a colormap with alpha=0 in some places. That is more work, but might be more correct. Thought?

brisvag · 2022-03-09T09:31:31Z

I agree that by default the cutoffs should be the min and max possible values of the data type... Not sure if -inf and inf would be safe in the shader, or if we need to detect them somehow either in the shader or outside, based on the dtype of the texture.

Notice that on main they are harcdoded to -99999 and 99999, which is definitely not good. I left those numbers for now just to avoid changing behaviour.

Yes, colormap withg alpha would be the right way to do it, if only transparency in vispy wasn't broken :P as you may remember, I went down the deferred rendering rabbit hole to implement OIT, but that's a massive amount of work and probably not gonna happen any time soon (if at all before vispy 2.0).

almarklein · 2022-03-09T09:34:37Z

if only transparency in vispy wasn't broken

I think it should work by doing something like this near the end of the shader (after the colormap, at least):

if (fragColor.a <= 0.0) {
    discard;
}

brisvag · 2022-03-09T09:35:23Z

Notice that on main they are harcdoded to -99999 and 99999, which is definitely not good. I left those numbers for now just to avoid changing behaviour.

Note that this PR does change behavior no matter what we choose (to keep the current limits or to use the extremes), because previously if the ray casting detected only a maximum that was lowe than -99999, it would be clipped to white. Now it either gets removed, or distinguished correctly from -1000000.

brisvag · 2022-03-09T09:37:28Z

if only transparency in vispy wasn't broken

I think it should work by doing something like this near the end of the shader (after the colormap, at least):
if (fragColor.a <= 0.0) {
    discard;
}

This would work for a=0, but make a mess of anything approaching 0, which will happen on the edges of objects.

almarklein · 2022-03-09T09:54:51Z

This would work for a=0, but make a mess of anything approaching 0, which will happen on the edges of objects.

Right, due to the linear interpolation of the colormap. mmm ...

Note that this PR does change behavior no matter what we choose (to keep the current limits or to use the extremes), because previously if the ray casting detected only a maximum that was lowe than -99999, it would be clipped to white. Now it either gets removed, or distinguished correctly from -1000000.

Yeah, I agree that the current behavior was technically incorrect.

After some more thought I can see how it makes sense to be able to hide/discard rays that don't meet a certain value in MIP, and this seems like the best way to do this. 👍

vispy/visuals/volume.py

brisvag · 2022-03-09T10:03:13Z

So would you suggest keeping -99999 or moving to some form of -inf?

almarklein · 2022-03-09T10:05:45Z

Shall we try using inf? I'm a bit scared of odd results on specific drivers, but we can revert if we do.

almarklein · 2022-03-09T10:06:02Z

Living on the edge 🤘

brisvag · 2022-03-09T10:11:09Z

It seems that our best bet is to use max and min float values... the internet says:

FLT_MAX 3.402823466e+38
FLT_MIN 1.175494351e-38

Inf can be obtained apparently with 1.0 / 0.0, but it's not guaranteed to work for glsl < 4.1.

djhoese · 2022-03-09T15:31:01Z

1.175494351e-38

Isn't that the smallest representable decimal? I mean, shouldn't the negative of the max be the minimum?

Edit: That number is "The minimum positive normal value", but zero would still be less than that.

brisvag · 2022-03-09T15:42:49Z

That number is "The minimum positive normal value", but zero would still be less than that.

Yup, oops, I just found the same as well! Googled too superficially. Obviously they should be opposites.

brisvag · 2022-03-09T15:49:34Z

Turns out we can rely on np.finfo to generate those values for us.

almarklein · 2022-03-09T20:18:20Z

What happens if you do float('inf')? Floats in shaders are not necessarily 32 bit. I suspect that they can still represent inf/-inf though. I suppose the max of a 32bit float might also work, it will likely become either inf or the max in the shader's float representation. I have no idea which approach would be safer, tbh.

brisvag · 2022-03-09T22:10:59Z

Yeah... I would hope that floats are converted (and clipped) somewhere in the gloo or program stuff... propbably they are equivalent, but I feel like it's a safer bet that clipping works properly rather than inf, since inf was apparently never a first class citizen before 4.1.

almarklein · 2022-03-10T11:58:02Z

I would hope that floats are converted (and clipped) somewhere in the gloo or program stuff...

I don't think we do any of that in vispy - it's up to the driver to do (or not do) this.

djhoese · 2022-03-10T18:39:07Z

Similar experience here with NaN. The CI jobs that use NaNs in the images are xfail because the GL drivers on the CI machines apparently can't handle NaN. On those machines NaN is just replaced with 0 if I remember correctly.

brisvag · 2022-03-10T21:03:03Z

So, I added some tests to see if things are working properly. I get failures here, but I think they are correct. I'm a bit tired so I might have done something very wrong...

brisvag · 2022-03-16T12:33:03Z

Ok, I tracked down the issue as far as I could. It turns out we're back to canvas sizing issues, like some time ago in another PR (forgot which...). Basically, very small canvas sizes and qt seem to not play well together. Things seem to go awry when the canvas context manager is entered, and self.show() gets called. Somewhere in there, the canvas gets resized to (75, 50), at least on my machine. This obviously messes up our assumptions about where colors are supposed to be. I fixed the tests by explicitly setting the canvas just before rendering.

Try this on your machine:

from vispy.scene import SceneCanvas

c = SceneCanvas(size=(40, 40))
c.size  # output is correct
c.show()
c.size  # output is wrong

In fact, it seems that anything below (75, 50) gets resized to that.

EDIT: this was meant as a comment to #2305, but likely applies here too.

djhoese · 2022-03-16T13:26:35Z

vispy/visuals/volume.py

+        if (maxi > -1) {
+            gl_FragColor = applyColormap(maxval);
+        }


Keeping in mind that I don't know much about volume rendering, is this just an optimization or does it actually fix things showing up when they shouldn't be there?

It fixes things to behave the same as the other visuals. Without this change, it was defaulting to always setting the value from the zeroth step of the raycasting, even when there was nothing (i.e: beyond the cutoff). Basically, the cutoff was not being used :P

djhoese · 2022-03-16T13:27:31Z

Strange find. Nice job. Any idea why the other tests that use a 40x40 canvas size work?

brisvag · 2022-03-16T13:33:21Z

Strange find. Nice job. Any idea why the other tests that use a 40x40 canvas size work?

I think it's just by luck and trial and error. I've had issues before with this, and now it's clear why. When this last happened, I changed my approach until this was no longer causing issues (for example by dynamically calculating the center of the rendered image rather than passing a specific index). Other tests simply don't care because the whole canvas is supposed to be the same color, or they just check that a color is shown somewhere but don't care where.

For the future, I suggest we never use below 75x50 to avoid any issues :P

djhoese · 2022-03-16T13:36:34Z

Sounds good!

brisvag · 2022-03-16T13:45:52Z

Just a note: we should double check in between merging this and #2305, cause there's probably going to be some conflicts. I would suggest merging this last.

…utoff

brisvag · 2022-03-19T10:29:34Z

All good from my side; conflicts are solved and tests pass locally. @djhoese let me know if there are unclear things; otherwise I think this is ready to go in as well!

djhoese · 2022-03-20T20:01:41Z

Given the passing tests and the prior approval from Almar, let's merge this...

add cutoff for mip and minip

2409acc

brisvag mentioned this pull request Mar 4, 2022

Set depth in VolumeVisual for mip/minip/attenuated_mip rendering modes #2305

Merged

add declaration of minip_cutoff

44c080b

almarklein reviewed Mar 9, 2022

View reviewed changes

vispy/visuals/volume.py Show resolved Hide resolved

almarklein approved these changes Mar 9, 2022

View reviewed changes

add docstrings

535df50

use min and max float for cutoffs

e3b6ca7

fix min max

50bfbca

add tests (fail)

b098901

brisvag added 2 commits March 16, 2022 13:52

fix minip bug

b12c878

fix tests

f8c2a18

djhoese reviewed Mar 16, 2022

View reviewed changes

brisvag mentioned this pull request Mar 16, 2022

Fixing some things alisterburt-forks/vispy#4

Merged

brisvag added 2 commits March 19, 2022 11:25

merge main

38f648e

Merge remote-tracking branch 'upstream/main' into feature/mip_minip_c…

b5d0a34

…utoff

djhoese added type: enhancement component: visuals labels Mar 20, 2022

djhoese merged commit 4283e6d into vispy:main Mar 20, 2022

brisvag mentioned this pull request May 20, 2022

Use mip/minip cutoff for Volume napari/napari#4556

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cutoff to mip and minip volume projection for fragment discard #2308

Add cutoff to mip and minip volume projection for fragment discard #2308

brisvag commented Mar 4, 2022

almarklein commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 9, 2022 •

edited

brisvag commented Mar 9, 2022

brisvag commented Mar 9, 2022 •

edited

almarklein commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 9, 2022

almarklein commented Mar 9, 2022 •

edited

brisvag commented Mar 9, 2022

djhoese commented Mar 9, 2022 •

edited

brisvag commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 10, 2022

djhoese commented Mar 10, 2022

brisvag commented Mar 10, 2022

brisvag commented Mar 16, 2022 •

edited

djhoese Mar 16, 2022

brisvag Mar 16, 2022

djhoese commented Mar 16, 2022

brisvag commented Mar 16, 2022

djhoese commented Mar 16, 2022

brisvag commented Mar 16, 2022

brisvag commented Mar 19, 2022

djhoese commented Mar 20, 2022

Add cutoff to mip and minip volume projection for fragment discard #2308

Add cutoff to mip and minip volume projection for fragment discard #2308

Conversation

brisvag commented Mar 4, 2022

almarklein commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 9, 2022 • edited

brisvag commented Mar 9, 2022

brisvag commented Mar 9, 2022 • edited

almarklein commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 9, 2022

almarklein commented Mar 9, 2022 • edited

brisvag commented Mar 9, 2022

djhoese commented Mar 9, 2022 • edited

brisvag commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 9, 2022

brisvag commented Mar 9, 2022

almarklein commented Mar 10, 2022

djhoese commented Mar 10, 2022

brisvag commented Mar 10, 2022

brisvag commented Mar 16, 2022 • edited

djhoese Mar 16, 2022

Choose a reason for hiding this comment

brisvag Mar 16, 2022

Choose a reason for hiding this comment

djhoese commented Mar 16, 2022

brisvag commented Mar 16, 2022

djhoese commented Mar 16, 2022

brisvag commented Mar 16, 2022

brisvag commented Mar 19, 2022

djhoese commented Mar 20, 2022

almarklein commented Mar 9, 2022 •

edited

brisvag commented Mar 9, 2022 •

edited

almarklein commented Mar 9, 2022 •

edited

djhoese commented Mar 9, 2022 •

edited

brisvag commented Mar 16, 2022 •

edited