Skip to content

Refactor a bit animation start/save interaction. #16078

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.

Sign up for GitHub

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
merged 2 commits into from
Jan 5, 2020
Merged

Refactor a bit animation start/save interaction. #16078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27d1949
Better error reporting on failure of cache_frame_data test.
anntzer Jan 3, 2020
b384e57
Simplify animation dont-start-on-save.
anntzer Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 7 additions & 15 deletions lib/matplotlib/animation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,9 +939,11 @@ def _start(self, *args):
Starts interactive animation. Adds the draw frame command to the GUI
handler, calls show to start the event loop.
"""
# Do not start the event source if saving() it.
if self._fig.canvas.is_saving():
return
# First disconnect our draw event handler
self._fig.canvas.mpl_disconnect(self._first_draw_id)
self._first_draw_id = None # So we can check on save

# Now do any initial draw
self._init_draw()
Expand Down Expand Up @@ -1057,14 +1059,6 @@ def func(current_frame: int, total_frames: int) -> Any
if savefig_kwargs is None:
savefig_kwargs = {}

# Need to disconnect the first draw callback, since we'll be doing
# draws. Otherwise, we'll end up starting the animation.
if self._first_draw_id is not None:
self._fig.canvas.mpl_disconnect(self._first_draw_id)
reconnect_first_draw = True
else:
reconnect_first_draw = False

if fps is None and hasattr(self, '_interval'):
# Convert interval in ms to frames per second
fps = 1000. / self._interval
Expand Down Expand Up @@ -1123,8 +1117,11 @@ def func(current_frame: int, total_frames: int) -> Any
_log.info("Disabling savefig.bbox = 'tight', as it may cause "
"frame size to vary, which is inappropriate for "
"animation.")
# canvas._is_saving = True makes the draw_event animation-starting
# callback a no-op.
with mpl.rc_context({'savefig.bbox': None}), \
writer.saving(self._fig, filename, dpi):
writer.saving(self._fig, filename, dpi), \
cbook._setattr_cm(self._fig.canvas, _is_saving=True):
for anim in all_anim:
anim._init_draw() # Clear the initial frame
frame_number = 0
Expand All @@ -1145,11 +1142,6 @@ def func(current_frame: int, total_frames: int) -> Any
frame_number += 1
writer.grab_frame(**savefig_kwargs)

# Reconnect signal for first draw if necessary
if reconnect_first_draw:
self._first_draw_id = self._fig.canvas.mpl_connect('draw_event',
self._start)

def _step(self, *args):
"""
Handler for getting events. By default, gets the next frame in the
Expand Down
13 changes: 5 additions & 8 deletions lib/matplotlib/tests/test_animation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,13 +237,8 @@ def test_failing_ffmpeg(tmpdir, monkeypatch):
make_animation().save("test.mpeg")


@pytest.mark.parametrize("cache_frame_data, weakref_assertion_fn", [
pytest.param(
False, lambda ref: ref is None, id='cache_frame_data_is_disabled'),
pytest.param(
True, lambda ref: ref is not None, id='cache_frame_data_is_enabled'),
])
def test_funcanimation_holding_frames(cache_frame_data, weakref_assertion_fn):
@pytest.mark.parametrize("cache_frame_data", [False, True])
def test_funcanimation_cache_frame_data(cache_frame_data):
fig, ax = plt.subplots()
line, = ax.plot([], [])

Expand Down Expand Up @@ -282,4 +277,6 @@ def frames_generator():
anim.save('unused.null', writer=writer)
assert len(frames_generated) == 5
for f in frames_generated:
assert weakref_assertion_fn(f())
# If cache_frame_data is True, then the weakref should be alive;
# if cache_frame_data is False, then the weakref should be dead (None).
assert (f() is None) != cache_frame_data
Copy link
Member

@timhoffm timhoffm Jan 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Woulndn't it be more clear to add a message to the assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the comments above are good enough, and adding code to generate the correct message would actually obfuscate the issue at hand.