feat(autoware_crop_box_filter): reimplementation in core repo

[liuXinGangChina]

Description

Extracted from universe-sensing-pointcloud-preprocessor and reimplemented on core repo

Reimplementation content

1. remove dependency to packages under universe repo
2. refactering package.xml and CMakeList.txt
3. add readme
4. add unit test code checking “only expected point pass the filter”
   4.1 construct a input pointcloud which contain points both inside and outside the box, and pass it to the filter
   4.2 check the total number of points in the output pointcloud, whether it equals expectation
   4.3 check every point of the output pointcloud, whether it should pass the filter

What's the difference from original code

1. set polygon pulisher's frenquency from always to when someone subscribed
2. add a launch paramter "input_pointcloud_frame", thus we can get pointcloud transformation matrix once and for all, rather than lookup tf everytime get new input pointlcoud

Related links

Parent Issue:

• Link implement crop_box_filter #266

How was this PR tested?

1. colcon build with entire autoware packages
2. using AWSIM lab‘s pointcloud for input, using rviz for visual checking that pointcloud has been croped correctlly
3. using rqt_reconfigure to dynamicly changing crop box parameters, and chaning is visualized on rviz

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

[github-actions]

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

• You've checked our contribution guidelines.
• Your PR follows our pull request guidelines.
• All required CI checks pass before marking the PR ready for review.

[codecov]

Codecov Report

Attention: Patch coverage is 20.98361% with 241 lines in your changes missing coverage. Please review.

Project coverage is 19.93%. Comparing base (4cb18f5) to head (785d1da).
Report is 102 commits behind head on main.

Files with missing lines	Patch %	Lines
...oware_crop_box_filter/src/crop_box_filter_node.cpp	21.14%	223 Missing and 12 partials ⚠️
.../autoware/crop_box_filter/crop_box_filter_node.hpp	14.28%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #279       +/-   ##
===========================================
- Coverage   78.75%   19.93%   -58.82%     
===========================================
  Files          11        3        -8     
  Lines         193      321      +128     
  Branches       73      140       +67     
===========================================
- Hits          152       64       -88     
- Misses         11      245      +234     
+ Partials       30       12       -18     

Flag	Coverage Δ
differential	19.93% <20.98%> (?)
total	?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sasakisasaki]

According to @mitsudome-r san, we'll implement the crop_box_filter as an additional new package which is unique to autoware.core. Thus, we keep the current crop_box_filter in the autoware.universe side. So,

• Please fix the PR description that mentions about removal of crop_box_filter from the autoware.universe side

Thank you in advance.

[liuXinGangChina]

According to @mitsudome-r san, we'll implement the crop_box_filter as an additional new package which is unique to autoware.core. Thus, we keep the current crop_box_filter in the autoware.universe side. So,

* Please fix the PR description that mentions about removal of `crop_box_filter` from the `autoware.universe` side

Thank you in advance.

Sasaki san， @sasakisasaki

I double check the pr and remove the sentence “porting autoware_crop_box_filter”
Now the rest of the pr description will not lead to the "removing" meaning.

Please feel free to leave further comment

Best regards

心刚

[youtalk]

Please add unit tests to check whether the point cloud passes or does not pass through the filter.

[liuXinGangChina]

Please add unit tests to check whether the point cloud passes or does not pass through the filter.

sure kondo san @youtalk
I will do that

Best regards

心刚

[liuXinGangChina]

Hello ，Kondo san @youtalk

Unit test for “filter function check” already added，in this test case :

1. construct a input pointcloud which contain points both inside and outside the box, and pass it to the filter
2. check the total number of points in the output pointcloud, whether it equals expectation
3. check every point of the output pointcloud, whether it should pass the filter

Have a nice day

心刚

[youtalk]

It’s difficult to point out specific issues, but overall, it reads as if the memory efficiency is poor.

[youtalk]

Function names should be verb phrases. I think filter_pointcloud or filter would be correct.

Suggested change

	void pointcloud_filter(const PointCloud2ConstPtr & cloud, PointCloud2 & output);
	void filter(const PointCloud2ConstPtr & cloud, PointCloud2 & output);

[liuXinGangChina]

Got it kondo san.
let me double check the code, especially the memory efficiency part.

[liuXinGangChina]

Good afternoon Kondo san @youtalk

Regarding “memory efficiency”， i picked several code blocks which access memeory

ID	CODE	COMMENT
1	cpp output.data.resize(cloud->data.size());	this code malloc a memory block which has same size as the input pointcloud, since this node aims at crop self points, it will not waste too much memory(most of the points are outside ego). Alternatively we can replace “resize” with "push_back() in the loop" but that will bring more cost for frequent asking cpu deal with memory, and every time "data.size" is not sufficient , it will expand the memory size by 150%, this may cause memory waste
2		this block picks a point from the input poincloud for filtering, rather than copy a point from the orginal input poincloud in the loop, we can use function pcl::from_msg to convert the msg into a pcl::pointcloud and iterate every point in the loop but that bring a accessary memory cost for storing the whole point cloud
3		this block add the filter result into output pointcloud consider transformation

Thanks for your review

Please feel free to leave your further advice

心刚

[youtalk]

With this writing style, if only need_preprocess_transform_ is true, it will be skipped. Instead of nesting, handling the need_preprocess_transform_ condition first and then processing the need_postprocess_transform_ condition will likely reduce the chance of bugs.

[mitsudome-r]

Actually, I think we can remove the condition in this L217.
We can just calculate the following calculation

Eigen::Vector4f point_postprocessed = eigen_transform_postprocess_ * point_preprocessed;

because we already did the following calculation beforehand in L203-L208

   Eigen::Vector4f point_preprocessed = point;

    // apply pre-transform if needed
    if (need_preprocess_transform_) {
      point_preprocessed = eigen_transform_preprocess_ * point;
    }

[liuXinGangChina]

Good afternoon mits san @mitsudome-r

That's right , i have already correct the code and push

Thanks

心刚