Skip to content

[MNT]: Replace BufferRegion/restore_region by (x, y, array) tuples and draw_image? #23882

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

Jump to bottom
Open
anntzer opened this issue Sep 13, 2022 · 3 comments
Open

[MNT]: Replace BufferRegion/restore_region by (x, y, array) tuples and draw_image? #23882

anntzer opened this issue Sep 13, 2022 · 3 comments
Labels
API: changes Maintenance

Comments

@anntzer
Copy link
Contributor

anntzer commented Sep 13, 2022 •
edited
Loading

Summary

Based on the discussion at https://discourse.matplotlib.org/t/method-to-add-transparency-to-a-saved-bufferregion/23121 (thanks @raphaelquast), I realized it may be possible to get rid of the internal BufferRegion type and restore_region, in favor of copy_from_bbox returning a tpl = (x, y, rgba_array) tuple and using canvas.get_renderer().draw_image(gc, x, y, rgba_array) to restore the region. draw_image could specifically add support for passing gc = None (i.e. create a throwaway gc just for this call), and then the restore call would be canvas.get_renderer().draw_image(None, *tpl) which seems just fine. Or we can just keep restore_region as a canvas method (on FigureCanvasBase, without need of being overridden by backends) which does the right draw_image call on the renderer, only getting rid of Renderer.restore_region.

Currently, restore_region just blindly copies the BufferRegion contents, without doing any alpha compositing; the new approach would instead alpha-composite against whatever was here before. However, most uses of copy_from_bbox/restore_region suppose opaque regions anyways, and callers can also modify the rgba_array's alpha channel as desired before calling draw_image. Hopefully the extra alpha compositing wouldn't be too much of a slowdown, or we could always extend draw_image to support rgb (not rgba) inputs and detect that in this case it can go through a fast path without alpha compositing.

The advantage of such a change would be to simplify the backend API (e.g. for third party backends), including removing the undocumented BufferRegion type. (Probably the correct transition path would be to first make the BufferRegion type fully opaque, by deprecating all its public methods.)

attn @tacaswell as I believe you have written quite a bit of docs about blitting.

Proposed fix

No response
@tacaswell
Copy link
Member

Interesting. Have only put a little thought into this, but my initial reactions

  • very wary of changing / deprecating anything in the blitting pipeline. My sense is that this is code that is written once and then not touched or well understood. I think even breaking changes here could cause large amounts of user pain (particularly users who have inherited code that uses blitting)
  • providing a way to get the tuple out of the BufferRegion as a one-liner would be good
  • documenting how to do this use draw_image to do this sort of thing would be good as it is conceptually more consistent with everything else

@anntzer anntzer mentioned this issue Oct 19, 2022
Merged
6 tasks
This was referenced Feb 26, 2023
Merged
Closed
@anntzer anntzer mentioned this issue Mar 13, 2024
Open
@pelson
Copy link
Member

pelson commented Mar 14, 2024

I agree that copy_from_bbox should return a well defined object (for a raster renderer at least), and that it shouldn't be an Agg-internal, inaccessible type. It is surprising that we don't have a decent Image representation (from my memory) which would be a good fit for the return type - a tuple of (x0, y0, img) is a poor-man's image type here 😉.

As for removing restore_region - I'm not convinced that we should bother, the base renderer class can implement the draw_image pass through, and other renderers can specialise (I doubt they would though). I think the removal of bbox and xy from the restore_region method would be reasonable (I see from the linked discussion that this would break at least one workflow) - there is no use of those arguments in the mpl codebase. IMO the use case served by those kwargs would be better placed in a non-poor-man's representation of an image + domain, where subsetting by bbox would be non renderer specific.

@pelson
Copy link
Member

pelson commented Mar 14, 2024

IMO the use case served by those kwargs would be better placed in a non-poor-man's representation of an image + domain, where subsetting by bbox would be non renderer specific.

I'm reminded that I wish the Image artist classes didn't do the subsetting/clipping for the renderer, and instead just passed the complete image (plus some way to know that the image itself hasn't changed). This would allow us to optimise the clipping using whatever tools are at our disposal in the renderer. There is no reason that the renderer couldn't call back to the image to do a software based clipping if the renderer doesn't want to implement anything itself (a reasonable default).

I appreciate that this is off-topic for the issue, but just wanted to share this thought somewhere 💡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
API: changes Maintenance
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tacaswell @pelson @anntzer