move class-specific draw methods out of View3d and into Vxyz and Pxyz #154
Conversation
bbean23
commented
Aug 10, 2024
•
bbean23
commented
- move View3d.draw_Vxyz() to Vxyz.draw_list()
- move View3d.draw_single_Pxyz() to Pxyz.draw_points()
There was a problem hiding this comment.
@bbean23, thanks for the changes. I have approved, but I have a few questions. I'll leave it up to you if you want to change anything after this. I would be happy to review again if needed.
opencsp/common/lib/csp/SolarField.py
Outdated
| view.draw_single_Pxyz(origin) | ||
| Pxyz(origin).draw_point(view) |
There was a problem hiding this comment.
Why is Pxyz(...) necessary here? It looks like origin is already type Pxyz. There are also several other cases of this I've seen but not pointed out each one.
If origin is in facet already a Pxyz, this is not necessarily wrong, just redundant and possibly confusing. You're call.
There was a problem hiding this comment.
This was because origin was a Vyxz and needed to be a Pxyz and I wanted to draw it with the draw_point() method. Now that the method is in Vyxz class I don't need this conversion anymore.
opencsp/common/lib/geometry/Vxyz.py
Outdated
| def draw_list( | ||
| self, | ||
| figure: rcfr.RenderControlFigureRecord | v3d.View3d, | ||
| close: bool = None, | ||
| style: rcps.RenderControlPointSeq = None, | ||
| label: str = None, | ||
| ) -> None: | ||
| """Calls figure.draw_xyz_list(self.data.T) with the default arguments in place for any None's.""" | ||
| kwargs = dict() | ||
| for key, val in [('close', close), ('style', style), ('label', label)]: | ||
| if val is not None: | ||
| kwargs[key] = val | ||
|
|
||
| view = figure if isinstance(figure, v3d.View3d) else figure.view | ||
| view.draw_xyz_list(self.data.T, **kwargs) |
There was a problem hiding this comment.
Why is this called "draw_list" when there are no lists involved?
There was a problem hiding this comment.
I'm basing it off of the method draw_xyz_list() in View3d.py. But you're right. I changed the name for this class (but not in View3d).
opencsp/common/lib/geometry/Pxyz.py
Outdated
| def draw_point( | ||
| self, | ||
| figure: rcfr.RenderControlFigureRecord | v3d.View3d, | ||
| style: rcps.RenderControlPointSeq = None, | ||
| labels: list[str] = None, | ||
| ): | ||
| """Calls figure.draw_xyz(p) for all points in this instance, and with | ||
| the default arguments in place for any None's.""" | ||
| if style is None: | ||
| style = rcps.default(markersize=2) | ||
| if labels is None: | ||
| labels = [None] * len(self) | ||
| view = figure if isinstance(figure, v3d.View3d) else figure.view | ||
| for x, y, z, label in zip(self.x, self.y, self.z, labels): | ||
| view.draw_xyz((x, y, z), style, label) |
There was a problem hiding this comment.
Would this make sense to add to Vxyz instead since Pxyz inherits Vxyz?
Furthermore, what is the difference between draw_point and draw_list? Could they be combined into one function altogether?
There was a problem hiding this comment.
The difference is that draw_points draws each point individually, vs draw_list which draws all points together as a single series. Updated the documentation to match and moved the method into Vxyz.
…yz.draw_list() -> Vxyz.draw_line()
bbean23
force-pushed
the
153-move-drawing-options-out-of-view3d-and-into-vxyz-and-pxzy-classes
branch
from
345bc3e to
0a45ef0
Compare
|
@braden6521 I think I addressed all your comments. Please re-approve so that I can merge. |
…d-into-vxyz-and-pxzy-classes
There was a problem hiding this comment.
@bbean23: Thank you for these changes. Please add documentation for parameters where noted. I'll work on getting these included in the sphinx docs after this is merged. If you still cannot merge after making the changes I requested, please let me know.
