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
Fix inverted LUT and blending #5487
Conversation
Note that both in main and here, anything below a minimum blending layer (say, some innocent points...) will wreak havoc to everything else due to the image being blended with the point :P |
Codecov Report
@@ Coverage Diff @@
## main #5487 +/- ##
==========================================
+ Coverage 89.35% 89.42% +0.06%
==========================================
Files 609 609
Lines 51170 51227 +57
==========================================
+ Hits 45724 45809 +85
+ Misses 5446 5418 -28
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Wow! Way to steal my thunder! 🤣 Being able to freely swap layers is pretty cool. And this would permit opening multichannel images with all the same blending mode, rather than the current behavior of translucent for bottom, then additive. Some observations from testing:
I think the best solution there is to make the empty area of point/shape area of the layers have 0 alpha (empty) instead of being black or white. |
Wow, what a mess xD Thanks for checking everything!
Good point, didn't think about that... This is more annoying, cause we need to check everything at a higher level, probably
Something's off with the
The primary issue here is that mixing minimum blending with inverted LUT and non-mimum blending with normal colors (i.e: points) is just a Bad Idea®. The only sensible way to this is to have any minimum blending layer be together in a block, and the bottommost one use a non-minimum blending with the below. But really, I don't know if there's really a good way to do it, short of having an extra all-white/all-black layer/minicanvas as a separator. Thios might be more doable once we have layergroups, maybe :P |
The behavior isn't any different than with Edit: all of the above behavior is also true if you have a Dark viewer and use minimum blending instead of additive, with the square having face So there are 2 issues:
|
All good points... I've hacked at this a bit. Changing blending modes in a few places, maybe I landed on something that makes sense. Still doesn't fix minimum, but maybe the rest looks ok now? It relies on the canvas having zero transparency as a start [automatically set to zero now!] :) |
Works super well, other than the blending with points/shapes at the bottom still being affected by theme in |
I changed a few things again, please take a look if the extra issues are solved! So, here's a more-or-less complete breakdown of how blending in this context works: Blending takes a
Knowing this, this is what we do:
So I think we have the most correct blending that we could hope for (without doing multi-pass rendering...). AFAIK The one limitation left is that if any The workaround in the case of multichannel is to set the blending of the bottommost image to This is all well and good theoretically; however, with these changes I get a terrible flickering effect with the canvas color set to non-black, and I can't figure out why :( |
@perlman, maybe you can see if I'm missing something obvious in the above? |
@brisvag I think things are working really well now! One odd thing: an empty Labels layer is solid black in Light theme. On live it's white and switching theme switches the shown color. The background of the labels layer should be transparent, so a singular, empty labels layer should be shown as the canvas color. But if that means alpha is 0, then with the changes, you get zeroing of both the src and dst colors, yielding a black box. Now in Dark mode this still happens with points/shapes/labels at the bottom, because they pass canvas color as you noted. It's a very corner case, but in theory one could permit the user to set the background color of those layers. Or maybe have a checkbox |
napari/_vispy/layers/base.py
Outdated
depth_test=True, | ||
cull_face=False, | ||
blend=True, | ||
blend_func=(src_color_blending, 'zero', 'one', 'one'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blend_func=(src_color_blending, 'zero', 'one', 'one'), | |
blend_func=(src_color_blending, 'one_minus_src_alpha', 'one', 'one'), |
I think this is needed to prevent black box when blending a src with 0 alpha.
This way if src has 0 alpha, then the src color is zeroed out and canvas color is used with full alpha.
This makes sense I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch; I'm thinking that maybe this needs to actually be different for minimum
and additive
like for src_color_blending
? Otherwise I think if you have an additive
stack with non-black canvas, you'd get leakage if you change opacity of the bottommost layer?
dst_color_blending = (
'one_minus_src_alpha'
if blending not in ('minimum', 'additive')
else 'zero'
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you test the last commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me the new behavior is still not right.
Try an image like File > Samples > Cell
Try to adjust opacity: no matter theme, it does nada (theme still plays a role
Use Light theme and make a Labels layer: black square.
Try Cells3D and put labels at the bottom—corner case, sure. Use Light theme and make both of the Cells layers minimum
with one of the new inverted caps. On live this works fine, with this PR it's a black box.
I don't think it's possible to have a concept of translucent
with alpha control for the bottom-most visible—or single—layer without taking into account the canvas.
In playing around with it, I get the most intuitive behavior when I comment out
color.alpha = 0
And:
if self.first_visible:
# if the first layer, then we should blend differently
# (ignore the canvas color but allow alpha blending)
# if blending in ('minimum', 'additive'):
# src_color_blending = 'src_alpha'
# dst_color_blending = 'one_minus_src_alpha'
# else:
# src_color_blending = 'one'
# dst_color_blending = 'zero'
blending_kwargs = dict(
depth_test=True,
cull_face=False,
blend=True,
blend_func=(
'src_alpha',
'one_minus_src_alpha',
'one',
'one',
),
blend_equation='func_add',
)
This basically treats bottom-most-visible as a translucent
layer. if you make it opaque (alpha = 1), which is the default, it eliminates blending with canvas. I think there is an expectation that if you make it less opaque, you will see an effect. So I think you have to blend with canvas, but at that point you control the effect
Just the minimum
+Dark theme+bottom-most-visible is shapes/points/labels makes a black box, but I think that's really (corner case)^2 because those layers don't have their own cmap/background.
2nd caveat is if the bottom most layer is something likeadditive
(or with my suggestion translucent
) and 0.7 opacity and then switch to minimum
. Obviously, we override minimum
, so then the opacity can be used, but cannot be changed, because minimum
setting overrides the slider. Again a real corner case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, so sorry, I actually inverted that if statement at some point during committing :/ It should be:
if blending in ('minimum', 'additive'):
src_color_blending = 'one'
dst_color_blending = 'zero'
else:
src_color_blending = 'src_alpha'
dst_color_blending = 'one_minus_src_alpha'
This is so that no blending with the canvas happens with additive and minimum (so you have full black or full white as the "background" color).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah, I should have caught that—sorry!
Works super nice now and makes a lot more sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just for the record, the shapes/points/labels at the bottom of a stack with minimum
& Dark theme does result in a black box, but pretty sure as mentioned about that requires letting those layers set their own background)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Yup, and I think we can leave that out of this PR for now, but let's keep it in mind if this issue comes up :)
My feeling is: let's cross that bridge when we get to it. I don't see any reason to putting the points at the bottom, so we can always suggest switching order if that happens. If we see it coming up more than we think, we can add a switch as you suggest. But I doubt that the fix will be so obvious to solve the confusion about the bug and prevent people from asking "wtf?" xD
Can you try with the cells 3D sample data and the following changes?
It's not super reproducible on my side either, and now that I wanted a video I can't get it to happen :/ |
Can't find a way to make flicker, sorry! |
@brisvag Thanks! I had something like that in mind but didn't know where/how to implement. Very clear. I just added a bit of clarity to the comments plus the toggle of visibility of the bottom layer, because I think this is actually a crucial element. On live if you open cells3D (additive over translucent default) it looks fine on white canvas (Light theme), but breaks when you toggle the vis of the bottom layer. |
…rted-lut-opacity
Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
…into fix/inverted-lut-opacity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful work @brisvag!
# Description Followup on discussion #3914 (comment). This is an attempt at solving those issues. This change blends "correctly" with the canvas in a translucent way, no matter the color of the canvas, and it does so transparently to the user (everything happens in the vispy backend). So you can actually change opacity of every layer and have it behave as expected, even with a non-black canvas. Unfortunately, openGL [does not use blending parameters for the MIN and MAX blending functions](https://www.khronos.org/opengl/wiki/Blending#Blend_Equations). So, if you have multiple channels in separate `minimum` blending image layers, you have 2 options: - the bottommost one is *not* `minimum` -> you get ugly stuff if you change opacity to not be 1 - the bottommost layer is `minimum` -> you cannot change opacity As mentioned in the discussion, a way around it may be to actually reroute the changes to `opacity` on a layer with `minimum` blending so that they actually modify the `contrast limits`. But that's even more magic :p --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description Followup on discussion #3914 (comment). This is an attempt at solving those issues. This change blends "correctly" with the canvas in a translucent way, no matter the color of the canvas, and it does so transparently to the user (everything happens in the vispy backend). So you can actually change opacity of every layer and have it behave as expected, even with a non-black canvas. Unfortunately, openGL [does not use blending parameters for the MIN and MAX blending functions](https://www.khronos.org/opengl/wiki/Blending#Blend_Equations). So, if you have multiple channels in separate `minimum` blending image layers, you have 2 options: - the bottommost one is *not* `minimum` -> you get ugly stuff if you change opacity to not be 1 - the bottommost layer is `minimum` -> you cannot change opacity As mentioned in the discussion, a way around it may be to actually reroute the changes to `opacity` on a layer with `minimum` blending so that they actually modify the `contrast limits`. But that's even more magic :p --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description Followup on discussion #3914 (comment). This is an attempt at solving those issues. This change blends "correctly" with the canvas in a translucent way, no matter the color of the canvas, and it does so transparently to the user (everything happens in the vispy backend). So you can actually change opacity of every layer and have it behave as expected, even with a non-black canvas. Unfortunately, openGL [does not use blending parameters for the MIN and MAX blending functions](https://www.khronos.org/opengl/wiki/Blending#Blend_Equations). So, if you have multiple channels in separate `minimum` blending image layers, you have 2 options: - the bottommost one is *not* `minimum` -> you get ugly stuff if you change opacity to not be 1 - the bottommost layer is `minimum` -> you cannot change opacity As mentioned in the discussion, a way around it may be to actually reroute the changes to `opacity` on a layer with `minimum` blending so that they actually modify the `contrast limits`. But that's even more magic :p --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description Followup on discussion #3914 (comment). This is an attempt at solving those issues. This change blends "correctly" with the canvas in a translucent way, no matter the color of the canvas, and it does so transparently to the user (everything happens in the vispy backend). So you can actually change opacity of every layer and have it behave as expected, even with a non-black canvas. Unfortunately, openGL [does not use blending parameters for the MIN and MAX blending functions](https://www.khronos.org/opengl/wiki/Blending#Blend_Equations). So, if you have multiple channels in separate `minimum` blending image layers, you have 2 options: - the bottommost one is *not* `minimum` -> you get ugly stuff if you change opacity to not be 1 - the bottommost layer is `minimum` -> you cannot change opacity As mentioned in the discussion, a way around it may be to actually reroute the changes to `opacity` on a layer with `minimum` blending so that they actually modify the `contrast limits`. But that's even more magic :p --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description Followup on discussion #3914 (comment). This is an attempt at solving those issues. This change blends "correctly" with the canvas in a translucent way, no matter the color of the canvas, and it does so transparently to the user (everything happens in the vispy backend). So you can actually change opacity of every layer and have it behave as expected, even with a non-black canvas. Unfortunately, openGL [does not use blending parameters for the MIN and MAX blending functions](https://www.khronos.org/opengl/wiki/Blending#Blend_Equations). So, if you have multiple channels in separate `minimum` blending image layers, you have 2 options: - the bottommost one is *not* `minimum` -> you get ugly stuff if you change opacity to not be 1 - the bottommost layer is `minimum` -> you cannot change opacity As mentioned in the discussion, a way around it may be to actually reroute the changes to `opacity` on a layer with `minimum` blending so that they actually modify the `contrast limits`. But that's even more magic :p --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
Followup on discussion #3914 (comment). This is an attempt at solving those issues.
This change blends "correctly" with the canvas in a translucent way, no matter the color of the canvas, and it does so transparently to the user (everything happens in the vispy backend). So you can actually change opacity of every layer and have it behave as expected, even with a non-black canvas.
Unfortunately, openGL does not use blending parameters for the MIN and MAX blending functions. So, if you have multiple channels in separate
minimum
blending image layers, you have 2 options:- the bottommost one is not
minimum
-> you get ugly stuff if you change opacity to not be 1- the bottommost layer is
minimum
-> you cannot change opacityAs mentioned in the discussion, a way around it may be to actually reroute the changes to
opacity
on a layer withminimum
blending so that they actually modify thecontrast limits
. But that's even more magic :pcc @psdobolewskiPhD
Type of change
References
How has this been tested?
as there are small differences between the two Qt bindings.
Final checklist:
trans.
to make them localizable.For more information see our translations guide.