Remove ros

[jspricke]

Advanced version of #110

[sdmiller]

Nice, you'll need to teach me what git-fu you used to clean the commits like that, offline. Should we make a Wiki explaining the switch, along with the pcl_ros conversions + a migration script?

As for the build, I'm getting the same VTK error that I could have sworn was fixed, but can't find the pull request. image_viewer.h is missing an #include <vtkImageSlice.h>, and will break on 5.10 without it.

[sdmiller]

Nevermind, I wasn't on HEAD. Looks good!

[jspricke]

• Stephen Miller [email protected] [2013-07-03 17:43]:

  Nice, you'll need to teach me what git-fu you used to clean the commits like that, offline.

Mostly git rebase -i and git commit --amend :).

Should we make a Wiki explaining the switch, along with the pcl_ros conversions + a migration script?

Here is a first draft:
https://github.com/PointCloudLibrary/pcl/wiki/Remove-ROS

Cheers Jochen

[sdmiller]

Small fix: you'll want to change common/CMakeLists.txt to add:

    include/pcl/register_point_struct.h
    include/pcl/conversions.h

To "incs", rather than "incs_common", so the install path works out. Probably easiest to amend this to the last commit. Otherwise I've verified that this + the migration script works on all my projects.

[jspricke]

Hi Stephen,

• Stephen Miller [email protected] [2013-07-04 19:32]:

  Small fix: you'll want to change common/CMakeLists.txt to add:

      include/pcl/register_point_struct.h
      include/pcl/conversions.h
  

  To "incs", rather than "incs_common", so the install path works out.

Oups, thanks for spotting! Could you open a pull request against the
"next" branch, I've just pushed? There we can collect patches for the
next rc.

Probably easiest to amend this to the last commit.

Will do so later.

[sdmiller]

See #165

[jspricke]

Thanks Stephen! I've included the fixup.

Also, I think it would be great to merge this into master soon, so the rc does not diverge even more. On the other hand it's maybe a good idea to wait for the ROS people to see what problems they are coming up with. @wjwwood what's the timeframe for packages of the rc in ROS?

[wjwwood]

I have released pcl 1.7.0rc1 into ROS hydro, I haven't had time to see what happened on the farm or to test packages on top of it yet. I'll do that today.

[jspricke]

William, did it all work? Stephen, should we merge this into master? Can you send a mail to both lists, explaining what's going on? Thanks!

[sdmiller]

I'd vote for merging it into master -- I'm already using the /next branch
for my own stuff, and am happy with it. But it largely depends on what
William's experience has been, since ROS users will be the most affected by
this. If Williams gives it a +1, I'll send something out to the community
after we merge, linking to the Wiki to explain what's going on.

On Mon, Jul 8, 2013 at 12:48 AM, Jochen Sprickerhof <
[email protected]> wrote:

William, did it all work? Stephen, should we merge this into master? Can
you send a mail to both lists, explaining what's going on? Thanks!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/164#issuecomment-20590181
.

[wjwwood]

I am still upgrade pcl_ros, and I have run into a lot of problems. Hopefully a lot of these errors will be hidden behind the pcl_ros code.

Everything appears to be functional, but I am not too happy about the performance hit from this new type you guys have created. Arguably this is a fundamental failing of the way ROS msgs are implemented (using vectors for storing arrays), but it prevents me from making a zero copy conversion from the ROS types to the PCL equivalents. This means, for example, when a user wants to subscribe to a sensor_msgs::PointCloud2 and write them to a pcd file, each PointCloud2 must first be copy converted into the almost identical pcl::PCLPointCloud2 before being written. It would be trivial to copy all of the writePCD(pcl::PointCloud2) code and convert it to be writePCD(sensor_msgs::PointCloud2), but that would be a lot of duplication of code.

The most correct way to fix this, in my opinion, would be to skip the intermediate format and just move functions like that specialization of writePCD out of PCL. For end users, people who are using the ROS data types directly should depend on ROS, or they shouldn't use those classes for storing data. It's that simple.

