Synced slider with frame index rather than active keyframe by Fifourche · Pull Request #99 · napari/napari-animation

Fifourche · 2021-05-29T10:24:37Z

A PR to link the AnimationSlider with the frame index being set.

Before it was synced with the change of key_frames.selection.active, and now with the signal of an Emitter : frame_index, added to Animation. This way, if we use set_movie_frame_index, it'll put the slider at the frame's position and not at the closest key frame.

For some context, the idea I had was to sync the slider with a frame_index signal to prepare the way for a "previewer" capability. The preview would be launched via a method of Animation, or a button next to the slider.

…. Frame sequence changed to contain the first keyframe.

codecov · 2021-05-29T10:28:39Z

Codecov Report

Merging #99 (643e5fc) into main (aab5909) will increase coverage by 0.51%.
The diff coverage is 90.38%.

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
+ Coverage   90.08%   90.59%   +0.51%     
==========================================
  Files          20       20              
  Lines         827      851      +24     
==========================================
+ Hits          745      771      +26     
+ Misses         82       80       -2

Impacted Files	Coverage Δ
napari_animation/animation.py	`86.17% <83.33%> (+7.34%)`	⬆️
napari_animation/_qt/animation_widget.py	`85.55% <88.88%> (-2.36%)`	⬇️
napari_animation/frame_sequence.py	`98.83% <95.83%> (-1.17%)`	⬇️
napari_animation/_tests/test_frame_sequence.py	`100.00% <100.00%> (ø)`

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 aab5909...643e5fc. Read the comment docs.

…sync with slider.

alisterburt

hey @Fifourche - I love the idea but am not 100% on the implementation, especially the generation of the whole list to obtain the required index

Could you explain the purpose of the extra code you added into the cache too? 🙂

alisterburt · 2021-06-01T08:49:30Z

+    def _on_frame_index_changed(self, event=None):
+        frame_index = event.value
+        self.animationSlider.blockSignals(True)
+        self.animationSlider.setValue(frame_index)
+        self.animationSlider.blockSignals(False)

+    def _on_active_keyframe_changed(self, event):
+        active_keyframe = event.value


just making sure I understand what's going on here

frame_index is linked to the frame in the whole animation, active keyframe is linked to the key-frame in the key-frames list?

If I've got that right, would you mind adding some docstrings to these methods?

Yes and yes ! will do :)

alisterburt · 2021-06-01T08:51:55Z

+        self.__set_frame_index = 0
+        self.events = EmitterGroup(source=self, _set_frame_index=None)
+
+    @property
+    def _set_frame_index(self):
+        return self.__set_frame_index
+
+    @_set_frame_index.setter
+    def _set_frame_index(self, frame_index):
+        if frame_index != self._set_frame_index:
+            self.__set_frame_index = frame_index
+            self.events._set_frame_index(value=frame_index)
+


What do you think about renaming this to _frame_index? I feel like it doesn't make sense to 'get' a _set_frame_index property

alisterburt · 2021-06-01T08:55:26Z


+    def _keyframe_frame_index(self, keyframe):
+        n_frames = len(self._frames)
+        kf1_list = [self._frames._frame_index[n][0] for n in range(n_frames)]


it feels quite heavy to generate the whole list of keyframes to get an index... maybe we can find a better way?

I'm switching back to something I wrote on a previous version. I hope it feels lighter ! :)

alisterburt · 2021-06-01T08:57:24Z

+        else:
+            f = 0
+            if len(self._key_frames) == 1:
+                kf1 = self._key_frames[0]
+            else:
+                for kf0, kf1 in pairwise(self._key_frames):
+                    for s in range(kf1.steps):
+                        fraction = s / kf1.steps
+                        self._frame_index[f] = (kf0, kf1, fraction)
+                        f += 1


can you remove the outer else here to make the whole block less nested?

I still kept an elif, for probably a better readability !

alisterburt · 2021-06-01T09:06:31Z

+        else:
+            f = 0
+            if len(self._key_frames) == 1:
+                kf1 = self._key_frames[0]


I don't really understand the purpose of this little section, could you explain it?

Previously, if there was only one keyframe, it was not added as a frame and _frames was empty. I just added this here so _frames would be empty only when there are no keyframes at all.

alisterburt · 2021-06-01T11:22:30Z

Okay cool! Maybe we could refactor this logic into a ‘has_frames’ properly rather than adding it here?

…tions.

…o tests_play

Fifourche · 2021-06-01T13:52:47Z

Wasn't sure of what you meant, as it's mostly the number of key frames that is needed there. Would you still go for an attribute for this ?

Okay cool! Maybe we could refactor this logic into a ‘has_frames’ properly rather than adding it here?
…
On 1 Jun 2021, at 12:19, Fifourche @.***> wrote: @Fifourche commented on this pull request. In napari_animation/frame_sequence.py: > self.events.n_frames(value=len(self)) return - f = 0 - for kf0, kf1 in pairwise(self._key_frames): - for s in range(kf1.steps): - fraction = s / kf1.steps - self._frame_index[f] = (kf0, kf1, fraction) - f += 1 + else: + f = 0 + if len(self._key_frames) == 1: + kf1 = self._key_frames[0] Previously, if there was only one keyframe, it was not added as a frame and _frames was empty. I just added this here so _frames would be empty only when there are no keyframes at all. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

…ethods for it.

Fifourche · 2021-07-04T10:10:10Z

Hey :)
I'm changing a bit this PR to switch the current frame index attribute from animation to FrameSequence ! In my eyes it makes FrameSequence a bit more like an virtual slider, which would be relevant to connect to the GUI slider, and thus prepare the ground for a play button with #100 .

alisterburt

hey @Fifourche - this is looking really nice now! Thanks for your patience with my reviewing here, let's get this in!

alisterburt · 2021-07-08T22:27:32Z

+        steps_to_keyframe = [
+            kf.steps for kf in self.key_frames[1 : keyframe_index + 1]
+        ]


this is looking much nicer! 🙂

alisterburt · 2021-07-08T22:31:22Z

+    @property
+    def _current_index(self):
+        return self.__current_index
+
+    @_current_index.setter
+    def _current_index(self, frame_index):
+        if frame_index != self._frame_index:
+            self.__current_index = frame_index
+            self.events._current_index(value=frame_index)


I really like having this directly on the FrameSequence

Synced slider with movie_frame_index rather than only active_keyframe…

cac71bf

…. Frame sequence changed to contain the first keyframe.

Switched event to an evented hidden attribute '_set_frame_index' for …

0e056a6

…sync with slider.

Fifourche mentioned this pull request May 31, 2021

Add a preview functionality #100

Open

alisterburt requested changes Jun 1, 2021

View reviewed changes

Fifourche added 2 commits June 1, 2021 15:44

Change way to compute the frame index of keyframe. Other minor correc…

8ee471b

…tions.

Merge branch 'main' of https://github.com/napari/napari-animation int…

63269bd

…o tests_play

Switched to a _current_index attribute for FrameSequence, and added m…

643e5fc

…ethods for it.

alisterburt approved these changes Jul 8, 2021

View reviewed changes

alisterburt merged commit 34ee43b into napari:main Jul 8, 2021

Uh oh!

Conversation

Fifourche commented May 29, 2021

Uh oh!

codecov Bot commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alisterburt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alisterburt commented Jun 1, 2021 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fifourche commented Jun 1, 2021

Uh oh!

Fifourche commented Jul 4, 2021

Uh oh!

alisterburt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 29, 2021 •

edited

Loading

alisterburt left a comment •

edited

Loading

alisterburt commented Jun 1, 2021 via email •

edited

Loading