Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mesh Filter tutorial with UR5 and Kinect simulated in Gazebo #558

Merged
merged 20 commits into from
May 4, 2021

Conversation

Abishalini
Copy link
Contributor

@Abishalini Abishalini commented Nov 20, 2020

Description

MeshFilter
Mesh Filter tutorial for Melodic.
The mesh filter takes in a point cloud, tf of robot, and a URDF and publishes a modified point cloud with the point cloud subtracted that overlaps with the current robot state.
A separate Gazebo tutorial still needs to be made (Gazebo setup for this tutorial does not depend on ARM_moveit_config's auto-generated files)

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@welcome
Copy link

welcome bot commented Nov 20, 2020

Thanks for helping in improving MoveIt and open source robotics!

@JafarAbdi JafarAbdi self-assigned this Nov 20, 2020
@Abishalini Abishalini changed the title Add sensor Mesh Filter tutorial with UR5 and Kinect simulated in Gazebo Nov 20, 2020
@JafarAbdi JafarAbdi changed the base branch from melodic-devel to master November 20, 2020 17:18
Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

Thank for working on this Abi!

doc/mesh_filter/launch/test_gazebo.launch Outdated Show resolved Hide resolved
doc/mesh_filter/mesh_filter_tutorial.rst Outdated Show resolved Hide resolved
doc/mesh_filter/meshes/hokuyo.dae Outdated Show resolved Hide resolved
Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

When I try to test this my console is flooded with this error:

ros.moveit_ros_perception: TF Problem: Invalid argument passed to lookupTransform argument target_frame in tf2 frame_ids cannot be empty

Also, I can't seem to get the visualization of the point cloud working in rviz like it shows in the images and gif in the tutorial.

doc/mesh_filter/mesh_filter_tutorial.rst Outdated Show resolved Hide resolved
doc/mesh_filter/launch/test_gazebo.launch Outdated Show resolved Hide resolved
@tylerjw
Copy link
Member

tylerjw commented Jan 5, 2021

Here is my vcs status from the src directory in case it differs from yours:

$ vcs status
.........
=== ./geometric_shapes (git) ===
On branch noetic-devel
Your branch is up to date with 'origin/noetic-devel'.

nothing to commit, working tree clean
=== ./moveit (git) ===
On branch pr-fix_mesh_filter
Your branch is up to date with 'jafar/pr-fix_mesh_filter'.

nothing to commit, working tree clean
=== ./moveit_msgs (git) ===
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
=== ./moveit_resources (git) ===
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
=== ./moveit_tutorials (git) ===
On branch add_sensor
nothing to commit, working tree clean
=== ./moveit_visual_tools (git) ===
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
=== ./panda_moveit_config (git) ===
On branch melodic-devel
Your branch is up to date with 'origin/melodic-devel'.

nothing to commit, working tree clean
=== ./rviz_visual_tools (git) ===
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
=== ./universal_robot (git) ===
On branch melodic-devel
Your branch is up to date with 'origin/melodic-devel'.

nothing to commit, working tree clean

Here is my catkin config:

$ catkin config
-------------------------------------------------------------------------------------------------
Profile:                     default
Extending:        [explicit] /opt/ros/melodic
Workspace:                   /home/tyler/code/moveit/ws_moveit
-------------------------------------------------------------------------------------------------
Build Space:        [exists] /home/tyler/code/moveit/ws_moveit/build
Devel Space:        [exists] /home/tyler/code/moveit/ws_moveit/devel
Install Space:      [unused] /home/tyler/code/moveit/ws_moveit/install
Log Space:          [exists] /home/tyler/code/moveit/ws_moveit/logs
Source Space:       [exists] /home/tyler/code/moveit/ws_moveit/src
DESTDIR:            [unused] None
-------------------------------------------------------------------------------------------------
Devel Space Layout:          linked
Install Space Layout:        None
-------------------------------------------------------------------------------------------------
Additional CMake Args:       -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo
Additional Make Args:        None
Additional catkin Make Args: None
Internal Make Job Server:    True
Cache Job Environments:      False
-------------------------------------------------------------------------------------------------
Whitelisted Packages:        None
Blacklisted Packages:        None
-------------------------------------------------------------------------------------------------
Workspace configuration appears valid.
-------------------------------------------------------------------------------------------------

@davetcoleman
Copy link
Member

@Abishalini what's the status of this PR?

@Abishalini Abishalini requested a review from JStech March 10, 2021 17:50
@JStech
Copy link
Contributor

JStech commented Mar 11, 2021

When I try to test this my console is flooded with this error:

ros.moveit_ros_perception: TF Problem: Invalid argument passed to lookupTransform argument target_frame in tf2 frame_ids cannot be empty

It took me a bit to track this down, but that's because the filter initializes the input optical frame with the empty string, and it doesn't get updated until it receives a depth map. However, it doesn't subscribe to the depth input until its output has a subscriber. So once you subscribe to /filtered/depth or /model/depth, the spam stops.

So, I don't think this is an error in this tutorial, but just undesirable behavior in the mesh filter itself. I'll look into it a bit more and see if I can fix it easily, otherwise I think we should just throttle that error.

Copy link
Contributor

@JStech JStech left a comment

Choose a reason for hiding this comment

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

Looks good! I mostly made some formatting suggestions.

The flood of transform errors is a MoveIt issue, and so I don't think you should work around it here--but thanks for trying!

I was able to build and run this as advertised. The necessary MoveIt PR has merged, so after removing the extra subscriber, I think this is good to go.

doc/mesh_filter/CMakeLists.txt Outdated Show resolved Hide resolved
doc/mesh_filter/launch/mesh_filter.launch Outdated Show resolved Hide resolved
doc/mesh_filter/launch/test_gazebo.launch Outdated Show resolved Hide resolved
doc/mesh_filter/mesh_filter_tutorial.rst Outdated Show resolved Hide resolved
doc/mesh_filter/mesh_filter_tutorial.rst Outdated Show resolved Hide resolved
doc/mesh_filter/mesh_filter_tutorial.rst Outdated Show resolved Hide resolved
doc/mesh_filter/mesh_filter_tutorial.rst Outdated Show resolved Hide resolved
doc/mesh_filter/mesh_filter_tutorial.rst Outdated Show resolved Hide resolved
doc/mesh_filter/src/filtered_depth_subscriber.cpp Outdated Show resolved Hide resolved
@davetcoleman
Copy link
Member

@Abishalini could we get the final fixes for this to be merged in? This feature came up in a meeting today.

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

I added another review.

Because the whole perception pipeline uses the mesh_filter, there is a slight overlap there.
Maybe you could add a sentence to the perception pipeline tutorial saying that the mesh_filter (link to this new tutorial) will already exclude the robot model from the processed point data.

The flood of transform errors is a MoveIt issue, and so I don't think you should work around it here--but thanks for trying!
... The necessary MoveIt PR has merged

Which PR? Are the mentioned error messages gone?

@@ -0,0 +1,39 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing in this file refers to UR. Also it's not just "sensor", but a camera model.
How about calling the file kinect_camera.gazebo?

why do you even specify the camera model in a separate file at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this file


<node pkg="moveit_tutorials" type="filtered_depth_subscriber_node" name="filtered_depth_subscriber_node" output="screen"/>

<include file="$(find moveit_tutorials)/doc/mesh_filter/launch/test_gazebo.launch"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

includes between launch files in the tutorials do not work because the files are installed in different locations between devel and install workspaces. I found the same issue in another tutorial yesterday.

Given that there is no other reference to test_gazebo.launch, I would just include its content here.

Copy link
Contributor

@JStech JStech Apr 29, 2021

Choose a reason for hiding this comment

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

Would $(dirname)/test_gazebo.launch work? I've not had a reason to use dirname before but it seems applicable here. Ref: bottom of this section

Copy link
Contributor

Choose a reason for hiding this comment

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

Would $(dirname)/test_gazebo.launch work?

Yes! I wonder how I managed to miss the introduction of that feature!
Last time I checked this website was before kinetic 😄 .
Thanks for pointing it out. I have to rewrite so many launch files now....

With this we can keep the separate files, but I would still propose to change the name. This is no test.
Something like ur5_gazebo.launch would work for me.

Also, I'm still no fan of using a different robot for this tutorial (it adds another dependency that will break at some point), but I don't know whether the panda gazebo simulation actually works at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chnaged the name of the launch file to ur5_gazebo.launch

doc/mesh_filter/launch/test_gazebo.launch Outdated Show resolved Hide resolved
doc/mesh_filter/CMakeLists.txt Outdated Show resolved Hide resolved
@Abishalini
Copy link
Contributor Author

Abishalini commented Apr 29, 2021

The error messages mentioned by @tylerjw was resolved in PR moveit/moveit#2550

@v4hn
Copy link
Contributor

v4hn commented Apr 29, 2021

@Abishalini I believe you messed up your PR branch. There are changes all over the place now.

@v4hn
Copy link
Contributor

v4hn commented Apr 29, 2021

It's a bit hard to see at the moment because of all the unrelated commits right now, but I believe you also did not add the new package dependencies to the package.xml yet.
That is necessary assuming we can't use the panda for this tutorial for some reason (I can imagine a few...).

install(DIRECTORY launch DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
install(DIRECTORY meshes DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
install(DIRECTORY urdf DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
install(DIRECTORY rviz DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the current layout all things except for the launch files should be installed to ${CATKIN_PACKAGE_SHARE_DESTINATION}/doc/<tutorial>.
So you don't have to do this weird dance with the additional parameter for the mesh in the xacro.

In theory we could also move all the launch files to this subfolder, but users likely expect them all in one place, so we don't.

Sorry for the confusion. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tutorial now works for both devel and install workspaces.

@v4hn v4hn mentioned this pull request May 3, 2021
Copy link
Contributor

@JStech JStech left a comment

Choose a reason for hiding this comment

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

LGTM. There are a few files that don't have final newlines in them, if you're looking for one more thing to change :-P

install(DIRECTORY launch DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
install(DIRECTORY meshes DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
install(DIRECTORY urdf DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
install(DIRECTORY rviz DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the install folder of moveit_tutorials looks like this:

❯ ls install/share/moveit_tutorials/
cmake  doc  launch  package.xml

where the doc folder already contains subfolders for some tutorials with auxiliary data.
I would propose to leave meshes, urdf and rviz files there as well, only launch files are grouped together in one folder.
If you want, I can do these changes myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated the changes you recommended

<!-- The universal robot package is not released for melodic -->
<!-- Make sure to clone the package (melodic-devel branch) onto your workspace -->
<!-- <run_depend>ur_gazebo</run_time> -->
<!-- <run_depend>ur_description</run_time> -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you comment out the run_depends?
If the packages are in the same workspace, these should be fine, right?
And if they aren't then rosdep tells you exactly the same as the comments above 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding those dependencies fails CI

Copy link
Contributor

Choose a reason for hiding this comment

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

This has to be resolved either way, but I'll merge this now and create a corresponding issue.

@v4hn
Copy link
Contributor

v4hn commented May 4, 2021

Thanks for your perseverance. 💐
I'm sure you hate me by now. :-)

@v4hn v4hn merged commit 13c4fc1 into moveit:master May 4, 2021
@welcome
Copy link

welcome bot commented May 4, 2021

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

@davetcoleman
Copy link
Member

Hurray! 🥇

@Abishalini Abishalini deleted the add_sensor branch May 11, 2021 18:53
@v4hn
Copy link
Contributor

v4hn commented Aug 6, 2021

Hurray! 🥇

Not really anymore by now. This patch added gazebo_ros to our MoveIt dependencies which takes forever to download and is a mountain of data to install in our CI for no merit 😿
My main criticism here is that this is the only place in the whole MoveIt rosinstall workspace where gazebo is required and that it hardly justifies the overhead...

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.

None yet

7 participants