-
Notifications
You must be signed in to change notification settings - Fork 287
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
Add blacklist to BodyNodeCollisionFilter #911
Changes from 15 commits
af9d36e
a3f01ff
43820d5
68373c6
7e5ed18
305f47e
2418f76
33f46fa
8cc365a
78b7e42
e6f2f00
96596fd
72a2b28
6a8e5ac
866947c
f46337b
8c7611a
b5879bc
23f10bd
42ad25e
3ced456
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,44 +38,164 @@ namespace dart { | |
namespace collision { | ||
|
||
//============================================================================== | ||
bool BodyNodeCollisionFilter::needCollision( | ||
bool CollisionFilter::needCollision( | ||
const CollisionObject* object1, const CollisionObject* object2) const | ||
{ | ||
return !ignoresCollision(object1, object2); | ||
} | ||
|
||
//============================================================================== | ||
void CompositeCollisionFilter::addCollisionFilter(const CollisionFilter* filter) | ||
{ | ||
if (!filter) | ||
return; | ||
|
||
const auto result = std::find(mFilters.begin(), mFilters.end(), filter); | ||
const bool found = (result != mFilters.end()); | ||
if (found) | ||
return; | ||
|
||
mFilters.insert(filter); | ||
} | ||
|
||
//============================================================================== | ||
void CompositeCollisionFilter::removeCollisionFilter( | ||
const CollisionFilter* filter) | ||
{ | ||
mFilters.erase(filter); | ||
} | ||
|
||
//============================================================================== | ||
void CompositeCollisionFilter::removeAllCollisionFilters() | ||
{ | ||
mFilters.clear(); | ||
} | ||
|
||
//============================================================================== | ||
bool CompositeCollisionFilter::ignoresCollision( | ||
const CollisionObject* object1, const CollisionObject* object2) const | ||
{ | ||
for (const auto* filter : mFilters) | ||
{ | ||
const bool collisionIgnored = filter->ignoresCollision(object1, object2); | ||
if (collisionIgnored) | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
//============================================================================== | ||
void BodyNodeCollisionFilter::addBodyNodePairToBlackList( | ||
const dynamics::BodyNode* bodyNode1, const dynamics::BodyNode* bodyNode2) | ||
{ | ||
if (!bodyNode1 || !bodyNode2) | ||
return; | ||
|
||
const auto* bodyNodeLess = bodyNode1; | ||
const auto* bodyNodeGreater = bodyNode2; | ||
|
||
if (bodyNodeLess > bodyNodeGreater) | ||
std::swap(bodyNodeLess, bodyNodeGreater); | ||
|
||
// Call insert in case an entry for bodyNodeLess doesn't exist. If it doesn't | ||
// exist, it will be initialized with an empty set. If it does already exist, | ||
// we will just get an iterator to the existing entry. | ||
const auto itLess = mBlackList.insert( | ||
std::make_pair(bodyNodeLess, | ||
std::unordered_set<const dynamics::BodyNode*>())).first; | ||
|
||
// Insert bodyNodeGreater into the set corresponding to bodyNodeLess. If the | ||
// pair already existed, this will do nothing. | ||
itLess->second.insert(bodyNodeGreater); | ||
} | ||
|
||
//============================================================================== | ||
void BodyNodeCollisionFilter::removeBodyNodePairFromBlackList( | ||
const dynamics::BodyNode* bodyNode1, const dynamics::BodyNode* bodyNode2) | ||
{ | ||
if (!bodyNode1 || !bodyNode2) | ||
return; | ||
|
||
const auto* bodyNodeLess = bodyNode1; | ||
const auto* bodyNodeGreater = bodyNode2; | ||
|
||
if (bodyNodeLess > bodyNodeGreater) | ||
std::swap(bodyNodeLess, bodyNodeGreater); | ||
|
||
// Remove the pair only when it already exists | ||
const auto resultLeft = mBlackList.find(bodyNodeLess); | ||
const bool foundLeft = (resultLeft != mBlackList.end()); | ||
if (foundLeft) | ||
{ | ||
auto& associatedRights = resultLeft->second; | ||
associatedRights.erase(bodyNodeGreater); | ||
|
||
if (associatedRights.empty()) | ||
mBlackList.erase(resultLeft); | ||
} | ||
} | ||
|
||
//============================================================================== | ||
void BodyNodeCollisionFilter::removeAllBodyNodePairsFromBlackList() | ||
{ | ||
mBlackList.clear(); | ||
} | ||
|
||
//============================================================================== | ||
bool BodyNodeCollisionFilter::ignoresCollision( | ||
const collision::CollisionObject* object1, | ||
const collision::CollisionObject* object2) const | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is really for line 171, but Github won't let me comment there: Why do we assume that non- |
||
if (object1 == object2) | ||
return false; | ||
return true; | ||
|
||
auto shapeNode1 = object1->getShapeFrame()->asShapeNode(); | ||
auto shapeNode2 = object2->getShapeFrame()->asShapeNode(); | ||
|
||
if (!shapeNode1 || !shapeNode2) | ||
return true; | ||
// We assume that non-ShapeNode is always being checked collision. | ||
if (!shapeNode1) | ||
{ | ||
dterr << "[BodyNodeCollisionFilter::ignoresCollision] Inappropriate " | ||
<< "collision filter is used for shapeNode1. This collision filter " | ||
<< "shouldn't be for non-ShapeNodes.\n"; | ||
assert(false); | ||
} | ||
|
||
if (!shapeNode2) | ||
{ | ||
dterr << "[BodyNodeCollisionFilter::ignoresCollision] Inappropriate " | ||
<< "collision filter is used for shapeNode2. This collision filter " | ||
<< "shouldn't be for non-ShapeNodes.\n"; | ||
assert(false); | ||
} | ||
|
||
auto bodyNode1 = shapeNode1->getBodyNodePtr(); | ||
auto bodyNode2 = shapeNode2->getBodyNodePtr(); | ||
|
||
if (bodyNode1 == bodyNode2) | ||
return false; | ||
return true; | ||
|
||
if (!bodyNode1->isCollidable() || !bodyNode2->isCollidable()) | ||
return false; | ||
return true; | ||
|
||
if (bodyNode1->getSkeleton() == bodyNode2->getSkeleton()) | ||
{ | ||
auto skeleton = bodyNode1->getSkeleton(); | ||
|
||
if (!skeleton->isEnabledSelfCollisionCheck()) | ||
return false; | ||
return true; | ||
|
||
if (!skeleton->isEnabledAdjacentBodyCheck()) | ||
{ | ||
if (areAdjacentBodies(bodyNode1, bodyNode2)) | ||
return false; | ||
return true; | ||
} | ||
} | ||
|
||
return true; | ||
if (hasBodyNodePairInBlacklist(bodyNode1, bodyNode2)) | ||
return true; | ||
|
||
return false; | ||
} | ||
|
||
//============================================================================== | ||
|
@@ -93,5 +213,31 @@ bool BodyNodeCollisionFilter::areAdjacentBodies( | |
return false; | ||
} | ||
|
||
} // namespace collision | ||
} // namespace dart | ||
//============================================================================== | ||
bool BodyNodeCollisionFilter::hasBodyNodePairInBlacklist( | ||
const dynamics::BodyNode* bodyNode1, | ||
const dynamics::BodyNode* bodyNode2) const | ||
{ | ||
const auto* bodyNodeLess = bodyNode1; | ||
const auto* bodyNodeGreater = bodyNode2; | ||
|
||
if (bodyNodeLess > bodyNodeGreater) | ||
std::swap(bodyNodeLess, bodyNodeGreater); | ||
|
||
const auto resultLeft = mBlackList.find(bodyNodeLess); | ||
const bool foundLeft = (resultLeft != mBlackList.end()); | ||
if (foundLeft) | ||
{ | ||
auto& key = resultLeft->second; | ||
|
||
const auto resultRight = key.find(bodyNodeGreater); | ||
const bool foundRight = (resultRight != key.end()); | ||
if (foundRight) | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
} // namespace collision | ||
} // namespace dart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two thoughts on this where the second thought makes the first through irrelevant, but I'll mention it anyway, just in case you're able to correct me:
I'm not sure if
std::find
will take advantage ofstd::unordered_set
's useful hashing properties. It's entirely plausible that it could (using either template specialization or function overloading), and I wouldn't be very surprised if it did, but I can't find any indication online that this is the case. So this might be performing an O(N) lookup even though our container could do a O(1) lookup. Please let me know if you're aware of any specialized optimizations thatstd::find
performs, because I'd be very interested in knowing.Calling
mFilters.insert(filter)
will insertfilter
if it wasn't already in the set; otherwise it will do nothing. So checking to see iffilter
is already in the set doesn't do anything for us here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there are no specialized optimizations or that
std::find
performs, and also believe that there is no advantage of usingstd::find
overstd::unordered_set::find
.This is just an oversight: This is a legacy code that was for when the type of
mFilters
wasstd::vector
, which should be removed asstd::unordered_set
is used now.