Fifourche commented 3 years ago

Following #99 , added functions to enable a preview ability. This preview capability is compatible with the slider as it allows to still change frame interactively using it (see video snippet).

https://user-images.githubusercontent.com/22133982/120185709-d98c9100-c212-11eb-8354-721b46145198.mp4

I'll add corresponding tests if you agree on the idea at least !

codecov[bot] commented 3 years ago

Codecov Report

Merging #100 (72dc5ae) into main (07f93c7) will increase coverage by 1.23%. The diff coverage is 87.88%.

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   84.91%   86.14%   +1.23%     
==========================================
  Files          21       24       +3     
  Lines         928     1256     +328     
==========================================
+ Hits          788     1082     +294     
- Misses        140      174      +34

Impacted Files	Coverage Δ
napari_animation/animation.py	`86.73% <ø> (-0.14%)`	:arrow_down:
napari_animation/_qt/playbutton_widget.py	`76.78% <76.78%> (ø)`
napari_animation/_qt/frameslider_widget.py	`89.18% <89.18%> (ø)`
napari_animation/_qt/_tests/test_play.py	`91.54% <91.54%> (ø)`
napari_animation/_qt/animation_widget.py	`85.52% <100.00%> (+1.15%)`	:arrow_up:
napari_animation/frame_sequence.py	`100.00% <100.00%> (+1.16%)`	: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 07f93c7...72dc5ae. Read the comment docs.

sofroniewn commented 3 years ago

Hi @Fifourche, sorry just seeing this now. Maybe @alisterburt @tlambert03 and I can give this some review when we get the chance. Am I right in thinking this is sort of like having a "play button" for our preview slider? If so we might be able to re-use some of the play button code we have for our dims slider. I'll check out #99 too when i can as well. Nice work here!!!

Fifourche commented 3 years ago

Hi ! :) Yes that was the idea indeed, but I also wanted to give the ability to have a play function even without the GUI.

That's why I started #99 in fact ! I wanted to have a current_frame attribute much like the dims.current_step, so that I could reuse the dims_slider play function there. In this PR I already use something similar to what's happening in the dims slider play (AnimationWorker, and precautions to allow the slider to still be interactive).

I still need to work on #99 though, and give the current_frame attribute to the FrameSequence rather than Animation for instance.

sofroniewn commented 3 years ago

@Fifourche - do you want to reuse the "play button" from inside napari here

It wasn't quite clear to me how to turn the playing on and off based on the GUI

Fifourche commented 3 years ago

Hey ! Yes, that's I want to do indeed ! But as we didn't have any equivalent of the Dims element, I wanted to wait for #99 to have something similar ; now I want to replace the slider we have for an equivalent of the QtDimSliderWidget from napari, maybe without some options like reverse play. I should probably put this PR as a draft one in fact, sorry ! ^^

sofroniewn commented 3 years ago

I should probably put this PR as a draft one in fact, sorry ! ^^

No worries, all good! Let us know if we can help!!

Fifourche commented 3 years ago

I have a working version of it, ~ready for review I guess ! :)

What I'd like to do still is to move the QtPlayButton from here to superqt, and use subclasses if needed in napari and napari-animation. Do you think this would be worth it @tlambert03 ?

Fifourche commented 3 years ago

The tests failed here but for a napari related issue ?

sofroniewn commented 3 years ago

The tests failed here but for a napari related issue ?

Ok thanks for flagging! @alisterburt or I should be able to review this soon, thanks!!

Fifourche commented 3 years ago

Once again, a napari related issue :

    File "/home/runner/work/napari-animation/napari-animation/.tox/py39-linux-pyside/lib/python3.9/site-packages/napari/_qt/utils.py", line 304, in remove_flash_animation
      widget.setGraphicsEffect(None)
  RuntimeError: Internal C++ object (QtWidgetOverlay) already deleted.

Otherwise it seems ok I think !

napari / napari-animation

Add a preview functionality #100

Codecov Report