Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a preview functionality #100

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add a preview functionality #100

wants to merge 10 commits into from

Conversation

Fifourche
Copy link
Contributor

@Fifourche Fifourche commented May 31, 2021

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).

animation_previewer.mp4

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

@codecov
Copy link

codecov bot commented May 31, 2021

Codecov Report

Attention: Patch coverage is 87.88732% with 43 lines in your changes missing coverage. Please review.

Project coverage is 86.14%. Comparing base (07f93c7) to head (72dc5ae).
Report is 140 commits behind head on main.

Files with missing lines Patch % Lines
napari_animation/_qt/frameslider_widget.py 89.18% 24 Missing ⚠️
napari_animation/_qt/playbutton_widget.py 76.78% 13 Missing ⚠️
napari_animation/_qt/_tests/test_play.py 91.54% 6 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sofroniewn
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

image

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

@Fifourche
Copy link
Contributor Author

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 ! ^^

@Fifourche Fifourche marked this pull request as draft July 20, 2021 23:03
@sofroniewn
Copy link
Collaborator

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
Copy link
Contributor Author

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 Fifourche marked this pull request as ready for review August 6, 2021 10:59
@Fifourche
Copy link
Contributor Author

The tests failed here but for a napari related issue ?

@sofroniewn
Copy link
Collaborator

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
Copy link
Contributor Author

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 !

@psobolewskiPhD psobolewskiPhD added the enhancement New feature or request label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants