Multi threaded collision checking test #1657
Conversation
|
I moved the test into the |
BryceStevenWilley
added
the
awaits 2nd review
|
Cool, this is one area where FCL and Bullet differ, no? I seem to remember that Bullet has some mutable members / persistent state that would make it not thread-safe. This PR looks fine, but making explicit the limitations of Bullet in the documentation somewhere is helpful and doesn't take away from all the work you have done. |
|
Yes, the current Bullet collision checking can not be used multi-threaded in the same way as FCL because of the mutable collision managers. I added a comment in the bullet readme, see this commit. |
| threads[i]->join(); | ||
| delete threads[i]; | ||
| } | ||
| ASSERT_TRUE(true); |
There was a problem hiding this comment.
Is this a critical test case? ;)
There was a problem hiding this comment.
wups, leftover from the commit before. Removed now.
There was a problem hiding this comment.
@j-petit it's still there?
There was a problem hiding this comment.
Sorry, I forgot to push I think.. Now it's definitely fixed, I double checked :)
|
@j-petit looks good, +1 after comments are resolved |
* Moved test into planning_scene * Assert for each collision check * Fixed includes
j-petit
force-pushed
the
multi-threaded-collision-test
branch
from
39a39c5
to
2b11446
Compare
|
Ping, can be merged? |
| planning_scene::PlanningScenePtr planning_scene_; | ||
| }; | ||
|
|
||
| TEST_F(CollisionDetectorThreadedTest, InitOk) |
There was a problem hiding this comment.
To ensure that the robot_model ist valid, it doesn't make sense to define a separate test like this, as each test runs from its own test fixture instance. From the official docs:
For each test defined with TEST_F(), googletest will create a fresh test fixture at runtime, immediately initialize it via SetUp(), run the test, clean up by calling TearDown(), and then delete the test fixture. Note that different tests in the same test suite have different test fixture objects, and googletest always deletes a test fixture before it creates the next one. googletest does not reuse the same test fixture for multiple tests. Any changes one test makes to the fixture do not affect other tests.
There was a problem hiding this comment.
But to ensure that in general this works? I assumed this to be good practice as this is done in all collision test files which existed like here
There was a problem hiding this comment.
It doesn't make sense there as well. Better it would be to ASSERT(robot_model_) in Setup().
There was a problem hiding this comment.
I made a PR #1777 to fix this. For now, just for this test as the other's will be changed to the templated version when feature-bullet is merged.
Description
This adds a unit test for multi-threaded collision checking using FCL.
Checklist