Modularize euclidean clustering #4268

shrijitsingh99 · 2020-07-13T03:28:11Z

Continuing discussion from shrijitsingh99#4

segmentation/include/pcl/segmentation/impl/extract_clusters.hpp

segmentation/include/pcl/segmentation/extract_clusters.h

…rator

kunaltyagi · 2020-07-13T06:31:55Z

I recommend "Refined GitHub" plugin to move the close button away from comment button

kunaltyagi · 2020-07-13T06:32:55Z

Depends on #4269

shrijitsingh99 · 2020-07-13T06:42:49Z

I recommend "Refined GitHub" plugin to move the close button away from comment button

This plugin is pretty cool and highly useful.
Though this time I encountered a strange GitHub bug (isaacs/github#750). The PR wasn't updating with my latest commits for some reason 😕

SergioRAgostinho

Partial (3/4)

segmentation/include/pcl/segmentation/extract_clusters.h

SergioRAgostinho · 2020-07-14T07:24:08Z

common/include/pcl/common/utils.h

-    template<typename T> void
-    ignore(const T&)
+    template<typename ...T> void
+    ignore(const T&...)


merged to master in the meantime

Will rebase, once issues are resolved.

examples/segmentation/example_extract_clusters_normals.cpp

kunaltyagi · 2020-07-14T09:12:00Z

segmentation/include/pcl/segmentation/extract_clusters.h

+  * \param min_pts_per_cluster minimum number of points that a cluster may contain (default: 1)
+  * \param max_pts_per_cluster maximum number of points that a cluster may contain (default: max int)
+  * \warning It is assumed that that the tree built on the same indices and cloud as passed here,
+  * if that is not the case then things can go very wrong. For speed reasons, we only check the sizes and not the content


Please reword to be in a passive voice. This example is not quite right but should point you in the correct direction:

The cloud/indices being passed here must be the same ones used to build the tree. PCL warns if the error is explicit (sizes are equal), but doesn't check for deep equality in the interest of performance.

Better now?

Spelling error

segmentation/include/pcl/segmentation/extract_clusters.h

kunaltyagi · 2020-07-14T09:21:55Z

segmentation/include/pcl/segmentation/extract_clusters.h

+  //////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+  /** \brief Decompose a region of space into clusters based on the Euclidean distance between points
+    * \param cloud the point cloud message
+    * \param additional_filter_criteria functor which is used as an additonal criteria that all points being added to the cluster must satisfy


What's the constraints on the functor? Does []{return true;} suffice? Will [](MyClass ...A){return AnotherClass{A...};} work?

Tighten the interface appropriately:

enough leeway for user

no need to read the documentation based on error message

Done. Pick it up form

pcl/filters/include/pcl/filters/functor_filter.h

Lines 16 to 21 in c262ec6

template <typename PointT, typename Function>

constexpr static bool is_functor_for_filter_v =

pcl::is_invocable_r_v<bool,

Function,

const pcl::remove_cvref_t<pcl::PointCloud<PointT>>&,

pcl::index_t>;

Slighlty off topic, whats the reason for using pcl::remove_cvref_t and then adding const and &?

That's a brain-fart. The remove_cvref was meant for something else, but in refactoring from old version to the newer simplified checker implementation, it managed to slip through.

segmentation/include/pcl/segmentation/extract_clusters.h

Morwenn · 2020-07-16T23:45:53Z

segmentation/include/pcl/segmentation/extract_clusters.h

+constexpr static bool is_functor_for_additional_filter_criteria_v =
+    pcl::is_invocable_r_v<bool,
+                          Function,
+                          const pcl::remove_cvref_t<pcl::PointCloud<PointT>>&,


With const and reference collapsing rules, isn't this the same as const remove_volatile_t<pcl::PointCloud<PointT>>&,?

It is. Which leads to the next question, why do you care if the user supplies a type which is volatile?

I think it's more about remove_cvref_t being used to strip const and reference qualifiers, and nobody gives a thought about volatile. As far as I'm concerned I wouldn't consider volatile in such a case and would go with const pcl::PointCloud<PointT>&? directly.

Just chiming in, being the author of the original snippet this is copied from: it's a mistake from when I was taking in the functor itself (typename FunctorT) and needed this to get arguments and constrain the input to const PointCloud<PointT>& (no volatile allowed). But the design changed, and got simplified thanks to the reviews but this managed to slip in.

We know the type here, and remove_X is doing nothing of value here

SergioRAgostinho

Partial (2/4)

SergioRAgostinho · 2020-07-17T08:57:59Z

segmentation/include/pcl/segmentation/extract_clusters.h

+                          Function,
+                          const pcl::remove_cvref_t<pcl::PointCloud<PointT>>&,
+                          pcl::index_t,
+                          const pcl::remove_cvref_t<Indices>&,


same comment here.

SergioRAgostinho · 2020-07-17T09:19:48Z

segmentation/include/pcl/segmentation/extract_clusters.h

+constexpr static bool is_functor_for_additional_filter_criteria_v =
+    pcl::is_invocable_r_v<bool,
+                          Function,
+                          const pcl::remove_cvref_t<pcl::PointCloud<PointT>>&,


It is. Which leads to the next question, why do you care if the user supplies a type which is volatile?

SergioRAgostinho · 2020-07-17T09:24:55Z

segmentation/include/pcl/segmentation/extract_clusters.h

+  template <typename PointT, typename FunctorT> void
+   extractEuclideanClusters(
+      ConstCloudIterator<PointT> &it, const PointCloud<PointT> &cloud, FunctorT additional_filter_criteria,
+      const typename search::Search<PointT>::Ptr &tree, float tolerance, std::vector<PointIndices> &clusters,
+      unsigned int min_pts_per_cluster = 1, unsigned int max_pts_per_cluster = std::numeric_limits<int>::max())


You need to move the implementation of this method to the .hpp file. There's no solid reason to keep it in the .h file

SergioRAgostinho · 2020-07-17T09:42:52Z

segmentation/include/pcl/segmentation/extract_clusters.h

+  */
+  template <typename PointT, typename FunctorT> void
+   extractEuclideanClusters(
+      ConstCloudIterator<PointT> &it, const PointCloud<PointT> &cloud, FunctorT additional_filter_criteria,


I'm not liking this pattern where you pass the cloud iterator as an argument, but still pass the whole point cloud as well to the function. Please change to the following.

extractEuclideanClusters(const PointCloud<PointT> &cloud, const Indices &indices, [...]);

Overloads which don't have indices in their function signature, should supply empty indices to this method. You call the appropriate constructor for CloudIterator after checking if indices is empty or not.

I think the cloudIterator is not well engineered enough for this.

Let's kill the idea of using the cloudIterator in public interface and hide it. It's still useful for removing common code, but beyond that, cloud is needed for size check as well as passing to the user's lambda

Overloads which don't have indices in their function signature, should supply empty indices to this method

That's a problematic behavior chain in pipelines. If after filtering/previous segmentation my indices became empty, now this clustering would run on the entire point cloud 😱

stale · 2020-08-16T12:28:51Z

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

shrijitsingh99 added 8 commits July 13, 2020 07:42

Modularize extractEuclideanClusters

d54bb58

Switch to CloudIterator

1b7a763

Switch to Indices & fix noop lambda

a985a57

Fix bugs

242b8f5

Concentrate checks into main overload

0353506

Replace '.points[' with '['

d4f96b1

Fix bugs

c7d6838

Update filter lambda name

4282d2f

kunaltyagi requested changes Jul 13, 2020

View reviewed changes

kunaltyagi added changelog: enhancement Meta-information for changelog generation module: segmentation needs: more work Specify why not closed/merged yet labels Jul 13, 2020

shrijitsingh99 added 5 commits July 13, 2020 09:43

Cleanup

fbf3b4b

Use pcl::utils::ignore to avoid unused variable warnings

20ac71c

Move function implementation to .h to avoid template instantiation

795b545

Switch to search::Kdtree instead of Kdtree

bd3bc6b

Use eigen for dot product

08b5155

shrijitsingh99 added the priority: gsoc Reason for prioritization label Jul 13, 2020

shrijitsingh99 added 3 commits July 13, 2020 11:42

Rename lambdas to be more descriptive

7d4f504

Split clustering into 3 parts for cloud, indices and common cloud ite…

a2bcb28

…rator

Update error messages to be technically accurate

95204a3

shrijitsingh99 closed this Jul 13, 2020

shrijitsingh99 reopened this Jul 13, 2020

kunaltyagi added the needs: pr merge Specify why not closed/merged yet label Jul 13, 2020

kunaltyagi removed the needs: pr merge Specify why not closed/merged yet label Jul 13, 2020

Add docstring

0128a1f

SergioRAgostinho reviewed Jul 14, 2020

View reviewed changes

kunaltyagi reviewed Jul 14, 2020

View reviewed changes

shrijitsingh99 added 2 commits July 17, 2020 04:24

Add functor checks and improve docstring

228fae4

Fix nullptr bug

b6283ca

Morwenn reviewed Jul 16, 2020

View reviewed changes

SergioRAgostinho reviewed Jul 17, 2020

View reviewed changes

stale bot added the status: stale label Aug 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize euclidean clustering #4268

Modularize euclidean clustering #4268

shrijitsingh99 commented Jul 13, 2020 •

edited

Loading

kunaltyagi commented Jul 13, 2020

kunaltyagi commented Jul 13, 2020

shrijitsingh99 commented Jul 13, 2020

SergioRAgostinho left a comment

SergioRAgostinho Jul 14, 2020

shrijitsingh99 Jul 14, 2020

kunaltyagi Jul 14, 2020

shrijitsingh99 Jul 16, 2020

kunaltyagi Jul 17, 2020

kunaltyagi Jul 14, 2020

shrijitsingh99 Jul 16, 2020 •

edited

Loading

kunaltyagi Jul 17, 2020

Morwenn Jul 16, 2020

SergioRAgostinho Jul 17, 2020

Morwenn Jul 17, 2020

kunaltyagi Jul 17, 2020 •

edited

Loading

SergioRAgostinho left a comment

SergioRAgostinho Jul 17, 2020

SergioRAgostinho Jul 17, 2020

SergioRAgostinho Jul 17, 2020

SergioRAgostinho Jul 17, 2020

kunaltyagi Jul 17, 2020

kunaltyagi Jul 17, 2020

stale bot commented Aug 16, 2020

	template <typename PointT, typename Function>
	constexpr static bool is_functor_for_filter_v =
	pcl::is_invocable_r_v<bool,
	Function,
	const pcl::remove_cvref_t<pcl::PointCloud<PointT>>&,
	pcl::index_t>;

Modularize euclidean clustering #4268

Are you sure you want to change the base?

Modularize euclidean clustering #4268

Conversation

shrijitsingh99 commented Jul 13, 2020 • edited Loading

kunaltyagi commented Jul 13, 2020

kunaltyagi commented Jul 13, 2020

shrijitsingh99 commented Jul 13, 2020

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrijitsingh99 Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunaltyagi Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Aug 16, 2020

shrijitsingh99 commented Jul 13, 2020 •

edited

Loading

shrijitsingh99 Jul 16, 2020 •

edited

Loading

kunaltyagi Jul 17, 2020 •

edited

Loading