PR Summary

This PR seeks to make the failure modes more consistent and manageable when blitting on the TkAgg backend. Draft for now because I don't think it is release critical and some feedback is needed on design choices.

Background

The thread safety changes of #18565 interact a bit awkwardly with the memory safety changes of #14478, namely when _tkagg.blit raises, it pops directly out of the tkinter mainloop call and poisons the current instance of tkapp so that pumping the event loop always raises the ValueError forever. This isn't a big deal for bad values of comp_rule or photoimage since those are matplotlib's responsibilities, however as a user-facing API this prevents a client application from gracefully handling bad bboxes; they are basically fatal.

Why, if this is so bad, did nobody notice? In #14461 the bbox limits are clipped to a mostly-safe range, so only very bad bboxes that fall entirely outside the canvas trigger the issue. (And only my app is crazy enough to generate such bboxes 🤦)

Proposed fix(es)

Extend safe-clipping

The initial commit in this PR makes the most minimal possible change to existing behavior, extending the safe-clipping to boxes outside the canvas; they are simply dropped, and with a fast path optimization to boot. This way if ValueErrors are generated, they are 100% our fault. The if expression comes from the check in _tkagg.cpp but stripping the rules that are redundant to the clipping logic. The disadvantage is that the only way a user knows that their bbox is being clipped is by weird visual artifacts.

Raise aggressively

An alternative fix is to promote the _tkagg.cpp check into the first lines of _backend_tk.blit:

def blit(photoimage, aggimage, offsets, bbox=None):
    ...
    if bbox is not None:
        # Does anyone know why we are calling __array__ directly?
        (x1, y1), (x2, y2) = bbox.__array__().astype(int).tolist()
        if (0 > y1 or y1 > y2 or y2 > height or 0 > x1 or x1 > x2 or x2 > width):
            raise ValueError("Attempting to draw out of bounds")
        bboxptr = (x1, x2, y1, y2)
        comp_rule = TK_PHOTO_COMPOSITE_OVERLAY
    ...

This raise is now catchable and the check inside _tkagg.blit is redundant (though it is super cheap so it should remain in both places). Adding tests for this would finally cover this branch of the if statement. Also ALL bad bbox blits are errors now.

I see two disadvantages to this alternative. The integer conversion has to be different (truncated) so that full-canvas blits are not liable to pick up an extra pixel and error out, however this floor rounding behavior is consistent with other backends. Some user code that worked by silently clipping will now raise.

Open questions

Should we consider this alternative option as exposing latent bugs that should have been caught before, or as making blit "less robust"? Should we reconsider the current rounding/clipping rules? How strict are other backends to weird bboxes and can we make things consistent across all of them?

PR Checklist

• Has pytest style unit tests (and pytest passes).
• Is Flake 8 compliant (run flake8 on changed files to check).
• New features are documented, with examples if plot related.
• 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).
• 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).