Enable packing w/o padding for PointCloudMsgs

[ntfshard]

🦟 Bug fix

Fixes #

Summary

According to comments in code, we have to add a padding for fields in packed points msg for ROS1. And due to current branch is not supporting ROS1, we can switch this padding off to increase throughput.
I checked this change with lidar and rviz and it seems working fine (which I mentioned in comment here)

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[azeey]

This makes sense to me, but I haven't verified if memory alignment is no longer necessary in ROS 2. @ahcorde / @mjcarroll can you confirm?

[iche033]

changes look good to me as well. Just need confirmation that this is fine to do and all these sensors should continue to work fine in ROS 2.

[ntfshard]

Ping C:

[azeey]

@peci1 I noticed you have some context seeing you created PointCloudLibrary/pcl#4939. Mind taking a look?

[peci1]

I don't think this part of Gazebo should care too much about ROS. That should be the job of ros_gz.

However, I think neither padding nor not padding is correct. I'd make this a configuration option.

[azeey]

Thanks for the input @peci1.

I don't think this part of Gazebo should care too much about ROS. That should be the job of ros_gz.

I agree, but if what we do here makes it more difficult to bridge to ROS efficiently, we should reconsider.

However, I think neither padding nor not padding is correct. I'd make this a configuration option.

What I don't quite get is if the padding would actually be useful here. Wouldn't the PCL PointCloud data structure be created from the message based on the values in the offset field in PointField regardless of the presence of padding bytes?

[ntfshard]

What I don't quite get is if the padding would actually be useful here. Wouldn't the PCL PointCloud data structure be created from the message based on the values in the offset field in PointField regardless of the presence of padding bytes?

I tried to answer the same question, seems all conversion magic happens here and I not sure how this packing offsets from GZ are propagated to ROS msg

[peci1]

If that is true, then any alignment is ignored.

[azeey]

I tried to answer the same question, seems all conversion magic happens here and I not sure how this packing offsets from GZ are propagated to ROS msg

That's in ros_gz_point_cloud, which has been broken for a while. The current way of bridging point clouds from Gazebo to ROS is directly via ros_gz_bridge which pretty much copies the data to a ROS message (https://github.com/gazebosim/ros_gz/blob/6156b3ece5064fd9b567496013a7476e2f195cf4/ros_gz_bridge/src/convert/sensor_msgs.cpp#L528-L664).

If that is true, then any alignment is ignored.

I guess we don't know for sure. @ntfshard since there's a lot of uncertainty here, I'd rather not merge this as-is without more evidence that this would not break existing systems. One way we can find out is to write test code that uses the PCL APIs (https://github.com/ros-perception/perception_pcl/blob/ros2/pcl_conversions/include/pcl_conversions/pcl_conversions.h) to convert a sensor_msgs::msg::PointCloud2 message to a PCL native type, pcl::PointCloud<T>, and verifying the contents.

Alternatively, we can make this configurable. If we go that route, we'll have to figure out where the configuration is specified (e.g. in the SDF <sensor> tag or a command line argument to gz-sim or a <gz:policy>).

[ntfshard]

I see, yes, it's understandable concerns.

One way we can find out is to write test code that uses the PCL APIs to convert a sensor_msgs::msg::PointCloud2 message to a PCL native type, pcl::PointCloud<T>, and verifying the contents.

I'd like to clarify this part. You mean not a unit test, but some application which performs conversion and comparing, like an experiment? Because we don't have a ROS code here it feels like something which is not belong here.

Alternatively, we can make this configurable. If we go that route, we'll have to figure out where the configuration is specified (e.g. in the SDF <sensor> tag or a command line argument to gz-sim or a <gz:policy>).

It feels quite heavy solution I suppose

[azeey]

I'd like to clarify this part. You mean not a unit test, but some application which performs conversion and comparing, like an experiment? Because we don't have a ROS code here it feels like something which is not belong here.

Yes, more of an experiment to convince ourselves that this is safe.

[ntfshard]

I'd like to clarify this part. You mean not a unit test, but some application which performs conversion and comparing, like an experiment? Because we don't have a ROS code here it feels like something which is not belong here.

Yes, more of an experiment to convince ourselves that this is safe.

pointcloud_conversion_.zip

root@6c0c9c6d5ddb:~/a# colcon build --packages-select pointcloud_conversion
Starting >>> pointcloud_conversion
Finished <<< pointcloud_conversion [0.16s]                

Summary: 1 package finished [0.28s]
root@6c0c9c6d5ddb:~/a# ./build/pointcloud_conversion/pointcloud_conversion 
Final offset: 13
Converted PCL Point Cloud:
x: 0, y: 0, z: 0
x: 1, y: 2, z: 3
x: 2, y: 4, z: 6
x: 3, y: 6, z: 9
x: 4, y: 8, z: 12

Also I tried GZ GPULidar plugin with Rviz (through the bridge), and it worked fine

[peci1]

Hmm... Can you also try with intensity somewhere in the middle?

[peci1]

And release mode, of course...?

[ntfshard]

Hmm... Can you also try with intensity somewhere in the middle?

And release mode, of course...?

I do not provide a new version of archive of sources due to it's just 2 line change (intensity goes up on 1 line and some extra keys in cmake file)
And yes, I install environment from scratch that's why I have different host name in docker console

It could be much more convenient if we have a design documentation or technical specification to ROS2 messages.

[ntfshard]

Do you need any other information from me or something?

[iche033]

thanks for putting together the pointcloud_conversion package. I also played around with it and the outputs look correct when there's no padding in the data. So looks good to me.

[azeey]

I've also played around with it (see https://github.com/azeey/sandbox/tree/master/pcl_padding_test, @ntfshard I hope you don't mind) and feel comfortable going forward with this.

[ntfshard]

I've also played around with it (see https://github.com/azeey/sandbox/tree/master/pcl_padding_test, @ntfshard I hope you don't mind) and feel comfortable going forward with this.

Sure, no problem. Also this code looks like a possible test for https://github.com/ros-perception/perception_pcl IMHO