FIX: update Axis3d artist positions before calculating tight_bbox#32048
FIX: update Axis3d artist positions before calculating tight_bbox#32048rcomer wants to merge 1 commit into
Conversation
34a01bb to
78ee4d3
Compare
78ee4d3 to
78aa433
Compare
78aa433 to
5ab58fd
Compare
| assert 1e-3 < zmin < zmax < 1e3, f"zlim corrupted: {(zmin, zmax)}" | ||
|
|
||
|
|
||
| def test_axes3d_tightbbox_includes_axis_labels(): |
There was a problem hiding this comment.
Superseded by new test above.
| def draw(self, renderer): | ||
| renderer.open_group('axis3d', gid=self.get_gid()) | ||
|
|
||
| for arts in self._update_all_positions(): |
There was a problem hiding this comment.
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()There was a problem hiding this comment.
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.
| 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
PR summary
Fixes #31277. The code from the example now gives
The
Axis3D.drawmethod has a lot of logic to place its child artists before they are drawn. Previously there was no equivalent logic forget_tightbbox, so the positions were unknown. #31548 aimed to address this by forcing a draw whenget_tightbboxwas called, but I do not think this is enough: for the case when the ticks or axis labels are on both sides of the axes, the draw logic updates the positions twice and draws the artists twice. So taking the bbox after a draw means we miss whichever position was drawn first. This PR instead factors out the positioning logic from thedrawmethod so that it can be used withinget_tightbbox.The image changes are expected: the stem plot uses constrained layout, which now correctly makes space for the tick labels. The primary views plot called
tight_layout, which made space for the tick labels before they were stripped off byremove_text. Since the baseline image had to be updated anyway, I took the opportunity to add the layout engine to the figure, following the logic of 44cda68.This does slow things down. With a simple
fig, ax = plt.subplots(subplot_kw={'projection': '3d'}), a call toplt.tight_layout()goes from 1.95 ms onmainto 4.71 ms with the branch.AI Disclosure
No AI used.
PR checklist