napari / napari-animation

A napari plugin for making animations
https://napari.github.io/napari-animation/
Other
79 stars 27 forks source link

Use boolean interpolation for `dims.ndisplay` #195

Closed psobolewskiPhD closed 10 months ago

psobolewskiPhD commented 10 months ago

Closes: https://github.com/napari/napari-animation/issues/156

This PR adds interpolate_bool to the Interpolation ENUM and then uses that for interpolating dims.ndisplay because this can only be 2 or 3, behaving like a boolean, which is how all strings are interpolated. Previously, the numeric interpolation was used because ndisplay type is int and so the outcome was cast to int, which truncated the decimal resulting in the opposite behavior from strings, making for a mismatch in the end behavior.

Try doing an animation where you change to 3D viewer and then rotate the object. On live, this won't result in the expected animation, but with this PR it will.

import napari
from napari_animation import Animation

viewer = napari.Viewer()
viewer.dims.ndisplay = 2
viewer.open_sample("napari", "binary_blobs_3D")
anim = Animation(viewer)
anim.capture_keyframe(steps=3)
viewer.dims.ndisplay = 3
viewer.camera.angles = (-30, 40, 15)
anim.capture_keyframe()
anim.animate("test.mp4")
codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5760ee2) 85.90% compared to head (9cd853c) 86.02%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #195 +/- ## ========================================== + Coverage 85.90% 86.02% +0.11% ========================================== Files 26 26 Lines 1064 1073 +9 ========================================== + Hits 914 923 +9 Misses 150 150 ```

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

jni commented 10 months ago

@psobolewskiPhD I don't see this as a problem but I don't understand the bug as you stated it — can you describe what happens before this PR in your script? e.g. in #154 I found that my slider was missing but the ndisplay machinery worked fine...

psobolewskiPhD commented 10 months ago

Here's the output of the script from main: https://github.com/napari/napari-animation/assets/76622105/6329fb61-02eb-4609-898f-31c5c7b486a5 Notice that because the ndisplay behavior is 2 the whole time, then switch 3 (the opposite of any string or boolean properties) then at the end, the camera change is basically neutered. Here's this PR branch: https://github.com/napari/napari-animation/assets/76622105/bee0c1ad-3bdf-4669-93fb-4862a9874799

Now you can see that the rotation is shown. It's even more noticeable if you change something like colormap or blending or a 3D specific setting.

jni commented 10 months ago

Ah, I guess in my example I bookended the ndisplay change with frame captures, so interpolation didn't matter. My spidey sense was "I don't know what is going to be done here so imma make sure I capture around it." 😂

I'm a little confused about the syntax of the script. Specifically the lines:

anim.capture_keyframe(steps=3)
viewer.dims.ndisplay = 3
viewer.camera.angles = (-30, 40, 15)
anim.capture_keyframe()

But if I look at the capture_keyframe docstring, the documentation for steps is:

steps : int Number of interpolation steps between last keyframe and captured one.

So as far as I understand it, there should only be a single 3D frame, since the capture with 3 frames happens before the viewer is 3D? Am I misunderstanding something or is this exposing another bug / a documentation error?

psobolewskiPhD commented 10 months ago

I'm not sure I get the question? The first keyframe is set with no change of the viewer, so I used 3 to save on file size. The next keyframe, uses the default 15, but yes before this PR yes, you will get 1 frame of 3D, no matter what steps. With this PR, with the IMO fixed interpolation for ndisplay, you see the switch to 3D in 1 frame and then 14 frames of the rotation. Everything is consistent with the docstring as far as I can tell.

jni commented 10 months ago

Yes, I'm an idiot, I didn't realise the default was 15 and was reading it as the default being 1, hence my confusion. 🤦 Thanks for your patience with me @psobolewskiPhD! 😅