This would involve taking PCL master and finding any code which references *_msgs::<ROS types>, then moving that code out of pcl or changing it to use PCL native types. There should be no pcl::PCL* intermediate types. I under stand that this would include lots of PCL features and apps, but this would prevent the scenario where we need a whole new set of conversions between ROS and near identical PCL types, like PointCloud2 and PCLPointCloud2 for example. I would be happier with this setup if we could convert or move the data from the ROS types to the PCL duplicates without a copy, but we currently cannot.

In summary I think there are two choices:

• Duplicate large segments of code and convert them to be for ROS types in packages outside of PCL
• Do away with the intermediate types (like PCLPointCloud2) and move ROS specific code out of PCL

Both involve moving code out of PCL, but the trade off here is maintaining duplicate code, versus more refactoring for developers and users.

[wjwwood]

@tfoote and @dirk-thomas in case you guys have input.

[dirk-thomas]

Clearly the second option to refactor the code to be out of PCL is the preferred solution (from a software engineering point). This would make reuse (not only for ROS) much easier.

If that is not going to happen the code blocks in question should at least be refactored in a way that the least possible amount of code needs to be duplicated. This could be achieved by providing hooks where the ROS specific types require different behavior.

[wjwwood]

I have found one thing that is an issue. Some of the things which were previously ROS messages were not changed, namely all the messages from the pcl_msgs package:

• pcl::ModelCoefficients
• pcl::PointIndices
• pcl::PolygonMesh
• pcl::Vertices

I'm not sure on the correct action here, but for consistency maybe they should be pcl::PCL* prefixed.

[sdmiller]

Hi @wjwwood.

Before going further, have you tried using vector::swap [1] for copying? If you're willing to destroy the original PCLPointCloud2 type (which I assume you are), you should be able to get pretty nice performance with vectors. AFAIK nothing about the PointFields has to change, so it shouldn't require touching the memory.

As far as the options go, I can definitely see where you guys are coming from with number 2. But I don't think that major an API break (losing binary blob representations of point clouds within PCL) is the right solution to this problem -- that's more generally useful to the community than just for ROS message serialization purposes. In the end if performance is really a bottleneck, I suspect the serialization code isn't all that bad to duplicate -- especially if we factored it out, or even templated it to work with PCL* and sensor_msgs* types (which will be virtually identical). In the end I think having duplicate code is bad from a software engineering point of view, but so is tying binary blobs explicitly to ROS when other message passing services might be used.

For ModelCoefficients, PointIndices, PolygonMesh, and Vertices, we discussed this, but I think it's a little less critical (and a bigger API break for most of us), since those don't tend to wind up in our include paths. Someone who's run into these conflicts might be able to weigh in on this more.

[1] http://www.cplusplus.com/reference/vector/vector/swap/

[wjwwood]

I thought vector::swap was C++11 specific along with the move semantics, but you're right it is in C++98, so that's good news. I'll look into using that, any ideas on how the destructive functions should be named?

---

So you guys plan to use pcl::PCLPointCloud2 as the binary blob representation which you might reuse for for some other transport layer, like zeromq? I would ask why you would want to tie PCL to any blob representation, most "message passing systems" provide both serialization and transport, so you will need to adhere to their serialization model. Part of why I am having trouble understanding this is that I don't know how the ROS types were being used in PCL before this change, like why would users use sensor_msgs::PointCloud2 over pcl::PointCloud<T> except to export it to ROS?

---

I would say the correct design decision would be to decouple PCL completely from serialization and transport. I think the serialization (marshaling) for PCD file format is fine as common format on which to trade datasets. If there is still a need for a plain "blob" representation, then you guys should create something like pcl::PointCloudBlob. But that still doesn't explain the use of things like std_msgs::Header, has that become the standard time structure for PCL, if not why is pcl::PCLHeader needed?

---

Hopefully you guys can fill me in on the above questions.

With the destructive swap, the performance is less of a problem and I am probably ok with the current state of affairs, but I think at best this is patch to remove ROS from PCL with minimal breaking changes. I think that a much cleaner solution could be reached with more changes, and you guys should really consider doing a cleaner separation next time you release an API break version of PCL.

[sdmiller]

Hmm, I'm not sure what the convention normally is for this sort of thing. Maybe toROSCopy vs toROS?

