Publishing pointclouds in os_sensor frame is misleading

[peci1]

I know the documentation describes the situation with frames quite extensively, but generally people expect that raw data from a sensor are published in the acquisition frame. That would be os_lidar. But the example ROS node transforms and publishes the data in os_sensor, which is rotated and shifted in Z.

One possible consequence of misinterpreting the frames is usage of the scans in a raytracing pipeline. These pipelines usually take the frame_id of the incoming scan and cast rays from the origin of the given frame. But if the frame is os_sensor, the cast rays are invalid because the origin is shifted lower than the real optical origin of the rays.

I thus suggest changing the data frame to os_lidar. Users who do not care about the origin of observation should not be affected (except receiving the data in a different frame), and users interested in the observation point would get the expected data by default.

[dmitrig]

Yep, agreed. Iirc the code predates the frames defined in the manual. Gonna leave this open for the next major version

[Aposhian]

Is there a reason why the frame on the messages can't be changed to the lidar_frame?

[Samahu]

Unfortunately, I don't have enough background on why this was the choice but I'll check on the subject. Especially for the ROS2 driver since it is still in the stage of BETA and we need to get this right.

[Aposhian]

Based on this documentation, the raw UDP data comes not as XYZ at all, but then can be transformed to the lidar frame, and then transformed to the sensor frame. It will take me some digging in the Ouster SDK to confirm whether it just converts to the lidar frame or also includes the sensor frame.
https://static.ouster.dev/sensor-docs/image_route1/image_route2/sensor_data/sensor-data.html

The pointclouds are stamped with the laser data frame in the unofficial driver: https://github.com/ros-drivers/ros2_ouster_drivers/blob/5425ed023d3c1c5293a9af90d2ae81c5df889334/ros2_ouster/src/ouster_driver.cpp#L127, and by making the clouds be stamped in the lidar frame in my fork of this version, I was able to reproduce results I get with the unofficial driver.

[Aposhian]

Ok it looks like the ouster sdk (as used by the driver) converts points into the sensor frame, not the data/lidar frame, since it calls make_xyz_lut with the sensor info. So I need to figure out why it wasn't working for me...
https://github.com/ouster-lidar/ouster_example/blob/ff46c961b690daae3e18e39999ef8651081cbc66/ouster_client/include/ouster/lidar_scan.h#L354-L355

[Aposhian]

Ok, I'm pretty sure I got to the bottom of this. The reason for the inconsistency between ouster_ros and ros2_ouster_drivers is because of a bug in ros2_ouster_drivers. ros2_ouster_drivers stamps data with the lidar frame, but it is actually in the sensor frame, since the (similar) make_xyz_lut function is called with the sensor info, which provides the lidar to sensor transform.
https://github.com/ros-drivers/ros2_ouster_drivers/blob/5425ed023d3c1c5293a9af90d2ae81c5df889334/ros2_ouster/include/ros2_ouster/processors/pointcloud_processor.hpp#L68

So based on this, I think this ticket can be closed: I believe this drivers' current behavior is correct. However, it does bring up the following points:

• If the lidar frame is not being used, then should the driver even publish the lidar to sensor transform?
• Maybe in the future it could be a configuration option to allow publishing in the lidar frame (only doing the beam transformation) instead of the sensor frame? I'm not sure if there is demand for that, and if there is, that should be a separate ticket.

[peci1]

Thanks for the digging.

I think this ticket can be closed

I don't think so. Please, read again my point in the first comment. It is still valid.

If the lidar frame is not being used, then should the driver even publish the lidar to sensor transform?

In no way. This frame at least allows people to transform the cloud to the frame in which it was actually captured.

• Maybe in the future it could be a configuration option to allow publishing in the lidar frame

That would currently be the only option for ROS 1. Too much code would get broken if the change was done silently.

@Samahu please, at least get this right for ROS 2 if it's sill in beta.

[Samahu]

Ok, I'm pretty sure I got to the bottom of this. The reason for the inconsistency between ouster_ros and ros2_ouster_drivers is because of a bug in ros2_ouster_drivers. ros2_ouster_drivers stamps data with the lidar frame, but it is actually in the sensor frame, since the (similar) make_xyz_lut function is called with the sensor info, which provides the lidar to sensor transform. https://github.com/ros-drivers/ros2_ouster_drivers/blob/5425ed023d3c1c5293a9af90d2ae81c5df889334/ros2_ouster/include/ros2_ouster/processors/pointcloud_processor.hpp#L68

So based on this, I think this ticket can be closed: I believe this drivers' current behavior is correct. However, it does bring up the following points:

• If the lidar frame is not being used, then should the driver even publish the lidar to sensor transform?
• Maybe in the future it could be a configuration option to allow publishing in the lidar frame (only doing the beam transformation) instead of the sensor frame? I'm not sure if there is demand for that, and if there is, that should be a separate ticket.

@Aposhian I did briefly look into the ros2_ouster_drivers and they are using the lidar_frame when publishing the processed point cloud.

Maybe in the future it could be a configuration option to allow publishing in the lidar frame
That would currently be the only option for ROS 1. Too much code would get broken if the change was done silently.

@peci1 We are considering to introduce an option for users to select the frame of reference as both of you suggested, the default for ROS1 is to maitain the same behavior, for ROS2 we probably want to set the lidar_frame to be the default since this would make it easier for people who are switching from the community driver to the official ros2 driver.

[peci1]

Sounds good!

[Aposhian]

for ROS2 we probably want to set the lidar_frame to be the default since this would make it easier for people who are switching from the community driver to the official ros2 driver.

I think the problem is that the community driver stamps the pointclouds with the lidar frame, but it actually transforms them to the sensor frame, so it is lying. You can't make the official driver backward compatible (work with the same TFs) without doing it incorrectly.

It seems that there is demand to get pointclouds in the lidar frame, rather than the sensor frame, so I think it does make sense to make this a configuration option.

[Aposhian]

Ok I made a draft PR with this configuration option that I will get around to testing sometime.

[Samahu]

@peci1 I am reviewing #127 PR that @Aposhian submitted 3 weeks ago, I agree with the conclusions that @Aposhian made. The driver's current xyz_lut object does transform the points into the sensor frame rather than the lidar frame. I think that just simply changing the frame_id as the unofficial driver is doing would be incorrect and misleading.

Users who need to have the transform into the lidar frame should rather transform the published point cloud from the sensor frame to the lidar frame using pcl::transformPointCloud and utilize the supplied transformation os_sensor->os_lidar TF transformation.

Alternatively, we pretty much need to take the same route that @Aposhian made in the linked PR.

[peci1]

I vote for backporting #127 to ROS 1. That would be the cleanest solution.

If you still can make breaking changes to ROS 2 branch, I suggest publishing in os_lidar by default. For ROS 1, this is already lost, the default cannot be changed and most people will still probably blindly use the default output thinking it is in the acquisition frame.

Maybe in ROS 1, a warning could be issued if the driver publishes in os_sensor? Something like transform_to_sensor_frame is set to true. If you reproject or raytrace the points, please note that the projection center of the lidar is not in point (0,0,0), but in (0,0,0.03618). See https://github.com/ouster-lidar/ouster-ros/issues/33 for more details. This might make a few more people aware of this issue. But it would be confusing for those not interested into knowing the projection center.

[Samahu]

Yeah definitely I have an upcoming upgrade to the ROS2 branch(s) that addresses several issues, one of which is the frame_id of published point cloud, you can take a look at these upcoming changes here: #146 I was about to follow the same approach that currently implemented in Steve's Ouster ROS2 driver for compatibility reasons but unfortunately we will have to diverge from Steve's implementation on this particular matter.

Back porting some these improvements to ROS1 will follow after I am done with #146.

It makes sense to warn the users for the ROS1 since I want to keep the sensor_frame as the default for ROS1 driver.

[juliangaal]

It makes sense to warn the users for the ROS1 since I want to keep the sensor_frame as the default for ROS1 driver.

I was under the impression that the pointcloud is published already transformed into the sensor frame. Yet, following this discussion, do I understand correctly that the data is published in the sensor frame (while actually being recorded in the lidar frame) here and the transform is published separately?

[Samahu]

It makes sense to warn the users for the ROS1 since I want to keep the sensor_frame as the default for ROS1 driver.

I was under the impression that the pointcloud is published already transformed into the sensor frame. Yet, following this discussion, do I understand correctly that the data is published in the sensor frame (while actually being recorded in the lidar frame) here and the transform is published separately?

No, in ROS1 the data is published in the sensor frame and the frame_id of the PointCloud2 message is correctly set to the sensor frame. However, some of our users who need to do ray tracing would rather have the PointCloud2 message published in the lidar frame, which isn't readily available. Simply changing the name of the frame_id to the lidar frame would not be correct. So if you expect the PointCloud to be published in the sensor frame then that's what is currently take place in ROS1 and it is correct. If you require the PointCloud be published in the lidar frame however you would need to wait until I back port parts of #146 to ROS1 that give you the option to do so. The default behavior for the ROS1 driver would be to keep publishing the point cloud in the sensor frame (with a warning or a hint). For ROS2 though the default would be to publish the PointCloud in the lidar frame.

[juliangaal]

Simply changing the name of the frame_id to the lidar frame would not be correct.

Because the data was already transformed into sensor_frame? At which level does this happen: the driver or the sdk? I can't seem to find the relevant code in the cloud nodelet

[Samahu]

Simply changing the name of the frame_id to the lidar frame would not be correct.

Because the data was already transformed into sensor_frame? At which level does this happen: the driver or the sdk? I can't seem to find the relevant code in the cloud nodelet

The object returned by the non-parameterized make_xyz_lut method generates a transformation that projects the xyz coordinates into the sensor frame. The mainline of ros1 & ros2 drivers currently uses this method. This is changing with #146 and #149 for ros2. I will soon propagates some of these changes back to the ros1 too 🤞 .

[juliangaal]

make_xyz_lut is part of the SDK?

It makes sense to warn the users for the ROS1 since I want to keep the sensor_frame as the default for ROS1 driver.

But the lidar points will be continued to be published in the sensor frame? While the users will receive a warning, as indicated in some other comment?

To my understanding, the IMU data is also published in the sensor frame, but looking at the imu handler, this doesn't seem to be the case. here?

Thanks for your patience, I really appreciate it.

[Samahu]

But the lidar points will be continued to be published in the sensor frame? While the users will receive a warning, as indicated in some other comment?

Yes

To my understanding, the IMU data is also published in the sensor frame, but looking at the imu handler, this doesn't seem to be the case. here?

The IMU data gets published in the IMU frame, which comes directly from the sensor, no additional transformation is applied to it in the ROS driver.