Skip to content

FIX: update Axis3d artist positions before calculating tight_bbox #32048

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

Open
rcomer wants to merge 1 commit into
base: main
Choose a base branch
from
+110 −85
Open

FIX: update Axis3d artist positions before calculating tight_bbox #32048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 68 additions & 62 deletions lib/mpl_toolkits/mplot3d/axis3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,8 +439,9 @@ def _axmask(self):
axmask[self._axinfo["i"]] = False
return axmask

def _draw_ticks(self, renderer, edgep1, centers, deltas, highs,
deltas_per_point, pos):
def _get_updated_ticks(self, edgep1, centers, deltas, highs,
deltas_per_point, pos):
"""Update the ticks ready for draw and return in a list."""
ticks = self._update_ticks()
info = self._axinfo
index = info["i"]
Expand All @@ -453,7 +454,7 @@ def _draw_ticks(self, renderer, edgep1, centers, deltas, highs,
axis = [self.axes.xaxis, self.axes.yaxis, self.axes.zaxis][index]
axis_trans = axis.get_transform()

# Draw ticks:
# Update ticks:
tickdir = self._get_tickdir(pos)
tickdelta = deltas[tickdir] if highs[tickdir] else -deltas[tickdir]

Expand Down Expand Up @@ -485,10 +486,12 @@ def _draw_ticks(self, renderer, edgep1, centers, deltas, highs,

_tick_update_position(tick, (x1, x2), (y1, y2), (lx, ly))
tick.tick1line.set_linewidth(tick_lw[tick._major])
tick.draw(renderer)

def _draw_offset_text(self, renderer, edgep1, edgep2, labeldeltas, centers,
highs, pep, dx, dy):
return ticks

def _update_offset_text(self, edgep1, edgep2, labeldeltas, centers,
highs, pep, dx, dy):
"""Update the offset text ready to draw."""
# Get general axis information:
info = self._axinfo
index = info["i"]
Expand Down Expand Up @@ -556,12 +559,11 @@ def _draw_offset_text(self, renderer, edgep1, edgep2, labeldeltas, centers,

self.offsetText.set_va('center')
self.offsetText.set_ha(align)
self.offsetText.draw(renderer)

def _draw_labels(self, renderer, edgep1, edgep2, labeldeltas, centers, dx, dy):
def _update_label_position(self, edgep1, edgep2, labeldeltas, centers, dx, dy):
"""Update the axis label position read to draw."""
label = self._axinfo["label"]

# Draw labels
lxyz = 0.5 * (edgep1 + edgep2)
lxyz = _move_from_center(lxyz, centers, labeldeltas, self._axmask())
tlx, tly, tlz = proj3d.proj_transform(*lxyz, self.axes.M)
Expand All @@ -572,13 +574,15 @@ def _draw_labels(self, renderer, edgep1, edgep2, labeldeltas, centers, dx, dy):
self.label.set_va(label['va'])
self.label.set_ha(label['ha'])
self.label.set_rotation_mode(label['rotation_mode'])
self.label.draw(renderer)

@artist.allow_rasterization
def draw(self, renderer):
def _update_all_positions(self):
"""
Update positions of all child artists ready to draw. Artists may be drawn
twice, e.g. if tick_position is 'both', the ticks and line show on both sides
of the axes. We therefore yield after setting the position on each side.
"""
self.label._transform = self.axes.transData
self.offsetText._transform = self.axes.transData
renderer.open_group("axis3d", gid=self.get_gid())

# Get general axis information:
mins, maxs, tc, highs = self._get_coord_info()
Expand Down Expand Up @@ -613,17 +617,17 @@ def draw(self, renderer):
dx, dy = (self.axes.transAxes.transform([pep[0:2, 1]]) -
self.axes.transAxes.transform([pep[0:2, 0]]))[0]

# Draw the lines
# Handle the lines
self.line.set_data(pep[0], pep[1])
self.line.draw(renderer)

# Draw ticks
self._draw_ticks(renderer, edgep1, centers, deltas, highs,
deltas_per_point, pos)
# Handle Offset text
self._update_offset_text(edgep1, edgep2, labeldeltas,
centers, highs, pep, dx, dy)

# Handle and yield the ticks
yield self._get_updated_ticks(edgep1, centers, deltas, highs,
deltas_per_point, pos)

# Draw Offset text
self._draw_offset_text(renderer, edgep1, edgep2, labeldeltas,
centers, highs, pep, dx, dy)

for edgep1, edgep2, pos in zip(*self._get_all_axis_line_edge_points(
minmax, maxmin, self._label_position)):
Expand All @@ -633,8 +637,25 @@ def draw(self, renderer):
dx, dy = (self.axes.transAxes.transform([pep[0:2, 1]]) -
self.axes.transAxes.transform([pep[0:2, 0]]))[0]

# Draw labels
self._draw_labels(renderer, edgep1, edgep2, labeldeltas, centers, dx, dy)
# Handle labels
self._update_label_position(edgep1, edgep2, labeldeltas, centers, dx, dy)
yield self.label

@artist.allow_rasterization
def draw(self, renderer):
renderer.open_group('axis3d', gid=self.get_gid())

for arts in self._update_all_positions():

@timhoffm timhoffm Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it would be better to move the complexity into _update_all_positions() and return a list of artists or yield individual artists only so that here we can do:

for artist in self._update_all_positions():
    artist.draw()

@rcomer rcomer Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yield individual artists only so that here we can do:

for artist in self._update_all_positions():
    artist.draw()

I actually did that initially, but then thought it was better to have the ticks in a list so we can still conveniently use _get_ticklabel_bboxes within get_tightbbox.

if isinstance(arts, list):
# We have a list of ticks, and the line and offset text are also ready
# to be drawn
self.line.draw(renderer)
for tick in arts:
tick.draw(renderer)
self.offsetText.draw(renderer)
else:
# We have the axis label
arts.draw(renderer)

renderer.close_group('axis3d')
self.stale = False
Expand Down Expand Up @@ -689,49 +710,34 @@ def get_tightbbox(self, renderer=None, *, for_layout_only=False):
# docstring inherited
if not self.get_visible():
return
# We have to directly access the internal data structures
# (and hope they are up to date) because at draw time we
# shift the ticks and their labels around in (x, y) space
# based on the projection, the current view port, and their
# position in 3D space. If we extend the transforms framework
# into 3D we would not need to do this different book keeping
# than we do in the normal axis
major_locs = self.get_majorticklocs()
minor_locs = self.get_minorticklocs()

ticks = [*self.get_minor_ticks(len(minor_locs)),
*self.get_major_ticks(len(major_locs))]
view_low, view_high = self.get_view_interval()
if view_low > view_high:
view_low, view_high = view_high, view_low
interval_t = self.get_transform().transform([view_low, view_high])

ticks_to_draw = []
for tick in ticks:
try:
loc_t = self.get_transform().transform(tick.get_loc())
except AssertionError:
# Transform.transform doesn't allow masked values but
# some scales might make them, so we need this try/except.
pass
else:
if mtransforms._interval_contains_close(interval_t, loc_t):
ticks_to_draw.append(tick)

ticks = ticks_to_draw
if self.axes.M is None:
# add the projection matrix to the renderer
self.axes.M = self.axes.get_proj()
self.axes.invM = np.linalg.inv(self.axes.M)

bboxes = []

bb_1, bb_2 = self._get_ticklabel_bboxes(ticks, renderer)
other = []
for arts in self._update_all_positions():
if isinstance(arts, list):
# We have a list of ticks
for tlabel_bbs in self._get_ticklabel_bboxes(arts, renderer):
bboxes.extend(tlabel_bbs)

if self.offsetText.get_visible() and self.offsetText.get_text():
other.append(self.offsetText.get_window_extent(renderer))
if self.line.get_visible():
other.append(self.line.get_window_extent(renderer))
if (self.label.get_visible() and not for_layout_only and
self.label.get_text()):
other.append(self.label.get_window_extent(renderer))
# Line and offset text were also updated
if self.offsetText.get_visible() and self.offsetText.get_text():
bboxes.append(self.offsetText.get_window_extent(renderer))
if self.line.get_visible():
bboxes.append(self.line.get_window_extent(renderer))

return mtransforms.Bbox.union([*bb_1, *bb_2, *other])
elif arts.get_visible() and arts.get_text() and not for_layout_only:
# Account for the axis label
bboxes.append(arts.get_window_extent(renderer))
Comment on lines +721 to +735

@timhoffm timhoffm Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not sure if my plain "return list of artists" would work here as well.

Alternatively, return a structured object, e.g. a data class with line, ticks, offsetText, label.

@rcomer rcomer Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The axis label is updated independently of the rest, because it depends on self.get_label_position() whereas the others are all handled together based on self.get_ticks_position(). Each of those returned positions could be "both", in which case the relevant update and yield happens twice within the _update_all_positions call. So I'm not sure we can return an object with all four items on it. We could yield an object with line, ticks, offsetText for the first part?


if bboxes:
return mtransforms.Bbox.union(bboxes)
else:
return None

d_interval = _api.deprecated(
"3.6", alternative="get_data_interval", pending=True)(
Expand Down
Binary file modified ...mpl_toolkits/mplot3d/tests/baseline_images/test_axes3d/axes3d_primary_views.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified lib/mpl_toolkits/mplot3d/tests/baseline_images/test_axes3d/stem3d.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
65 changes: 42 additions & 23 deletions lib/mpl_toolkits/mplot3d/tests/test_axes3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,14 +152,13 @@ def test_axes3d_primary_views():
(0, 180, 0)] # -YZ
# When viewing primary planes, draw the two visible axes so they intersect
# at their low values
fig, axs = plt.subplots(2, 3, subplot_kw={'projection': '3d'})
fig, axs = plt.subplots(2, 3, subplot_kw={'projection': '3d'}, layout='tight')
for i, ax in enumerate(axs.flat):
ax.set_xlabel('x')
ax.set_ylabel('y')
ax.set_zlabel('z')
ax.set_proj_type('ortho')
ax.view_init(elev=views[i][0], azim=views[i][1], roll=views[i][2])
plt.tight_layout()


@mpl3d_image_comparison(['bar3d.png'], style='mpl20')
Expand Down Expand Up @@ -2817,6 +2816,47 @@ def test_axis_get_tightbbox_includes_offset_text():
f"bbox.y1 ({bbox.y1}) should be >= offset_bbox.y1 ({offset_bbox.y1})"


@pytest.mark.parametrize('labeltype', ['ticks', 'axis label'])
def test_axes3d_tightbbox_includes_labels(labeltype):
fig = plt.figure()
ax = fig.add_subplot(projection='3d')

renderer = fig._get_renderer()

tight_bbs = {}

if labeltype == 'axis label':
ax.set_zlabel('foo')

# Remove ticks so they don't affect the result
ax.zaxis.set_ticks_position('none')

for pos in ['lower', 'upper', 'both', 'none']:
if labeltype == 'ticks':
ax.zaxis.set_ticks_position(pos)
else:
ax.zaxis.set_label_position(pos)
tight_bbs[pos] = ax.get_tightbbox(renderer)

for pos in ['lower', 'both']:
# Should make space for labels on the left
assert tight_bbs[pos].xmin < tight_bbs['none'].xmin, \
f'No space for labels on left with {labeltype} position "{pos}"'

for pos in ['upper', 'both']:
# Should make space for labels on the right
assert tight_bbs['both'].xmax > tight_bbs['none'].xmax, \
f'No space for labels on right with {labeltype} position "{pos}"'

# No space on the right for 'lower'
assert tight_bbs['lower'].xmax == tight_bbs['none'].xmax, \
f'Unexpected space for labels on right with {labeltype} position "lower"'

# No space on the left for 'upper'
assert tight_bbs['upper'].xmin == tight_bbs['none'].xmin, \
f'Unexpected space for labels on left with {labeltype} position "{pos}"'


def test_ctrl_rotation_snaps_to_5deg():
fig = plt.figure()
ax = fig.add_subplot(projection='3d')
Expand Down Expand Up @@ -3227,24 +3267,3 @@ def test_plot_surface_log_scale_invalid_values():

zmin, zmax = ax.get_zlim()
assert 1e-3 < zmin < zmax < 1e3, f"zlim corrupted: {(zmin, zmax)}"


def test_axes3d_tightbbox_includes_axis_labels():

@rcomer rcomer Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by new test above.

fig = plt.figure()
ax = fig.add_subplot(projection='3d')
ax.scatter([1], [1], [1])

fig.draw_without_rendering()
renderer = fig._get_renderer()
bb_no_labels = ax.get_tightbbox(renderer)

ax.set_xlabel('X label')
ax.set_ylabel('Y label')
ax.set_zlabel('Z label')

fig.draw_without_rendering()
bb_full = ax.get_tightbbox(renderer)

# The full bbox must be strictly larger in at least one dimension than the
# bbox not including labels.
assert bb_full.width > bb_no_labels.width or bb_full.height > bb_no_labels.height
Loading