Refactor VoxelGrid by tin1254 · Pull Request #4813 · PointCloudLibrary/pcl

tin1254 · 2021-06-25T20:00:27Z

Goals of this PR:

Create a base class GridFilter
- Allow user to provide their own grid to the class
  - e.g.: SphericalVoxelGrid Filter Contribution #2044
- Allow later upgrading to parallelized iterations in applyFilter
- Separate the generic grid filter logic and filter specific log
  - Allowing later reduce the boilerplate code in other grid type filters
    - e.g.: ApproximateVoxelGrid, GridMinimum, VoxelGridLabel, UniformSampling
Increased performance by using std::unordered_map instead of sorting to gather points into voxel
Solve [filters] Increase the volumetric capacity of voxel_grid #4365

* Implement a base class GridFilter * Implement the basic functionality of VoxelGrid * Implement basic unit tests

* Pass grid_ to setUp as an input

kunaltyagi

Can we split this into 2 PRs?

Just grid-filter + tests
voxel-grid + eqivalency tests

kunaltyagi · 2021-06-26T03:50:32Z

+template <typename T>
+using get_grid_type = typename T::Grid;
+
+template <typename GridStruct,


Thoughts on splitting this class into 2 parts?

minimal wrt API, like FunctionFilter, and call it something like TransformAndFilter

GridFilterBase which implements the common API for VoxelFilter, etc.

Rationale:

easy to review the functionality and the API as 2 different things

Users can use TransformAndFilter class when they don't need the API. Eg: setDownsampleAllData

Do you mean some counterpart like FilterIndices? But do some transform (e.g. random sampling) instead of binary removal? But then this kind of transform filters don't have common logic/method basically

But then this kind of transform filters don't have common logic/method basically

No common logic/method needed. Just bare bone functionality. The common API will be provided by GridFilterBase

What would be included in TransformAndFilter? Like the logic inside applyFilter?

applyFilter and the absolute minimum required stuff

I would prefer not splitting for now.

The API in GridFilter now is already quite minimum IMO

We have to also consider other non binary removal filters, since in principle they also belong to TransformAndFilter

We can come back to this after finishing the grid filters

kunaltyagi · 2021-06-26T04:18:01Z

VoxelGrid looks busy, but appears to have a solid implementation. Good work :)

tin1254 · 2021-06-26T21:45:05Z

Can we split this into 2 PRs?
1. Just grid-filter + tests

2. voxel-grid + eqivalency tests

It's also good for me, but does that mean I have to develop them in separate branches simultaneously? @kunaltyagi

