Skip to content

FIX: Update blitting and drawing on the macosx backend #21790

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
merged 4 commits into from
Mar 5, 2022

Conversation

greglucas
Copy link
Contributor

PR Summary

This moves some of the drawing and blitting code to the AGG canvas (instead of trying to use the objc routines) and leverages that to handle when a full AGG re-render should happen. The macosx code always grabs the full AGG buffer when drawing to the GUI window at the objc level (inside drawRect). I removed _draw() and added an update() routine in objc that requests a screen draw within the main run loop.

closes #10980
closes #17445 (Only does a full redraw once now!)
Fixes the example from #17642, but I'm not sure this is still feature-parity with what "flush_events" means on the other backends, so I guess it probably shouldn't be closed.

  • I tried to draw on some of the logic in the QT backend. That portion would be good to have someone take a look at and provide suggestions for improvement.
  • When creating the NavigationToolbar, there are two draw_idle calls that are made that I don't fully understand where they are coming from. I put some comments into the PR to show where that is happening.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@greglucas greglucas mentioned this pull request Nov 29, 2021
6 tasks
@greglucas greglucas force-pushed the macosx-draw-refactor branch 2 times, most recently from 5c10202 to fce4431 Compare December 1, 2021 00:02
@jklymak
Copy link
Member

jklymak commented Dec 16, 2021

Can you give a bit of a guide for how to test this worked? I think the general idea is great - to make the mac backend as similar as possible to the others....

@greglucas
Copy link
Contributor Author

I was testing on the examples from the linked issues as they had pretty good minimal code samples. All of them basically didn't show any updates (redraw the entire canvas background which was white, rather than blitting and updating only the new portion of the figure).

#10980 (Should rotate around)

Code for reproduction
from mpl_toolkits.mplot3d import axes3d
import matplotlib.pyplot as plt

fig = plt.figure()
ax = fig.add_subplot(111, projection='3d')

# load some test data for demonstration and plot a wireframe
X, Y, Z = axes3d.get_test_data(0.1)
ax.plot_wireframe(X, Y, Z, rstride=5, cstride=5)

# rotate the axes and update
for angle in range(0, 360):
    ax.view_init(30, angle)
    plt.draw()
    plt.pause(.001)

#17445 (Should not print "Drawing!" 40+ times and also update the arrow animation)

Code for reproduction
import matplotlib.pyplot as plt
from matplotlib import animation
import numpy as np

class BaseVisualizer():
    def __init__(self, **kwargs):
        self.__iteration_number = kwargs.get('iteration_number', 1)
        self.__intervals = 1
        self.__interval_ms = kwargs.get('interval', 2000)


        self._marker_size = 4
        self._index = 0

        self._positions = np.zeros(2)
        self._velocities = np.ones(2)
        self.__frame_interval = 50  # ms

    def replay(self):
        self._fig = plt.figure()
        self._fig.canvas.mpl_connect('draw_event', lambda ev: print('Drawing!'))
        self._fig.stale_callback = None

        ax = self._fig.add_subplot(1, 1, 1, label='BaseAxis')

        # Plot all particle pos
        self.__particles = ax.scatter([], [], marker='o', zorder=2, color='red')

        # Plot all velocities
        self.__particle_vel = ax.quiver([], [], [], [], angles='xy', scale_units='xy', scale=1)

        self.__rectangle = plt.Rectangle([0,0], 1, 1, ec='none', lw=2, fc='none')
        ax.add_patch(self.__rectangle)

        # Amount of frames to play considering one interval should last __interval ms.
        frames = int(self.__intervals*self.__interval_ms/self.__frame_interval)

        # iteration_number+1 for initialization frame
        _ = animation.FuncAnimation(self._fig, self._animate, frames=frames, interval=self.__frame_interval,
                                    blit=True, repeat=False, fargs=[frames])

        plt.show()

    def _animate(self, i: int, frames: int):
        """
        Animation function for animations. Only used for FuncAnimation
        """

        self._marker_size = int(50 * self._fig.get_figwidth()/self._fig.dpi)

        # Calculate the scale to apply to the data in order to generate a more dynamic visualization
        scale = i / (frames/self.__intervals)

        # Calculate scaled position and velocity

        pos_scaled = self._positions + scale * self._velocities
        vel_scaled = (1-scale)*self._velocities

        # Update the particle position
        self.__particles.set_offsets(pos_scaled)
        self.__particles.set_sizes([self._marker_size**2])

        # Update the velocities
        ax = self._fig.gca(label='BaseAxis')
        self.__particle_vel = ax.quiver(pos_scaled[0], pos_scaled[1], vel_scaled[0], vel_scaled[1], angles='xy', scale_units='xy', scale=1, color='#CFCFCF', width=self._marker_size*0.001)
        return self.__particles, self.__particle_vel


if __name__ == '__main__':
    vis = BaseVisualizer()
    vis.replay()

#17642 (A sine wave should move across the screen)

Code for reproduction
import matplotlib.pyplot as plt
import numpy as np

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

fig, ax = plt.subplots()

# animated=True tells matplotlib to only draw the artist when we
# explicitly request it
(ln,) = ax.plot(x, np.sin(x), animated=True)

# make sure the window is raised, but the script keeps going
plt.show(block=False)

# stop to admire our empty window axes and ensure it is rendered at
# least once.
#
# We need to fully draw the figure at its final size on the screen
# before we continue on so that :
#  a) we have the correctly sized and drawn background to grab
#  b) we have a cached renderer so that ``ax.draw_artist`` works
# so we spin the event loop to let the backend process any pending operations
plt.pause(0.1)

# get copy of entire figure (everything inside fig.bbox) sans animated artist
bg = fig.canvas.copy_from_bbox(fig.bbox)
# draw the animated artist, this uses a cached renderer
ax.draw_artist(ln)
# show the result to the screen, this pushes the updated RGBA buffer from the
# renderer to the GUI framework so you can see it
fig.canvas.blit(fig.bbox)

for j in range(100):
    # reset the background back in the canvas state, screen unchanged
    fig.canvas.restore_region(bg)
    # update the artist, neither the canvas state nor the screen have changed
    ln.set_ydata(np.sin(x + (j / 100) * np.pi))
    # re-render the artist, updating the canvas state, but not the screen
    ax.draw_artist(ln)
    # copy the image to the GUI state, but screen might not changed yet
    fig.canvas.blit(fig.bbox)
    # flush any pending GUI events, re-painting the screen if needed
    fig.canvas.flush_events()
    # you can put a pause in if you want to slow things down
    # plt.pause(.1)

@dstansby
Copy link
Member

I can confirm this fixes the attached examples. I don't think I have the expertise to thoroughly review the code changes, but within what I know they look fine. Is there a way to add a blitting test for the macosx backend to the test suite?

@greglucas
Copy link
Contributor Author

Is there a way to add a blitting test for the macosx backend to the test suite?

This is definitely a good idea, but as far as I can tell if I try to save anything during the process, the buffer gets updated and the draw does actually happen, so the old versions would actually still appear to work from a test perspective, even though they don't on the screen. I tried grabbing the buffer during one of the animations, and the buffer is getting updated I believe, it is just Mac isn't drawing the buffer to the screen. If anyone has a suggestion for how to do the final screen buffer grab I'm happy to implement that.

Here is the quick check I was trying comparing new/old buffer bytes before/after blitting or animating:

new_bytes = bytes(fig.canvas.buffer_rgba())
print(new_bytes == orig_bytes)  # False on both main and this PR

@dstansby, did the final example with the sine wave work for you? When I go back to that today I don't get the flush_events to work as expected, so I may have had something crossed up in my environments. I think I may also need to bring over the commit from #21768 after all.

self._draw_pending = True
with self._idle_draw_cntx():
self._draw_pending = False
self.draw()
Copy link
Member

Choose a reason for hiding this comment

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

This should not call draw as draw_idle should always be "cheap" .

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 I took this from the qt_backend, which appears to do the same self.draw() call from within draw_idle? I'm not entirely clear where the "pending" of draw_idle() should take place here, whether it should be within the Python context managers, or if perhaps I'm just missing putting this into the Mac-specific event loop similar to the singleShot call from QT?

