search

ros-drivers / ros2_ouster_drivers

ROS2 Drivers for the Ouster OS-0, OS-1, and OS-2 Lidars
https://ouster.com/
Apache License 2.0
134 stars 79 forks source link

Filter out all-zero points in the pointcloud processor #79

Closed simonwaterer closed 2 years ago

simonwaterer commented 3 years ago

Adds a parameter pointcloud_filter_zero_points (default: false) that affects the PointcloudProcessor. The default is false so as to make this an opt-in behaviour.

When set to true, it will cause the PointcloudProcessor to filter out the all-zero points (i.e. where XYZ co-ordinates result in 0,0,0) before converting the pointcloud to a ROS PointCloud2 message. The 0,0,0 points occur when there is no-return for the beam at the given position.

This reduces the size of the pointcloud when it is sent over the network and thus reduces overall network bandwidth requirements. This is especially beneficial if an azimuth window has been set on the lidar device, there is no point in transmitting these 0,0,0 points.

Sidenote: it would also be interesting to use the azimuth window returned by the device (column_window in the data_format struct returned as part of the sensor_info) to automatically set the size of the constructed pointcloud to only the required number of points, however this is a more involved change to how the toCloud() function would need to work.

SteveMacenski commented 3 years ago

Something to be aware of though is that this makes the pointcloud unstructured. Before, you had a regular lattice grid to work with that knowing the width / height you could back out coordinates. This is now unstructured so I'm on the fence about whether this should be merged in since it would change the actual underlying structure of the thing being returned (from HxW to 1xH*W).

p0wdrdotcom commented 3 years ago

As this is functionality is "opt-in" is it pretty safe to merge once the readme is updated to reflect the fact the point becomes unstructured? This functionality offers a pretty significant network performance improvement when multiple lidar are in use.

Dokans commented 2 years ago

Is there possibilty to have this feature merged into main branch or are there any known issues that prevents it?

vinnnyr commented 2 years ago

I agree with @p0wdrdotcom -- Having this as an option, even if it removes the structured nature, could help people out if they are trying to unmuck up their network.

I am curious though, how a downstream consumer of said pointcloud would be able to back out coordinates in this case though. I guess the downstream consumer would need to know the aziumth blocking parameters somehow :thinking:

SteveMacenski commented 2 years ago

Sorry for the (almost year) delay :sweat_smile:

simonwaterer commented 2 years ago

Thanks! Great to see this has been merged.