Refactor voxel grid by tin1254 · Pull Request #4829 · PointCloudLibrary/pcl

tin1254 · 2021-07-02T00:56:02Z

Splitted from #4813

Description

Design uniform code structure for existing voxel filters
Refactor VoxelGrid such that it inherits from CartesianFilter and TransformFilter
Reduce the boilerplate code in grid based filters
Improve runtime performance up to 40% faster by using std::unordered_map instead of sorting for grouping points to voxels
Implement benchmark comparing the performance of the old and new VoxelGrid implementation
Write tutorial to introduce the design concept of new VoxelGrid code structure and TransformFilter

Current issue

Throw segfault with label point types (description)
- Currently it is avoided by having raw pointer of CentroidPoint

TODO

Write TransformFilter tutorial
Finalize the leaf_layout_ design

kunaltyagi

Improve performance by using std::unordered_map instead of sorting

@mvieth Should we have a perf comparison using the new benchmarks module?

tin1254

int in vector<int> leaf_layout_ will cause a big headache after upgrading the hashing to use size_t

because the contents in the vector are the indices of voxels, which the total number of voxels can excess the range of int after enlarging the hashing range

If we want to solve limited range issue, all those API will have to be changed definitely
I would suggest to upgrade those API to use vector<size_t>, or even one step further to use unordered_map<size_t,size_t>

Because the purpose of leaf_layout_ is to access voxel index by

voxel_idx = leaf_layout_[point_hash]

it makes more sense to use unordered_map for this. Otherwise we'll need a big vector<size_t> if filtering a large point cloud

tin1254 · 2021-07-03T09:31:56Z

+
+  /** \brief The leaf layout information for fast access to cells relative to current
+   * position **/
+  std::vector<int> leaf_layout_;


vector<int> ->

vector<size_t> OR unordered_map<size_t,size_t> ?

vector<size_t> if we maintain compatibility, else unordered_map<size_t, size_t>

* Implement GridFilter for the common members * Implement TransformFilter for the abstracted filtering logic

* Showcase the bare bone structure of GridStruct * Test the functions of GridStruct required in applyFilter * Test different type of grid

* Move applyFilter to protected * Remove passing GridFilter pointer to GridStruct as can be done with static cast

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

* Declare new variables for testing instead of using variables from base class

* Move hashPoint, move checkIfOverflow and rename to checkHashRange

* 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

* 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

kunaltyagi · 2021-08-22T01:56:21Z

    ~Centroid() {}

-    CentroidPoint<PointT> all_fields;
+    CentroidPoint<PointT>* all_fields;


unique_ptr instead?

segfault during move assign again, it throws error while resetting itself

Definitely needs to be solved properly. Perhaps after GSoC. Please make a big note in the file. Current implementation is not copy-safe

kunaltyagi · 2021-08-22T01:58:03Z

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


Usage of heap doesn't seem right to me. If this is "solving" the segfault, I think the issue might be elsewhere

It does solve the segfault but I have no clue why...

kunaltyagi · 2021-08-22T10:23:24Z

The current explanation is a bit too technical and not very friendly for beginners

kunaltyagi · 2021-08-22T10:17:38Z

+Inheritance structure of VoxelGrid
+----------------------------------
+
+.. image:: images/voxel_grid_code_structure.jpg


Would it be possible to use plantuml to generate the image and also add that code in the repo?

pcl/doc/doxygen/doxyfile.in

Lines 358 to 360 in 85fc170

PLANTUML_JAR_PATH =

PLANTUML_CFG_FILE =

PLANTUML_INCLUDE_PATH =

Do we have to set up the plugin?

I actually don't know. I was referring to still adding a png/jpg, but adding a source code as well. If we can use plantuml in doxygen, that'd be crazy.

@larshg @SunBlack Any thoughts?

In case it works, I think using the source code of an image is better, as we can than modify it later if necessary.

kunaltyagi · 2021-08-22T10:21:40Z

+5. ``TransformFilter``
+
+
+``VoxelFilter<VoxelStruct<Voxel<PointT>,PointT>>`` is aliased as ``VoxelGrid<PointT>`` in our source code, it implements the API specified for ``VoxelGrid``.


Why don't we keep things simple and just focus on one class at a time instead of all at once?

TransformFilter is separate, so it doesn't factor in the tutorial for VoxelGrid at all

VoxelStruct without template is used to explain the basic needs for VoxelFilter

Thoughts?

the basic needs for VoxelFilter

like requirements of GridStruct?

VoxelStruct without template

But would it be better if we introduce also Voxel along side? (with template)

TransformFilter is separate, so it doesn't factor in the tutorial for VoxelGrid at all

sounds good, I will write a separate tutorial for TransformFilter .

Yes, like requirements of GridStruct, but in a very-basic manner.

But would it be better if we introduce also Voxel along side?

You can add it, but it's still a tutorial. Make it as approachable for a new comer as possible.

I rewrote it yesterday to try to just introduce the Voxel, VoxelStruct (and its requirement), and VoxelFilter, the three main pieces of VoxelGrid, while trying to minimize the stuff related to TransformFilter.

Could you take a look to see if it's newcomer-friendly enough? (I don't have much experience in writing such tutorials)

kunaltyagi · 2021-08-23T13:16:42Z

+Introduction
+============
+
+This document is intended to explain our new ``VoxelGrid`` design and help you implementing your own grid based filters easily by reusing our implementations.


Suggested change

This document is intended to explain our new ``VoxelGrid`` design and help you implementing your own grid based filters easily by reusing our implementations.

This document is intended to explain our new ``VoxelGrid`` design and help you implement your own grid based filters easily by reusing our building blocks.

kunaltyagi · 2021-08-23T13:17:59Z

+
+We will explain the pieces composing ``VoxelGrid``, then elaborate their purpose and demonstrate how they can be reused to implement your custom grid filters.
+
+In a high level view, ``VoxelGrid`` is comprised from three important classes:


Suggested change

In a high level view, ``VoxelGrid`` is comprised from three important classes:

``VoxelGrid`` is composed using three important classes:

kunaltyagi · 2021-08-23T13:18:36Z

+
+In a high level view, ``VoxelGrid`` is comprised from three important classes:
+
+1. ``Voxel``: define the information storing in a single voxel and its operations


Suggested change

1. ``Voxel``: define the information storing in a single voxel and its operations

1. ``Voxel``: define the information stored in a single voxel and its operations

kunaltyagi · 2021-08-23T13:18:53Z

+In a high level view, ``VoxelGrid`` is comprised from three important classes:
+
+1. ``Voxel``: define the information storing in a single voxel and its operations
+2. ``VoxelStruct``: define ``VoxelGrid`` filtering logic


Suggested change

2. ``VoxelStruct``: define ``VoxelGrid`` filtering logic

2. ``VoxelStruct``: define the filtering logic

kunaltyagi · 2021-08-23T13:20:22Z

+2. ``VoxelStruct``: define ``VoxelGrid`` filtering logic
+3. ``VoxelFilter``: implement common set of API for voxel grid filters
+
+Together they form ``VoxelFilter<VoxelStruct<Voxel<PointT>,PointT>>`` which is is aliased as ``VoxelGrid<PointT>`` internally.


Add something like: Your custom Voxel Grid Filter would look like: VoxelFilter<MyCustomLogic<MyVoxel>>

kunaltyagi · 2021-08-23T13:25:10Z

+
+.. note::
+  **Tips**:
+  If your application needs to filter some incoming point clouds repeatedly,


Is this tip relevant for MyVoxel? There might be a better place for this top

kunaltyagi · 2021-08-23T13:25:40Z

+2. Define a custom VoxelStruct
+------------------------------
+
+This is a simple custom ``VoxelStruct`` withs a fixed grid size and fixed condition for selecting eligible voxels.


Suggested change

This is a simple custom ``VoxelStruct`` withs a fixed grid size and fixed condition for selecting eligible voxels.

This is a simple custom ``VoxelStruct`` with a fixed grid size and fixed condition for selecting eligible voxels.

kunaltyagi · 2021-08-23T13:26:47Z

+.. note::
+  The following member functions except our custom ``myHash`` are mandatory.
+
+.. code-block:: cpp


Would it be possible to instead add a CMakeLists.txt + source file and refer to the lines in that instead?

kunaltyagi · 2021-08-23T13:27:52Z

+--------------------------
+
+With our custom ``Voxel`` and ``VoxelStruct``, we can pass them to ``TransformFilter`` and it will handle everything for you.
+Here we pass ``pcl::Filter`` for our non binary removal filter. For binary removal filters, you should pass ``pcl::FilterIndices`` .


What's binary removal?

kunaltyagi · 2021-08-23T13:28:22Z

+.. code-block:: cpp
+
+  template <typename PointT>
+  using MyVoxelGrid = TransformFilter<pcl::Filter, MyVoxelStruct<PointT>, PointT>;


Can we use VoxelFilter here instead of TransformFilter?

BaltashovIlia · 2021-09-16T13:08:03Z

Great job.
On Ryzen 5800x + GCC 9.3.0 + pcl default compilation settings:

----------------------------------------------------------------------------
Benchmark                                  Time             CPU   Iterations
----------------------------------------------------------------------------
table_VoxelGrid                         3.56 ms         3.56 ms          197
table_ApproximateVoxelGrid              2.88 ms         2.88 ms          243
table_FunctorVoxelGrid                  2.27 ms         2.27 ms          308
milk_cartoon_VoxelGrid                  4.63 ms         4.63 ms          151
milk_cartoon_ApproximateVoxelGrid       3.22 ms         3.22 ms          217
milk_cartoon_FunctorVoxelGrid           2.66 ms         2.66 ms          261
office_VoxelGrid                        8.06 ms         8.06 ms           86
office_ApproximateVoxelGrid             4.98 ms         4.98 ms          140
office_FunctorVoxelGrid                 6.02 ms         6.02 ms          105
five_people_VoxelGrid                   8.18 ms         8.18 ms           85
five_people_ApproximateVoxelGrid        4.70 ms         4.70 ms          149
five_people_FunctorVoxelGrid            4.04 ms         4.04 ms          171

