Fix for #1413

[SergioRAgostinho]

Addresses #1413 , based on #1170, modified to make use of #586.

No elegant way to modify pcl::VoxelGridCovariance at this stage.

[taketwo]

Changes to voxel_grid.hpp look good except to the way the downsample_all_data_ option is handled. I'm pretty sure it was introduced for performance reasons. With your proposed changes, however, centroid computation takes place for every field regardless of the setting, it only affects the output. I think we should have a separate loop to accumulate only Euclidean centroid in CentroidPoint<pcl::PointXYZ>.

voxel_grid.cpp and tests look good as well.

Can you please explain the problem with covariance variant of the VoxelGrid?

[SergioRAgostinho]

Changes to voxel_grid.hpp look good except to the way the downsample_all_data_ option is handled. I'm pretty sure it was introduced for performance reasons. With your proposed changes, however, centroid computation takes place for every field regardless of the setting, it only affects the output. I think we should have a separate loop to accumulate only Euclidean centroid in CentroidPointpcl::PointXYZ.

Makes sense. I'll make changes to limit the operations to the coordinates.

Can you please explain the problem with covariance variant of the VoxelGrid?

For the normal VoxelGrid a temporary Eigen vector was being created on applyFilter just for the purpose of computing the centroid. CentroidPoint removed the need for this.

In VoxelGrid covariance, each voxel leaf has its own Leaf structure, which holds a bunch of relevant information about the leaf (as Eigen types) and persists in leaves_ after applyFilter. If you check the source for applyFilter, you'll notice that the accumulation is performed on these leaf structures and that the centroid is only converted to a point type and added to the point cloud at the very end. I can try to make use of CentroidPoint, but I'll need to add it as new attribute to Leaf (which is a public type in the api), and in the end convert the result, back to an Eigen type, to be used in the covariance calculations of each leaf. It doesn't feel like a more elegant solution compared to the one in place at the moment. Nevertheless, I may be overlooking something.

[taketwo]

Ok, makes sense to keep it this way. Could you adapt the code to PCL style guide (i.e. spaces before braces everywhere)? And squash.

[SergioRAgostinho]

I'll address the changes.

More importantly, I noticed the new test I wrote, trying out the downsamplealldata as false is segfaulting! I ran the test before committing, therefore I'm not sure if I'm able to reproduce this on my development environment. Can you give it a try?

Edit: I suspect it is something with my use of getVector4fMap(). Will try to remove it and see if the tests pass.

[taketwo]

Hi, I get segfault as well, here is backtrace. Unfortunately I don't have cycles to investigate further.

Program received signal SIGSEGV, Segmentation fault.
std::string::size (this=<optimized out>) at /usr/bin/../lib64/gcc/x86_64-unknown-linux-gnu/5.2.0/../../../../include/c++/5.2.0/bits/basic_string.h:3121
3121          { return _M_rep()->_M_length; }
(gdb) bt
#0  std::string::size (this=<optimized out>) at /usr/bin/../lib64/gcc/x86_64-unknown-linux-gnu/5.2.0/../../../../include/c++/5.2.0/bits/basic_string.h:3121
#1  std::operator==<char> (__lhs=..., __rhs=...) at /usr/bin/../lib64/gcc/x86_64-unknown-linux-gnu/5.2.0/../../../../include/c++/5.2.0/bits/basic_string.h:4911
#2  pcl::VoxelGrid<pcl::PCLPointCloud2>::applyFilter (this=0x7fffffffb6a0, output=...) at /media/Workspace/Libraries/pcl-patches/filters/src/voxel_grid.cpp:270

[SergioRAgostinho]

Interesting. That issue is unrelated to the changes I did, but the new test triggered the problem.

[SergioRAgostinho]

It should be good for merging now. The only test failing is TEST (PCL, IterativeClosestPoint_PointToPlane) which is something that has been polluting the CI during this past month.

[taketwo]

Sorry for being picky, but please add spaces on lines 412 and 415 in voxel_grid.hpp. Apart from that no further requests from my side. If @jspricke is also fine, let's merge.

[SergioRAgostinho]

No worries ;) Should be ok now.