Repository navigation
FIX: Autoscale support in add_collection3d for Line3DCollection and Poly3DCollection - #28403
Conversation
69ea30c to
195a208
Compare
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
49ee773 to
0c7b7db
Compare
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
| if isinstance(col, art3d.Line3DCollection): | ||
| self.auto_scale_xyz(*np.array(col._segments3d).transpose(), | ||
| had_data=had_data) | ||
| elif isinstance(col, art3d.Poly3DCollection): | ||
| self.auto_scale_xyz(*col._vec[:-1], had_data=had_data) | ||
| elif isinstance(col, art3d.Patch3DCollection): | ||
| pass | ||
| # FIXME: Implement auto-scaling function for Patch3DCollection | ||
| # Currently unable to do so due to issues with Patch3DCollection | ||
| # See https://github.com/matplotlib/matplotlib/issues/14298 for details |
There was a problem hiding this comment.
While technically correct, I believe we should move the calculation of data lims down into the artists.
Current logic
Extract all data coordinates xyz from the artist and feed them into
auto_scale_xyz().
This updates the xy_dataLim, zz_dataLim BBoxes via .Bbox.update_from_data_xy/x/y(), which in turn creates a Path and calls .Bbox.update_from_path().
This is quite a lot data pushing (and maybe copying) just to update dataLim bboxes.
Proposed logic
Separation of concerns: The Artists should have a function to get their data lims. These limits should be used to update the Bbox. Basically some Bbox.update_from_box, a kind of an in-place Bbox.union, which we don't seem to have.
This whole block should read something like
if autolim:
self.auto_scale_lim(col.get_data_lims())
Note: "data lim" is intentionally vague as we currently don't have a good structure to describe it (we use two 2D Bboxes, xy_dataLim and zz_dataLim.
The minimal approach - if we don't want to redesign for proper 3d boxes, would be to let get_data_lims() return two BBoxes as well. In that case, I would keep this interface private _get_data_lims() so that we can still change to a single 3D instance later.
Edit: This is basically a generalization of Collection.get_datalim.
matplotlib/lib/matplotlib/collections.py
Lines 239 to 306 in fa16860
Rough outline: We may start minimal with rolling a small
class _Bbox3d:
def __init__(self, points):
((self.xmin, self.xmax),
(self.ymin, self.ymax),
(self.zmin, self.zmax)) = points
def to_bbox_xy(self):
return Bbox(((self.xmin, self.xmax), (self.ymin, self.ymax)))
def to_bbox_zz(self):
# first component contains z, second is unused
return Bbox(((self.zmin, self.zmax), (0, 0)))
and implement _get_datalim3d() -> _Bbox3d on the 3d collections.
Then
def add_collection(col, autolim=True):
...
if autolim:
self.auto_scale_lim(col.get_datalim3d())
and
def auto_scale_lim(bbox3d):
...
self.dataLim_xy.update_from_box(bbox3d.to_bbox_xy())
self.dataLim_zz.update_from_box(bbox3d.to_bbox_zz())
There was a problem hiding this comment.
I like this structure, but could we make that refactor its own issue?
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
…n3d for Line3DCollection and Poly3DCollection
…03-on-v3.9.x Backport PR #28403 on branch v3.9.x (FIX: Autoscale support in add_collection3d for Line3DCollection and Poly3DCollection
PR summary
Closes #23317
Closes #17130
Code copied in large part from @lganic's work in #25751 (thank you!), which he closed.
I would consider this a bugfix for a missing kwarg, but if we want to consider this an enhancement for 3.10.0 please feel free to shift the milestone.
PR checklist