perception_pcl
perception_pcl copied to clipboard
ros-perception
ROS2 port pcl_ros/filters
This is a ros2 porting of pcl_ros filters.
I have been working on my ROS2 migration project and found that most of the pcl_ros functions are not ported yet. https://discourse.ros.org/t/call-for-help-pcl-ros-for-ros2/15131
I have just ported filters because I need them, and they could be ported without type adaptation REP-2007.
sub_input_ = pnh_->subscribe<sensor_msgs::PointCloud2> (
"input", max_queue_size_, bind (&Filter::input_indices_callback, this, _1, pcl_msgs::PointIndicesConstPtr ()));
https://github.com/ros-perception/perception_pcl/blob/melodic-devel/pcl_ros/src/pcl_ros/filters/filter.cpp#L130
contributions
This work is based on #255
- all filters (node/component)
- lazy subscription (based on a workaround)
- integration tests for filters
If I'm being honest, this is such an overwhelmingly large PR that I'm not sure I'll ever have time to fully review it to the point that I would be fine merging it.
If you could break this down into a series of PRs, that would be much more likely to be merged in (and you'd get more fake internet points for contributions :wink: ). Perhaps 1 PR for the preliminaries, and then a PR for every filter separately (or a couple if short). Large PRs are incredibly difficult and time intensive to review all at once. I can easily spend 15 minutes every couple of days reviewing, but its difficult for me to spend 4 hours all in 1 sitting on it.
I'd rather be up front with you, cards on the table, than let this languish silently.
@SteveMacenski Thank you for your feedback! I will break this down and make another PR!
@SteveMacenski Thank you for your feedback! I will break this down and make another PR!
I migrated a subset of the filters into a smaller PR if it helps: https://github.com/ros-perception/perception_pcl/pull/397
I hadn't realized you were already working on it.
@asymingt It helps! I may not have time to do the last piece of PR soon (maybe next week).
I'm sure @daisukes doesn't mind @asymingt! Please reopen any PRs that aren't already having others from @daisukes open. @daisukes, you might want to tell him what you're working on so neither of you have to do duplicate porting :-)
@asymingt I'd prefer smaller PRs (1-2 filters each). It makes it much easier for me to review and for us to do cycles. The more code, the longer of a delay it will be for review cycles (since review times grow non-linearly with PR size)
These three filters are not migrated yet. I copied the code in this PR branch to the latest branch and made fixes based on modifications of previous PRs. @asymingt, please let me know if you take these filters. If not, I will work on it by the end of next week. Thanks!
radius_outlier_removal.cpp
statistical_outlier_removal.cpp
voxel_grid.cpp
These three filters are not migrated yet. I copied the code in this PR branch to the latest branch and made fixes based on modifications of previous PRs. @asymingt, please let me know if you take these filters. If not, I will work on it by the end of next week. Thanks!
radius_outlier_removal.cpp statistical_outlier_removal.cpp voxel_grid.cpp
Cool -- I'll set up three PRs to each add each one in isolation.
Done, here they are:
- https://github.com/ros-perception/perception_pcl/pull/398/files [VoxelGrid]
- https://github.com/ros-perception/perception_pcl/pull/399/files [RadiusOutlierRemoval]
- https://github.com/ros-perception/perception_pcl/pull/400/files [StatisticalOutlierRemoval]
Let me know if you want to handle CropBox too.
Let me know if you want to handle CropBox too.
Oh, I missed it. Please handle it. Thank you!
Let me know if you want to handle CropBox too.
Oh, I missed it. Please handle it. Thank you!
OK -- I'm starting to hate the linter in Jenkins. Is there an easy way to to do it pre-commit?
ament_cpplintament_uncrustifyament_...
Totally, but wheres the ament_cpplint --reformat and ament_cppcheck --reformat :)
Let me know if you want to handle CropBox too.
Oh, I missed it. Please handle it. Thank you!
@SteveMacenski and @daisukes : Looks like I missed a key fact that the filter-specific ros2 parameters are meant to be declared in their constructor. I'll need to go back and take them out of the filter.ccp file. I'll move all my PRs to draft until they are ready for review. Sorry for the thrash.
EDIT: I refactored all four PRs and they are ready for a review. I'm ignoring the cpplint and uncrustify errors in CI because they are in different files from my PR.
Let me know if you want to handle CropBox too.
Oh, I missed it. Please handle it. Thank you!
It's done here: https://github.com/ros-perception/perception_pcl/pull/401
Do you all perhaps have a turnaround time on the reviews? I don't want them to bit rot.
They're in my queue but admittedly perception_pcl is not on my priority list like Nav2 is. PCL_ROS only gets time when I'm able to find it on a volunteer basis. They're not forgotten, they're in my notifications tray to get to.