PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.65k stars 4.6k forks source link

[filters] fixing ignored `pcl::Indices` in `VoxelGrid` of `PCLPointCloud2` #5979

Closed kai-waang closed 3 months ago

kai-waang commented 3 months ago

This PR fixed issue #5880. And it looks like there is a missing test in VoxelGrid.Filters which should test whether the given indices have been handled correctly.

mvieth commented 3 months ago

Hi, thank you for this pull request. It would actually be necessary to add two new getMinMax3D functions, instead of adding the indices parameter to the existing functions. The reason is that people might use the existing getMinMax3D functions (without the indices parameter), and their code would no longer compile if we just add the parameter.

And it looks like there is a missing test in VoxelGrid.Filters which should test whether the given indices have been handled correctly.

You are right, such a test would be good. If you like, feel free to add one, otherwise someone else might do that. An option could be to take the existing test, but add more points (e.g. very large points) to the cloud, and construct the indices vector such that VoxelGrid should disregard these additional points.

kai-waang commented 3 months ago

@mvieth I've added back these functions. I am not sure about how to add points to test. Should I add points to the pcd files or just coding that in the test files.

mvieth commented 3 months ago

@mvieth I've added back these functions. I am not sure about how to add points to test. Should I add points to the pcd files or just coding that in the test files.

No, please don't modify the PCD files. You could start by copying cloud, then adding points, then convert to PCLPointCloud2 via toPCLPointCloud2. Something like:

PointCloud<PointXYZ>::Ptr cloud_modified (new PointCloud<PointXYZ>);
*cloud_modified = *cloud;
add points
PCLPointCloud2::Ptr cloud_blob_modified (new PCLPointCloud2);
toPCLPointCloud2(*cloud_modified, *cloud_blob_modified);
pass to voxel grid
kai-waang commented 3 months ago

@mvieth Thanks! I've added additional tests.

larshg commented 3 months ago

@kai-waang You need to make a copy of cloud, like @mvieth suggested:

PointCloud<PointXYZ>::Ptr cloud_modified (new PointCloud<PointXYZ>);
*cloud_modified = *cloud;

Else all subsequent test that use cloud, now fails as it has been changed and does not meet the requirements of those tests :smile: