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 3 commits 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
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
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.
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.
3dcb434 to
e31aff1
Compare
e31aff1 to
d78afb3
Compare
|
@ksunden @story645 Thanks for the feedback! I've updated the architecture based on your suggestion to make Here are the updates pushed in the latest commit:
|
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:
_children_by_layerdictionary inFigureBase. Artists are now routed to distinct lists based on their layer (e.g.,"base","overlay"), andself.patchhas been isolated into its own dedicated"patch"layer.Figure.draw()andSubFigure.draw()to execute in multiple passes using a new, generic_draw_layer(renderer, layer_name)method. The strict sequence is now:"patch"layer: Renders the figure background first."base"layer: Renders all standard artists."overlay"layer: Renders all overlay artists last.layer=Nonekeyword-only argumentadd_artistandget_childrenAddresses #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