search

ros-perception / perception_pcl

PCL (Point Cloud Library) ROS interface stack
http://wiki.ros.org/perception_pcl
421 stars 332 forks source link

sorts pc msg fields by offset (ros2) #438

Closed cosama closed 2 months ago

cosama commented 9 months ago

Closes https://github.com/ros-perception/perception_pcl/issues/434

This is in analogy to https://github.com/ros-perception/perception_pcl/pull/437 and fixes https://github.com/ros-perception/perception_pcl/issues/434 but for ros2. I haven't tested it locally but if it passes the tests, it should be working as expected, because the same changes were also applied to the test, which requires fields being sorted.

cosama commented 2 months ago

This has been sitting around for 6 month and more. It would be ready to be merged. Is there anything else I need to do? Happy to move this forward so that we can finish it up.

SteveMacenski commented 2 months ago

Personally, its a lack of my own knowledge about the ramifications of this change for the entire breadth of downstream users that makes me nervous. The sort operations also adds in additional compute / latency, which makes me think this should be an optional thing rather than always done when converting PCL pointclouds.

@mvieth any objections with merging if its parameterized whether to include? Any reason we shouldn't do this that you can think of since you posted this block in the ticket as a recent PCL change?

mvieth commented 2 months ago

Personally, its a lack of my own knowledge about the ramifications of this change for the entire breadth of downstream users that makes me nervous. The sort operations also adds in additional compute / latency, which makes me think this should be an optional thing rather than always done when converting PCL pointclouds.

I believe this additional latency is negligible since pfs (the vector to be sorted) is small (typically < 10 elements) and in most cases already sorted.

I also can't think of any problem the sorting might cause: in most cases, nothing changes (already sorted). In the cases where sorting does change the order, it could only cause a problem if a user assumes that a specific field is always at a fixed position in the fields vector, which is to my knowledge not guaranteed anywhere and a rather fragile behaviour (in contrast to iterating through the field vector and checking the names).

@mvieth any objections with merging if its parameterized whether to include? Any reason we shouldn't do this that you can think of since you posted this block in the ticket as a recent PCL change?

How would you make it parameterized? What would be the default? If you are thinking about adding a parameter to the fromPCL function, then copyPCLPointCloud2MetaData would also need a parameter (it calls fromPCL), as well as all functions that call copyPCLPointCloud2MetaData.

I have no objections, no matter if sorting is optional or always done.

SteveMacenski commented 2 months ago

Makes sense, merging!

I don't work with overly sophisticated pointcloud fields, so I wanted to check in with someone else before I made a foreseeable mistake. I maintain this repository by consensus & cut the releases when asked, I'm admittedly not overly active here and mostly help as a volunteer to keep the gears moving :smile:

SteveMacenski commented 2 months ago

Thanks for the ping @cosama!

cosama commented 2 months ago

@mvieth @SteveMacenski Thank you so much for being so quick on this.