More flexible input for PolygonVisual #1481
Conversation
… optionally use PolygonData instead of position array
|
Open question: should the PolygonData.faces property be read-only? I can't think of a situation where we want the user to be able to set it; the whole purpose of PolygonData seems to be triangulation. |
|
..and while I'm digging, the |
vispy/geometry/polygon.py
Outdated
| edges = np.empty((npts-1, 2), dtype=np.uint32) | ||
| edges[:, 0] = np.arange(npts) | ||
| edges[:, 1] = edges[:, 0] + 1 | ||
| self._edges = edges |
There was a problem hiding this comment.
Would it be better to move this chunk of code to the setter and then in init use self.edges = edges (instead of the private)?
There was a problem hiding this comment.
I usually prefer to avoid any potentially expensive operations until I am sure they are needed. For example, the user could create the PolygonData with just vertices, and then set the edges manually later on.
There was a problem hiding this comment.
I might be missing something, but self._edges is never read directly, it is always self.edges. Additionally self._edges is never written to directly except for init. This block of code is just a validation step of the edges being applied to the PolygonData object so shouldn't that validation happen at set (one time) rather than at read (possibly many times).
There was a problem hiding this comment.
Have a look at the latest version; the code makes a bit more sense now. self.edges can be either specified manually (at init or by setter) or automatically generated.
There was a problem hiding this comment.
Ok it makes more sense now because I realize that they are just autogenerated from the vertices. However, when you set vertices don't you need to set self._edges to None so they are regenerated?
|
...except for the failed tests that is. |
|
@campagnola Any chance you can fix the CI failures? |
|
@campagnola You around? Think you could merge master in to this and see what the state of the tests are? |
edgesproperty (previously it was ignored).