search

ADVRHumanoids / point_cloud2_filters

BSD 3-Clause "New" or "Revised" License
8 stars 3 forks source link

Consistency with pcl_ros #1

Closed peci1 closed 1 year ago

peci1 commented 1 year ago

To make this package a bit easier to use, I suggest to make its API closer to pcl_ros.

What you implemented here, is actually better done with pcl::CropBox. And there is pcl_ros nodelet https://wiki.ros.org/pcl_ros/Tutorials/filters#CropBox (don't wonder you haven't found it before - this filter has long been undocumented; I've added the documentation today).

So if you want to keep this package closer to pcl_ros interface, you should rename the parameters to match those of CropBox. And you can actually also use the pcl::CropBox internally to do the filtering in a more efficient way.

Just note that pcl::CrobBox parameter keep_organized is broken on Melodic and doesn't work there (if you still use Melodic).

torydebra commented 1 year ago

Ok, I was not aware of the crop box filter. I implemented it (b107b2d9aa155b305352f0a9d21e982917a2c330) with the same parameters as in pcl_ros, and probably I will use this instead of the passthrough.

At the moment passthrough can be kept as it is maybe in future I can expand to exploit its other functionalities (eg to filter the intensity)

peci1 commented 1 year ago

Awesome, thanks!

Do you also plan to make a binary release of the package?

torydebra commented 1 year ago

Do you mean as official ROS package? I would like to do so once package is mature enough maybe by adding some other filters, to not release updated versions too often

peci1 commented 1 year ago

;) Don't worry, the process is not as difficult as it might seem. Even in the current state, the package would seem useful to me.

When you do the release, feel free to ping me and I'll update the readme in sensor_filters.

torydebra commented 1 year ago

BTW, the package is released on the ROS official repository now (noetic)

peci1 commented 1 year ago

Yes, I've noticed, great!