kunaltyagi reviewed Jul 2, 2021

View reviewed changes

Comment thread filters/include/pcl/filters/experimental/voxel_grid.h Outdated

Comment thread filters/include/pcl/filters/experimental/voxel_grid.h Outdated

tin1254 force-pushed the refactor_voxel_grid branch from 9347b5e to a7fe430 Compare July 2, 2021 23:59

tin1254 commented Jul 3, 2021

View reviewed changes

mvieth reviewed Jul 3, 2021

View reviewed changes

tin1254 force-pushed the refactor_voxel_grid branch 11 times, most recently from 24e473e to 98e2c2d Compare July 7, 2021 19:46

tin1254 added 10 commits July 7, 2021 21:55

Implement the base of grid filters

21328d4

* Implement GridFilter for the common members * Implement TransformFilter for the abstracted filtering logic

Implement unit tests for the grid filter base

9ffd7b5

* Showcase the bare bone structure of GridStruct * Test the functions of GridStruct required in applyFilter * Test different type of grid

Improve grid filter base comment

488decb

Add nan point check to applyFilter

64afcb6

Swap the inheriting relation of grid filter base

99821c6

* Move applyFilter to protected * Remove passing GridFilter pointer to GridStruct as can be done with static cast

Improve grid filter base based on review

301416d

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

Improve code style in new VoxelGrid unit test

0e3aad5

* Declare new variables for testing instead of using variables from base class

Remove dedicated functions from GridFilterBase

651b930

Move functions of hashing point to base

dc6aa39

* Move hashPoint, move checkIfOverflow and rename to checkHashRange

Remove initializer list in grid filter base

e911680

tin1254 force-pushed the refactor_voxel_grid branch from 98e2c2d to 9f36208 Compare July 7, 2021 19:56

Remove PassThrough behavior in GridFilterBase

abce11e

tin1254 force-pushed the refactor_voxel_grid branch 2 times, most recently from 55489e2 to 058cdc2 Compare July 10, 2021 23:26

tin1254 changed the title ~~[WIP] Refactor voxel grid~~ Refactor voxel grid Jul 10, 2021

tin1254 added 8 commits August 16, 2021 17:20

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 7 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

Fix new VoxelGrid seg fault with label point types

755a61c

* Fix error on MSVC and Mac clang

kunaltyagi reviewed Aug 22, 2021

View reviewed changes

tin1254 added 2 commits August 23, 2021 00:02

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

kunaltyagi requested changes Aug 23, 2021

View reviewed changes

Implement centroid with boost variant

2fe2200

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

	PLANTUML_JAR_PATH =
	PLANTUML_CFG_FILE =
	PLANTUML_INCLUDE_PATH =

		5. ``TransformFilter``


		``VoxelFilter<VoxelStruct<Voxel<PointT>,PointT>>`` is aliased as ``VoxelGrid<PointT>`` in our source code, it implements the API specified for ``VoxelGrid``.

	This document is intended to explain our new ``VoxelGrid`` design and help you implementing your own grid based filters easily by reusing our implementations.
	This document is intended to explain our new ``VoxelGrid`` design and help you implement your own grid based filters easily by reusing our building blocks.


		We will explain the pieces composing ``VoxelGrid``, then elaborate their purpose and demonstrate how they can be reused to implement your custom grid filters.

		In a high level view, ``VoxelGrid`` is comprised from three important classes:

	In a high level view, ``VoxelGrid`` is comprised from three important classes:
	``VoxelGrid`` is composed using three important classes:


		In a high level view, ``VoxelGrid`` is comprised from three important classes:

		1. ``Voxel``: define the information storing in a single voxel and its operations

	2. ``VoxelStruct``: define ``VoxelGrid`` filtering logic
	2. ``VoxelStruct``: define the filtering logic

	This is a simple custom ``VoxelStruct`` withs a fixed grid size and fixed condition for selecting eligible voxels.
	This is a simple custom ``VoxelStruct`` with a fixed grid size and fixed condition for selecting eligible voxels.

Uh oh!

Conversation

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

Uh oh!

Description

Related

Current issue

TODO

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tin1254 left a comment

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunaltyagi commented Aug 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tin1254 Aug 22, 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!

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!

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!

BaltashovIlia commented Sep 16, 2021

Uh oh!

Reviewers

tin1254 commented Jul 2, 2021 •

edited

Loading

tin1254 Aug 22, 2021 •

edited

Loading