fix(crop_box_filter): can cause error in the subsequent module #881
Conversation
Signed-off-by: plane.li <plane.li@autocore.ai>
| @@ -92,7 +83,7 @@ class CropBoxFilterComponent : public pointcloud_preprocessor::Filter | |||
| rcl_interfaces::msg::SetParametersResult paramCallback(const std::vector<rclcpp::Parameter> & p); | |||
|
|
|||
| public: | |||
| PCL_MAKE_ALIGNED_OPERATOR_NEW | |||
| EIGEN_MAKE_ALIGNED_OPERATOR_NEW | |||
There was a problem hiding this comment.
I think this can't be changed due to Rolling/Humble support.
https://github.com/autowarefoundation/autoware.universe/pull/804/files#r860083464
Codecov Report
@@ Coverage Diff @@
## main #881 +/- ##
========================================
- Coverage 9.53% 9.53% -0.01%
========================================
Files 931 931
Lines 57663 57696 +33
Branches 10401 10401
========================================
Hits 5500 5500
- Misses 47636 47669 +33
Partials 4527 4527
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
| pcl::PCLPointCloud2::Ptr pcl_input(new pcl::PCLPointCloud2); | ||
| pcl_conversions::toPCL(*(input), *(pcl_input)); | ||
| impl_.setInputCloud(pcl_input); | ||
| impl_.setIndices(indices); | ||
| pcl::PCLPointCloud2 pcl_output; | ||
| impl_.filter(pcl_output); | ||
| pcl_conversions::moveFromPCL(pcl_output, output); |
There was a problem hiding this comment.
As far as I know, there are two reasons not to use pcl cropbox.
- The conversion is too heavy since the data size is large.
- pcl cropbox is a larger calculation cost than current implement. because pcl cropbox has many
ifinfor.
https://github.com/PointCloudLibrary/pcl/blob/master/filters/src/crop_box.cpp
There was a problem hiding this comment.
reference : tier4/AutowareArchitectureProposal.iv#773
There was a problem hiding this comment.
Oh, Now I understand why not the PCL.
However,at present, we have tried many times and found that the crop_box_filter can not be used before "down_sample" and "ground_filter". I think the reason is that "down_sample" and "ground_filter" nodes behind the crop_box_filter use the PCL. The may be some wrong parameters:
https://github.com/autowarefoundation/autoware.universe/blob/main/sensing/pointcloud_preprocessor/src/crop_box_filter/crop_box_filter_nodelet.cpp#L124-L130
more details at #705
@yukkysaito says "The conversion is too heavy since the data size is large." I think we can use "pcl down sample" before "pcl crop box filter" in the xxxx.launch.py to solve the problem.
At present ,Function availability is more important than performance.
There was a problem hiding this comment.
However,at present, we have tried many times and found that the crop_box_filter can not be used before "down_sample" and "ground_filter". I think the reason is that "down_sample" and "ground_filter" nodes behind the crop_box_filter use the PCL.
Thank you for the reply.
It may have some wrong paramえters. I guess we need to see what is wrong.
@yukkysaito says "The conversion is too heavy since the data size is large." I think we can use "pcl down sample" before "pcl crop box filter" in the xxxx.launch.py to solve the problem.
Yes. It can solve to use "cropbox filter" after "downsample". But I think it is better to be able to use it even before downsample.
Also, there are three problems with downsampling.
- Many DNN algorithms do not assume downsampling.
- INTENSITY is removed by downsample filter(voxel grid filter).
- Errors are increased in the distance to obstacles. It is harder to guarantee not to hit an obstacle.
At present ,Function availability is more important than performance.
I agree with you 👍
There was a problem hiding this comment.
Also, there are three problems with downsampling.
- Many DNN algorithms do not assume downsampling.
- INTENSITY is removed by downsample filter(voxel grid filter).
- Errors are increased in the distance to obstacles. It is harder to guarantee not to hit an obstacle.
At present:
Our DNN directly uses the point cloud published by lidar_driver without any pcl handler.
and compared to pcl_crop_box_filter,DNN consumes more and more computing power and memory.
And in euclidean_cluster and ndt_scan_matcher , INTENSITY is useless.
There was a problem hiding this comment.
I think the original error about invalid point comes from the fact that current implementation assumes that incoming points have height=1. (i.e., unordered points). Rather than changing the cropbox filter itself, I would suggest to change L126
from
output.height = input->height;
to
output.height = 1;
This is reasonable because the pointcloud would become unordered points once you delete certain number of points after cropbox filtering.
There was a problem hiding this comment.
Thank u! We have tried this modification method before,and it can not cause errors in subsequent nodes("down_sample" and "ground_filter"). But I'm not sure if other nodes using PCL will have errors.
@mitsudome-r
And I think modify the height to 1 is a nice idea and can solve the "larger calculation cost“ problem.
There was a problem hiding this comment.
thank you!
@mitsudome-r
By viewing relevant materials ，I think height to 1 is useful.
The below code is nice.
output.width = static_cast<uint32_t>(output.data.size() / output.height / output.point_step);
output.row_step = static_cast<uint32_t>(output.data.size() / output.height);
Beacuse the PCL is to heavy, I will do an other PR(change the height to 1) to fix the issue.
yukkysaito
added
the
component:sensing
plane-li
changed the title
|
New PR without PCL #951. I close this PR. |
…ion for approval (autowarefoundation#881) Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
Signed-off-by: plane.li plane.li@autocore.ai
Description
the pr is to fix the "crop_box_filter node "which can cause error in the subsequent module.
related issue: #705
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.