napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.07k stars 410 forks source link

Two bug fixes concerning 4D surfaces, ie surfaces with (t, z, y, x) vertices #6874

Closed odinsbane closed 1 day ago

odinsbane commented 3 weeks ago

This makes two small updates. One, it makes sure that mesh indices are integer which causes issues 6870 and two it checks if there are any displayed vertices before updating the normals that causes issue 6872.

References and relevant issues

https://github.com/napari/napari/issues/6872 https://github.com/napari/napari/issues/6870

Description

It fixes the two bugs described above. Bug 6872 is not present in the 0.41 release!

brisvag commented 3 weeks ago

Thanks for the issues and PR @odinsbane, great work!

These are sneaky bugs that coudl show up again; you think you could add some small tests to check these failing cases? Probably somewhere in napari/layers/surface/_tests/test_surface.py.

odinsbane commented 2 weeks ago

Ill take a look at how the tests are structured. Both of these seem to require the gui, and they throw the exceptions on the event thread. I suspect if I can change the frame and then fire the event action on the surface they should appear.

brisvag commented 2 weeks ago

I don't think you'll need to do complex interactions with the gui; actually, the right place to look is rather napari/_vispy/_tests/test_vispy_surface_layer.py, which should have also similar tests already present!

odinsbane commented 2 weeks ago

Cool, I actuall found a way to make the integer index happend. It happens when the layer is active, which means the viewer calls 'get_status' which ends up calling surface._get_value_3d. So something like this will case the error:

l3 = napari.layers.Surface( (np.array(d2), np.array(f2)) )
l3._get_value_3d(np.array([4., 1., 0, 0]), np.array([4., 12., 0, 0]), [1,2,3])

Maybe that should still go in the surface test. I can check the vispy one later.

odinsbane commented 2 weeks ago

I added two tests, both of them will fail with an exception.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 95.23810% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.42%. Comparing base (5527240) to head (57330e0). Report is 3 commits behind head on main.

Files Patch % Lines
napari/layers/surface/surface.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6874 +/- ## ========================================== - Coverage 92.43% 92.42% -0.01% ========================================== Files 614 614 Lines 54903 54919 +16 ========================================== + Hits 50748 50758 +10 - Misses 4155 4161 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Czaki commented 2 weeks ago

As I push patches here, then I think that you @brisvag should review also.

jni commented 1 day ago

Thanks for your contribution @odinsbane!