Container cleanup and organized pointclouds

[spuetz]

As discussed in PR #65 I cleaned up a lot and integrated the organized feature using the containers. Features:

• organized feature using the point containers
• no pcl -> more elegant and much faster
• easier now to add new containers
• removed color node, the color node can be expressed by the ring number and also now if really necessary, it can be expressed as a new container
• the transform node is not longer needed, since its functionality is completely covered by the containers, we could remove it or wrap it somehow.
• points could be transformed online using different timestamps and transformations.

Still some things to do, but the code now is much nicer and faster. Let me know what I need to fix and adapt. Cheers.

[JWhitleyWork]

@spuetz - It looks like a few of the tests are failing. Would you mind running catkin run_tests velodyne on your machine and debugging the issue? Looks like it's something with the use of a boost::shared_ptr.

Also, FYI - I merged changes that were made to master and added a missing dependency.

[spuetz]

There are some points we need to discuss:

• Should we merge the transform and the convert node, since they almost have the same implementation. We could activate transformation when the source and the target frame differs?
• Should the transformation be in double or in float precision?
• Should we have a parameter to select the container? Since, I guess more and more containers will be implemented with point normals and so on.

[JWhitleyWork]

@spuetz

• The only downside to merging the transform and convert nodes is performance. As separate nodes, they can run as separate threads and one being slow (transform) doesn't have as much of an impact on the other. If they both have similar implementations, though, I don't see any reason why they both couldn't inherit from the same class. Them being separate also offers more introspection for debugging (if you choose to run them as separate nodes instead of nodelets under the same manager).

• The underlying types used are floats (Eigen::Vector4f instead of Eigen::Vector4d) so doing the extra math probably isn't necessary.

• We should probably assume that XYZIR is the norm and offer options for different pieces of information, deducing the type to use based on the options. e.g. if Point Normals are requested, choose the correct new container. My 2 cents anyway.

[JWhitleyWork]

@spuetz - Ping

[JWhitleyWork]

@spuetz - Would you mind merging master back into your branch? I've changed CI vendors and applied roslint to all packages. You will likely get some weird conflicts but you're definitely the best one to handle the merge.

[spuetz]

Sure, I'll try to find some time for that next week.

[spuetz]

@JWhitleyAStuff I merged the master. All tests succeed.

The only downside of merging the transform and convert nodes is performance. As separate nodes, they can run as separate threads and one being slow (transform) doesn't have as much of an impact on the other. If they both have similar implementations, though, I don't see any reason why they both couldn't inherit from the same class. Them being separate also offers more introspection for debugging (if you choose to run them as separate nodes instead of nodelets under the same manager).

Regarding the merging of the transform and the convert node: Since the transformation is done while inserting each point into the final buffer, there is no real loss of performance, except the read of one bool variable which indicates to transform the point or not. Furthermore, since we do the transformation here directly with eigen, not using pcl, the transformation node should be much fast and also should save memory, since we directly transform and insert each point to the final PointCloud2 buffer.

Now the transformation code is in the DataContainerBase. This gives the possibility to use it or not in the derived container classes.

The underlying types used are floats (Eigen::Vector4f instead of Eigen::Vector4d) so doing the extra math probably isn't necessary.

Yes, sure, that makes sense, will do that.

We should probably assume that XYZIR is the norm and offer options for different pieces of information, deducing the type to use based on the options. e.g. if Point Normals are requested, choose the correct new container. My 2 cents anyway.

I think the easiest is to let the user choose the container, to derive the container by the attributes is a bit overhead and does not give real benefits. Since the container names clearly specify what attributes are processed. Also, in my opinion, when developing new containers, choosing just the container by name will be less work and is much more intuitive.

I think we should get the code to the master quickly. Later I can go on with enhancing and cleaning up the rest of the code, since there are still a lot of issues which I discovered while developing (e.g., the config structs are not necessary) .

[JWhitleyWork]

@spuetz - Thank you for all of your work on this! Would you mind adding a description for the parameter to http://wiki.ros.org/velodyne_pointcloud ? I'll go ahead and release a new version of the driver for Melodic once you verify that this version has been tested and on what hardware.

[YoshuaNava]

@spuetz this is a great contribution, thank you!

Regarding the implementation, I noticed that you organize the point cloud in a way in which the actual rows and cols of the LiDAR 'image' (e.g. beams = rows, packets = cols) are inverted. Thus, the point clouds are stored in column-major order.

Why was this done in this way? Is it because you assume that in the Velodyne LiDARs the 'rolling shutter' effect happens across the columns, not within rows?