(I'll separate the two unit testing to two cpp for each class)

* Use numeric_limits instead of cfloat * Upgrade unsigned int to size_t * Add const to input of setters * Remove duplicate function * Remove clearing output if empty input * Move grid_to GridStruct * Reformat the documentation

kunaltyagi · 2021-06-27T01:45:52Z

Yes, if the code is split in 2 PRs, you'll have to develop them independently. But you can simply make the branch for VoxelFilter derive from GridFilter and then you'll only have to develop the features once.

You can use the rebase functionality (or merge functionality) to continually get the latest work from the GridFilter branch into the VoxelFilter branch.

tin1254 · 2021-06-27T02:08:45Z

Yes, if the code is split in 2 PRs, you'll have to develop them independently. But you can simply make the branch for VoxelFilter derive from GridFilter and then you'll only have to develop the features once.

You can use the rebase functionality (or merge functionality) to continually get the latest work from the GridFilter branch into the VoxelFilter branch.

Then I'll close this draft PR and open 2 new ones after I finish the WIP code to here, since the commits here already mixed up the implementation of the two classes

* Reduce O(N^2) output points test to O(NlogN) * Implement test for VoxelGrid protected methods

kunaltyagi · 2021-06-27T18:26:42Z

+  /** \brief Set to true if all fields need to be downsampled, or false if just XYZ.
+   * \param[in] downsample the new value (true/false)
+   */
+  inline void
+  setDownsampleAllData(const bool downsample)
+  {
+    downsample_all_data_ = downsample;
+  }
+
+  /** \brief Get the state of the internal downsampling parameter (true if all fields
+   * need to be downsampled, false if just XYZ).
+   */
+  inline bool
+  getDownsampleAllData() const
+  {
+    return downsample_all_data_;
+  }
+
+  /** \brief Set the minimum number of points required for a voxel to be used.
+   * \param[in] min_points_per_voxel the minimum number of points for required for a
+   * voxel to be used
+   */
+  inline void
+  setMinimumPointsNumberPerVoxel(const std::size_t min_points_per_voxel)
+  {
+    min_points_per_voxel_ = min_points_per_voxel;
+  }
+
+  /** \brief Return the minimum number of points required for a voxel to be used.
+   */
+  inline std::size_t
+  getMinimumPointsNumberPerVoxel() const
+  {
+    return min_points_per_voxel_;
+  }
+
+  /** \brief Set to true if leaf layout information needs to be saved for later access.
+   * \param[in] save_leaf_layout the new value (true/false)
+   */
+  inline void
+  setSaveLeafLayout(const bool save_leaf_layout)
+  {
+    save_leaf_layout_ = save_leaf_layout;
+  }
+
+  /** \brief Returns true if leaf layout information will to be saved for later access.
+   */
+  inline bool
+  getSaveLeafLayout() const
+  {
+    return save_leaf_layout_;
+  }
+
+  /** \brief Provide the name of the field to be used for filtering data. In conjunction
+   * with \a setFilterLimits, points having values outside this interval will be
+   * discarded.
+   * \param[in] field_name the name of the field that contains values used for filtering
+   */
+  inline void
+  setFilterFieldName(const std::string& field_name)
+  {
+    filter_field_name_ = field_name;
+  }
+
+  /** \brief Get the name of the field used for filtering. */
+  inline std::string const
+  getFilterFieldName() const
+  {
+    return filter_field_name_;
+  }
+
+  /** \brief Get the field filter limits (min/max) set by the user. The default values
+   * are numeric_limits<double>::min(), numeric_limits<double>::max().
+   * \param[out] limit_min the minimum allowed field value
+   * \param[out] limit_max the maximum allowed field value
+   */
+  inline void
+  getFilterLimits(double& limit_min, double& limit_max) const
+  {
+    limit_min = filter_limit_min_;
+    limit_max = filter_limit_max_;
+  }
+
+  /** \brief Set to true if we want to return the data outside the interval specified by
+   * setFilterLimits (min, max).
+   * Default: false.
+   * \param[in] limit_negative return data inside the interval (false) or outside (true)
+   */
+  inline void
+  setFilterLimitsNegative(const bool limit_negative)
+  {
+    filter_limit_negative_ = limit_negative;
+  }
+
+  /** \brief Get whether the data outside the interval (min/max) is to be returned
+   * (true) or inside (false).
+   * \param[out] limit_negative true if data \b outside the interval [min; max] is to be
+   * returned, false otherwise
+   */
+  inline void
+  getFilterLimitsNegative(bool& limit_negative) const
+  {
+    limit_negative = filter_limit_negative_;
+  }
+
+  /** \brief Get whether the data outside the interval (min/max) is to be returned
+   * (true) or inside (false).
+   * \return true if data \b outside the interval [min; max] is to be returned, false
+   * otherwise
+   */
+  inline bool
+  getFilterLimitsNegative() const
+  {
+    return filter_limit_negative_;
+  }
+
+protected:
+  /** \brief Set to true if all fields need to be downsampled, or false if just XYZ. */
+  bool downsample_all_data_;
+
+  /** \brief Set to true if leaf layout information needs to be saved in \a
+   * leaf_layout_. */
+  bool save_leaf_layout_;
+
+  /** \brief The desired user filter field name. */
+  std::string filter_field_name_;
+
+  /** \brief The minimum allowed filter value a point will be considered from. */
+  double filter_limit_min_;
+
+  /** \brief The maximum allowed filter value a point will be considered from. */
+  double filter_limit_max_;
+
+  /** \brief Set to true if we want to return the data outside (\a filter_limit_min_;\a
+   * filter_limit_max_).
+   * Default: false. */
+  bool filter_limit_negative_;
+
+  /** \brief Minimum number of points per voxel for the centroid to be computed */
+  std::size_t min_points_per_voxel_;


This is the API that makes GridFilter non-minimalistic. By keeping this in GridFilter and moving the applyFilter and friends out into TransforAndFilter, reasoning about the required API for the filter becomes easier

I see, I can try that out

tin1254 · 2021-07-01T17:25:15Z

Close and split into two new PRs

tin1254 · 2021-07-01T22:23:49Z

+    const size_t max_size = std::numeric_limits<size_t>::max();
+
+    // check unsigned int overflow/wrap-around
+    if (dy > max_size / dx || dx * dy > max_size / dz) {


By doing this, we now can use the full range of size_t

tin1254 added 3 commits June 25, 2021 16:29

Develop the backbone of refactoring VoxelGrid

1727b24

* Implement a base class GridFilter * Implement the basic functionality of VoxelGrid * Implement basic unit tests

Implement all API from the original VoxelGrid

bbdf766

Change input arguments of GridFilter::setUp

e264955

* Pass grid_ to setUp as an input

tin1254 commented Jun 25, 2021

View reviewed changes

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

Use GCC built-in function to check overflow

9fedb4f

tin1254 force-pushed the refactor_voxelgrid branch from a858b54 to 9fedb4f Compare June 26, 2021 00:00

kunaltyagi requested changes Jun 26, 2021

View reviewed changes

kunaltyagi reviewed Jun 26, 2021

View reviewed changes

Comment thread test/filters/test_grid_filter.cpp Outdated

tin1254 added 2 commits June 27, 2021 02:36

Improve code structure and style

ce68c61

* Use numeric_limits instead of cfloat * Upgrade unsigned int to size_t * Add const to input of setters * Remove duplicate function * Remove clearing output if empty input * Move grid_to GridStruct * Reformat the documentation

Fix MSVC and Mac compiling error

0306e1b

tin1254 force-pushed the refactor_voxelgrid branch from f03bd7e to 0306e1b Compare June 27, 2021 01:16

Improve and add more GridFilter unit tests

1813946

* Reduce O(N^2) output points test to O(NlogN) * Implement test for VoxelGrid protected methods

tin1254 force-pushed the refactor_voxelgrid branch from afd5da2 to 1813946 Compare June 27, 2021 15:05

kunaltyagi reviewed Jun 27, 2021

View reviewed changes

Fix get_point_type error with MSVC

e576ead

tin1254 force-pushed the refactor_voxelgrid branch from 5344891 to ce88853 Compare July 1, 2021 00:13

Implement new overflow check for VoxelGrid

064fd26

tin1254 force-pushed the refactor_voxelgrid branch from ce88853 to 064fd26 Compare July 1, 2021 00:14

Add missing API of VoxelGrid

4e9db59

tin1254 force-pushed the refactor_voxelgrid branch from 877c686 to 4e9db59 Compare July 1, 2021 12:36

tin1254 mentioned this pull request Jul 1, 2021

Grid filter base #4828

Open

tin1254 closed this Jul 1, 2021

tin1254 commented Jul 1, 2021

View reviewed changes

tin1254 mentioned this pull request Jul 2, 2021

Refactor voxel grid #4829

Open

tin1254 deleted the refactor_voxelgrid branch August 11, 2021 00:58

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kunaltyagi Jun 26, 2021

Choose a reason for hiding this comment

Uh oh!

tin1254 Jun 26, 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!

kunaltyagi Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

tin1254 Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

kunaltyagi Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

tin1254 Jun 27, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunaltyagi commented Jun 26, 2021

Uh oh!

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

Uh oh!

Uh oh!

kunaltyagi commented Jun 27, 2021

Uh oh!

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

Uh oh!

Uh oh!

kunaltyagi Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

tin1254 Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tin1254 Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tin1254 commented Jun 25, 2021 •

edited

Loading

tin1254 Jun 26, 2021 •

edited

Loading

tin1254 Jun 27, 2021 •

edited

Loading

tin1254 commented Jun 26, 2021 •

edited

Loading

tin1254 commented Jun 27, 2021 •

edited

Loading

tin1254 commented Jul 1, 2021 •

edited

Loading