Bug #699

PCL ROS Integration

Added by Rich Mattes over 2 years ago. Updated about 2 years ago.

Status:ClosedStart date:06/02/2012
Priority:NormalDue date:
Assignee:Radu B. Rusu% Done:

0%

Category:ROS
Target version:-

Description

This bug is to track some PCL-ROS integration issues brought up on the mailing list (http://www.pcl-developers.org/PCL-ROS-integration-questions-td5706762.html).

Currently, the pcl-wg repository on github contains ROS stack metadata and ROS messages that aren't included in the PCL tarball release. I propose that we include these extra files in the PCL source, and when the USE_ROS=ON CMake option is specified, PCL will install the ROS message definitions and stack manifest to PREFIX/share/pcl-1.x. It should probably also generate the message header files from the message definitions, and use/install them instead of the currently included message stubs.

By relying on the USE_ROS flag, standalone PCL operation should be unaffected, and when ROS is detected PCL can register itself as a ROS stack without effecting standalone users and without having to rely on a fork in github.

pcl-1.6-rosintegration.diff Magnifier - Patch to integrate ROS stack info into PCL (178 KB) Rich Mattes, 06/25/2012 06:31 pm

pcl-1.6-rosintegration-v2.diff Magnifier (37.5 KB) Rich Mattes, 06/26/2012 06:00 pm

History

#1 Updated by Radu B. Rusu over 2 years ago

  • Assignee changed from Michael Dixon to Alexandru-Eugen Ichim

Alex, let's see what we have to bring into the TGZ to make this happen.

#2 Updated by Radu B. Rusu over 2 years ago

Rich, sorry for the late reply. Can you please provide us a patch with the files that we need to include in the 1.6 TGZ? Thanks!

#3 Updated by Rich Mattes over 2 years ago

I'm working on it, but it's giving me some issues. The main problem is four include files currently found in common/include/pcl:

ModelCoefficients.h
PointIndices.h
PolygonMesh.h
Vertices.h

The new versions of these header files depend heavily on ROS, so they shouldn't be used unless the USE_ROS=ON option is set. I can't figure out an elegant way to swap them back and forth depending on whether or not USE_ROS is specified, any ideas?

I also brought this up on the ros-users mailing list to see if anyone else was working on it: http://code.ros.org/lurker/message/20120616.020117.92c8a898.en.html

#4 Updated by Radu B. Rusu over 2 years ago

  • Status changed from New to Resolved / Feedback
  • Assignee changed from Alexandru-Eugen Ichim to Radu B. Rusu

Rich, I have been following up the e-mail thread, but I don't know if we have found a fix for this problem. I'm more inclined to resume the 2.0 work that will fix all this once and for all. What do you think?

Thanks!

#5 Updated by Geoffrey Biggs about 2 years ago

You could use configure_file to easily swap in the correct version of the header. Name the alternatives something appropriate with an extension like ".in". In the CMakeLists.txt, based on USE_ROS, configure one or the other into the actual header file stored in the current binary directory. Ensure that directory is in the include path.

#6 Updated by Radu B. Rusu about 2 years ago

Rich, is that an acceptable solution?

#7 Updated by Rich Mattes about 2 years ago

Thanks for the suggestion Geoff, it looks like that's probably the path of least resistance. I've got it working so that the USE_ROS flag triggers which version of the five headers to use, and whether or not to install the ROS manifest and messages to PREFIX/share/pcl-x.y

I've attached an initial cut against svn trunk that seems to be working, though it needs to be cleaned up a little. If there is some alternate solution planned for 2.0, maybe it doesn't make sense to push this all into PCL just yet.

#8 Updated by Radu B. Rusu about 2 years ago

Geoff, can you look over it as well please?
Rich, is there a reason why we should copy the existing point_cloud.h into an additional point_cloud_noros.h rather than just using it as it is?

The way I see it is, some files should simply get included from ros/X.h instead of pcl/X.h, or am I missing something?

#9 Updated by Rich Mattes about 2 years ago

Actually we might not have to copy point_cloud.h. There was a one-line change between the two files:

// Copy the obvious
- properties.acquisition_time = pc.header.stamp;
+ properties.acquisition_time = pc.header.stamp.sec;
properties.sensor_origin = pc.sensor_origin_;//.head<3> ();

We might be able to make that change in-place, I didn't try it yet.

The headers could certainly be moved to a separate folder so as not to gunk up the include directory, all the places they're referenced would just need to change.

#10 Updated by Jack O'Quin about 2 years ago

For maintainability, I would like to see a much smaller version of this patch.

Perhaps the ROS message include headers could be added to the -I search path ahead of the corresponding built-in PCL versions, using some kind of cmake magic.

If there are a few places in common code where the references need to change, an #ifdef USE_ROS around that would be much cleaner and more maintainable than copying a large header to make small changes.

Does this patch apply to the main PCL trunk? I might try some simplification experiments.

#11 Updated by Radu B. Rusu about 2 years ago

I agree with Jack. Please let me know how I can help speed this up. I believe we are going to release 1.6 right as soon as we fix this.

#12 Updated by Rich Mattes about 2 years ago

I tried to do this by inserting a different include path at first, but I kept running into issues with the module dependency system in pcl_targets.cmake, in that it only includes a single path for every module. So then I tried to make the ROS headers a separate module and have common depend on it, which worked for common but broke everything that depended on common. There isn't any module inheritance, so every place with a dependency on "common" also needed a dependency on the "ros" module I added before common.

After thinking about it for a bit longer this morning, I think that we can leave the .noros headers as they were before, and in the case where ROS is detected, we can use configure_file to put new headers in the binary directory. Then all we have to do is make sure include_directories(BINARY_DIR/modulename/include) comes before include_directories(SOURCE_DIR/modulename/include) in pcl_targets.cmake and common/CMakeLists.txt. This allows for optional overriding of headers via generated files in any module, and doesn't have any effect when ROS isn't used.

If nobody gets to it, I will try to do this tonight, and also try to make ros/manifest.xml use configure_file to fill in the correct .pc file names (they're all hardcoded to pcl-1.5 at the moment.)

#13 Updated by Jack O'Quin about 2 years ago

Perhaps this is naive (I just looked at the PCL build for the first time in quite a while).

I prefer USE_ROS to add already-installed ROS headers as a dependency, like is done automatically for `eigen` and `boost`. Then, pkg-config would resolve the CFLAGS for `sensor_msgs` and `std_msgs` in the normal way, without needing an additional PCL `ros` module include dependency.

That makes ROS common_msgs a PCL dependency, but only when building with `USE_ROS=1`. The `pcl_common` versions of those headers would be included when USE_ROS is false.

The biggest problem I see (so far) is the difference between the `stamp` field in PCL and ROS versions of `std_msgs/Header.h`. That suggests some `#ifdef USE_ROS` logic in `pcl_common`. Ugly, but better than duplicating entire headers, I think.

#14 Updated by Rich Mattes about 2 years ago

That's basically what the USE_ROS flag was doing: when it's off, the PCL included std_msgs and sensor_msgs were used, and when it's on the ROS messages are used. All I'm doing is expanding it to handle the four point cloud messages as well. I'm trying to keep away from inserting #ifdefs by using header substitution.

I've come up with a cleaner version of the patch, it just adds a pcl_msgs subfolder which holds the messages that need ROS. When USE_ROS is enabled, they get copied to the binary install dir and override the non-ros stubs through include path priority. I also added a time struct that mimics the ros::Time to the PCL Header.h so that point_cloud.h doesn't need to be modified with #ifdefs.

#15 Updated by Jack O'Quin about 2 years ago

That is not at all what I was suggesting.

I don't want to copy ROS headers to PCL and install them there. I want PCL to use the ROS headers that are already installed on the system as a dependency.

But, I don't understand your approach, either. Why do we need to pre-process those four header files? I can't tell what is being done to them.

#16 Updated by Rich Mattes about 2 years ago

Which ROS headers are you talking about? The std_msgs and sensor_msgs that PCL includes in common/include are only used when ROS isn't present; when ROS is installed on the system, PCL ignores its included std_msgs and sensor_msgs stubs, and uses the ones ROS provides.

The four pointcloud messages, ModelCoefficients.h, PointIndices.h, PolygonMesh.h, and Vertices.h, don't exist anywhere in ROS except for in the wg-debs github repository at https://github.com/wg-debs/pcl. PCL includes its own stubs that stand in when ROS isn't around. I'm trying to make it so that PCL includes the headers from wg-debs when ROS is detected, and when ROS isn't around does exactly what it did before in stand-alone mode. This necessitates the copying of the four message headers from github that were created by the ROS message generation utilities from the .msg files.

The pre-processing isn't doing anything but copying the message headers that depend on ROS to a place where it's easy for the buildsystem to find them. The way PCL's internal dependency system is set up makes it difficult to include additional arbitrary include paths for other modules to include. I'm getting around this by mirroring the include path from the source directories in the build directory, and including the build directory's include path before the source directory's include path. That way, if the headers get copied by configure_file to the build directory, they'll override the stubs in the source tree.

Hope this all makes sense; if there's a better way to integrate the changes from https://github.com/wg-debs/pcl so that the official PCL can be built stand-alone or with ROS without modification then I haven't figured it out.

#17 Updated by Jack O'Quin about 2 years ago

Thanks for the detailed explanation, Rich.

  • I was confused by the sensor_msgs and std_msgs issue. As you say, those headers are already handled reasonably well.
  • The big problem is how to generate ROS versions of the four messages that PCL defines and ROS does not (ModelCoefficients.h, PointIndices.h, PolygonMesh.h, and Vertices.h).

Now I understand why wg-debs keeps these changes separate from the PCL sources. Their solution is just too messy to include with the base sources. I suppose a better PCL 1.x solution could probably be developed using the ROS message generation scripts within the PCL build. The `genmsg_cpp` script is available on pypi now.

But, I don't favor pushing that into the PCL 1.6 sources, which are otherwise ready to release. So, I agree with Radu, who recommended pushing ahead with PCL 2.0 as a way of fixing this problem.

Thanks for all your efforts to help straighten this mess out. We will fix it, but maybe not in 1.6.

#18 Updated by Radu B. Rusu about 2 years ago

  • Target version changed from 1.6 to 1.7

Thanks a lot for all the work Rich, Jack. I'll keep this issue open for now and bump it to 1.7, just in case we find another trick. However, we would really hope that after 1.6 we'll shoot directly for 2.0, as there are other changes that we want to inflict on our code base :)

#19 Updated by Radu B. Rusu about 2 years ago

  • Status changed from Resolved / Feedback to Closed
  • Target version deleted (1.7)

Closing this as there is no follow up for the 1.x series. We learned how and what to do next, so we'll keep this in mind for the next release (2.0). Thanks all!

Also available in: Atom PDF