[Citadel][TPE] Implement collision filtering using collide bitmasks and add CollisionFilterMaskFeature #69
Conversation
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
iche033
changed the title
Codecov Report
@@ Coverage Diff @@
## ign-physics2 #69 +/- ##
================================================
+ Coverage 82.46% 82.74% +0.27%
================================================
Files 106 106
Lines 3587 3645 +58
================================================
+ Hits 2958 3016 +58
Misses 629 629
Continue to review full report at Codecov.
|
tpe/lib/src/CollisionDetector.cc
Outdated
| // cache of collide bitmasks | ||
| std::map<std::size_t, uint16_t> collideBitmasks; | ||
|
|
There was a problem hiding this comment.
A few comments about this.
First of all you can probably see coming my "usual" feedback about using hash maps, since we are also doing quite a lot of operations on this map (number of models in the simulation squared).
But I am also not too sure about using a cache in the first place, from what I can see we are trying to avoid the overhead of iterating over the children in the Entity::GetCollideBitmask() function, what do you think about calculating / caching the entity bitmask in the Entity object in the first place?
Also small comment in the worldAabb map, if it's not out of the scope of the PR I would still suggest changing to a hash map to save a logn binary search in every find() and insert operation. All the function calls should be unchanged since the API is quite similar.
There was a problem hiding this comment.
ok yeah I think the cache for collide bitmask is probably premature optimization so I just removed it. The GetCollideBitmask function is just doing bitwise OR operations which should be fast. Updated worldAabb to use unordered_map. 1555caa
There was a problem hiding this comment.
I'm still quite interested in the idea of caching the collide bitmask at the entity level (i.e. whenever a "child" is added / removed recalculate the bitmask and make the Get function be a simple property getter), do you think that would be a good approach? Otherwise this looks good to me!
There was a problem hiding this comment.
I tested the caching approach and ended up adding more code than I had planned so I created a separate pull request for it, #73. Feedback welcome.
There was a problem hiding this comment.
ok merged #73 into this PR
| // set collide bitmask | ||
| uint16_t collideBitmask = 0xFF; | ||
| if (_sdfCollision.Element()) | ||
| { | ||
| // TODO(anyone) add category_bitmask as well | ||
| auto elem = _sdfCollision.Element(); | ||
| if (elem->HasElement("surface")) | ||
| { | ||
| elem = elem->GetElement("surface"); | ||
| if (elem->HasElement("contact")) | ||
| { | ||
| elem = elem->GetElement("contact"); | ||
| if (elem->HasElement("collide_bitmask")) | ||
| { | ||
| collideBitmask = elem->Get<unsigned int>("collide_bitmask"); | ||
| this->SetCollisionFilterMask(collisionIdentity, collideBitmask); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Just a note on this (you are probably aware of it already), the original implementation in the dart SDFFeatures was done before the availability of optional features and the ign-gazebo PR that takes care of this logic using the SDF Surface DOM. Admittedly when I did that PR I didn't double check that it could be compatible with Citadel, if that's the case we could probably backport it and get rid of this logic?
There was a problem hiding this comment.
I backported the collide bitmask changes to citadel in gazebosim/gz-sim#223
I just noticed that if the logic here is removed, ign-physics alone won't be able to consume the collide bitmasks flags from sdf as it will now rely on ign-gazebo to set these flags, This causes SimulationFeature_TEST.CollideBitmasks to fail unless we write extra logic in the integration test to set them. What do you think about leaving the logic here for ign-physics users that do not use ign-gazebo? I think it'll just be overriden by ign-gazebo as it parses SDF surface DOMs?
There was a problem hiding this comment.
Admittedly I don't know much about use cases for ignition libraries (such as physics, or rendering) without ignition gazebo, but no strong feelings here we can leave it if it helps with testing.
There was a problem hiding this comment.
ok yeah let's leave it in there for testing purposes.
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
iche033
force-pushed
the
tpe_collide_bitmask
branch
from
d753f1c to
82578e9
Compare
|
fixed windows build in 82578e9 |
Added collide bitmask to Collisions in tpelib for doing collision filtering.
Since collision detection in TPE is done between models (not collisions), I also added a function to Entity to get a single combined (bitwise OR'd) collide bitmask from all child collisions. e.g. if one collision within model A can collide with any one of the collisions in model B then the two models should collide.
Also added the CollisionFilterMaskFeature similar to the way it was done for dart in this pull reqeuest