Fixes collision problem

[costashatz]

Fixes #666 issue. It's done quickly and dirty. So feel free to not accept or polish it. It's just that we are using this functionality and needed it asap, so I coded it and sharing it..

Many thanks..

[mxgrey]

Thanks for bringing this to our attention!

Indeed, it used to be the responsibility of the collision detector to call BodyNode::setColliding(false) on all relevant bodies to clear out whichever bodies are not in collision and then call BodyNode::setColliding(true) on all the bodies that it detects as being in collision. This behavior has been removed, but the BodyNode::setColliding(~) and BodyNode::isColliding() API still exists (even though it no longer does anything).

I'm not sure if removing this behavior was an intentional design choice or an oversight. Either we should replace the setColliding(~) and isColliding() API with something comparable, or we should revert to having the CollisionDetectors set the mIsColliding flag correctly. But maybe @jslee02 already has plans related to this.

[jslee02]

This functionality was removed when I was trying to remove the dependency of dynamics from collision, then it wasn't recovered after we decided not to so. So this was definitely not intentional.

Merging now.

[mxgrey]

I don't think we're actually setting the flags in the right place with this pull request. This would only work if the user is checking collisions through a ConstraintSolver. If they are checking collisions through a CollisionDetector directly, then this fix will not work for them. I think whenever a collision test is requested, the CollisionDetector should set the flags for the bodies of any ShapeNodes held by any groups involved in the test.

[mxgrey]

Also, I don't know how comfortable I am with the need to perform a const-cast. I would much rather have CollisionObject store its ShapeFrame reference as non-const.

[mkoval]

I am not a fan of CollisionDetector mutating the state of BodyNodes during a collision check:

1. this introduces a dependency (at least conceptually) from dynamics to collision.
2. it is difficult to interpret the isColliding field unless you do a check on the entire Skeleton at once
3. this will introduce concurrency problems if we ever parallelize collision checks

I would prefer to maintain backwards compatibility with a const_cast, deprecate this functionality, and remove it in the next major release. Is there a compelling reason for this flag to exist?

[mxgrey]

I think there's value in being able to easily track when a body comes into collision. Otherwise you may need to pour through the CollisionResult data each time to find if the particular body you're interested in has collided, so you'd have an additional O(N) operation where N is the number of collision points.

The current approach might not be ideal, but if we're going to remove it, then I'd like to replace it with something analogous.

[mkoval]

Fair enough. I would prefer to improve the API CollisionResult structure, rather than putting this functionality on Skeleton. Perhaps we could add helper functions that test whether a ShapeNode or BodyNode is included in a CollisionResult?

[mxgrey]

Perhaps we could add helper functions that test whether a ShapeNode or BodyNode is included in a CollisionResult?

That's plausible; it could include a std::unordered_set of ShapeNodes and/or BodyNodes that are in collision. However, if a user is also simulating using a World instance, then this data would get intercepted by the ConstraintSolver and would not be accessible to the user. We would also need ConstraintSolver to hold onto the last CollisionResult that it wound up with and provide the user viewing access to it. This is all doable, albeit awkward and potentially confusing.

[mkoval]

Like I hinted at in point (2) above, I think having isColliding on BodyNode makes sense only if you run one pairwise check on entire Skeleton (or collection of Skeletons). I am open to making this information available in ConstraintSolver and World because the dynamics engine does query the collision detector in this way.

In other applications, like motion planning, one conceptual collision detection query requires multiple CollisionGroup collision detection queries. E.g. we commonly to separate self-collisions from robot-environment collisions to avoid detecting collision between a pair of stationary objects in the environment.

In this case, the value of isColliding only reflects the outcome of the latter query. I think it is natural to store this information in the CollisionResult object that is specific to a query. Storing the result in BodyNode makes its value appear less ephemeral than it actually is.

[costashatz]

I don't think we're actually setting the flags in the right place with this pull request. This would only work if the user is checking collisions through a ConstraintSolver. If they are checking collisions through a CollisionDetector directly, then this fix will not work for them. I think whenever a collision test is requested, the CollisionDetector should set the flags for the bodies of any ShapeNodes held by any groups involved in the test.

I totally agree with you on that one. As I said in the description, this was a quick fix..

I think there's value in being able to easily track when a body comes into collision. Otherwise you may need to pour through the CollisionResult data each time to find if the particular body you're interested in has collided, so you'd have an additional O(N) operation where N is the number of collision points.

You're absolutely right. There should be an easy way to check for BodyNode collisions. In some cases (like running 1 million simulations for an evolutionary algorithm), it is important to be able to check easily if a BodyNode is colliding. The user may not care about the exact collision details, but just if the BodyNode is colliding.

Like I hinted at in point (2) above, I think having isColliding on BodyNode makes sense only if you run one pairwise check on entire Skeleton (or collection of Skeletons). I am open to making this information available in ConstraintSolver and World because the dynamics engine does query the collision detector in this way.

What you are suggesting, makes sense.. It's better to expose this functionality in ConstraintSolver and World than in BodyNode.

I think it is natural to store this information in the CollisionResult object that is specific to a query. Storing the result in BodyNode makes its value appear less ephemeral than it actually is.

This is true..

[mxgrey]

I completely agree with @mkoval 's points.

Since a ConstraintSolver instance performs a single collision check per iteration and a World has a single ConstraintSolver we can offer the following functions:

const collision::CollisionResult& ConstraintSolver::getLastCollisionResult() const
{
  return mLastCollisionResult;
}

const collision::CollisionResult& World::getLastCollisionResult() const
{
  return mConstraintSolver->getLastCollisionResult();
}

We'll add a std::unordered_map<ShapeNode> and std::unordered_map<BodyNode> to CollisionResult as well as

bool CollisionResult::isColliding(const BodyNode* bn) const;
bool CollisionResult::isColliding(const ShapeNode* node) const;

Then BodyNode::setColliding(~) and BodyNode::isColliding() can be deprecated.

If that sounds good to everyone, then I'll implement this and make a pull request.

[jslee02]

If that sounds good to everyone, then I'll implement this and make a pull request.

👍 Sounds good to me.