port of bullet collision to ros2 #322

jrgnicho · 2020-12-24T01:39:15Z

Description

Port of bullet collision module to ROS2

@henningkayser please review

AndyZe · 2021-01-12T16:00:39Z

I'll take a quick look at this since I've been poking around Bullet lately

AndyZe

I don't see anything wrong 👍 but haven't tested it

AndyZe · 2021-01-12T16:03:26Z

moveit_core/CMakeLists.txt

-# endif()
+ find_package(Bullet 2.87)
+
+ # TODO(j-petit): Version check can be dropped when Xenial reaches end-of-life


we're so close to the 16.04 EOL that you could probably drop this (April 2021)

henningkayser · 2021-01-05T10:10:09Z

moveit_core/package.xml

@@ -28,7 +28,7 @@
  <depend>assimp</depend>
  <depend>boost</depend>
  <depend>eigen</depend>
-  <depend>bullet</depend>
+  <depend>libbullet-dev</depend>


This doesn't seem to be available with rosdep, bullet however is. Were there any issues with the bullet dependency?

I think at some point I had an issue when I was building in eloquent but I no longer do in foxy

henningkayser · 2021-01-05T10:24:03Z

moveit_core/CMakeLists.txt

-# endif()
+ find_package(Bullet 2.87)
+
+ # TODO(j-petit): Version check can be dropped when Xenial reaches end-of-life


I think we can safely remove this check since bullet is available from Foxy/Focal.

Do you mean this entire section

set(BULLET_ENABLE "Bullet") set(BULLET_LIB "moveit_collision_detection_bullet") set(BULLET_INC "collision_detection_bullet/include") message(STATUS "Compiling with Bullet") else() message(STATUS "Version of Bullet too old or not available: disabling Bullet collision detection plugin. Try using Ubuntu 18.04 or later.") endif()

?

henningkayser · 2021-01-05T10:29:03Z

moveit_core/CMakeLists.txt

-# else()
-#   message(STATUS "Version of Bullet too old or not available: disabling Bullet collision detection plugin. Try using Ubuntu 18.04 or later.")
-# endif()
+ find_package(Bullet 2.87)


The current version in focal is 2.88 so this should be updated. We could probably also disable the check altogether.

I think in our meeting we agreed to leave it in this version (2.87). However let me know if you'd prefer I change this.

Actually, I think we can ignore the case that bullet is not available because MoveIt 2 doesn't support systems older than 20.04 and even with 18.04 bullet is available with the right version. Also, rosdep already installs bullet on the system since it's listed in the package.xml. I would simply add a REQUIRED to the find_package() and remove the BULLET_FOUND check altogether.

henningkayser · 2021-01-05T11:04:52Z

..._detection_bullet/include/moveit/collision_detection_bullet/bullet_integration/basic_types.h

