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

CollisionGroup class and refactoring of CollisionDetector #631

Merged
merged 79 commits into from Apr 13, 2016

Conversation

Projects
None yet
4 participants
@jslee02
Member

jslee02 commented Mar 11, 2016

This pull request introduces collision group feature with CollisionDetector refactoring.

An overview of the changes is:

  • CollisionGroup class is added. Collision detector used to containe collision objects in it and only could check collisions for the objects. So we couldn't be able to check collisions for arbitrary (distict) collision objects. CollisionGroup now plays the roll of collision object container, and collision detector can check collisions for object-object, object-group, and group-group.
  • collision is now only dependent on Shape class. So once we move Shape and the derived classes to common or math namespaces then collision would be completely independent on 'dynamics' namespace.
  • FCLCollisionDetector now takes the advantage of broad phase collision detectorion of FCL to avoid checking all pairs of objects.
  • Collision detector manages Shapes so that the collision detector doesn't create multiple collision detector specific collision object (e.g., btGImpactMeshShape, fcl::BVHModel) per Shape.

This pull request is not completed yet. The major issue remained is the ownership between CollisionDetector and CollisionObject. I'm considering apply the same mechanism of the customized shared pointer BodyNodePtr by making it general to be able to be used for CollisionObject.

Note that this pull request includes the changes of #608 .

jslee02 added some commits Feb 15, 2016

Merge remote-tracking branch 'origin/master' into collision_group
Conflicts:
	dart/constraint/ConstraintSolver.cpp
	dart/constraint/ConstraintSolver.h
	dart/utils/SkelParser.cpp
Enforcing CollisionDetector to create unique collision object for the…
… same associated object -- not completed yet

@jslee02 jslee02 added this to the DART 6.0.0 milestone Mar 11, 2016

jslee02 added some commits Mar 11, 2016

Merge remote-tracking branch 'origin/shapenode' into collision_group_…
…shapenode

Conflicts:
	apps/rigidShapes/Main.cpp
	dart/collision/bullet/BulletCollisionDetector.cpp
	dart/collision/bullet/BulletCollisionNode.cpp
	dart/collision/bullet/BulletCollisionObjectData.h
	dart/collision/dart/DARTCollisionDetector.cpp
	dart/collision/fcl/FCLCollisionDetector.cpp
	dart/collision/fcl/FCLCollisionNode.cpp
	dart/collision/fcl/FCLCollisionObjectData.h
	dart/collision/fcl_mesh/FCLMeshCollisionNode.cpp
	dart/utils/SkelParser.cpp
	dart/utils/sdf/SoftSdfParser.cpp
	tutorials/tutorialBiped-Finished.cpp
	tutorials/tutorialBiped.cpp
	unittests/testConstraint.cpp
@mxgrey

This comment has been minimized.

Show comment
Hide comment
@mxgrey

mxgrey Mar 15, 2016

Member

Out of curiosity, is there a particular motivation for making the collision namespace independent of the dynamics namespace? Are we trying to provide the general public with a generic API that will allow other (non-DART) projects to arbitrarily swap between the different collision engines that we support? If so, then I can understand the motivation, but otherwise I don't see why we'd make that separation.

Member

mxgrey commented Mar 15, 2016

Out of curiosity, is there a particular motivation for making the collision namespace independent of the dynamics namespace? Are we trying to provide the general public with a generic API that will allow other (non-DART) projects to arbitrarily swap between the different collision engines that we support? If so, then I can understand the motivation, but otherwise I don't see why we'd make that separation.

@mxgrey

This comment has been minimized.

Show comment
Hide comment
@mxgrey

mxgrey Mar 15, 2016

Member

The major issue remained is the ownership between CollisionDetector and CollisionObject. I'm considering apply the same mechanism of the customized shared pointer BodyNodePtr by making it general to be able to be used for CollisionObject.

I'm not clear on what the purpose of this would be. Does a CollisionObject have any meaning outside of its CollisionDetector? Do we expect users to be handling, manipulating, and modifying CollisionObjects independently of the CollisionDetector that manages them? What would be the scenario where the user has a reference to a CollisionObject while its CollisionDetector has no references left?

But if we do decide that we need a special type of smart pointer for CollisionObjects, here's something to take into account: The main factor that makes BodyNodePtr so complex is that a BodyNode might get swapped between Skeletons. Because of this, we need to do some extra weirdness. Assuming a CollisionObject cannot be transferred between two CollisionDetectors, we should really make its smart pointer more like TemplateNodePtr. All it would have to do is hold onto a reference to a CollisionDetectorPtr. We wouldn't need to do any extra reference counting inside of the CollisionObject the way we need to for BodyNode.

Member

mxgrey commented Mar 15, 2016

The major issue remained is the ownership between CollisionDetector and CollisionObject. I'm considering apply the same mechanism of the customized shared pointer BodyNodePtr by making it general to be able to be used for CollisionObject.

I'm not clear on what the purpose of this would be. Does a CollisionObject have any meaning outside of its CollisionDetector? Do we expect users to be handling, manipulating, and modifying CollisionObjects independently of the CollisionDetector that manages them? What would be the scenario where the user has a reference to a CollisionObject while its CollisionDetector has no references left?

But if we do decide that we need a special type of smart pointer for CollisionObjects, here's something to take into account: The main factor that makes BodyNodePtr so complex is that a BodyNode might get swapped between Skeletons. Because of this, we need to do some extra weirdness. Assuming a CollisionObject cannot be transferred between two CollisionDetectors, we should really make its smart pointer more like TemplateNodePtr. All it would have to do is hold onto a reference to a CollisionDetectorPtr. We wouldn't need to do any extra reference counting inside of the CollisionObject the way we need to for BodyNode.

@jslee02

This comment has been minimized.

Show comment
Hide comment
@jslee02

jslee02 Mar 16, 2016

Member

Out of curiosity, is there a particular motivation for making the collision namespace independent of the dynamics namespace?

The motivation behind this pull request is to resolve the circular dependencies of collision and dynamics unless we want to eventually merge the two namespaces.

Are we trying to provide the general public with a generic API that will allow other (non-DART) projects to arbitrarily swap between the different collision engines that we support?

This would be the natural result of the separation, but this is not the main reason.

Member

jslee02 commented Mar 16, 2016

Out of curiosity, is there a particular motivation for making the collision namespace independent of the dynamics namespace?

The motivation behind this pull request is to resolve the circular dependencies of collision and dynamics unless we want to eventually merge the two namespaces.

Are we trying to provide the general public with a generic API that will allow other (non-DART) projects to arbitrarily swap between the different collision engines that we support?

This would be the natural result of the separation, but this is not the main reason.

@mxgrey

This comment has been minimized.

Show comment
Hide comment
@mxgrey

mxgrey Mar 16, 2016

Member

The motivation behind this pull request is to resolve the circular dependencies of collision and dynamics unless we want to eventually merge the two namespaces.

I can understand why constraint would depend on both collision and dynamics since that's where collision constraints are resolved against the dynamics, but I'm not clear on why dynamics would have to depend on collision. To me it would seem more natural for collision to depend on dynamics, especially since I feel it's much more likely that a user would want to use the dynamics features without the collision features rather than vice-versa.

Member

mxgrey commented Mar 16, 2016

The motivation behind this pull request is to resolve the circular dependencies of collision and dynamics unless we want to eventually merge the two namespaces.

I can understand why constraint would depend on both collision and dynamics since that's where collision constraints are resolved against the dynamics, but I'm not clear on why dynamics would have to depend on collision. To me it would seem more natural for collision to depend on dynamics, especially since I feel it's much more likely that a user would want to use the dynamics features without the collision features rather than vice-versa.

@mkoval

This comment has been minimized.

Show comment
Hide comment
@mkoval

mkoval Mar 16, 2016

Collaborator

Does a CollisionObject have any meaning outside of its CollisionDetector? Do we expect users to be handling, manipulating, and modifying CollisionObjects independently of the CollisionDetector that manages them? What would be the scenario where the user has a reference to a CollisionObject while its CollisionDetector has no references left?

It is only valid to use a CollisionObject with the CollisionDetector that created it. The only reason we are considering adding new smart pointer types for CollisionGroup and CollisionObject is to avoid creating a circular smart_ptr reference.

I do remember concluding that the ownership needs to be coupled, but now I can't recall why. Hopefully @jslee02 can remind us. 😓

Collaborator

mkoval commented Mar 16, 2016

Does a CollisionObject have any meaning outside of its CollisionDetector? Do we expect users to be handling, manipulating, and modifying CollisionObjects independently of the CollisionDetector that manages them? What would be the scenario where the user has a reference to a CollisionObject while its CollisionDetector has no references left?

It is only valid to use a CollisionObject with the CollisionDetector that created it. The only reason we are considering adding new smart pointer types for CollisionGroup and CollisionObject is to avoid creating a circular smart_ptr reference.

I do remember concluding that the ownership needs to be coupled, but now I can't recall why. Hopefully @jslee02 can remind us. 😓

@mkoval

This comment has been minimized.

Show comment
Hide comment
@mkoval

mkoval Mar 16, 2016

Collaborator

To me it would seem more natural for collision to depend on dynamics, especially since I feel it's much more likely that a user would want to use the dynamics features without the collision features rather than vice-versa.

I don't see a problem with having collision depend on dynamics, as long as we don't introduce a circular dependency. @jslee02 can you explain again why this was not possible?

Collaborator

mkoval commented Mar 16, 2016

To me it would seem more natural for collision to depend on dynamics, especially since I feel it's much more likely that a user would want to use the dynamics features without the collision features rather than vice-versa.

I don't see a problem with having collision depend on dynamics, as long as we don't introduce a circular dependency. @jslee02 can you explain again why this was not possible?

@jslee02

This comment has been minimized.

Show comment
Hide comment
@jslee02

jslee02 Mar 16, 2016

Member

I can understand why constraint would depend on both collision and dynamics since that's where collision constraints are resolved against the dynamics, but I'm not clear on why dynamics would have to depend on collision. To me it would seem more natural for collision to depend on dynamics, especially since I feel it's much more likely that a user would want to use the dynamics features without the collision features rather than vice-versa.

I think constraints and dynamics should be merged first unless we want to distinct unconstrained dynamics and constrained dynamics. Also, the two namespaces are already dependent on each other. That means that dynamics should depend on collision for resolving contact constraints.

I agree that a user would want to use the dynamics features without the collision features like to compute mass matrix and compute inverse dynamics. I think this is for the case of using DART for controller design (as we expect the users to use DART for either of controller design and kinematics/dynamics simulation). However, in the sense of simulation, I also imagine a user would want to check collision in kinematics simulation without dynamics (e.g., kinematic motion planning).

Member

jslee02 commented Mar 16, 2016

I can understand why constraint would depend on both collision and dynamics since that's where collision constraints are resolved against the dynamics, but I'm not clear on why dynamics would have to depend on collision. To me it would seem more natural for collision to depend on dynamics, especially since I feel it's much more likely that a user would want to use the dynamics features without the collision features rather than vice-versa.

I think constraints and dynamics should be merged first unless we want to distinct unconstrained dynamics and constrained dynamics. Also, the two namespaces are already dependent on each other. That means that dynamics should depend on collision for resolving contact constraints.

I agree that a user would want to use the dynamics features without the collision features like to compute mass matrix and compute inverse dynamics. I think this is for the case of using DART for controller design (as we expect the users to use DART for either of controller design and kinematics/dynamics simulation). However, in the sense of simulation, I also imagine a user would want to check collision in kinematics simulation without dynamics (e.g., kinematic motion planning).

@mxgrey

This comment has been minimized.

Show comment
Hide comment
@mxgrey

mxgrey Mar 16, 2016

Member

I think it would make sense to separate implicitly solved constrains (such as what's done with the Articulated Body Algorithm in the dynamics namespace) and explicitly solved constraints (such as collision constraints in the constraint namespace). If dynamics is currently dependent on constraint, then I would prefer if that were changed somehow.

Member

mxgrey commented Mar 16, 2016

I think it would make sense to separate implicitly solved constrains (such as what's done with the Articulated Body Algorithm in the dynamics namespace) and explicitly solved constraints (such as collision constraints in the constraint namespace). If dynamics is currently dependent on constraint, then I would prefer if that were changed somehow.

@mkoval

This comment has been minimized.

Show comment
Hide comment
@mkoval

mkoval Mar 16, 2016

Collaborator

However, in the sense of simulation, I also imagine a user would want to check collision in kinematics simulation without dynamics (e.g., kinematic motion planning).

Even kinematic motion planning requires the forward (and, often, inverse kinematics) found in the dynamics namespace. I don't see a strong use-case for using collision without dynamics unless, like @mxgrey said, we are trying to provide the general public with a collision detection API.

I don't think that is an immediate goal for any of us, so I don't see an issue with making collision depend on dynamics. This assumes that we resolve the circular dependency issue so dynamics does not also depend on collision.

Collaborator

mkoval commented Mar 16, 2016

However, in the sense of simulation, I also imagine a user would want to check collision in kinematics simulation without dynamics (e.g., kinematic motion planning).

Even kinematic motion planning requires the forward (and, often, inverse kinematics) found in the dynamics namespace. I don't see a strong use-case for using collision without dynamics unless, like @mxgrey said, we are trying to provide the general public with a collision detection API.

I don't think that is an immediate goal for any of us, so I don't see an issue with making collision depend on dynamics. This assumes that we resolve the circular dependency issue so dynamics does not also depend on collision.

jslee02 added some commits Mar 23, 2016

Merge remote-tracking branch 'origin/master' into collision_group
Conflicts:
	CMakeLists.txt
	apps/rigidShapes/Main.cpp
	dart/constraint/ConstraintSolver.cpp
	dart/constraint/ConstraintSolver.h
	tutorials/tutorialBiped-Finished.cpp
	tutorials/tutorialBiped.cpp
	unittests/testConstraint.cpp
DART_COMMON_MAKE_SHARED_WEAK(DARTCollisionDetector)
#if HAVE_BULLET_COLLISION
DART_COMMON_MAKE_SHARED_WEAK(BulletCollisionDetector)
#endif

This comment has been minimized.

@mkoval

mkoval Apr 5, 2016

Collaborator

Shouldn't you have a similar block for FCLCollisionDetector? I believe FCL is technically an optional dependency.

@mkoval

mkoval Apr 5, 2016

Collaborator

Shouldn't you have a similar block for FCLCollisionDetector? I believe FCL is technically an optional dependency.

This comment has been minimized.

@jslee02

jslee02 Apr 5, 2016

Member

FCL is currently the default collision detector of DART so we have it as a mandatory dependency.

@jslee02

jslee02 Apr 5, 2016

Member

FCL is currently the default collision detector of DART so we have it as a mandatory dependency.

/// ShapeFrames and CollisionOjbects added to this CollisionGroup
std::vector<std::pair<const dynamics::ShapeFrame*,
CollisionObjectPtr>> mShapeFrameMap;

This comment has been minimized.

@mxgrey

mxgrey Apr 11, 2016

Member

For the sake of performance, we could also hold onto a std::unordered_map<const dynamics::ShapeFrame*, size_t> where the size_t points to the location in mShapeFrameMap. That would allow us to preserve the ordering of the ShapeFrames in the CollisionGroup while also giving us good scalability for large groups. The one catch is that we would have two member fields to keep in sync.

But it's probably not an important matter, so we could put it off until v7.1.

@mxgrey

mxgrey Apr 11, 2016

Member

For the sake of performance, we could also hold onto a std::unordered_map<const dynamics::ShapeFrame*, size_t> where the size_t points to the location in mShapeFrameMap. That would allow us to preserve the ordering of the ShapeFrames in the CollisionGroup while also giving us good scalability for large groups. The one catch is that we would have two member fields to keep in sync.

But it's probably not an important matter, so we could put it off until v7.1.

This comment has been minimized.

@jslee02

jslee02 Apr 12, 2016

Member

Yeah, that approach sounds good. We could implement something similar to nlohmann/fifo_map for the sync guarantee.

I would like to do this as a separate PR since this PR blocks others.

@jslee02

jslee02 Apr 12, 2016

Member

Yeah, that approach sounds good. We could implement something similar to nlohmann/fifo_map for the sync guarantee.

I would like to do this as a separate PR since this PR blocks others.

class CollisionFilter;
struct CollisionOption

This comment has been minimized.

@mxgrey

mxgrey Apr 11, 2016

Member

Since the name of the struct is CollisionOption, I wonder if we should also name the header CollisionOption.h rather than Option.h.

@mxgrey

mxgrey Apr 11, 2016

Member

Since the name of the struct is CollisionOption, I wonder if we should also name the header CollisionOption.h rather than Option.h.

namespace dart {
namespace collision {
class CollisionResult

This comment has been minimized.

@mxgrey

mxgrey Apr 11, 2016

Member

Same as CollisionOption: I wonder if we should change the header name to CollisionResult.h. Alternatively, we could change the class name to Result so that it's collision::Result.

@mxgrey

mxgrey Apr 11, 2016

Member

Same as CollisionOption: I wonder if we should change the header name to CollisionResult.h. Alternatively, we could change the class name to Result so that it's collision::Result.

This comment has been minimized.

@jslee02

jslee02 Apr 11, 2016

Member

This class and CollisionOption were just Result and Option, then I renamed them adding Collision- prefix because Distance[Option/Result] will be added for distanct (or raycast) query.

One concern is if having four files for [Collision/Distance][Option/Result] would be too many.

@jslee02

jslee02 Apr 11, 2016

Member

This class and CollisionOption were just Result and Option, then I renamed them adding Collision- prefix because Distance[Option/Result] will be added for distanct (or raycast) query.

One concern is if having four files for [Collision/Distance][Option/Result] would be too many.

This comment has been minimized.

@mxgrey

mxgrey Apr 11, 2016

Member

Ohhh, I see. If there will be new classes with similar names, then it makes sense to give the header a generic name like this. 👍

@mxgrey

mxgrey Apr 11, 2016

Member

Ohhh, I see. If there will be new classes with similar names, then it makes sense to give the header a generic name like this. 👍

CollisionDetector();
/// Create CollisionObject
virtual std::unique_ptr<CollisionObject> createCollisionObject(
const dynamics::ShapeFrame* shapeFrame) = 0;

This comment has been minimized.

@mxgrey

mxgrey Apr 11, 2016

Member

Is there a particular reason that we create these as a unique_ptr when it seems as though we always end up converting them into a shared_ptr? Is this to make it impossible to jumble up claimCollisionObject(~) with createCollisionObject(~)?

@mxgrey

mxgrey Apr 11, 2016

Member

Is there a particular reason that we create these as a unique_ptr when it seems as though we always end up converting them into a shared_ptr? Is this to make it impossible to jumble up claimCollisionObject(~) with createCollisionObject(~)?

This comment has been minimized.

@jslee02

jslee02 Apr 12, 2016

Member

Yes. The main motivation behind this is to make CollisionDetector create single CollisionObject per ShapeFrame using RAII.

claimCollisionObject(~) returns std::shared_ptr<CollisionObject>. The returning pointer is either of one created from createCollisionObject if it's not created for the passed in ShapeFrame or one already created for the ShapeFrame. The management of the uniquess of CollisionObject per ShapeFrame is done using std::map<ShapeFrame*, std::weak_ptr<CollisionObject> in CollisionDetector (more precisely, in CollisionDetector::SharingCollisionObjectManager). The CollisionObject should be removed from the map when it's not shared by any CollisionObject. However, the thing is std::shared_ptr<CollisionObject> will be automatically destructed when it's not referenced by any CollisionGroup (as the map holds CollisionObject as weak pointer), but CollisionDetector doesn't know when it's happening.

To handle this, a custom deleter of shared pointer is used where it removes CollisionObject from the map before it deletes CollisionObject. I could make createCollisionObject create shared_ptr with the custom deleter by passing it from CollisionDetector to each concrete collision detector, but also wondered if it's a good practice. To me, it seemed the management of collision object management should be totally the responsibility of CollisionDetector and the each collision detector just should create the concrete collision object (e.g., FCLCollisionObject).

I'm open to hearing better solution here.

@jslee02

jslee02 Apr 12, 2016

Member

Yes. The main motivation behind this is to make CollisionDetector create single CollisionObject per ShapeFrame using RAII.

claimCollisionObject(~) returns std::shared_ptr<CollisionObject>. The returning pointer is either of one created from createCollisionObject if it's not created for the passed in ShapeFrame or one already created for the ShapeFrame. The management of the uniquess of CollisionObject per ShapeFrame is done using std::map<ShapeFrame*, std::weak_ptr<CollisionObject> in CollisionDetector (more precisely, in CollisionDetector::SharingCollisionObjectManager). The CollisionObject should be removed from the map when it's not shared by any CollisionObject. However, the thing is std::shared_ptr<CollisionObject> will be automatically destructed when it's not referenced by any CollisionGroup (as the map holds CollisionObject as weak pointer), but CollisionDetector doesn't know when it's happening.

To handle this, a custom deleter of shared pointer is used where it removes CollisionObject from the map before it deletes CollisionObject. I could make createCollisionObject create shared_ptr with the custom deleter by passing it from CollisionDetector to each concrete collision detector, but also wondered if it's a good practice. To me, it seemed the management of collision object management should be totally the responsibility of CollisionDetector and the each collision detector just should create the concrete collision object (e.g., FCLCollisionObject).

I'm open to hearing better solution here.

@mxgrey

This comment has been minimized.

Show comment
Hide comment
@mxgrey

mxgrey Apr 11, 2016

Member

I'm under the impression that NoneSharingCollisionObjectManager is supposed to be used by CollisionDetector implementations in which it is not possible to share their collision objects between groups, so a new CollisionObject instance needs to be created for each group in which a single ShapeFrame instance is being referenced. Then SharingCollisionObjectManager is used by CollisionDetector implementations which do allow multiple groups to share the same collision object instances. Assuming that's the case, I would recommend some name changes:

NoneSharingCollisionObjectManager --> UnshareableCollisionObjectManager or ManagerForUnshareableCollisionObjects

SharingCollisionObjectManager --> ShareableCollisionObjectManager or ManagerForShareableCollisionObjects

Mostly I think "NoneSharing" makes for an awkward class name, so I'd like to change that if we can.

Member

mxgrey commented Apr 11, 2016

I'm under the impression that NoneSharingCollisionObjectManager is supposed to be used by CollisionDetector implementations in which it is not possible to share their collision objects between groups, so a new CollisionObject instance needs to be created for each group in which a single ShapeFrame instance is being referenced. Then SharingCollisionObjectManager is used by CollisionDetector implementations which do allow multiple groups to share the same collision object instances. Assuming that's the case, I would recommend some name changes:

NoneSharingCollisionObjectManager --> UnshareableCollisionObjectManager or ManagerForUnshareableCollisionObjects

SharingCollisionObjectManager --> ShareableCollisionObjectManager or ManagerForShareableCollisionObjects

Mostly I think "NoneSharing" makes for an awkward class name, so I'd like to change that if we can.

mxgrey and others added some commits Apr 11, 2016

Merge remote-tracking branch 'origin/master' into collision_group
Conflicts:
	dart/collision/CollisionObject.h
	dart/collision/bullet/BulletCollisionNode.cpp
@jslee02

This comment has been minimized.

Show comment
Hide comment
@jslee02

jslee02 Apr 12, 2016

Member

Assuming that's the case, I would recommend some name changes:

Yes, exactly. I like the new names. To me, the pair of ManagerFor[Uns/S]hareableCollisionObjects sounds clearer than the other pair since [Uns/S]harable seems to describe managers rather than collision objects.

Member

jslee02 commented Apr 12, 2016

Assuming that's the case, I would recommend some name changes:

Yes, exactly. I like the new names. To me, the pair of ManagerFor[Uns/S]hareableCollisionObjects sounds clearer than the other pair since [Uns/S]harable seems to describe managers rather than collision objects.

@mxgrey

This comment has been minimized.

Show comment
Hide comment
@mxgrey

mxgrey Apr 13, 2016

Member

Overall, I think this pull request introduces a much needed renovation of the collision namespace and introduces many wonderful features. I'm happy with its current state and think it's ready to be merged, unless anyone has an objection.

I do have a few comments for moving forward. These certainly don't need to be addressed in this pull request, but it's something to think about (we can open issues for them later):

  1. When a user adds a BodyNode or a Skeleton to a CollisionGroup, that CollisionGroup should update automatically whenever a new ShapeNode is added or removed from the object. I can think of two ways to accommodate this:
    1. Version tracking. Whenever the CollisionGroup spots a change in a version number, it can check whether the layout of ``ShapeFrames has changed.
    2. Signals and slots. Have BodyNode and Skeleton both offer a signal that gets triggered when a ShapeNode is added, and another one that is triggered when the ShapeNode is removed. We could even generalize it so that users can register their own ShapeFrameCreated signals to a CollisionGroup. I think this approach would be preferable over (i), especially given its generalizability.
  2. It looks like it's very easy for a CollisionGroup to be unknowingly invalidated since they hold raw pointers to the ShapeFrames that they reference. It would be good if they could do automatic cleanup when a ShapeFrame that they refer to gets destroyed. Of course this is complicated because SimpleFrames get managed by std::shared_ptr whereas ShapeNodes get managed by ShapeNodePtr which are not compatible smart pointer types. An easy (but not thread-safe) solution would be to use the fact that both types have a common::Subject base, so they can give out a notice when they destruct, but we'd probably want a cleaner solution than this in the long run.
Member

mxgrey commented Apr 13, 2016

Overall, I think this pull request introduces a much needed renovation of the collision namespace and introduces many wonderful features. I'm happy with its current state and think it's ready to be merged, unless anyone has an objection.

I do have a few comments for moving forward. These certainly don't need to be addressed in this pull request, but it's something to think about (we can open issues for them later):

  1. When a user adds a BodyNode or a Skeleton to a CollisionGroup, that CollisionGroup should update automatically whenever a new ShapeNode is added or removed from the object. I can think of two ways to accommodate this:
    1. Version tracking. Whenever the CollisionGroup spots a change in a version number, it can check whether the layout of ``ShapeFrames has changed.
    2. Signals and slots. Have BodyNode and Skeleton both offer a signal that gets triggered when a ShapeNode is added, and another one that is triggered when the ShapeNode is removed. We could even generalize it so that users can register their own ShapeFrameCreated signals to a CollisionGroup. I think this approach would be preferable over (i), especially given its generalizability.
  2. It looks like it's very easy for a CollisionGroup to be unknowingly invalidated since they hold raw pointers to the ShapeFrames that they reference. It would be good if they could do automatic cleanup when a ShapeFrame that they refer to gets destroyed. Of course this is complicated because SimpleFrames get managed by std::shared_ptr whereas ShapeNodes get managed by ShapeNodePtr which are not compatible smart pointer types. An easy (but not thread-safe) solution would be to use the fact that both types have a common::Subject base, so they can give out a notice when they destruct, but we'd probably want a cleaner solution than this in the long run.
Merge remote-tracking branch 'origin/master' into collision_group
Conflicts:
	dart/utils/sdf/SdfParser.cpp
@mkoval

This comment has been minimized.

Show comment
Hide comment
@mkoval

mkoval Apr 13, 2016

Collaborator

👍 Great work @jslee02! I am very happy with how the API turned out.

When a user adds a BodyNode or a Skeleton to a CollisionGroup, that CollisionGroup should update automatically whenever a new ShapeNode is added or removed from the object.

I am conflicted about this: this adds a lot of complexity and may not always have the desired effect. I'd prefer to keep CollisionGroup as a "dumb" collection of ShapeFrames and implement automatic management in a helper (or wrapper) class. I am fine with encouraging people to use this "smart" group by default and leaving the "dumb" group for advanced users.

Of the two options, I slightly prefer option (i) for two reasons. First, creating a CollisionObject may be expensive. It is undesirable to constantly create and delete CollisionObjects when modifying the kinematic tree. Second, signals and slots makes it much harder to reason about control flow.

It looks like it's very easy for a CollisionGroup to be unknowingly invalidated since they hold raw pointers to the ShapeFrames that they reference. It would be good if they could do automatic cleanup when a ShapeFrame that they refer to gets destroyed. Of course this is complicated because SimpleFrames get managed by std::shared_ptr whereas ShapeNodes get managed by ShapeNodePtr which are not compatible smart pointer types.

I agree. This is a problem far beyond the CollisionGroup API: I have the same challenge in a lot of the code that I write that uses Frames. In most cases, I end up using Node to dodge the question entirely.

What is the motivation/use case for having Frame, ShapeFrame, etc outside of a Skeleton? If the concern is overhead, perhaps we can add a very lightweight base class to Skeleton that is solely responsible for managing ownership. This would let us use NodePtr universally.

Collaborator

mkoval commented Apr 13, 2016

👍 Great work @jslee02! I am very happy with how the API turned out.

When a user adds a BodyNode or a Skeleton to a CollisionGroup, that CollisionGroup should update automatically whenever a new ShapeNode is added or removed from the object.

I am conflicted about this: this adds a lot of complexity and may not always have the desired effect. I'd prefer to keep CollisionGroup as a "dumb" collection of ShapeFrames and implement automatic management in a helper (or wrapper) class. I am fine with encouraging people to use this "smart" group by default and leaving the "dumb" group for advanced users.

Of the two options, I slightly prefer option (i) for two reasons. First, creating a CollisionObject may be expensive. It is undesirable to constantly create and delete CollisionObjects when modifying the kinematic tree. Second, signals and slots makes it much harder to reason about control flow.

It looks like it's very easy for a CollisionGroup to be unknowingly invalidated since they hold raw pointers to the ShapeFrames that they reference. It would be good if they could do automatic cleanup when a ShapeFrame that they refer to gets destroyed. Of course this is complicated because SimpleFrames get managed by std::shared_ptr whereas ShapeNodes get managed by ShapeNodePtr which are not compatible smart pointer types.

I agree. This is a problem far beyond the CollisionGroup API: I have the same challenge in a lot of the code that I write that uses Frames. In most cases, I end up using Node to dodge the question entirely.

What is the motivation/use case for having Frame, ShapeFrame, etc outside of a Skeleton? If the concern is overhead, perhaps we can add a very lightweight base class to Skeleton that is solely responsible for managing ownership. This would let us use NodePtr universally.

@mxgrey

This comment has been minimized.

Show comment
Hide comment
@mxgrey

mxgrey Apr 13, 2016

Member

I'd prefer to keep CollisionGroup as a "dumb" collection of ShapeFrames and implement automatic management in a helper (or wrapper) class.

This sounds like a good plan to me.

It is undesirable to constantly create and delete CollisionObjects when modifying the kinematic tree. Second, signals and slots makes it much harder to reason about control flow.

These are very good points. If we're going to implement it with wrapper classes, then we could offer a wrapper for each approach if we can't make a decision between the two. But your points do make me lean more towards (i) now.

What is the motivation/use case for having Frame, ShapeFrame, etc outside of a Skeleton?

What it boils down to is the fact that Frame is an interface class which is agnostic to any concept of a Skeleton. Requiring every derivation of Frame to be owned and managed by a Skeleton seemed pointlessly strict to me.

The SimpleFrame implementation has an important advantage over any feasible Node type: The parent frame of a SimpleFrame can be changed to any Frame at any time. In contrast, a Node must currently descend from a BodyNode and that parent BodyNode can't change. It's also plausible that users may want to implement their own Frame-based kinematic structures independent of the Skeleton concept, and I think they should have the freedom to do so.

I'm open to the idea of being able to move Nodes between parent BodyNodes, but we would have to reinvent the NodePtr infrastructure, and I'm having trouble thinking of a better design at the moment.

In any case, we can move this conversation to an issue if everyone is okay with merging this pull request.

Member

mxgrey commented Apr 13, 2016

I'd prefer to keep CollisionGroup as a "dumb" collection of ShapeFrames and implement automatic management in a helper (or wrapper) class.

This sounds like a good plan to me.

It is undesirable to constantly create and delete CollisionObjects when modifying the kinematic tree. Second, signals and slots makes it much harder to reason about control flow.

These are very good points. If we're going to implement it with wrapper classes, then we could offer a wrapper for each approach if we can't make a decision between the two. But your points do make me lean more towards (i) now.

What is the motivation/use case for having Frame, ShapeFrame, etc outside of a Skeleton?

What it boils down to is the fact that Frame is an interface class which is agnostic to any concept of a Skeleton. Requiring every derivation of Frame to be owned and managed by a Skeleton seemed pointlessly strict to me.

The SimpleFrame implementation has an important advantage over any feasible Node type: The parent frame of a SimpleFrame can be changed to any Frame at any time. In contrast, a Node must currently descend from a BodyNode and that parent BodyNode can't change. It's also plausible that users may want to implement their own Frame-based kinematic structures independent of the Skeleton concept, and I think they should have the freedom to do so.

I'm open to the idea of being able to move Nodes between parent BodyNodes, but we would have to reinvent the NodePtr infrastructure, and I'm having trouble thinking of a better design at the moment.

In any case, we can move this conversation to an issue if everyone is okay with merging this pull request.

@mxgrey mxgrey merged commit 25e3a8b into master Apr 13, 2016

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.6%) to 43.359%
Details

@jslee02 jslee02 referenced this pull request Apr 13, 2016

Closed

Use FCL's broad phase #20

@jslee02 jslee02 deleted the collision_group branch Apr 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment