Skip to content

Fix drawing animated artists changed in selector callback #21342

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

Merged

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Oct 12, 2021

PR Summary

Alternative to #20977.

PR Checklist

  • [N/A] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@anntzer
Copy link
Contributor

anntzer commented Oct 12, 2021

I like the general approach more here.

@ericpre
Copy link
Member Author

ericpre commented Oct 12, 2021

Yes, I agree, this is much better! :)

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+/- stylistic point above.

@ericpre ericpre marked this pull request as draft October 13, 2021 10:17
@ericpre ericpre force-pushed the fix_drawing_animated_artists_callback branch from b6475bc to 515aa53 Compare October 14, 2021 16:01
@ericpre
Copy link
Member Author

ericpre commented Oct 14, 2021

In 515aa53, I remove the if artist.stale:, because when using blitting, the animated artists which are not stale will be missing, because they will either: not be part of the background or not be drawn (because stale is False!).

Adapting one of the blitting example:

import matplotlib.pyplot as plt
from matplotlib.widgets import SpanSelector
import numpy as np

x = np.linspace(0, 2 * np.pi, 100)
values = np.sin(x)

fig, ax = plt.subplots()
(ln,) = ax.plot(x, values, animated=True)
(ln2 , ) = ax.plot([], animated=True)

plt.show(block=False)
plt.pause(0.1)

bg = fig.canvas.copy_from_bbox(fig.bbox)
ax.draw_artist(ln)
fig.canvas.blit(fig.bbox)

def mean(vmin, vmax):
    indmin, indmax = np.searchsorted(x, (vmin, vmax))
    v = values[indmin:indmax].mean()
    ln2.set_data(x, v)
    
span = SpanSelector(ax, mean, direction='horizontal',
                    onmove_callback=mean,
                    interactive=True,
                    drag_from_anywhere=True,
                    useblit=True)

@ericpre ericpre marked this pull request as ready for review October 14, 2021 16:17
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks 👍 - is there a test we an add for this?

@ericpre ericpre force-pushed the fix_drawing_animated_artists_callback branch 2 times, most recently from 453b4cd to 1e988b3 Compare October 17, 2021 16:58
@ericpre
Copy link
Member Author

ericpre commented Oct 17, 2021

I have added a test and switch to list comprehension instead of using filter.

@ericpre
Copy link
Member Author

ericpre commented Oct 18, 2021

2a5c86c fixes an issue with zorder of the artists. Before 2a5c86c, the selector artists will be drawn before other artists and therefore it was behind, as in the following example:

import matplotlib.pyplot as plt
from matplotlib.widgets import SpanSelector
import numpy as np

values = np.ones((10, 10))

fig, ax = plt.subplots()
ax.imshow(values, animated=True)

span = SpanSelector(ax, print, direction='horizontal',
                    interactive=True,
                    drag_from_anywhere=True,
                    useblit=True)

when sorting the artists order, all artists need to sorted together!

@ericpre
Copy link
Member Author

ericpre commented Oct 20, 2021

@dstansby, any chance you can have a look at this again, please? Thanks!

@tacaswell
Copy link
Member

This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details.

To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote "upstream"

git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease   # assuming you are tracking your branch

If you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you.

Thank you for your contributions to Matplotlib and sorry for the inconvenience.

@ericpre ericpre force-pushed the fix_drawing_animated_artists_callback branch 2 times, most recently from 991cdb6 to 67f6c90 Compare October 20, 2021 20:45
@ericpre ericpre force-pushed the fix_drawing_animated_artists_callback branch from 67f6c90 to a189593 Compare November 13, 2021 09:40
@ericpre
Copy link
Member Author

ericpre commented Nov 13, 2021

Can anyone review this PR, please?

@ericpre
Copy link
Member Author

ericpre commented Dec 4, 2021

@dstansby, if this PR looks good to you - from your comment in #21342 (review); can you please approve and merge? :) Thanks!

Convenience method to get all animated artists of a figure, except
those already present in self.artists. 'z_order' is ignored.
"""
return tuple([a for ax_ in self.ax.get_figure().get_axes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a tuple and not a list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason.

@ericpre ericpre force-pushed the fix_drawing_animated_artists_callback branch from a189593 to ea8e6c9 Compare December 31, 2021 16:17
Comment on lines 1848 to 1850
return tuple([a for ax_ in self.ax.get_figure().get_axes()
for a in ax_.get_children()
if a.get_animated() and a not in self.artists])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return tuple([a for ax_ in self.ax.get_figure().get_axes()
for a in ax_.get_children()
if a.get_animated() and a not in self.artists])
return tuple(a for ax_ in self.ax.get_figure().get_axes()
for a in ax_.get_children()
if a.get_animated() and a not in self.artists)

if we are going with a tuple might as well not bother with the list in the middle. I have a slight preference for tuple over list because immutablability is good, but given that his is an internal API either is fine with me!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@tacaswell tacaswell added this to the v3.6.0 milestone Dec 31, 2021
@ericpre ericpre force-pushed the fix_drawing_animated_artists_callback branch from ea8e6c9 to 2eb2b32 Compare January 1, 2022 13:22
@ericpre ericpre force-pushed the fix_drawing_animated_artists_callback branch from 2eb2b32 to 7603999 Compare January 8, 2022 14:01
@ericpre
Copy link
Member Author

ericpre commented Jan 9, 2022

Rebased to get the test suite to pass, can anyone merge this PR?

@dstansby dstansby merged commit ea04978 into matplotlib:main Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants