Skip to content

Using Bullet as a Collision Checker#337

Merged
AndyZe merged 9 commits intomoveit:masterfrom
j-petit:adding_collision_checker
Nov 19, 2020
Merged

Using Bullet as a Collision Checker#337
AndyZe merged 9 commits intomoveit:masterfrom
j-petit:adding_collision_checker

Conversation

@j-petit
Copy link
Copy Markdown
Contributor

@j-petit j-petit commented Jun 14, 2019

Description

This tutorial demonstrate how to use Bullet as a collision checker and is part of the GSoC 2019. It shows normal collision detection as well as continuous collision detection. I am using a lot of code from the Visualizing Collisions tutorial to have the same interactive robot setup with the visualized collisions.

This does not build and pass yet, as the feature branch has to be merged into master first.

@welcome
Copy link
Copy Markdown

welcome Bot commented Jun 14, 2019

Thanks for helping in improving MoveIt!

Comment thread doc/adding_collision_detector/adding_collision_checker.rst Outdated
Comment thread doc/adding_collision_detector/src/adding_collision_checker_tutorial.cpp Outdated
@rhaschke rhaschke changed the title Adding a new collision checker [WIP] [WIP] Adding a new collision checker Aug 9, 2019
@j-petit j-petit changed the title [WIP] Adding a new collision checker [WIP] Bullet as collision checker Aug 20, 2019
@j-petit j-petit changed the title [WIP] Bullet as collision checker [WIP] Using Bullet as a collision checker Aug 20, 2019
@j-petit j-petit force-pushed the adding_collision_checker branch from c9618f9 to 3ca2e43 Compare August 28, 2019 09:59
@j-petit j-petit changed the title [WIP] Using Bullet as a collision checker Using Bullet as a Collision Checker Aug 28, 2019
@j-petit j-petit force-pushed the adding_collision_checker branch from 3ca2e43 to 6feb67b Compare August 28, 2019 10:05
Comment thread doc/bullet_collision_checker/src/bullet_collision_checker_tutorial.cpp Outdated
moveit_visual_tools::MoveItVisualTools visual_tools("panda_link0");
ros::Publisher robot_state_publisher(node_handle.advertise<moveit_msgs::DisplayRobotState>("interactive_robot_state", 1));
ros::Publisher robot_state_publisher_2(node_handle.advertise<moveit_msgs::DisplayRobotState>("robot_state_before", 1));
ros::Publisher world_state_publisher(node_handle.advertise<visualization_msgs::Marker>("interactive_robot_markers", 100));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a Marker publisher is already included in MoveItVisualTools (see rviz_visual_tools, the parent class), so you should not need this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I use the one from MoveItVisualTools, then all topic names get prefixed by bullet_collision_detection which means that I can't use the same topic as the interactive robot uses. This means that I can't show both markers / robot states through the same visualization because they need different topics.

spinner.start();

moveit_visual_tools::MoveItVisualTools visual_tools("panda_link0");
ros::Publisher robot_state_publisher(node_handle.advertise<moveit_msgs::DisplayRobotState>("interactive_robot_state", 1));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a DisplayRobotState publisher is already included in MoveItVisualTools, so you should not need this unless you need three of those publishers...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above comment

Comment thread doc/bullet_collision_checker/src/bullet_collision_checker_tutorial.cpp Outdated
Comment thread doc/bullet_collision_checker/src/bullet_collision_checker_tutorial.cpp Outdated
Comment thread doc/bullet_collision_checker/src/bullet_collision_checker_tutorial.cpp Outdated
Copy link
Copy Markdown
Contributor

@mamoll mamoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some small suggestions to use std::make_shared in two places. There may be more places you could do this. See https://stackoverflow.com/questions/20895648/difference-in-make-shared-and-normal-shared-ptr-in-c on detailed info on why this is generally a good idea.

Comment thread doc/bullet_collision_checker/src/bullet_collision_checker_tutorial.cpp Outdated
Comment thread doc/bullet_collision_checker/src/bullet_collision_checker_tutorial.cpp Outdated
@j-petit j-petit force-pushed the adding_collision_checker branch from 6feb67b to 2cf6bfc Compare November 10, 2019 20:32
@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 10, 2019

I addressed the review comments. However, this PR still requires Bullet to be merged via the feature branch before.

@mamoll
Copy link
Copy Markdown
Contributor

mamoll commented Nov 12, 2019

Which PRs have to be merged in the moveit repo before this PR can be merged?

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 12, 2019

The feature branch feature-bullet-trajopt into master.

@mamoll
Copy link
Copy Markdown
Contributor

mamoll commented Nov 12, 2019

Can you create a PR for that? Or do your existing PRs contain the same commits?

// BEGIN_TUTORIAL
// The code starts with creating an interactive robot and a new planning scene.
InteractiveRobot interactive_robot;
g_planning_scene = new planning_scene::PlanningScene(interactive_robot.robotModel());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use raw pointers here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either use smart pointer or give a reason why we are using raw pointers here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adapted the code from the visualizing tutorial for this tutorial and as it uses raw pointers I thought it would be fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced all raw pointers with smart ones.

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 16, 2019

Can you create a PR for that? Or do your existing PRs contain the same commits?

We first have to discuss how we deal with the fact that the Bullet version available through apt in Xenial is outdated and does not support the collision checking before we can do any merging. See this issue. I added this topic to the agenda for the maintainer meeting next month.

@v4hn
Copy link
Copy Markdown
Contributor

v4hn commented Nov 18, 2019

We first have to discuss how we deal with the fact that the Bullet version available through apt in Xenial is outdated

In the issue you linked, the proposed solution was to check the bullet versions at compile time and just disable the feature if bullet is too old. Personally, I don't see the problem with that.

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 18, 2019

So this means enabling/disabling this in the cmake files right? Then I can provide a PR for this.

@mamoll
Copy link
Copy Markdown
Contributor

mamoll commented Nov 18, 2019

So this means enabling/disabling this in the cmake files right? Then I can provide a PR for this.

Yes, typically, you'd have something like this:

find_package(bullet 2.88)
set(MOVEIT_HAVE_BULLET bullet_FOUND)
# this file contains: \cmakedefine01 MOVEIT_HAVE_BULLET
configure_file(some_header.h.in some_header.h)

If Bullet doesn't include cmake config files or doesn't support version number checking, you have to somehow get the version some other way.

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 18, 2019 via email

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 20, 2019

Bullet does set ( BULLET_VERSION_STRING "2.87" ) in the BulletConfig.cmake. Do I get this correctly that this is not compatible with the cmake find_package(Bullet [version]) command? When I do this, it does not matter which number I put in the version. I am not so experienced with setting up a build system with cmake.

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 20, 2019

All further discussions concerning Bullet and cmake happening here.

@felixvd
Copy link
Copy Markdown
Contributor

felixvd commented Sep 11, 2020

Would be too bad if this was left by the roadside. It has had a birthday already. Do you think you can give this another push @j-petit ?

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Sep 11, 2020

wups, completely missed that this is still pending. Thanks for reminding me Felix! I'll try fixing those loose ends in the near future. As I am not working in the ROS environment anymore, it's hard to get started again...

@j-petit j-petit force-pushed the adding_collision_checker branch 2 times, most recently from b3a4fe0 to ef67cfb Compare September 29, 2020 16:57
@j-petit j-petit closed this Oct 3, 2020
@j-petit j-petit reopened this Oct 3, 2020
@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Oct 3, 2020

So, this is finally ready to be merged from my side. ping @felixvd, @mamoll

Copy link
Copy Markdown
Contributor

@mamoll mamoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Let's get this merged (finally!).

@j-petit j-petit force-pushed the adding_collision_checker branch from 01fbcc3 to 415a87b Compare October 24, 2020 09:44
@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Oct 25, 2020

Fixed Travis, can we get this merged, @mamoll ?

@mamoll
Copy link
Copy Markdown
Contributor

mamoll commented Oct 25, 2020

Fixed Travis, can we get this merged, @mamoll ?

IMO, yes. I'd like to get the approval from anther maintainer. @davetcoleman or @felixvd, perhaps?

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 2, 2020

Or @mlautman ?

@felixvd
Copy link
Copy Markdown
Contributor

felixvd commented Nov 2, 2020

I would like to actually test it before I approve it, but I probably won't get around to it before ROS World :(

@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 2, 2020

That's fine, I think it would be great if someone is actually testing it beforehand!

@AndyZe
Copy link
Copy Markdown
Member

AndyZe commented Nov 19, 2020

As you know, I've started using Bullet recently. It's great so far!

I downloaded the tutorial, built the webpage, and ran it. It all looks good. Let's finally get this merged.

@AndyZe AndyZe merged commit 7e6ca05 into moveit:master Nov 19, 2020
@j-petit
Copy link
Copy Markdown
Contributor Author

j-petit commented Nov 20, 2020

Thanks Andy!

@j-petit j-petit deleted the adding_collision_checker branch November 20, 2020 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants