Update TubeVisual points instead of vertices/faces #2442
Conversation
Override MeshVisual .set_data() method so that a user can update the Tube's points as opposed to the Tube's vertices and faces. Also updated the __init__ method to call methods that compute the faces and indices to allow for code re-use when calling .set_data()
|
Sorry for committing twice in this pull request. I didn't realize I had a print statement in my first commit that I forgot to clean up during testing. The second commit is the actual commit that needs to be reviewed. Thanks! |
There was a problem hiding this comment.
Thanks for making this PR. This could be helpful if I'm understanding the purpose correctly. I don't have a lot of time this week or next (holidays and then sickness), so a proper review will have to come later. Maybe one of the other vispy maintainers can find time to take a look.
PEP-8 Compliance
There was a problem hiding this comment.
Really nice. I like how obvious these changes are. I made a few inline comments that I'm hoping you can clean up. One other thing is, would it be possible to add a test that uses this new functionality? It doesn't look like there is an existing module for the tube visual in vispy/visuals/tests/ but it would be nice if you could add one.
vispy/visuals/tube.py
Outdated
| # if single radius, convert to list of radii | ||
| if not isinstance(radius, collections.abc.Iterable): | ||
| radius = [radius] * len(points) | ||
| elif len(radius) != len(points): | ||
| raise ValueError('Length of radii list must match points.') |
There was a problem hiding this comment.
This is repeated from the code in __init__. Can this be put in a helper method and called from both locations?
Actually I see that all of this method is repeated in __init__. Maybe create a method that gets vertices, indices, vertex_colors, and face_colors, and use it in both? Or is it possible to create an empty MeshVisual and then call this set_data from the __init__ method?
vispy/visuals/tube.py
Outdated
| tube_points=8, shading='smooth', | ||
| vertex_colors=None, face_colors=None, | ||
| mode='triangles'): |
There was a problem hiding this comment.
It looks like a couple of these keyword args aren't used here: shading and mode, right?
|
@JamesTRoss1 do you think you'll have any time to review my requested changes? |
|
oh sorry, i've been super busy with school and haven't gotten around to adding the test cases. My goal is to work on it over this weekend |
Reviewed and updated changes. Added _get_radius method and removed unused keyword args.
|
I will add the tube test file sometime this week |
|
@JamesTRoss1 we are thinking of releasing a new version in the coming weeks. Do you have time to push this over the line? |
Overrides the MeshVisual .set_data() method so that a user can update the Tube's points as opposed to the Tube's vertices and faces. Also updated the init method to call methods that compute the faces and indices to allow for code re-use when calling .set_data()