-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
Implement Figure-level overlay architecture with two-pass drawing #32199
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -209,6 +209,8 @@ def __init__(self, **kwargs): | |||||
| self._localaxes = [] # track all Axes | ||||||
| self.subfigs = [] | ||||||
| self._children = [] # All artists except SubFigure and Axes | ||||||
| self._overlay_children = [] # Artists drawn in overlay pass | ||||||
| # (transparent, no patch) | ||||||
| self.stale = True | ||||||
| self.suppressComposite = None | ||||||
| self.set(**kwargs) | ||||||
|
|
@@ -257,6 +259,43 @@ def _get_draw_artists(self, renderer): | |||||
| locator(child, renderer) if locator else None) | ||||||
| return artists | ||||||
|
|
||||||
| 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) | ||||||
|
|
||||||
|
Comment on lines
+262
to
+298
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once Additionally, I would suggest adding a |
||||||
| def autofmt_xdate( | ||||||
| self, bottom=0.2, rotation=30, ha='right', which='major'): | ||||||
| """ | ||||||
|
|
@@ -585,7 +624,7 @@ def set_frameon(self, b): | |||||
|
|
||||||
| frameon = property(get_frameon, set_frameon) | ||||||
|
|
||||||
| def add_artist(self, artist, clip=False): | ||||||
| def add_artist(self, artist, clip=False, *, _overlay=False): | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We can move towards making |
||||||
| """ | ||||||
| Add an `.Artist` to the figure. | ||||||
|
|
||||||
|
|
@@ -608,8 +647,9 @@ def add_artist(self, artist, clip=False): | |||||
| The added artist. | ||||||
| """ | ||||||
| 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 | ||||||
|
|
||||||
|
Comment on lines
649
to
653
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once you have a dictionary for the |
||||||
| if not artist.is_transform_set(): | ||||||
| artist.set_transform(self.transSubfigure) | ||||||
|
|
@@ -1076,6 +1116,7 @@ def clear(self, keep_observers=False): | |||||
| self.delaxes(ax) # Remove ax from self._axstack. | ||||||
|
|
||||||
| self._children = [] | ||||||
| self._overlay_children = [] | ||||||
| self.subplotpars.reset() | ||||||
| if not keep_observers: | ||||||
| self._axobservers = cbook.CallbackRegistry() | ||||||
|
|
@@ -2494,13 +2535,13 @@ def draw(self, renderer): | |||||
| if not self.get_visible(): | ||||||
| return | ||||||
|
|
||||||
| artists = self._get_draw_artists(renderer) | ||||||
|
|
||||||
| try: | ||||||
| renderer.open_group('subfigure', gid=self.get_gid()) | ||||||
| # Pass 1: base layer (patch + all non-overlay children) | ||||||
| self.patch.draw(renderer) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that once the above things are implemented, then 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 |
||||||
| mimage._draw_list_compositing_images( | ||||||
| renderer, self, artists, self.get_figure(root=True).suppressComposite) | ||||||
| self._draw_base_layer(renderer) | ||||||
| # Pass 2: overlay layer (transparent, no patch) | ||||||
| self._draw_overlay_layer(renderer) | ||||||
| renderer.close_group('subfigure') | ||||||
|
|
||||||
| finally: | ||||||
|
|
@@ -3348,7 +3389,6 @@ def draw(self, renderer): | |||||
|
|
||||||
| with self._render_lock: | ||||||
|
|
||||||
| artists = self._get_draw_artists(renderer) | ||||||
| try: | ||||||
| renderer.open_group('figure', gid=self.get_gid()) | ||||||
| if self.axes and self.get_layout_engine() is not None: | ||||||
|
|
@@ -3358,9 +3398,12 @@ def draw(self, renderer): | |||||
| pass | ||||||
| # ValueError can occur when resizing a window. | ||||||
|
|
||||||
| # Pass 1: base layer — figure patch + all non-overlay artists | ||||||
| self.patch.draw(renderer) | ||||||
| mimage._draw_list_compositing_images( | ||||||
| renderer, self, artists, self.suppressComposite) | ||||||
| self._draw_base_layer(renderer) | ||||||
|
|
||||||
| # Pass 2: overlay layer — transparent, no patch | ||||||
| self._draw_overlay_layer(renderer) | ||||||
|
|
||||||
| renderer.close_group('figure') | ||||||
| finally: | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import platform | ||
| import sys | ||
| from threading import Timer | ||
| import unittest.mock as mock | ||
| from types import SimpleNamespace | ||
| import warnings | ||
|
|
||
|
|
@@ -1966,3 +1967,78 @@ def test_artist_sublist_deprecations(): | |
| del fig.lines[-1] | ||
| with pytest.warns(mpl.MatplotlibDeprecationWarning, match=match): | ||
| del fig.lines[1:] | ||
|
|
||
|
|
||
| @image_comparison( | ||
| baseline_images=['two_pass_base_only'], extensions=['png'], style='mpl20' | ||
| ) | ||
| def test_two_pass_base_only(): | ||
| """Verify that bypassing the overlay pass leaves only the base layer.""" | ||
| fig, ax = plt.subplots() | ||
| ax.plot([0, 1], [0, 1], color='blue', lw=5) | ||
|
|
||
| # Add overlay elements | ||
| from matplotlib.text import Text | ||
| overlay_text = Text( | ||
| 0.5, 0.5, "Overlay Text", color='red', fontsize=20, ha='center', | ||
| transform=fig.transFigure, figure=fig | ||
| ) | ||
| fig.add_artist(overlay_text, _overlay=True) | ||
| import matplotlib.lines as mlines | ||
| overlay_line = mlines.Line2D( | ||
| [0, 1], [1, 0], color='red', lw=5, transform=fig.transFigure | ||
| ) | ||
| fig.add_artist(overlay_line, _overlay=True) | ||
|
|
||
| # Mock _draw_overlay_layer to be a no-op so ONLY the base layer is drawn | ||
| fig._draw_overlay_layer = lambda renderer: None | ||
|
|
||
|
|
||
| @image_comparison( | ||
| baseline_images=['two_pass_overlay_only'], extensions=['png'], style='mpl20' | ||
| ) | ||
| def test_two_pass_overlay_only(): | ||
| """ | ||
| Verify that bypassing the base pass leaves only the overlay layer (transparent). | ||
| """ | ||
| fig, ax = plt.subplots() | ||
| ax.plot([0, 1], [0, 1], color='blue', lw=5) | ||
|
|
||
| # Add overlay elements | ||
| from matplotlib.text import Text | ||
| overlay_text = Text( | ||
| 0.5, 0.5, "Overlay Text", color='red', fontsize=20, ha='center', | ||
| transform=fig.transFigure, figure=fig | ||
| ) | ||
| fig.add_artist(overlay_text, _overlay=True) | ||
| import matplotlib.lines as mlines | ||
| overlay_line = mlines.Line2D( | ||
| [0, 1], [1, 0], color='red', lw=5, transform=fig.transFigure | ||
| ) | ||
| fig.add_artist(overlay_line, _overlay=True) | ||
|
|
||
| # Mock _draw_base_layer to be a no-op so ONLY the overlay layer is drawn. | ||
| # Note: Figure patch is drawn *before* _draw_base_layer, so we must make | ||
| # it transparent manually to see the pure overlay output. | ||
| fig._draw_base_layer = lambda renderer: None | ||
| fig.patch.set_alpha(0.0) | ||
| ax.patch.set_alpha(0.0) | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you need three tests here:
b/c if you have to knock out the patch on the overlay, that seems to indicate you're not getting clean independence. |
||
|
|
||
| def test_two_pass_draw_calls_each_layer_once(): | ||
| """Figure.draw() calls _draw_base_layer and _draw_overlay_layer exactly once.""" | ||
| fig = plt.figure() | ||
| fig.add_subplot().plot([1, 2]) | ||
| from matplotlib.text import Text | ||
| overlay_text = Text(0.5, 0.5, "test", transform=fig.transFigure, figure=fig) | ||
| fig.add_artist(overlay_text, _overlay=True) | ||
|
|
||
| with mock.patch.object( | ||
| fig, '_draw_base_layer', wraps=fig._draw_base_layer | ||
| ) as base_spy, mock.patch.object( | ||
| fig, '_draw_overlay_layer', wraps=fig._draw_overlay_layer | ||
| ) as overlay_spy: | ||
| fig.draw(fig._get_renderer()) | ||
|
|
||
| assert base_spy.call_count == 1 | ||
| assert overlay_spy.call_count == 1 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider tracking children as a dictionary mapping Layer name to lists of child artists
I would be careful about changing
self._childrendirectly, as that is used in a number of placesSo something like
self._children_by_layer = {"base": self._children, "overlay = []}