Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Player animation#24554
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Player animation#24554
Uh oh!
There was an error while loading.Please reload this page.
Conversation
looks like I need to rebase my two commits, but latest main branch didn't seem very stable. any recommendations? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
9cc4e30 to642167dCompareIf event_source was stopped, it should remain stopped after resize event
642167d to6e5456aCompareI think that the animation module has seen some work recently, so that is probably the reason for the need to rebase. In general, the approach is just to keep up rebasing in case of conflicts, although, of course, review can happen in parallel. |
| if self.useblit: | ||
| self.label.set_animated(True) | ||
| self.ax.patch.set_animated(True) | ||
| self.background = self.canvas.copy_from_bbox(self.ax.bbox) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I do not think it is safe to assume that we have drawn the figure at least once here, we need to defer this to the first draw event.
We are 👍🏻 on force-pushing to feature branches (as you did). Unfortunately if you are changing lines that are being worked on in parallel you are going to have to resolve the merge conflicts. |
| ONE_FORWARD_SYMBOL = "$\u29D0$" | ||
| def __init__(self, func, init_func, min_value=0, max_value=100, | ||
| pos=(0.125, 0.92), valstep=1, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is going to need a docstring including the required signatures of the functions passed in.
This is cool work and am supportive of it going in, however I have some concerns about the blitting implementation in the widgets (being more careful about when we aresure that the background has been drawn at least once and all of the sizes are correct etc) and that this is documented well enough that someone can understand what the signatures of the functions passed in need to be passed in. Rather than using |
| @@ -400,6 +429,8 @@ def __init__(self, ax, label, valmin, valmax, valinit=0.5, valfmt=None, | |||
| super().__init__(ax, orientation, closedmin, closedmax, | |||
| valmin, valmax, valfmt, dragging, valstep) | |||
| self.useblit = useblit and self.canvas.supports_blit | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can this sort of thing live inSliderBase?
If blitting is going to be supported on slider then it ought to also be supported onRangeSlider
great, I will probably just wait until those fixes are merged then |
I am going to push this to 3.8 as I am not confident that we will both of those other PRs resolved and have time to get this re-implemented on top of them by the end of the month. |
Moving this to draft.@mikaeltulldahl Hopefully you come back to it after the PRs above are merged! |
PR Summary
-Fix bug in Animation where the animation resumes when resizing window
-Make Button and Slider blittable
-Add PlayerAnimation class with play/pause, time functionality
this is still work in progress, I just thought it was done enough to get some feedback regarding if this is the way to go
should solve#20564
but with a bit less features. but if we actually want all those buttons, it's easy to add
could also add a speed slider/button for slow-motion/fast-forward effects
Known Bugs:
-PlayerAnimation does not work so well when blitting is turned off
-The slider get some strange artifacts with blitting(I have no idea why)
-The sliders value text does not update with blitting(I have no idea why)
PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst