-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Refactor for issue 28444 [Introduce _Bbox3D to represent 3D axes limits] #30115
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
56ec318
b6e8b43
8d39945
109aadc
7b8c5e1
224f9c0
8f9e5d6
8dc8584
4e132d6
676a365
a6561fa
8be447c
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 |
---|---|---|
|
@@ -608,6 +608,25 @@ | |
# Let autoscale_view figure out how to use this data. | ||
self.autoscale_view() | ||
|
||
def auto_scale_lim(self, bbox3d, had_data=False): | ||
""" | ||
Expand the 3D axes data limits to include the given Bbox3d. | ||
|
||
Parameters | ||
---------- | ||
bbox3d : _Bbox3d | ||
The 3D bounding box to incorporate into the data limits. | ||
had_data : bool, default: False | ||
Whether the axes already had data limits set before. | ||
""" | ||
self.xy_dataLim.update_from_bbox(bbox3d.to_bbox_xy(), ignore=not had_data) | ||
self.zz_dataLim.update_from_bbox(bbox3d.to_bbox_zz(), ignore=not had_data) | ||
if not had_data: | ||
self._xy_dataLim_set = True | ||
self._zz_dataLim_set = True | ||
self.autoscale_view() | ||
|
||
|
||
def autoscale_view(self, tight=None, | ||
scalex=True, scaley=True, scalez=True): | ||
""" | ||
|
@@ -2888,18 +2907,16 @@ | |
col.set_sort_zpos(zsortval) | ||
|
||
if autolim: | ||
if isinstance(col, art3d.Line3DCollection): | ||
self.auto_scale_xyz(*np.array(col._segments3d).transpose(), | ||
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. Does anything use auto_scale_xyz internally after this? Should still keep it since it's a public method, but should make sure everything is switched over 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 see in your initial comment that this is meant to lay the groundwork for switching everything over in a follow-on PR - I think that incrementalism is fine but also feel free to put everything into one if you prefer. 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. Hi Scott! I did a full review of the remaining auto_scale_xyz() usages in axes3d.py. I noticed that: 1 - Some cases are tied to 3D collections like Line3DCollection and Poly3DCollection, which now have _get_datalim3d() implemented. These can be cleanly refactored to use auto_scale_lim() instead. 2 - Other cases operate directly on raw X, Y, Z array inputs. In these, there's no associated artist or _Bbox3d abstraction, so converting to auto_scale_lim() would require constructing temporary bounding boxes manually — which feels like a larger shift in design. Before I proceed, I wanted to check: Thanks in advance for the guidance! 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. Hm, I think it makes sense to move to bounding boxes for the cases where they're currently operating on raw X, Y, Z array inputs. It will make clearer what's going on, and since the array data is not being copied should not introduce any overhead. |
||
had_data=had_data) | ||
elif isinstance(col, art3d.Poly3DCollection): | ||
self.auto_scale_xyz(col._faces[..., 0], | ||
col._faces[..., 1], | ||
col._faces[..., 2], 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 | ||
if hasattr(col, "_get_datalim3d"): | ||
bbox3d = col._get_datalim3d() | ||
self.auto_scale_lim(bbox3d, had_data=had_data) | ||
else: | ||
warnings.warn( | ||
(f"{type(col).__name__} does not implement `_get_datalim3d`," | ||
" so it will not be autoscaled."), | ||
category=UserWarning, | ||
stacklevel=2 | ||
) | ||
|
||
collection = super().add_collection(col) | ||
return collection | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
from matplotlib.transforms import Bbox | ||
|
||
|
||
class _Bbox3d: | ||
""" | ||
A helper class to represent a 3D bounding box. | ||
|
||
This class stores the minimum and maximum extents of data in 3D space | ||
(xmin, xmax, ymin, ymax, zmin, zmax). It provides methods to convert | ||
these extents into 2D bounding boxes (`Bbox`) for compatibility with | ||
existing matplotlib functionality. | ||
|
||
Attributes | ||
---------- | ||
xmin, xmax : float | ||
The minimum and maximum extents along the x-axis. | ||
ymin, ymax : float | ||
The minimum and maximum extents along the y-axis. | ||
zmin, zmax : float | ||
The minimum and maximum extents along the z-axis. | ||
|
||
Methods | ||
------- | ||
to_bbox_xy(): | ||
Converts the x and y extents into a 2D `Bbox`. | ||
to_bbox_zz(): | ||
Converts the z extents into a 2D `Bbox`, with the y-component unused. | ||
""" | ||
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.ymin), (self.xmax, self.ymax))) | ||
|
||
def to_bbox_zz(self): | ||
# first component contains z, second is unused | ||
return Bbox(((self.zmin, 0), (self.zmax, 0))) |
Uh oh!
There was an error while loading. Please reload this page.