@@ -33,6 +33,9 @@

 namespace collision_detection_bullet
 {
+
+static const rclcpp::Logger BULLET_LOGGER = rclcpp::get_logger("collision_detection.bullet");


It's not ideal to put static const instances in header files like this as it will produce logger instances for all compilation units. Better declare the logger and define a single global instance in a cpp file. We are still at C++14 so using extern would be the best solution for now. With C++17 we should switch to inline

This has been addressed. I couldn't use extern as I had to declare this global variable in each cpp file that needed it and so having extern would cause a multiple definition build error.

jrgnicho · 2021-01-19T21:10:03Z

@henningkayser I've made the requested change, please review

henningkayser

Looks pretty good to me code-wise. Were you already able to test this somehow?

henningkayser · 2021-01-26T13:36:49Z

moveit_core/CMakeLists.txt

-# else()
-#   message(STATUS "Version of Bullet too old or not available: disabling Bullet collision detection plugin. Try using Ubuntu 18.04 or later.")
-# endif()
+ find_package(Bullet 2.87)


Actually, I think we can ignore the case that bullet is not available because MoveIt 2 doesn't support systems older than 20.04 and even with 18.04 bullet is available with the right version. Also, rosdep already installs bullet on the system since it's listed in the package.xml. I would simply add a REQUIRED to the find_package() and remove the BULLET_FOUND check altogether.

henningkayser · 2021-01-26T13:38:31Z

moveit_core/collision_detection_bullet/CMakeLists.txt

+target_link_libraries(collision_detector_bullet_plugin
+  ${MOVEIT_LIB_NAME}
+  moveit_planning_scene
+)

 install(TARGETS ${MOVEIT_LIB_NAME} collision_detector_bullet_plugin


The plugin needs to be exported here for symlink support. See https://github.com/ros-planning/moveit2/pull/339/files#diff-049302fd4031b4efb956b186b90e32e634cc629a6a7740c3feb205b805881522

henningkayser · 2021-01-26T13:40:38Z

...detection_bullet/include/moveit/collision_detection_bullet/bullet_integration/bullet_utils.h

-    return false;
-  }
-}
+bool acmCheck(const std::string& body_1, const std::string& body_2,


I agree that this should be moved to the cpp files. @j-petit could you comment on any potential drawbacks with this change?

I think this is fine to move. I simply used the structure which was given in the old tesseract / bullet code.

henningkayser · 2021-01-26T13:41:42Z

moveit_core/collision_detection_bullet/src/bullet_integration/contact_checker_common.cpp

+ * POSSIBILITY OF SUCH DAMAGE.
+ *********************************************************************/
+
+/* Author: Jorge Nicho*/


Could you also list the original authors of these functions?

jrgnicho · 2021-01-26T16:21:00Z

Looks pretty good to me code-wise. Were you already able to test this somehow?

I haven't, Is there a unit test or demo program I can run?

jrgnicho · 2021-02-03T19:02:44Z

@henningkayser I've made the requested changes
Would you have any recommendations on how to test this change?

codecov · 2021-02-03T20:16:15Z

Codecov Report

Merging #322 (45b539c) into main (0eb366f) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   53.23%   53.23%   +0.01%     
==========================================
  Files         209      209              
  Lines       18849    18849              
==========================================
+ Hits        10032    10033       +1     
+ Misses       8817     8816       -1

Impacted Files	Coverage Δ
moveit_ros/moveit_servo/src/servo_calcs.cpp	`78.84% <0.00%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0eb366f...16f5666. Read the comment docs.

AndyZe · 2021-02-03T20:20:59Z

Sorry if this is too obvious but take a look at the Bullet tutorial from MoveIt1.

Bullet tutorial

Source code here:

source code

You should be able to take any MoveIt2 application and switch to Bullet like this, I believe:

planning_scene->setActiveCollisionDetector(collision_detection::CollisionDetectorAllocatorBullet::create());

jrgnicho · 2021-02-03T21:53:21Z

Sorry if this is too obvious but take a look at the Bullet tutorial from MoveIt1.

Bullet tutorial

Source code here:

source code

You should be able to take any MoveIt2 application and switch to Bullet like this, I believe:

planning_scene->setActiveCollisionDetector(collision_detection::CollisionDetectorAllocatorBullet::create());

OK, thanks for providing that link. Looks like I may have to port that example just to test it

jrgnicho · 2021-02-03T22:10:22Z

It looks like ament lint is coming back with an error. How do I run this locally? I tried the directions here with the .clang-tidy file in the moveit2 repo but it didn't find any changes to make.

AndyZe · 2021-02-03T22:56:41Z

OK, thanks for providing that link. Looks like I may have to port that example just to test it

I don't think you need to do that. Just find an existing example and set it to use Bullet. There must be some unit tests for FCL collisions you could modify?

jrgnicho · 2021-03-02T16:42:55Z

This just passed the CI checks

…p files

jrgnicho · 2021-03-09T20:38:47Z

@henningkayser looks like merging caused a build error, I just rebased and resolved the conflicts

jrgnicho · 2021-03-10T19:07:14Z

Did anyone get to test this?

jrgnicho · 2021-03-10T19:15:44Z

@henningkayser I'm trying to test using the bullet collision detector in the run_moveit_cpp demo program but I haven't been able to get it to find Bullet from the exports coming from the moveit_core package. I'm far from being a Cmake wizard so I could use some guidance on this, my branch with the demo can be found here
https://github.com/jrgnicho/moveit2/tree/test_bullet_collision_detector

* Fix to move_group list and change formatting of references to move_group to be consistent * Run pre-commit Co-authored-by: JafarAbdi <cafer.abdi@gmail.com>

AndyZe reviewed Jan 12, 2021

View reviewed changes

henningkayser reviewed Jan 13, 2021

View reviewed changes

henningkayser added this to In progress in Sprint 00 via automation Jan 19, 2021

jrgnicho force-pushed the bullet-collision-ros2-port branch from 3cbe341 to c10244d Compare January 19, 2021 20:51

jrgnicho force-pushed the bullet-collision-ros2-port branch from 12378cd to 92f66e0 Compare January 21, 2021 14:51

henningkayser reviewed Jan 26, 2021

View reviewed changes

henningkayser assigned henningkayser and unassigned henningkayser Jan 26, 2021

jrgnicho force-pushed the bullet-collision-ros2-port branch from 92f66e0 to 5854fec Compare February 3, 2021 19:01

henningkayser moved this from In progress to Review in progress in Sprint 00 Feb 23, 2021

jrgnicho force-pushed the bullet-collision-ros2-port branch from ce0c6eb to 45b539c Compare March 2, 2021 19:01

jrgnicho added 9 commits March 9, 2021 14:33

port of bullet collision to ros2

a24c8c6

moved implementation of some fuctions from the headers into source cp…

6e05c05

…p files

applied clang formatting

b72b273

Renamed BULLET to Bullet

af63b9d

reverted back to bullet instead of libbullet-dev dependency

87712bd

exports bullet plugin

a9406a4

removed Bullet Found if check

b4f91a9

added original author of the function to new cpp file

e04821e

fixed ament lint errors

bf98396

jrgnicho force-pushed the bullet-collision-ros2-port branch from f0bcf75 to bf98396 Compare March 9, 2021 20:37

Merge branch 'main' into bullet-collision-ros2-port

16f5666

henningkayser approved these changes Mar 10, 2021

View reviewed changes

Sprint 00 automation moved this from Review in progress to Reviewer approved Mar 10, 2021

henningkayser merged commit c47020c into moveit:main Mar 10, 2021

Sprint 00 automation moved this from Reviewer approved to Done Mar 10, 2021

henningkayser mentioned this pull request May 22, 2021

Re-enable Bullet, finalize Migration & Tests #473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port of bullet collision to ros2 #322

port of bullet collision to ros2 #322

jrgnicho commented Dec 24, 2020

AndyZe commented Jan 12, 2021

AndyZe left a comment

AndyZe Jan 12, 2021

henningkayser Jan 5, 2021

jrgnicho Jan 19, 2021

henningkayser Jan 5, 2021

jrgnicho Jan 19, 2021

henningkayser Jan 5, 2021

jrgnicho Jan 19, 2021

henningkayser Jan 26, 2021

henningkayser Jan 5, 2021

jrgnicho Jan 19, 2021

jrgnicho commented Jan 19, 2021

henningkayser left a comment

henningkayser Jan 26, 2021

henningkayser Jan 26, 2021

henningkayser Jan 26, 2021

j-petit Jan 26, 2021

henningkayser Jan 26, 2021

jrgnicho commented Jan 26, 2021 •

edited

Loading

jrgnicho commented Feb 3, 2021

codecov bot commented Feb 3, 2021 •

edited

Loading

AndyZe commented Feb 3, 2021

jrgnicho commented Feb 3, 2021

jrgnicho commented Feb 3, 2021

AndyZe commented Feb 3, 2021

jrgnicho commented Mar 2, 2021

jrgnicho commented Mar 9, 2021

jrgnicho commented Mar 10, 2021

jrgnicho commented Mar 10, 2021

port of bullet collision to ros2 #322

port of bullet collision to ros2 #322

Conversation

jrgnicho commented Dec 24, 2020

Description

AndyZe commented Jan 12, 2021

AndyZe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgnicho commented Jan 19, 2021

henningkayser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgnicho commented Jan 26, 2021 • edited Loading

jrgnicho commented Feb 3, 2021

codecov bot commented Feb 3, 2021 • edited Loading

Codecov Report

AndyZe commented Feb 3, 2021

jrgnicho commented Feb 3, 2021

jrgnicho commented Feb 3, 2021

AndyZe commented Feb 3, 2021

jrgnicho commented Mar 2, 2021

jrgnicho commented Mar 9, 2021

jrgnicho commented Mar 10, 2021

jrgnicho commented Mar 10, 2021

jrgnicho commented Jan 26, 2021 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading