Refactor voxel grid by tin1254 · Pull Request #2 · tin1254/pcl

tin1254 · 2021-08-09T10:07:18Z

No description provided.

kunaltyagi

❤️

kunaltyagi · 2021-08-14T01:39:48Z

+
+template <typename>
+void
+EXPECT_POINT_EQ(const PointXYZRGB& pt1, const PointXYZRGB& pt2)


Is the point type unknown that we have to use template specialization?

pcl/test/filters/test_experimental_voxel_grid.cpp

Lines 74 to 82 in 8c893c0

auto pt_cmp = [](const PointT& p1, const PointT& p2) -> bool {

return p1.x > p2.x || (p1.x == p2.x && p1.y > p2.y) ||

(p1.x == p2.x && p1.y == p2.y && p1.z > p2.z);

};

std::sort(pc1.begin(), pc1.end(), pt_cmp);

std::sort(pc2.begin(), pc2.end(), pt_cmp);

for (size_t i = 0; i < pc1.size(); ++i)

EXPECT_POINT_EQ<PointT>(pc1.at(i), pc2.at(i));

Is there any better way than using template specialization? because I don't want to duplicate the sorting code here

I couldn't think of any. LGTM

* Take leaf size instead of inverse leaf size * Fix documentation * Adapt unit tests

* Remove the first multiplication as divb_mul_[0] is always 1 * Add test for 2D grid * Move outside as free functions

* Remove confusing comment in applyFilter * Add line endings * Fix typo

* Inherit from TransformFilter and GridFilterBase * Complete the API of old VoxelGrid * Implement new way for checking unsigned int overflow * Improve performance by switching from sorting to unordered_map

* Inherit from GridFilterBase instead of TransformFilter

* Avoid storing two centroids with different downsample_all_data option by getVector4fMap

* Remove hashPoint and checkIfOverflow from GridStruct * Pass TransformFilter pointer to setUp for further refactoring * Remove getDerived() * Remove redundant hashing unit test

* Avoid depending on variables generated by the grid filters, and rename to HashingPoint * Remove friend for unit testing

* Input arguments are passed as const * Add const for unchange variables * Remove boilerplate code and comments * Remove initializer list and initialize default variables directly

* TransformFilter holds GridStruct instead of inheriting it * Add helper functions in VoxelStructT for accessing Grid

* Add office1.pcd and fivepeople.pcd

* Allocate new object is faster than clear by benchmarking

* Avoid repeatedly allocate voxels for by unordered_map::clear()

* Reduce copying unnecessary field when only downsampling xyz

* Pass TransformFilter point to constructor of GridStruct instead of setUp

* Allow reusing VoxelStruct members

* Rename GridIterator to iterator * Adapt changes in experimental VoxelGrid * Adapt changes in experimental VoxelGrid test

* Template VoxelStruct with FilterBase

tin1254 · 2021-08-20T12:49:21Z

