Fix spherical markers depth buffer with scaling='visual' #2506
Conversation
There was a problem hiding this comment.
Ok so this is not only a fix (specifically in the if u_spherical == true part I'm guessing?) but also just a general cleanup of the code. Put another way, the main fix is that the correct transformation is applied to the spherical == true block in the shader, right?
vispy/visuals/markers.py
Outdated
| self._scaling = value | ||
| self._scaling_int = scaling_int | ||
| self._scaling_mode = scaling_modes[value] |
There was a problem hiding this comment.
Is there any need for a separate _scaling_mode and _scaling now that an integer isn't based to the shader?
There was a problem hiding this comment.
If we want to retain the ability to do:
marker.scaling = False
assert marker.scaling == FalseOtherwise we'd get:
marker.scaling = False
assert marker.scaling == 'fixed'I'd be ok with the latter, but I thought you wanted to retain the previous behaviour in #2470?
There was a problem hiding this comment.
I think I was hoping for backwards compatibility with someone setting it to False/True. My comment here is to say that self._scaling and self._scaling_mode are now the same for users specifying the new flags. If someone is doing assert marker.scaling == X then I personally am OK with that failing now. But maybe I'm misremembering what I said before.
There was a problem hiding this comment.
yeah for the new flags those are identical, but I agree it's nicer to just ditch the true/false otherwise.
There was a problem hiding this comment.
Updated to only keep the new value.
Yup! And also in the |
|
Is this ready to go in @djhoese? |
# Description Just a simple bump to our vispy dependency, now that 0.14 is out. This comes with a few fixes and new features relevant for napari: - Fix for wrong spherical marker depth: vispy/vispy#2506 - Fix for wrong shading of surfaces with flipped normals: vispy/vispy#2493 - picking filters for mesh and markers by @aganders, which can now be used in napari to drastically speed up our get-status: vispy/vispy#2500 --------- Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> Co-authored-by: napari-bot <napari-bot@users.noreply.github.com>
Just a simple bump to our vispy dependency, now that 0.14 is out. This comes with a few fixes and new features relevant for napari: - Fix for wrong spherical marker depth: vispy/vispy#2506 - Fix for wrong shading of surfaces with flipped normals: vispy/vispy#2493 - picking filters for mesh and markers by @aganders, which can now be used in napari to drastically speed up our get-status: vispy/vispy#2500 --------- Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> Co-authored-by: napari-bot <napari-bot@users.noreply.github.com>
When we introduced the new
visualscaling for markers in #2359 (later tweaked in #2470), we forgot to do the same changes in the depth buffer calculation. This lead to a mismatch between depth size and actual size. See this example in napari where with high scaling we end up with sphere that "merge" when overlapping, rather than simply obscuring each other as they should:balls_mering.mp4
This PR makes sure everything is calculated the same way. To avoid having many if/else and making it easier to maintain, I also moved the logic for choosing the scaling "mode" to outside the shader.