def _draw_idle(self):
with self._idle_draw_cntx():
if not self._draw_pending:
return
self._draw_pending = False
if self.height() < 0 or self.width() < 0:
return
try:
self.draw()
except Exception:
# Uncaught exceptions are fatal for PyQt5, so catch them.
traceback.print_exc()

Copy link
Member

Choose a reason for hiding this comment

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

I think the logic with Qt is that every call to canvas.draw_idle set self._draw_pending to True and then schedules a single-shot timer to run canvas._draw_idle. Thus many calls to draw_idle within a block of Python (which runs without letting the event loop spin) will stack up a bunch of single-shot timers. When ever the main loop gets to run (which is the earliest we would be able to paint the screen anyway!) the timers start to expire. The first one actually does the update and the rest short-circuit out.

I suspect that you can do the same single-shot trick with OSX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the clear explanation! That makes sense to me (stacking a bunch of callbacks in the event loop, and those callbacks depend on whether the draw has already happened or not). Let me know if you think the implementation is wrong here, or if I misunderstood.

@greglucas
Copy link
Contributor Author

OK, I must have had something mixed up in my test cases before and this does need the update to flush_events so that the macosx event loop has time to process the updates. I brought the commit from #21790 over here, but I'm happy to reopen that one again if it would be easier consider them separately.

@tacaswell tacaswell added this to the v3.6.0 milestone Dec 24, 2021
@greglucas greglucas force-pushed the macosx-draw-refactor branch from ad04a33 to ecd8c84 Compare January 9, 2022 17:51
@greglucas
Copy link
Contributor Author

I just pushed up a new commit that I think addresses the draw_idle callback. I tried to add a singleshot timer and start that without needing to stop the timer, but unfortunately that segfaults on the example from #17445. There is something about threading going on in there (if I remove Py_BEGIN_ALLOW_THREADS it works as expected), but I couldn't figure out how to resolve it at the C level. It also only affects the single-shot timers, if I make it a repeating timer then everything works just fine again. So, my guess is there is some race condition going on with the deallocation between threads and when the timers get released.

I was able to get around this by stopping the timer at the Python level after firing. I'm not sure that is the most elegant way around this, so I'm open to suggestions if anyone has other ideas for how to approach the runloop singleshot timers here.

@greglucas greglucas force-pushed the macosx-draw-refactor branch from 335355b to 72a028a Compare January 11, 2022 05:12
@greglucas
Copy link
Contributor Author

I have now added a test to verify that we aren't getting multiple draw_events emitted from the backends. I added it into test_backends_interactive as that is where the other show() based tests are, but I could see it going over to test_animation too. "gtk3agg" was getting multiple draw events for me locally, and "gtk3cairo" was complaining about blitting on the canvas, so I decided to skip gtk3 for now with a FIXME to look into that later.

