search

napari / napari-animation

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

Add `KeyFrame` object #88

Closed tlambert03 closed 3 years ago

tlambert03 commented 3 years ago

Haven't been around for the earlier conversations ... so not sure how you'll feel about this, but I was starting to look into updating the KeyFramesListWidget to take advantage of some of the new QListWidget stuff in napari (since you're already using the EventedList) ... however, capturing a keyframe as a pure dict presents some challenges (particularly when it comes a set-based selection model), since dicts are not hashable.

Also, as a relative newcomer to the codebase, I found myself wondering "what exactly is this mysterious keyframe object?" What are the possible, keys, etc...

So this PR introduces a KeyFrame and ViewerState dataclass, and pulls some of the methods off of Animation that are just there to create a new KeyFrame from the current viewer and adds them as KeyFrame.from_viewer and ViewerState.from_viewer.

if you were liking the "keyframe-as-pure-dict" model, feel free to reject this... but something like this will be necessary if you want to use the SelectableEventedList and associated listviews.

codecov[bot] commented 3 years ago

Codecov Report

Merging #88 (337c8c3) into main (d75e45b) will increase coverage by 0.93%. The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   66.95%   67.89%   +0.93%     
==========================================
  Files          17       18       +1     
  Lines         802      816      +14     
==========================================
+ Hits          537      554      +17     
+ Misses        265      262       -3
Impacted Files Coverage Δ
napari_animation/_qt/animationslider_widget.py 33.33% <0.00%> (ø)
napari_animation/_qt/frame_widget.py 34.88% <0.00%> (+0.79%) :arrow_up:
napari_animation/_qt/keyframeslist_widget.py 30.68% <0.00%> (+0.68%) :arrow_up:
napari_animation/animation.py 74.28% <88.88%> (-5.26%) :arrow_down:
napari_animation/__init__.py 100.00% <100.00%> (ø)
napari_animation/_tests/conftest.py 100.00% <100.00%> (ø)
napari_animation/_tests/test_animation.py 100.00% <100.00%> (ø)
napari_animation/_tests/test_interpolation.py 100.00% <100.00%> (ø)
napari_animation/key_frame.py 100.00% <100.00%> (ø)
napari_animation/utils.py 97.36% <100.00%> (+1.91%) :arrow_up:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d75e45b...337c8c3. Read the comment docs.

tlambert03 commented 3 years ago

Thanks for fixing my merge snafu :)

Glad you like this. Just want to alert you to the one thing that I think might want further attention: which is that the interpolation functions expect dicts, so I'm using asdict here. But if you don't intend to support interpolation between arbitrary dicts (that is, if it's always going to be a key frame or viewer state), then we could update those functions too to accept the dataclass objects directly ... in a later PR

alisterburt commented 3 years ago

good point - will open an issue now!