Currently the reason we use pcl::PCLPointCloud2 throughout the code, is that it provides nice functionality for dealing with point clouds without overtemplatizing everything. For instance, we might have code that checks to see if a point cloud has a normal field -- if it does, we'll exploit that. The blob representation makes it easy to do this, whereas the pcl::PointCloud type gets hairy. And yeah, zeromq was the alternative I had in mind, since (as far as I know) it handles arbitrary binary blobs.

I think std_msgs::Header is really just a remnant of the ROS serialization, and it's true that we probably have no need for it (hence header.stamp being a long rather than a ROSDuration). I think our main objective here was to break the API minimally since the release was just around the corner. In the long run, we should definitely look at a cleaner solution.

[rbrusu]

The (crippled) sensor_msgs::PointCloud2 copy in PCL comes from a very old legacy of PCL being a ROS package when I started developing it. We basically made a decision a long time ago that all ROS "libraries" (e.g., packages) should use primary ROS messages as data types, and everyone at Willow Garage followed it. You could probably blame both Brian and myself equally for the current ROS-PCL mess, but it's too late now, and this is not about that.

PCL needs a binary blob datatype for storing arbitrary data that can be introspected at runtime. After decoupling the PCL package from ROS, we all agreed that carrying on the messages by copying them over and removing the ROS parts should be fine (in order to avoid PCL API breakage), as we would have build scripts that would automatically switch to ROS true messages types if ROS was found and enabled, as well as used our copies otherwise (e.g., for standalone PCL users). What a mistake that was in the long run, especially for ROS users who have been suffering integration issues with PCL trunk versions for a long time. In our defense, we never anticipated anything but stable PCL releases to go into ROS. Users have proven us wrong, where everyone wanted the latest and greatest features from trunk running on their ROS powered robots.

Moving forward, we cannot remove the binary blobs, but we can rename them. If we're worried about efficiency, then we need to figure out a way to convert between PointCloud directly into the true ROS message type sensor_msgs::PointCloud2, as well as keep the current conversions in PCL to the new pcl::PointCloud2 (or pcl::PointCloudBlob - if it gets renamed). Code duplication cannot be avoided if efficiency is more important than portability and cleanliness, unless the swap methods work.

The clean way to do this is PCL 2.0, but due to a lot of other things, that's been delayed for a long time.

PS. There is currently no serialization code in PCL, and no transport other than Boost's asio for tools/apps/demos. Basically, there is no ROS code within PCL other than the naming. The PCL sensor_msgs::PointCloud2 has very little in common with the ROS sensor_msgs::PointCloud2, as it's a dumbed down version of it, without any serialization and the such.

Let me know if that makes sense please.

[wjwwood]

I understand, like I said I am happy with the current state of affairs as long as we can address some of the performance problems with code duplication and the destructive std::vector::swap.

---

What about fromPCLMove or swapToPCL.

[sdmiller]

I like swapToPCL

[wjwwood]

I thought about it more and I think moveDataToPCL is better, or for short moveToPCL, because we aren't actually "swapping" all of the data, just the data vector, the rest is a copy.

[jspricke]

@wjwwood: How did you compare the performance of both versions? Can you post some code? Also, Before the conversion you needed to convert from sensor_msgs::PointCloud2 into pcl::PointCloud most of the times, so this should be the same still, or?

[wjwwood]

@jspricke There are lots of pcl::* functions which used to take sensor_msgs::PointCloud2 directly and now take pcl::PCLPointCloud2 messages.

Often functions will take a const sensor_msgs::PointCloud2 and now I have to create a pcl::PCLPointCloud2 and copy the data over before passing to PCL, which incurs a new, additional copy which wasn't required before. When ever possible I create an alternative function which takes non-const sensor_msgs::PointCloud2 so that I can destructively "move" the data into a pcl::PCLPointCloud2 before passing it to PCL.

I don't have performance tests, but I know that when dealing with potentially very large point clouds, any unnecessary copies are going to be noticed.

I'll work up some examples of changes I have had to make, but you can probably just look at the types of changes I have had to make to pcl_ros:

ros-perception/perception_pcl@39824f1

[jspricke]

@wjwwood Actually there are not so many. ``git grep sensor_msgs` shows some in filters and common, the rest, i.e., in apps, io, outofcore, tools and visualization should all not be relevant to ROS.

Ragarding const methods. What about providing a PCLPointCloud2 where you can pass a pointer to the data vector in the constructor? This would allow sharing of the same vector in both objects without changing the original one.

[wjwwood]

The ones I have run into are toROSMsg and fromROSMsg (which are just gone now) and the writePCD and readPCD functions. These seem to be the pcl functions that most often get called directly by ROS packages and have been changed.

There have been a couple of other cases where pcl classes/functions where templated on sensor_msgs::PointCloud2 (or similar) and the generic implementation tries to set the sensor_msgs version's header equal to a pcl::PCLHeader internally in pcl, so I just changed the templating to use pcl::PCLPointCloud2 and convert the clouds when passing them in.

The pointer constructor would be an option, though we would want to be careful to only use that when the lifetime of the sensor_msgs::PointCloud2 was guaranteed to be longer than the pcl::PCLPointCloud2, which would be the case in the const functions I was talking about.

[jspricke]

@wjwwood Did you come up with a test case? I would guess it's not important, as 95% of our algorithms need conversions into pcl::PointCloud anyhow.

Regarding the pointer, I didn't thought this through completely, but I think we would need to change the definition of ``pcl::PCLPointCloud2` to get this in. This would result in different semantics when using objects of this type (-> instead of .). @sdmiller, what do you think?

I really would like to get this out asap!

[sdmiller]

I think providing a data vector is reasonable, though it does raise the question of who is in charge of the memory: requiring users to delete it by hand is a recipe for memory leaks. Unless you were thinking a boost::shared_ptr to the data vector?

This does also make it a bit harder to automatically migrate, since there's no simple find and replace to do it. I'm OK with that, but it's unclear first whether most (any?) users really have a reason to make a PCL and ROS cloud share the same data. @wjwwood?

[wjwwood]

If I were to redo the ROS message implementation I would allocate arrays of memory in a shared_ptr<std::vector>, then we wouldn't have to worry about implicit copies and we could efficiently do conversions from one cloud format to the next, but I can tell you we will not get such a thing into ROS by Hydro. Because we probably can't get this feature into ROS we will still not have bidirectional no-copy, non-destructive conversions.

Without both sensor_msgs::PointCloud2 and pcl::PCLPointCloud2 having this shared_ptr, we will have problems with memory ownership, like I said before, but it could still be useful in converting clouds efficiently when you can guarantee the lifetime of the source will be longer than the destination, which is the case sometimes in conversions.

@sdmiller To answer your question more directly I think the use case of conversions would be the obvious case where you would want to have PCL and ROS cloud's share the same data.

[wjwwood]

@jspricke No I have not come up with a test case, I have been swamped with other Hydro beta stuff.

[sdmiller]

@wjwwood I guess my concern is that the data vector will be shared, but other information (like the header, height, width, is_dense, etc) isn't. So unless the use case involves modifying the data without modifying the number of points or any other high level things about the cloud, it seems like being destructive would be better than a half copy. Of course, the header could also be a pointer, which would eliminate that issue.

[wjwwood]

Agreed, I wouldn't advocate users should copy the shared_ptr directly, but this just makes it possible to do efficient conversions, there will still be pitfalls for the users if they do not consider things like this. This can all be hidden behind the conversion functions.

[wjwwood]

Either way, for Hydro and PCL 1.7.0 I wouldn't consider the shared_ptr implementation. And I'm not sure if the pointer constructor for PCLPointCloud2 will affect enough use cases to justify it.

[jspricke]

Looks like we can merge this into master. If no one objects, I will do it tomorrow morning along with a mail to both lists. Thanks for the feedback!

[jack-oquin]

This change is going to disrupt all the ROS PCL users.

Previous PCL 1.x versions maintained excellent backwards compatibility. Users have come to expect that their code will still work after upgrading to a new release. That is as it should be.

Like Radu, I've seen a fair number of user requests over the past few years for "how to run a newer PCL version than the one originally released with some particular ROS distribution". But, I do not consider them representative of the ROS PCL community as a whole. While some have a legitimate need for a new PCL feature not available in the earlier version, most seem only to be casually experimenting with PCL while naively assuming that the latest version will always suit their needs best.

But, PCL has long provided a rich set of features that work well. Serious ROS product developers using PCL are happy to run a stable, well-tested and integrated version of the library, even if it's not the latest.

How can this transition can be made more user-friendly?

• We really need a complete release cycle in which the old and new interfaces are both supported, with the old one deprecated. That allows people an opportunity to upgrade their code without experiencing sudden unexpected breakage.
• Lacking that, the incompatible API should have major version number 2.x, so people are not surprised by the lack of API compatibility.

I am very concerned that failure to deal with these issues will harm the reputation of both ROS and PCL among professional product developers.

[jspricke]

Hi Jack,

thanks for the input!

• jack-oquin [email protected] [2013-07-15 13:16]:

  * We really need a complete release cycle in which the old and new interfaces are both supported, with the old one deprecated. That allows people an opportunity to upgrade their code without experiencing sudden unexpected breakage.

That would be definitely cleaner, although my experience is, that people
ignore deprecation warnings anyhow.

• Lacking that, the incompatible API should have major version number 2.x, so people are not surprised by the lack of API compatibility.

Truth is, we are braking the API with every minor version, but PCL is
big, so it's really hard to keep everything consistent. On the other
hand, we didn't pushed many minor updates till now, so for me the x in
pcl-1.x is rather part of the mayor version number ;).

I am very concerned that failure to deal with these issues will harm the reputation of both ROS and PCL among professional product developers.

From my point of view the changes are less disruptive. For one, almost
all algorithms in PCL accept only pcl::PointCloud, i.e., you had to use
from/toROS till now. Second, we are trying to make the transition as
easy as possible, i.e., Stephen wrote a script for the PCL people and I
hope the ROS guys will come up with something similar. Third, we should
really get this mess cleared.

In essence: +1 for deprecated versions, if someone comes up with a patch
really soon (as in tomorrow?) Otherwise I would just release this.

[wjwwood]

I do not believe it is viable to automate the changes which are required in ROS code, as the right choice is often context dependent. The best I can do is to do 90% of the work in pcl_conversions, many times including the header is the only change required to upgrade.

I agree with @jack-oquin in principle, but in practice I think the breakage is worth the additional improvements. There have been many improvements mixed in with the new development work from 1.6->1.7, an example of this is that pcl 1.7 builds with almost no warnings on clang, where as pcl 1.6 doesn't even build with clang. I am far more interested in the intrinsic improvements with pcl 1.7 than the new features. I have requested backports of fixes to 1.6 from 1.7 previously, but for better or worse they were too intertwined with new features in 1.7 to reasonably backport. Additionally, my experience has been that upgrading is relatively straight forward process for most ROS packages. I think that the current state of PCL and ROS integration represents making the best of a situation which was brought about by incorrectly mixing the two originally.

[jspricke]

• William Woodall [email protected] [2013-07-15 13:49]:

  I do not believe it is viable to automate the changes which are required in ROS code, as the right choice is often context dependent.

You could try Coccinelle [1], but I don't think it's worth the trouble.

[1] https://lwn.net/Articles/315686/

[wjwwood]

@jspricke I've seen it before, but unfortunately I have a full plate already. Upgrading the ROS integration with PCL is something I picked up because I thought it was important, but I would gladly take contributions from the community.

However, I will reiterate that I don't think the solution to the problem is automating it. Even for @jack-oquin's package, the change was minimal:

https://github.com/wjwwood/velodyne/commit/6b1437f3a618a7cc2369bd2cf3993d7efd94b5ee

[jack-oquin]

I should clarify that it is not the small breakage in my own code that concerns me. Even if @wjwwood had not already patched it for me, those changes are small.

My real concern comes from talking with various people in industry building products with ROS. I used to be a product developer myself, and I understand that they feel differently about things like this than academics do. They don't like unnecessary changes, and they don't like surprises. I first learned about this change, when the ROS build farm started sending me e-mail saying my already-released package no longer builds. That is not the way to notify developers of a change. We really prefer to plan these things in advance.

William has been diligently fixing problems in released ROS packages, including mine. But, we should understand that those are just the tip of the iceberg. Most ROS product development is not open source and will never appear in the OSRF repositories where he can find and fix any breakage.

@jspricke: You know much more than I about the release-to-release compatibility of PCL. My experience has been much better you suggest, however. The PCL code in the driver William mentioned has remained essentially unchanged since being released on 2012-04-03 for ROS Electric. I believe Electric included PCL 1.0.2, so at least the fundamental interfaces have been quite stable for a long time.

[wjwwood]

@jack-oquin I would point out that this only breaks things on Hydro which is just now arriving at Beta status. I doubt anyone is building a product on Hydro, and if they are then they should have the expectation that the software is not stable yet.

There will be an announcement on the PCL mailing list when 1.7.0 is finalized, and there will be notes about the changes in the Hydro beta announcement and on the Hydro migration guide.

Your packages broke because they are in the staging area for a new distribution of ROS which will have changes from the previous version. I commend you for getting your packages released into Hydro so quickly, but you should expect that they may break in Hydro up until the release.

I'll try to do a better job of warning the releasers when a large breaking change is coming in the unstable distros, but at some level people may have unrealistic expectations on the stability of currently in development code bases. We did, however, break our contract of freeze date for Hydro, but we opted to continue pushing integration with new versions of libraries along, because Hydro will be a long release cycle and we will be stuck with this version of these libraries for a long time. The ROS releases will be getting longer and more stable going forward.

[jack-oquin]

William is of course correct about the not-yet-beta status of Hydro, and I never intended to imply anything different. Again: it is not my package I am worried about.

Product developers are certainly not using Hydro. Most of them are probably still running Fuerte or Electric. That is one of the reasons why recent surveys of ROS users revealed a widespread preference for more stability and less-frequent releases. By the time they adopt Hydro, this breakage will no longer be much of a surprise.

I do believe the lack of a good migration path will lower developers' opinions of PCL and ROS, at least somewhat. For the ROS developers, that is somewhat unfair since they seem to have no other viable options. the obvious alternative is sticking with 1.6, but William mentioned that 1.6 is not appealing for other reasons.

[sdmiller]

I understand where you're coming from, @jack-oquin, but as a head-in-the-clouds researcher I'm too detached from the ROS industrial community to understand the weight of this API break. In my mind it's actually quite minor: a class changed names but no members or methods, the naming system makes it very clear what was altered, and conversion functions exist to switch between them. Every time PCL is upgraded, minor API breakages occur, as I recall being the case with ROS (switching from electric to fuerte always took a bit of find- and replace- work). This time the only part of anyone's code which needs to change is the one which receives and sends ROS messages, which I imagine only bookends the internal logic of most codebases. If users were writing customer-oriented code meant to support multiple versions of ROS simultaneously, I'd understand the pain -- boost::filesystem 2 vs 3 errors come to mind. But I presume most industry use cases involve upgrading an entire, deployable system at once. For any dev in that situation, this should be a simple fix.

It's a shame to break the API -- ideally it would never happen. But from the other perspective, building PCL from scratch and having linker errors and/or segfaults just because ROS happens to also be installed on the machine, doesn't help my opinion of a library either. I know many researchers who are afraid to upgrade PCL because they know the build process will require hacks; and this is a community with a famously high tolerance for hacks. It's a somewhat ugly break to correct a (in my mind) much uglier underlying problem, and at least to the current contributors of PCL who weighed in, it was almost unanimously worth the cost.

Of course, pcl_ros could optionally deploy with the original headers instead (with, e.g., a couple typedefs in a header file to keep the naming conventions), if we're worried about Hydro's stability. But the dependency should be the ROS package's side, where any user clearly expects it -- making a standalone library clash with ROS by default doesn't make sense.

[jack-oquin]

@sdmiller: I have great respect for my many friends in academia. Their heads are not in the clouds. But, working with them for the past several years, I've become very conscious of the extent to which their goals and priorities differ from those of product engineers. There are good reasons for that, and neither group is right or wrong. They are just working on different tasks.

From what you say, it seems I am too late to this discussion and most people's minds are already made up. If so, there is no need to waste my time or yours with further comments that can have no effect. I was only hoping we could come up with a workable tick-tock scheme for this release.

I don't see how anyone could reasonably expect the ROS developers to do more than the considerable amount @wjwwood has already accomplished.