+  {
+    num_pt_ = 0;
+    if (downsample_all_data_)
+      centroid_.all_fields = CentroidPoint<PointT>();


I have been trying to fix the seg fault with the new VoxelGridILabel for a while but didn't success. I found our approach to union with CentroidPoint doesn't work with msvc and clang on mac for point types with label (e.g. PointXYZRGBL)

It throws a seg fault when assigning a new CentroidPoint to all_fields, but doesn't have error if assigning to a normal variable (not union). When I look at the call stack, the error happens during move assignment of std::map<std::uint32_t, std::size_t> which should come from:

pcl/common/include/pcl/common/impl/accumulators.hpp

Line 189 in 7194dd7

std::map<std::uint32_t, std::size_t> labels;

Do you have any idea why this happens? @kunaltyagi @mvieth I feel like this is out of my knowledge..

Call stack with msvc

doesn't work for point types with label

Is this true for even simple stuff like PointXYZL?

yes, it throws the same error and call stack

What about points with intensity?

It works all fine

The sole difference between XYZI and XYZL is that intensity is inside a struct in the union instead of just a variable

But I think the biggest difference is the types with labels has std::map in the struct, other are all float or float array

In the CentroidPoint you mean?

yes, accumulator specifically

kunaltyagi · 2021-08-20T18:27:32Z

+
+template <typename>
+void
+EXPECT_POINT_EQ(const PointXYZRGB& pt1, const PointXYZRGB& pt2)


I couldn't think of any. LGTM

kunaltyagi · 2021-08-20T19:03:04Z

+  union Centroid {
+    Centroid() {}
+    ~Centroid() {}
+
+    CentroidPoint<PointT> all_fields;
+    Eigen::Array4f xyz;
+  };


Do you think some alignment issue due to Eigen is causing the issue here? CentroidPoint uses PCL_MAKE_ALIGNED_OPERATOR_NEW and Eigen definitely has it's own alignment.

Could we try using something similar?

I tried both macro from PCL and Eigen and it's still the same.

Could you try to use valgrind to find why there's a weird issue with stack data and not heap data?

I'm still investigating this but the trace always go deep into stl library makes it difficult to debug

FYI

track by valgrind

==120237== Conditional jump or move depends on uninitialised value(s) ==120237== at 0x125F59: std::_Rb_tree<unsigned int, std::pair<unsigned int const, unsigned long>, std::_Select1st<std::pair<unsigned int const, unsigned long> >, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >::_M_erase(std::_Rb_tree_node<std::pair<unsigned int const, unsigned long> >*) (stl_tree.h:1911) ==120237== by 0x1232FB: std::_Rb_tree<unsigned int, std::pair<unsigned int const, unsigned long>, std::_Select1st<std::pair<unsigned int const, unsigned long> >, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >::clear() (stl_tree.h:1266) ==120237== by 0x12AB8B: std::_Rb_tree<unsigned int, std::pair<unsigned int const, unsigned long>, std::_Select1st<std::pair<unsigned int const, unsigned long> >, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >::_M_move_assign(std::_Rb_tree<unsigned int, std::pair<unsigned int const, unsigned long>, std::_Select1st<std::pair<unsigned int const, unsigned long> >, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >&, std::integral_constant<bool, true>) (stl_tree.h:1681) ==120237== by 0x125E65: std::_Rb_tree<unsigned int, std::pair<unsigned int const, unsigned long>, std::_Select1st<std::pair<unsigned int const, unsigned long> >, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >::operator=(std::_Rb_tree<unsigned int, std::pair<unsigned int const, unsigned long>, std::_Select1st<std::pair<unsigned int const, unsigned long> >, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >&&) (stl_tree.h:1723) ==120237== by 0x120FD4: std::map<unsigned int, unsigned long, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >::operator=(std::map<unsigned int, unsigned long, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned long> > >&&) (stl_map.h:100) ==120237== by 0x121002: pcl::detail::AccumulatorLabel::operator=(pcl::detail::AccumulatorLabel&&) (accumulators.hpp:211) ==120237== by 0x121030: boost::fusion::vector_detail::store<2ul, pcl::detail::AccumulatorLabel>::operator=(boost::fusion::vector_detail::store<2ul, pcl::detail::AccumulatorLabel>&&) (vector.hpp:156) ==120237== by 0x121094: boost::fusion::vector_detail::vector_data<std::integer_sequence<unsigned long, 0ul, 1ul, 2ul>, pcl::detail::AccumulatorXYZ, pcl::detail::AccumulatorRGBA, pcl::detail::AccumulatorLabel>::operator=(boost::fusion::vector_detail::vector_data<std::integer_sequence<unsigned long, 0ul, 1ul, 2ul>, pcl::detail::AccumulatorXYZ, pcl::detail::AccumulatorRGBA, pcl::detail::AccumulatorLabel>&&) (vector.hpp:184) ==120237== by 0x1210C2: boost::fusion::vector<pcl::detail::AccumulatorXYZ, pcl::detail::AccumulatorRGBA, pcl::detail::AccumulatorLabel>::operator=(boost::fusion::vector<pcl::detail::AccumulatorXYZ, pcl::detail::AccumulatorRGBA, pcl::detail::AccumulatorLabel>&&) (vector.hpp:262) ==120237== by 0x121106: pcl::CentroidPoint<pcl::PointXYZRGBL>::operator=(pcl::CentroidPoint<pcl::PointXYZRGBL>&&) (centroid.h:1022) ==120237== by 0x1211B5: pcl::experimental::Voxel<pcl::PointXYZRGBL>::Voxel(bool) (voxel_grid.h:37) ==120237== by 0x11D7D8: pcl::experimental::LabeledVoxel::LabeledVoxel(bool) (voxel_grid_label.h:27)

It goes through several move assign, and when I tried to declare my move assign function the trace go somewhere also related to std::map in AccumulatorLabel

I'm trying to use boost::variant to avoid this but it is only a bit slower

Since we will transit to std::variant anyway once we move to C++17, I think it is better to use boost::variant so that the transition can be easier.

althrough we might sacrifice some performance in theory (boost::get has a runtime check)

Performance

But runtime results are very similar in my benchmark

union

---------------------------------------------------------------------------- Benchmark Time CPU Iterations ---------------------------------------------------------------------------- table_VoxelGrid 6.26 ms 6.26 ms 111 table_ApproximateVoxelGrid 5.08 ms 5.08 ms 137 table_FunctorVoxelGrid 3.99 ms 3.99 ms 174 milk_cartoon_VoxelGrid 8.01 ms 8.01 ms 88 milk_cartoon_ApproximateVoxelGrid 5.49 ms 5.49 ms 126 milk_cartoon_FunctorVoxelGrid 4.61 ms 4.61 ms 151 office_VoxelGrid 13.4 ms 13.4 ms 51 office_ApproximateVoxelGrid 8.10 ms 8.10 ms 85 office_FunctorVoxelGrid 11.7 ms 11.7 ms 55 five_people_VoxelGrid 14.0 ms 14.0 ms 51 five_people_ApproximateVoxelGrid 7.48 ms 7.47 ms 92 five_people_FunctorVoxelGrid 8.50 ms 8.50 ms 76

boost::variant

---------------------------------------------------------------------------- Benchmark Time CPU Iterations ---------------------------------------------------------------------------- table_VoxelGrid 6.42 ms 6.42 ms 110 table_ApproximateVoxelGrid 5.09 ms 5.08 ms 138 table_FunctorVoxelGrid 4.16 ms 4.16 ms 168 milk_cartoon_VoxelGrid 8.17 ms 8.17 ms 85 milk_cartoon_ApproximateVoxelGrid 5.56 ms 5.56 ms 122 milk_cartoon_FunctorVoxelGrid 4.87 ms 4.87 ms 139 office_VoxelGrid 13.6 ms 13.6 ms 52 office_ApproximateVoxelGrid 8.08 ms 8.08 ms 85 office_FunctorVoxelGrid 11.3 ms 11.3 ms 59 five_people_VoxelGrid 13.7 ms 13.7 ms 51 five_people_ApproximateVoxelGrid 7.49 ms 7.49 ms 92 five_people_FunctorVoxelGrid 8.38 ms 8.38 ms 80

* Replace point check with existing marcos * Remove setUp check with different point types * Template equivalency check with point types

* Fix error on MSVC and Mac clang

tin1254 force-pushed the refactor_voxel_grid branch from 8c8f13a to 943d33e Compare August 11, 2021 13:08

kunaltyagi approved these changes Aug 14, 2021

View reviewed changes

kunaltyagi reviewed Aug 16, 2021

View reviewed changes

Comment thread benchmarks/filters/voxel_grid.cpp Outdated

Comment thread benchmarks/filters/voxel_grid.cpp Outdated

tin1254 added 27 commits August 16, 2021 17:20

Improve hashing API

4b3fa22

* Take leaf size instead of inverse leaf size * Fix documentation * Adapt unit tests

Improve hashPoint and add overload for 2D grid

bdb5fd0

* Remove the first multiplication as divb_mul_[0] is always 1 * Add test for 2D grid * Move outside as free functions

Improve grid filter base based on review

7124853

* Remove confusing comment in applyFilter * Add line endings * Fix typo

Implement refactored VoxelGrid

dfc3853

* Inherit from TransformFilter and GridFilterBase * Complete the API of old VoxelGrid * Implement new way for checking unsigned int overflow * Improve performance by switching from sorting to unordered_map

Update the inheritance the relation of VoxelGrid

ec324bf

* Inherit from GridFilterBase instead of TransformFilter

Improve storing centroid in new VoxelGrid

8e1bb80

* Avoid storing two centroids with different downsample_all_data option by getVector4fMap

Implement unit tests for new VoxelGrid

440a9e4

Move VoxelGrid functions to GridStruct

760872e

Move functions of hashing point to base

8f81565

* Remove hashPoint and checkIfOverflow from GridStruct * Pass TransformFilter pointer to setUp for further refactoring * Remove getDerived() * Remove redundant hashing unit test

Refactor hashing point unit test

2c50fb2

* Avoid depending on variables generated by the grid filters, and rename to HashingPoint * Remove friend for unit testing

Improve code style in new VoxelGrid

d0e7115

* Input arguments are passed as const * Add const for unchange variables * Remove boilerplate code and comments * Remove initializer list and initialize default variables directly

Implement leaf layout tests for new VoxelGrid

f107d6a

Speed up new VoxelGrid by caching options

b382fb2

Add benchmark for new VoxelGrid

d7e50b0

Remove PassThrough behavior in new VoxelGrid

6d7c6c2

Define pcl::experimental::optional

ed5d59a

Refactor new VoxelGrid to avoid inheritance

36e7d4c

* TransformFilter holds GridStruct instead of inheriting it * Add helper functions in VoxelStructT for accessing Grid

Remove const_cast in VoxelGrid

b176bf2

Add comments and documentation in VoxelGrid

ddc25bf

Rename GridFilterBase to CartesianFilter

07e4957

Change hashPoint input type in new VoxelGrid

49141d8

Add more testing clouds to filter benchmark

cbfca85

* Add office1.pcd and fivepeople.pcd

Improve new VoxelGrid performance

9d3d57f

* Allocate new object is faster than clear by benchmarking

Implement CentroidPoint::clear()

d998228

* Avoid repeatedly allocate voxels for by unordered_map::clear()

Implement voxel as union of CentroidPoint and Array

0b25941

* Reduce copying unnecessary field when only downsampling xyz

Adapt hashing API changes

39f72ef

Implement adjusting max_load_factor in voxel filters

6f49a8b

tin1254 added 9 commits August 16, 2021 17:20

Rename cmake name of new voxel grid test

f4859f0

Adapt TransformFilter API change

9523f05

* Pass TransformFilter point to constructor of GridStruct instead of setUp

Implement placeholder for VoxelStruct::setUp

d18c24a

* Allow reusing VoxelStruct members

Adapt hashing API change

7ce3e6a

Adapt TransformFilter API changes

b6d2588

* Rename GridIterator to iterator * Adapt changes in experimental VoxelGrid * Adapt changes in experimental VoxelGrid test

Update license year

f528d6f

Allow reusing VoxelStruct

aba077d

* Template VoxelStruct with FilterBase

Fix unit test cmake name

ceb6d53

Enable computing centroid with SSE by Eigen

2a47736

tin1254 force-pushed the refactor_voxel_grid branch 3 times, most recently from 5ad2687 to 9eaa8e9 Compare August 18, 2021 14:36

tin1254 commented Aug 20, 2021

View reviewed changes

tin1254 force-pushed the refactor_voxel_grid branch 2 times, most recently from 279a71b to e8b077e Compare August 20, 2021 18:05

kunaltyagi reviewed Aug 20, 2021

View reviewed changes

tin1254 force-pushed the refactor_voxel_grid branch 2 times, most recently from 1401a73 to b1e8c1e Compare August 21, 2021 21:14

tin1254 added 3 commits August 22, 2021 00:39

Pass voxel filters as template for benchmarking

d07c240

Improve experimental VoxelGrid test

09325e3

* Replace point check with existing marcos * Remove setUp check with different point types * Template equivalency check with point types

Improve code style of new voxel grid tests

04b8559

tin1254 force-pushed the refactor_voxel_grid branch from b1e8c1e to 04b8559 Compare August 21, 2021 22:39

tin1254 added 3 commits August 22, 2021 01:49

Fix new VoxelGrid seg fault with label point types

755a61c

* Fix error on MSVC and Mac clang

Add documentation for new voxel grid design

220ea44

Fix returning size of non-empty voxels

1a47bc6

tin1254 force-pushed the refactor_voxel_grid branch from 9b6e67b to 1a47bc6 Compare August 22, 2021 22:02

tin1254 mentioned this pull request Aug 23, 2021

Refactor voxel grid PointCloudLibrary/pcl#4829

Open

Implement centroid with boost variant

2fe2200

tin1254 force-pushed the refactor_voxel_grid branch from f89e81e to 2fe2200 Compare September 6, 2021 20:48

	auto pt_cmp = [](const PointT& p1, const PointT& p2) -> bool {
	return p1.x > p2.x \|\| (p1.x == p2.x && p1.y > p2.y) \|\|
	(p1.x == p2.x && p1.y == p2.y && p1.z > p2.z);
	};
	std::sort(pc1.begin(), pc1.end(), pt_cmp);
	std::sort(pc2.begin(), pc2.end(), pt_cmp);

	for (size_t i = 0; i < pc1.size(); ++i)
	EXPECT_POINT_EQ<PointT>(pc1.at(i), pc2.at(i));

Conversation

tin1254 commented Aug 9, 2021

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tin1254 Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Call stack with msvc

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tin1254 Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunaltyagi Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tin1254 Sep 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

FYI

Uh oh!

Choose a reason for hiding this comment

Performance

union

boost::variant

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tin1254 Aug 16, 2021 •

edited

Loading

tin1254 Aug 21, 2021 •

edited

Loading

kunaltyagi Aug 23, 2021 •

edited

Loading

tin1254 Sep 5, 2021 •

edited

Loading