Skip to content
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

Improve MultiSphereShape rendering #862

Merged
merged 2 commits into from
Mar 30, 2017
Merged

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Mar 29, 2017

Previously, DART only draws the spheres in a MultiSphereShape while the actual shape is the convex hull enclosing all the spheres. This PR still doesn't render the convex hull but improves the visualization by drawing the cylinders that connect the pairs of the spheres.

Before:
image

After:
image

@jslee02 jslee02 added this to the DART 6.2.0 milestone Mar 29, 2017
@mxgrey
Copy link
Member

mxgrey commented Mar 29, 2017

I'm a bit concerned by the name of the class. I don't think the name really conveys what the class is actually representing. This whole time, I've been thinking that the MultiSphereShape is something more like a point cloud. What if it were called something like MultiSphereConvexHullShape? Or if there's a technical term for this type of shape, that would probably be better. We could keep a MultiSphereShape alias for backwards compatibility. I can open this as a separate issue if you think it warrants more of a discussion.

@jslee02
Copy link
Member Author

jslee02 commented Mar 29, 2017

I agree with you. I named it inspired by Bullet's btMultiSphereShape and commented that it's a convex hull shape, but it would be much better to change the name to show that explicitly.

MultiSphereConvexHullShape sounds good to me. I also came up with SphereSweptConvexHullShape, but it could confuse people that the shape was created by sweeping with a single sphere I think.

By the way, let's address the naming in a separate PR to wait for other feedback on the name.

@psigen
Copy link
Collaborator

psigen commented Mar 29, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants