Refactor voxel grid #4829
Refactor voxel grid #4829
Conversation
There was a problem hiding this comment.
Improve performance by using std::unordered_map instead of sorting
@mvieth Should we have a perf comparison using the new benchmarks module?
tin1254
force-pushed
the
refactor_voxel_grid
branch
from
9347b5e to
a7fe430
Compare
There was a problem hiding this comment.
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
|
|
||
| /** \brief The leaf layout information for fast access to cells relative to current | ||
| * position **/ | ||
| std::vector<int> leaf_layout_; |
There was a problem hiding this comment.
vector<int> ->
vector<size_t> OR unordered_map<size_t,size_t> ?
There was a problem hiding this comment.
vector<size_t> if we maintain compatibility, else unordered_map<size_t, size_t>
tin1254
force-pushed
the
refactor_voxel_grid
branch
11 times, most recently
from
24e473e to
98e2c2d
Compare
* 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
tin1254
force-pushed
the
refactor_voxel_grid
branch
from
98e2c2d to
9f36208
Compare
tin1254
force-pushed
the
refactor_voxel_grid
branch
2 times, most recently
from
55489e2 to
058cdc2
Compare
* 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
force-pushed
the
refactor_voxel_grid
branch
7 times, most recently
from
1401a73 to
b1e8c1e
Compare
* Replace point check with existing marcos * Remove setUp check with different point types * Template equivalency check with point types
tin1254
force-pushed
the
refactor_voxel_grid
branch
from
b1e8c1e to
04b8559
Compare
* Fix error on MSVC and Mac clang
| @@ -26,22 +26,22 @@ struct Voxel { | |||
| Centroid() {} | |||
| ~Centroid() {} | |||
|
|
|||
| CentroidPoint<PointT> all_fields; | |||
| CentroidPoint<PointT>* all_fields; | |||
There was a problem hiding this comment.
segfault during move assign again, it throws error while resetting itself
There was a problem hiding this comment.
Definitely needs to be solved properly. Perhaps after GSoC. Please make a big note in the file. Current implementation is not copy-safe
| Eigen::Array4f xyz; | ||
| }; | ||
|
|
||
| Voxel(const bool downsample_all_data) : downsample_all_data_(downsample_all_data) | ||
| { | ||
| num_pt_ = 0; | ||
| if (downsample_all_data_) | ||
| centroid_.all_fields = CentroidPoint<PointT>(); | ||
| centroid_.all_fields = new CentroidPoint<PointT>(); |
There was a problem hiding this comment.
Usage of heap doesn't seem right to me. If this is "solving" the segfault, I think the issue might be elsewhere
There was a problem hiding this comment.
It does solve the segfault but I have no clue why...
|
The current explanation is a bit too technical and not very friendly for beginners |
| Inheritance structure of VoxelGrid | ||
| ---------------------------------- | ||
|
|
||
| .. image:: images/voxel_grid_code_structure.jpg |
There was a problem hiding this comment.
Would it be possible to use plantuml to generate the image and also add that code in the repo?
There was a problem hiding this comment.
Lines 358 to 360 in 85fc170
Do we have to set up the plugin?
There was a problem hiding this comment.
In case it works, I think using the source code of an image is better, as we can than modify it later if necessary.
| 5. ``TransformFilter`` | ||
|
|
||
|
|
||
| ``VoxelFilter<VoxelStruct<Voxel<PointT>,PointT>>`` is aliased as ``VoxelGrid<PointT>`` in our source code, it implements the API specified for ``VoxelGrid``. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
tin1254
force-pushed
the
refactor_voxel_grid
branch
from
9b6e67b to
1a47bc6
Compare
| 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. |
There was a problem hiding this comment.
| 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: |
There was a problem hiding this comment.
| 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 |
There was a problem hiding this comment.
| 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 |
| 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 |
There was a problem hiding this comment.
| 2. ``VoxelStruct``: define ``VoxelGrid`` filtering logic | |
| 2. ``VoxelStruct``: define the filtering logic |
| 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. |
There was a problem hiding this comment.
Add something like: Your custom Voxel Grid Filter would look like: VoxelFilter<MyCustomLogic<MyVoxel>>
|
|
||
| .. note:: | ||
| **Tips**: | ||
| If your application needs to filter some incoming point clouds repeatedly, |
There was a problem hiding this comment.
Is this tip relevant for MyVoxel? There might be a better place for this top
| 2. Define a custom VoxelStruct | ||
| ------------------------------ | ||
|
|
||
| This is a simple custom ``VoxelStruct`` withs a fixed grid size and fixed condition for selecting eligible voxels. |
There was a problem hiding this comment.
| 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. |
| .. note:: | ||
| The following member functions except our custom ``myHash`` are mandatory. | ||
|
|
||
| .. code-block:: cpp |
There was a problem hiding this comment.
Would it be possible to instead add a CMakeLists.txt + source file and refer to the lines in that instead?
| -------------------------- | ||
|
|
||
| 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`` . |
| .. code-block:: cpp | ||
|
|
||
| template <typename PointT> | ||
| using MyVoxelGrid = TransformFilter<pcl::Filter, MyVoxelStruct<PointT>, PointT>; |
There was a problem hiding this comment.
Can we use VoxelFilter here instead of TransformFilter?
tin1254
force-pushed
the
refactor_voxel_grid
branch
from
f89e81e to
2fe2200
Compare
|
Great job. |
Splitted from #4813
Description
VoxelGridsuch that it inherits fromCartesianFilterandTransformFilterstd::unordered_mapinstead of sorting for grouping points to voxelsVoxelGridimplementationVoxelGridcode structure andTransformFilterRelated
std::unordered_mapinstead of sorting for gathering points into voxelCurrent issue
CentroidPointTODO
TransformFiltertutorialleaf_layout_design