Implement Figure-level overlay architecture with two-pass drawing - #32199
Implement Figure-level overlay architecture with two-pass drawing#32199Vikash-Kumar-23 wants to merge 1 commit into
Conversation
5da353c to
3dcb434
Compare
| fig._draw_base_layer = lambda renderer: None | ||
| fig.patch.set_alpha(0.0) | ||
| ax.patch.set_alpha(0.0) | ||
|
|
There was a problem hiding this comment.
I think you need three tests here:
- base
- overlay
- composite
b/c if you have to knock out the patch on the overlay, that seems to indicate you're not getting clean independence.
ksunden
left a comment
ksunden
left a comment
There was a problem hiding this comment.
The main idea for this review is to push towards making the layering system more generic. Instead of just one overlay, it is possible to extend into more, which has the added benefit of enabling us to clean up the code and reduce duplicated code.
I've laid out a series of specific changes that I think will add up to making this more useful and cleaner, outlined below.
| for ax in self._localaxes: | ||
| locator = ax.get_axes_locator() | ||
| ax.apply_aspect(locator(ax, renderer) if locator else None) | ||
|
|
||
| for child in ax.get_children(): | ||
| if hasattr(child, 'apply_aspect'): | ||
| locator = child.get_axes_locator() | ||
| child.apply_aspect( | ||
| locator(child, renderer) if locator else None) |
There was a problem hiding this comment.
These lines can be extracted into their own helper function that is called just the once in draw
| def _get_draw_artists(self, renderer): | ||
| """Also runs apply_aspect""" | ||
| artists = self.get_children() |
There was a problem hiding this comment.
| def _get_draw_artists(self, renderer, layer): | |
| """Also runs apply_aspect""" | |
| artists = self.get_children(layer=layer) |
Once the apply_aspect portions of this method are extracted, the rest of this method can be made pretty generic by adding layer as a parameter and adding per-layer functionality to self.get_children
| def _draw_base_layer(self, renderer): | ||
| """ | ||
| Draw the base layer: all non-overlay children, sorted by zorder. | ||
|
|
||
| This is the first of the two passes in `.Figure.draw`. It draws | ||
| every artist that was added through the normal insertion path | ||
| (i.e. not via ``_overlay=True``). | ||
|
|
||
| Parameters | ||
| ---------- | ||
| renderer : `.RendererBase` | ||
| """ | ||
| artists = self._get_draw_artists(renderer) | ||
| mimage._draw_list_compositing_images( | ||
| renderer, self, artists, self.suppressComposite) | ||
|
|
||
| def _draw_overlay_layer(self, renderer): | ||
| """ | ||
| Draw the overlay layer: artists added with ``_overlay=True``. | ||
|
|
||
| The overlay is transparent — no figure or axes patch is drawn before | ||
| these artists. By default ``_overlay_children`` is empty, making this | ||
| a no-op that preserves backward-compatible behaviour. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| renderer : `.RendererBase` | ||
| """ | ||
| artists = [ | ||
| a for a in self._overlay_children if not a.get_animated() | ||
| ] | ||
| if not artists: | ||
| return | ||
| artists.sort(key=lambda a: a.get_zorder()) | ||
| mimage._draw_list_compositing_images( | ||
| renderer, self, artists, self.suppressComposite) | ||
|
|
There was a problem hiding this comment.
Once _get_draw_artists accepts a layer argument and the apply_aspect portions are extracted, these two methods can be made generic by accepting a layer argument that gets passed on to _get_draw_artists
Additionally, I would suggest adding a render.open_group(layer) (this doesn't actually do much outside of SVGs, but will give a collapsible/able to be hidden section per layer, which helps differentiate them)
| frameon = property(get_frameon, set_frameon) | ||
|
|
||
| def add_artist(self, artist, clip=False): | ||
| def add_artist(self, artist, clip=False, *, _overlay=False): |
There was a problem hiding this comment.
| def add_artist(self, artist, clip=False, *, _overlay=False): | |
| def add_artist(self, artist, clip=False, *, layer=None): |
We can move towards making layer a generic thing instead of a boolean "is overlay"/"not overlay"
| self._children = [] # All artists except SubFigure and Axes | ||
| self._overlay_children = [] # Artists drawn in overlay pass | ||
| # (transparent, no patch) |
There was a problem hiding this comment.
Consider tracking children as a dictionary mapping Layer name to lists of child artists
I would be careful about changing self._children directly, as that is used in a number of places
So something like self._children_by_layer = {"base": self._children, "overlay = []}
| artist.set_figure(self) | ||
| self._children.append(artist) | ||
| artist._remove_method = self._children.remove | ||
| target = self._overlay_children if _overlay else self._children | ||
| target.append(artist) | ||
| artist._remove_method = target.remove | ||
|
|
There was a problem hiding this comment.
Once you have a dictionary for the _children_by_layer this code should become slightly simpler (as well as other segments that mirror this code elsewhere)
| try: | ||
| renderer.open_group('subfigure', gid=self.get_gid()) | ||
| # Pass 1: base layer (patch + all non-overlay children) | ||
| self.patch.draw(renderer) |
There was a problem hiding this comment.
I think that once the above things are implemented, then patch can be its own layer that needs less special casing
It becomes just a layer that has a single artist (self.patch) which does not appear in any other layer, and is drawn first, resulting in less need to specifically reject self.patch elsewhere.
| def get_children(self): | ||
| """Get a list of artists contained in the figure.""" | ||
| return [self.patch, | ||
| *self.artists, | ||
| *self._localaxes, | ||
| *self.lines, | ||
| *self.patches, | ||
| *self.texts, | ||
| *self.images, | ||
| *self.legends, | ||
| *self.subfigs] |
There was a problem hiding this comment.
get_children can grow a parameter to get specifically the children from a specific layer
By default it should return all of the children from all of the layers
In its current form, most of the lists that are expanded are already filtered from self._children. As such, it will only pick up things from the base layer.
The way this returns will affect the order of the artists listed, and that may be useful to preserve.
3 participants
PR summary
This PR introduces a foundational Figure-level overlay architecture to
figure.py. It implements a two-pass drawing system, allowing developers to cleanly segregate base plot artists from overlay artists.Key Changes:
_overlay_childrentoFigureBase. Figure elements are now routed to either_children(base layer) or_overlay_children(overlay layer).Figure.draw()to execute in two distinct passes:_draw_base_layer(): Renders the figure patch (background) and all artists in_children._draw_overlay_layer(): Renders all artists in_overlay_children._overlay=Falsekeyword-only argument to key artist insertion methods (add_artist,text,legend,figimage)._overlaykeyword through thesuptitle,supxlabel, andsupylabelfamily (via_suplabels) down to the underlyingtextcalls.Addresses #30515
AI Disclosure
AI tools were used to assist in drafting text and suggesting validation scenarios.
All code changes, final implementation decisions, and verification were done manually.
PR quality check