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

Fix World::clone() doesn't clone the collision detector #658

Merged
merged 4 commits into from
Apr 14, 2016

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Apr 6, 2016

No description provided.

@jslee02 jslee02 added this to the DART 6.0.0 milestone Apr 6, 2016
@jslee02 jslee02 changed the title Fix world cloning without collision detector Fix World::clone() doesn't clone the collision detector Apr 12, 2016
@mxgrey
Copy link
Member

mxgrey commented Apr 13, 2016

There seems to be a bit of a conflict where this PR has introduced the function getStaticType() for derived CollisionDetector implementations while an earlier PR introduced getTypeStatic().

I think I prefer getStaticType(), so I'll choose that version as I merge this, unless someone has a reason for preferring getTypeStatic().

@jslee02
Copy link
Member Author

jslee02 commented Apr 13, 2016

To clarify, we use getStatic[Name]() for a static member function, and use get[Name]Static() for fixed-size version of get[Name]() (especially when the function returns Eigen's Vector objects) where get[Name]Static() is a non-static function, right?

@mxgrey
Copy link
Member

mxgrey commented Apr 13, 2016

Yes, that has been our convention so far.

I'm also spotting some conflicts in naming conventions for the strings of the different collision detectors. We'll need to choose between:

"DART" or "dart"
"Bullet" or "bullet"
"FCL" or "fcl"

I don't really have any personal preference for any of these, but a decision must be made 😆

@jslee02
Copy link
Member Author

jslee02 commented Apr 13, 2016

Yeah, I would slightly prefer lower letter versions though since it's been used in SKEL format to specify the collision detectors. As these type would be used to print messages, it would be good to use same strings.

@mxgrey
Copy link
Member

mxgrey commented Apr 13, 2016

Sounds good.

@mxgrey mxgrey self-assigned this Apr 13, 2016
…ion_detector

Conflicts:
	dart/collision/CollisionDetector.h
	dart/collision/bullet/BulletCollisionDetector.cpp
	dart/collision/bullet/BulletCollisionDetector.h
	dart/collision/dart/DARTCollisionDetector.cpp
	dart/collision/dart/DARTCollisionDetector.h
	dart/collision/dart/DARTCollisionObject.h
	dart/collision/fcl/FCLCollisionDetector.cpp
	dart/collision/fcl/FCLCollisionDetector.h
	dart/collision/fcl_mesh/FCLMeshCollisionDetector.cpp
@jslee02
Copy link
Member Author

jslee02 commented Apr 13, 2016

Merged master branch into this. I think this is ready to merge.

@mxgrey
Copy link
Member

mxgrey commented Apr 13, 2016

I was also working on merging this, but I came across something fishy, and I'm seeing it in your merge as well.

cloneWithoutCollisionObjects() creates a unique_ptr, but setCollisionDetector(~) now expects a shared_ptr. So how is it that setCollisionDetector(cd->cloneWithoutCollisionObjects()); is being permitted? I can only guess it has to do with CollisionDetector inheriting std::enable_shared_from_this, but I thought the object needed to be managed by a shared_ptr to begin with, so can we really just implicitly cast from unique_ptr to shared_ptr?

@jslee02
Copy link
Member Author

jslee02 commented Apr 13, 2016

Oh, sorry for the confliction on work.

Good catch by the way. I think cloneWithoutCollisionObjects should be changed to return shared_ptr anyways because it should be able to be shared by collision groups.

Hm, I'm not sure how it was accepted the code. I'm trying to figure it out googling..

@mxgrey
Copy link
Member

mxgrey commented Apr 13, 2016

I'm happy with this PR. I'll merge it once the CI tests pass 👍

@jslee02 jslee02 merged commit 8c895a2 into master Apr 14, 2016
@jslee02 jslee02 deleted the bugfix/clone_collision_detector branch April 15, 2016 18:28
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.

2 participants