Remove unnecessary offset addition

[nachovizzo]

Sorry for parachuting this PR out of nowhere, but instead of opening an issue, it's easier for me to discuss it with a piece of code.

Problem description

According to my broken brain, the line of code I was removing was incorrect unless I needed clarification. It also allocates more memory than we require. The nice thing is that that extra allocated memory is not used or read. But if we want to add a new field, this might be a problem.

Let's say I'm creating a point cloud message out of nowhere, and I only want to add x, y, and z as floats; then the point step size should be 12 bytes. With the current implementation, it would be 16:

offset = addPointField( cloud_msg_, "x", 1, sensor_msgs::msg::PointField::FLOAT32, offset);
offset = addPointField( cloud_msg_, "y", 1, sensor_msgs::msg::PointField::FLOAT32, offset);
offset = addPointField( cloud_msg_, "z", 1, sensor_msgs::msg::PointField::FLOAT32, offset);
offset += sizeOfPointField(sensor_msgs::msg::PointField::FLOAT32);

Another way to also see this is that we call the other function for setting the field, we will get a different point step:

sensor_msgs::msg::PointCloud2Modifier modifier(cloud_msg);
modifier.setPointCloud2Fields(3,
  "x", 1, sensor_msgs::msg::PointField::FLOAT32,
  "y", 1, sensor_msgs::msg::PointField::FLOAT32,
  "z", 1, sensor_msgs::msg::PointField::FLOAT32);

I did some experiments but the most interesting one was creating a pointcloud message from XYZ points and seeing the difference in the memory footprint

Before this change: point_step: 16, 4 extra trash bytes per point

header:
  stamp:
    sec: 1692265839
    nanosec: 200554371
  frame_id: base_footprint
height: 1
width: 57632
fields:
- name: x
  offset: 0
  datatype: 7
  count: 1
- name: y
  offset: 4
  datatype: 7
  count: 1
- name: z
  offset: 8
  datatype: 7
  count: 1
is_bigendian: false
point_step: 16
row_step: 922112
data:

After this change point_step: 12

header:
  stamp:
    sec: 1692266325
    nanosec: 100530386
  frame_id: base_footprint
height: 1
width: 57600
fields:
- name: x
  offset: 0
  datatype: 7
  count: 1
- name: y
  offset: 4
  datatype: 7
  count: 1
- name: z
  offset: 8
  datatype: 7
  count: 1
is_bigendian: false
point_step: 12
row_step: 691200
data:

[tfoote]

Maintaining 16 byte alignment means that there are significant benefits for processing the data due to SIMD acceleration https://en.wikipedia.org/wiki/Single_instruction,_multiple_data the memory size is a compromise to maintain that significant acceleration for processing.

[nachovizzo]

@tfoote, thanks for the fast check! I see what you mean. I doubt that many folks are directly dumping the PointCloud2 data into memory (I think most people will go through the point cloud using sensor_msgs::PointCloud2Iterator), and thus, those extra bytes allocated won't ever be used.

That said, if someone is relying on this, it makes sense to keep it as it is. I will close the PR now to avoid extra noise in this repo, but if anyone in the future has a stronger opinion, feel free to comment to re-open the discussion.