moveit / moveit_core

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
30 stars 76 forks source link

Fix fcl implementation so the fcl::CollisionObject's are not created every time a request is made. #295

Closed Levi-Armstrong closed 8 years ago

Levi-Armstrong commented 8 years ago

Every time a collision or distance request is made all of the fcl::CollisionObjects are recreated shown here. During the creation of the fcl::CollisionObject it computes the AABB of the passed geometry as shown here. This calculation is very expensive because it has to walk the mesh twice as shown here. The calculation of the geometries AABB is only required once and then it can easily be update by providing the TF using CollisionObject::setTransform followed by calling the CollisionObject::updateAABB which simply re-sizes the AABB based on the transform as shown here. It is also seen here that I am creating a copy of the cached fcl::CollisionObject which is to maintain thread safe.

I performed some baseline testing. The testing was performed using a joint interpolated planner and performing 100 planning requests using the same start and goal then taking the average of the time it took to calculate the trajectory where each performed a 100 collision checks. I performed this using both methods where I changes the meshing of a single object in the urdf to where the total triangle count was 10K, 500K and 10,000K. The performance improvements are shown below and they are significant.

Triangle Count (K) indigo-devel (sec) New Method (sec) % Improvement Speed Improvement
10 0.0129 0.00229 82.25% 5.6X
500 0.1459 0.00229 98.43% 63.7X
10,000 3.1449 0.00263 99.92% 1195.8X
Jmeyer1292 commented 8 years ago

This change allows us to actually take advantage of FCL's broad phase collision detection. We no longer pay a significant price for complex models that are not near collision during runtime. Profiling of our applications has shown many spend 99% of their time in collision checking. I encourage you to run any of your moveit applications that may take into account collisions under something like valgrind --tool=callgrind and see for yourself.

It would be great if the community at large could inspect this change, test it out, and lend their brains to the implications to related factors such as thread-safety.

I'm tagging maintainers: @isucan @mikeferguson @acornacorn and frequent contributor: @davetcoleman This may be relevant to your work as well.

davetcoleman commented 8 years ago

This is a great find! I will begin to test this on my end.

FYI the following link is not viewable for me: https://raesgit.datasys.swri.edu/larmstrong/moveit_core/blob/improveCollRobotIndigo/collision_detection_fcl/src/collision_robot_fcl.cpp#L90-L98

Levi-Armstrong commented 8 years ago

FYI the following link is not viewable for me: https://raesgit.datasys.swri.edu/larmstrong/moveit_core/blob/improveCollRobotIndigo/collision_detection_fcl/src/collision_robot_fcl.cpp#L90-L98

The link has been fixed.

davetcoleman commented 8 years ago

I just ran this using the default Hilgendorf (two UR5's and a base stand) with a simple wall obstacle and floor obstacle around it. I randomly sampled 10,000 joint configurations and asked MoveIt! if the state was collision free. I unfortunately only saw a 12.6% improvement in the speed of this task:

Before this commit: 21.0 sec After this commit: 18.4 sec

Is my significantly test different than yours? Note I'm compiled in debug mode and using FCL through OMPL.

hilgendorf_with_wall

Levi-Armstrong commented 8 years ago

The reason may be that the performance is driven by the number of triangles in the urdf collision geometries so the triangle count may be significantly lower and 10,000. You may try adding an object with around 500,000 triangles and you should see a big difference.

Jmeyer1292 commented 8 years ago

@davetcoleman I would also add that:

  1. Levi's numbers are sort of 'best case scenario' as they are mostly collision checks against objects whose bounding boxes do not intersect (Hence the flat cost of collision checks despite growing mesh size).
  2. All tests were done in Release mode. I doubt it makes a huge difference here, but making statements about the performance of code should always be done with optimization enabled.
Levi-Armstrong commented 8 years ago

@davetcoleman, I made the requested changes. I can also fixup this change so there is only one commit if you like.

davetcoleman commented 8 years ago

Since two of you are in agreement on the PR and since I have been running it successfully for a week I'll give it the green light, thanks!

Do you by chance have a re-usable benchmark setup for collision checking that we should save for future use? Not sure where the best place to put it would be but seems like a useful tool to have.

Also, do you mind sharing a summary of this great improvement on the mailing list to help us drum up support for MoveIt! development?

Levi-Armstrong commented 8 years ago

Do you by chance have a re-usable benchmark setup for collision checking that we should save for future use? Not sure where the best place to put it would be but seems like a useful tool to have.

There will be one available today or tomorrow as part of the industrial_moveit repository. This is where we have been make enhancements to moveit so we have a benchmarking package.

Levi-Armstrong commented 8 years ago

@davetcoleman Should I create a pull request against jade-devel or will you cherry pick it?

davetcoleman commented 8 years ago

I would welcome you to create a quick PR, I'm actively working on Docker images to make it easier for me to do things like cherry-pick