Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(autoware_crop_box_filter): reimplementation in core repo #279

Merged
merged 8 commits into from
Mar 25, 2025

Conversation

liuXinGangChina
Copy link
Contributor

@liuXinGangChina liuXinGangChina commented Mar 15, 2025

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:

How was this PR tested?

  1. colcon build with entire autoware packages
    1.1 type the command in a terminal -- colcon build --cmake-args -DCMAKE_BUILD_TYPE=Release --symlink-install
  2. using AWSIM lab‘s pointcloud for input, using rviz for visual checking that pointcloud has been croped correctlly
    2.1 in terminal 1 launch AWSIM and use ros2 topic list to confirm that the pointcloud topic exist
    2.2 in terminal 2 source autoware workspace first, then type ros2 run tf2_ros static_transform_publisher 2.0 3.2 1.3 0 0 0 1 velodyne_top_base_link base_link to publish a static tf manually
    2.3 in terminal 3 source autoware workspace, launch crop_box_filter_node by typing ros2 launch autoware_crop_box_filter crop_box_filter_node.launch.xml
    2.4 in terminal 4 source autoware workspace, launch rviz, set frame id to base_link, add bbox topic and output pointcloud topic using add button.
    2.5 you will see that all the point cloud inside the green bbox is cropped, which mean "self-crop" succeed
  3. using rqt_reconfigure to dynamicly changing crop box parameters, and chaning is visualized on rviz
    3.1 repeat operation 2.2 ~ 2.4
    3.2 in a new terminal open parameter reconfigure tool rqt_reconfigure using command ros2 run rqt_reconfigure rqt_reconfigure
    3.3 in rqt_reconfigure, change the parameter and see the pointcloud meets your expectation.

e.g. set negative to false will crop the pointcloud outside the bbox and keep the pointcloud inside the bbox

Screenshot from 2025-03-15 15-18-21

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

@liuXinGangChina liuXinGangChina added run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) labels Mar 15, 2025
@liuXinGangChina liuXinGangChina self-assigned this Mar 15, 2025
@liuXinGangChina liuXinGangChina requested a review from a team as a code owner March 15, 2025 09:02
Copy link

github-actions bot commented Mar 15, 2025

Thank you for contributing to the Autoware project!

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

Please ensure:

Copy link

codecov bot commented Mar 15, 2025

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 (d561a12).
Report is 105 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.

@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch 2 times, most recently from 2fba5f5 to b6d2303 Compare March 15, 2025 09:51
@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch from 01af502 to 1b2c7a5 Compare March 15, 2025 10:06
@sasakisasaki
Copy link
Contributor

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
Copy link
Contributor Author

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

心刚

Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch from 2e18215 to b52a5ed Compare March 18, 2025 01:03
@liuXinGangChina
Copy link
Contributor Author

liuXinGangChina commented Mar 18, 2025

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 liuXinGangChina force-pushed the autoware_crop_box_filter branch 3 times, most recently from 9bf5343 to 44df055 Compare March 18, 2025 07:49
@liuXinGangChina
Copy link
Contributor Author

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

心刚

@liuXinGangChina liuXinGangChina requested a review from youtalk March 18, 2025 08:04
Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xmfcx xmfcx force-pushed the autoware_crop_box_filter branch from 31b3b98 to 5a45c3d Compare March 19, 2025 02:42
@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch from 5a45c3d to 4ca6f61 Compare March 19, 2025 05:26
@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch from ef31beb to 7ed6522 Compare March 19, 2025 05:40
@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch 2 times, most recently from 9a0ac28 to 838d5f4 Compare March 20, 2025 00:51
@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch 2 times, most recently from 04ca6ae to 785d1da Compare March 24, 2025 08:19
@mitsudome-r mitsudome-r requested a review from YoshiRi March 25, 2025 04:59
Copy link
Member

@mitsudome-r mitsudome-r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is good enough for initial implementation.
We can improve the memory efficiency in the other PRs if we need improvements.

I just would like to have someone from original Sensing/Perception engineers to have a quick review as well. (cc. @YoshiRi )

liuXinGangChina and others added 4 commits March 25, 2025 14:00
…oware_crop_box_filter, extracted from universe-sensing-pointcloud-preprocessor reimplemented on core repo : v0.0

Signed-off-by: liuXinGangChina <[email protected]>
…oware_crop_box_filter, add readme: v0.1

Signed-off-by: liuXinGangChina <[email protected]>
@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch from 785d1da to 530b6e3 Compare March 25, 2025 06:00
Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compared the original with the implementation, and I think it's safe to say that it's a completely different implementation. So, if the crop box algorithm itself isn't subject to any licensing, I think it's fine to remove it. If any problems arise, I'll take responsibility for now.
https://github.com/PointCloudLibrary/pcl/blob/master/filters/src/crop_box.cpp
https://github.com/autowarefoundation/autoware_core/pull/279/files

Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@youtalk youtalk requested a review from drwnz March 25, 2025 06:41
@liuXinGangChina liuXinGangChina force-pushed the autoware_crop_box_filter branch from 4435a64 to d561a12 Compare March 25, 2025 06:49
@youtalk youtalk requested a review from drwnz March 25, 2025 06:52
@mitsudome-r mitsudome-r merged commit 03fdcc7 into main Mar 25, 2025
23 of 26 checks passed
@mitsudome-r mitsudome-r deleted the autoware_crop_box_filter branch March 25, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
Development

Successfully merging this pull request may close these issues.

6 participants