src/_macosx.m Outdated
@@ -392,6 +385,9 @@ static CGFloat _get_device_scale(CGContextRef cr)
static PyObject*
FigureCanvas_flush_events(FigureCanvas* self)
{
// We need to interrupt the runloop very briefly to allow the view to be
Copy link
Member

Choose a reason for hiding this comment

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

Is this interrupting the runloop (which is what I assume that OSX calls the event loop?) or letting it actually run (and interrupting the Python interpreter)?

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'm not certain... Is there a way to debug that and see what is going on? I think you are right though, the text I had was misleading about "interrupting". I'm pretty sure we are doing the latter, so I updated the comment.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I left one comment about a comment on the runloop, but otherwise looks good 👍🏻

@greglucas greglucas force-pushed the macosx-draw-refactor branch 2 times, most recently from b5afb9e to b130b75 Compare January 11, 2022 14:37
@greglucas
Copy link
Contributor Author

It looks like the test I added is not happy on some architectures, before I go banging on CI I figured I would see what others would prefer. I see a few options:

  1. Make it a macosx only test since that is what this PR is focused on and fixing
  2. Increase the timeouts/pauses even more to allow the first draw events to flush out (I think that is why tkagg is failing, as it passes for me locally), this comes at the cost of increased test time.
  3. Start removing cases from the tests here (wx[agg], and tkagg), comes at the cost of less test coverage. wx/wxagg I'm not exactly sure what is going on there, if a longer pause would help or if the blitting is broken there too.
  4. Find a better way to test this (would need some suggestions here!)

@tacaswell
Copy link
Member

A possible idea: frames can be a generator, make it yield a few times and then schedule a single-shot timer to close the figure. You can then do plt.show(block=True), rely on the generator to close the window and exit the event loop and not have to have an arbitrary timeout.

@dstansby
Copy link
Member

  1. I apologise for asking if a test was possible 😄
  2. I'm going to unsubscribe, but feel free to ping me when this is ready for review again

@greglucas greglucas force-pushed the macosx-draw-refactor branch 3 times, most recently from 0fd4022 to 0c92c89 Compare January 12, 2022 14:56
@tacaswell
Copy link
Member

Fully avoiding the animation framework and just testing the thing we want to test is a much better idea than inviting generators to the party!

@greglucas
Copy link
Contributor Author

@dstansby, I think this is good to go for a check from you again. i.e. can you double check me on those examples to make sure I didn't screw something up? Don't apologize for the test request! It is always good to add tests when we can.

@tacaswell, This steals most of your other blitting example for the test, so thanks for that :) Unfortunately, the actual reason for going this direction was because macosx has an issue with single-shot timers and not quitting out of the test on the runners when using the generator/animation approach. Locally it hangs until I move my mouse or do something else interactive on the system, so there is some other issue with macosx event loops and the handoff from Python <-> NSApp which I will try and make a reproducible example for and open an issue.

@dstansby
Copy link
Member

The examples in #10980 and #17445 work well for me on this branch 👍

@greglucas greglucas force-pushed the macosx-draw-refactor branch from 0c92c89 to 25f9b92 Compare January 21, 2022 23:37
This inherits some of the drawing and blitting code from the
AGG canvas and leverages that to handle when a full AGG re-render
should happen. The macosx code always grabs the full AGG buffer
when doing the draw to the GUI window.
The macosx backend would not update on flush_events calls due to the
loop being run too fast for the view to update properly in the NSApp.
Fix that by adding an unnoticeable RunLoop timer slowdown that
allows the app to see the changes.
This adds a singleshot timer to the draw_idle method. This has
a callback that triggers the draw upon firing the timer if the
draw has not occurred yet, otherwise it short-circuits out of
the draw and removes the timer.
@greglucas
Copy link
Contributor Author

Any mac users want to test this out and push it through the finish line?

@greglucas greglucas force-pushed the macosx-draw-refactor branch from 8536c7a to a4c007e Compare March 4, 2022 14:56
This adds a test that only one draw_event is called when using
blitting with animations. There will be more than one draw_event
if blitting is not implemented properly and instead calls a full
redraw of the canvas each time.
@greglucas greglucas force-pushed the macosx-draw-refactor branch from a4c007e to 5415418 Compare March 4, 2022 16:39
@tacaswell tacaswell requested a review from dstansby March 4, 2022 20:45
@tacaswell
Copy link
Member

ping @dstansby who recently reported owning a M1 machine and volunteering to test mac things ;)

@dstansby
Copy link
Member

dstansby commented Mar 5, 2022

Of the examples listed in #21790 (comment), the 1st and 3rd work for me with this branch, but the 2nd does not. It prints "Drawing!" once, and nothing changes in the empty plot that appears.

@greglucas
Copy link
Contributor Author

Thanks for checking @dstansby! That one has a very small marker and thin grey line that are updated, so maybe it just didn't show up as obviously (especially with HiDPI screen)? It does work for me locally copy-pasting that example.

Can you try changing the marker size to make it more obvious: self._marker_size = int(500 * self._fig.get_figwidth() / self._fig.dpi)

If that doesn't work, my guess is this would be an M1 issue then :-/

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.

I spotted the marker - clearly wasn't looking closely enough first time!

@greglucas
Copy link
Contributor Author

Thanks for checking! (It was not the easiest example to see) I'm going to self-merge based on the two approvals and this isn't targeting 3.5.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants