Skip to content

Modularize euclidean clustering #4268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Modularize euclidean clustering #4268

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d54bb58
Modularize extractEuclideanClusters
shrijitsingh99 Jul 13, 2020
1b7a763
Switch to CloudIterator
shrijitsingh99 Jul 13, 2020
a985a57
Switch to Indices & fix noop lambda
shrijitsingh99 Jul 13, 2020
242b8f5
Fix bugs
shrijitsingh99 Jul 13, 2020
0353506
Concentrate checks into main overload
shrijitsingh99 Jul 13, 2020
d4f96b1
Replace '.points[' with '['
shrijitsingh99 Jul 13, 2020
c7d6838
Fix bugs
shrijitsingh99 Jul 13, 2020
4282d2f
Update filter lambda name
shrijitsingh99 Jul 13, 2020
fbf3b4b
Cleanup
shrijitsingh99 Jul 13, 2020
20ac71c
Use `pcl::utils::ignore` to avoid unused variable warnings
shrijitsingh99 Jul 13, 2020
795b545
Move function implementation to `.h` to avoid template instantiation
shrijitsingh99 Jul 13, 2020
bd3bc6b
Switch to `search::Kdtree` instead of `Kdtree`
shrijitsingh99 Jul 13, 2020
08b5155
Use eigen for dot product
shrijitsingh99 Jul 13, 2020
7d4f504
Rename lambdas to be more descriptive
shrijitsingh99 Jul 13, 2020
a2bcb28
Split clustering into 3 parts for cloud, indices and common cloud ite…
shrijitsingh99 Jul 13, 2020
95204a3
Update error messages to be technically accurate
shrijitsingh99 Jul 13, 2020
0128a1f
Add docstring
shrijitsingh99 Jul 14, 2020
228fae4
Add functor checks and improve docstring
shrijitsingh99 Jul 16, 2020
b6283ca
Fix nullptr bug
shrijitsingh99 Jul 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions segmentation/include/pcl/segmentation/extract_clusters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,21 @@
namespace pcl
{

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/** \brief Decompose a region of space into clusters based on the Euclidean distance between points
* \param it cloud iterator for iterating over the 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
* \param tree the spatial locator (e.g., kd-tree) used for nearest neighbors searching
* \note the tree has to be created as a spatial locator on \a cloud and \a indices
* \param tolerance the spatial cluster tolerance as a measure in L2 Euclidean space
* \param clusters the resultant clusters containing point indices (as a vector of PointIndices)
* \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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling error

* \ingroup segmentation
*/
template <typename PointT, typename FunctorT> void
extractEuclideanClusters(
ConstCloudIterator<PointT> &it, const PointCloud<PointT> &cloud, FunctorT additional_filter_criteria,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😱

Expand Down Expand Up @@ -109,6 +124,20 @@ namespace pcl
}
}

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/** \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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@shrijitsingh99 shrijitsingh99 Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* \param tree the spatial locator (e.g., kd-tree) used for nearest neighbors searching
* \note the tree has to be created as a spatial locator on \a cloud and \a indices
* \param tolerance the spatial cluster tolerance as a measure in L2 Euclidean space
* \param clusters the resultant clusters containing point indices (as a vector of PointIndices)
* \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
* \ingroup segmentation
*/
template <typename PointT, typename FunctorT> void
extractEuclideanClusters (
const PointCloud<PointT> &cloud,
Expand All @@ -120,6 +149,21 @@ namespace pcl
extractEuclideanClusters(it, cloud, additional_filter_criteria, tree, tolerance, clusters, min_pts_per_cluster, max_pts_per_cluster);
}

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/** \brief Decompose a region of space into clusters based on the Euclidean distance between points
* \param cloud the point cloud message
* \param indices a list of point indices to use from \a cloud
* \param additional_filter_criteria functor which is used as an additonal criteria that all points being added to the cluster must satisfy
* \param tree the spatial locator (e.g., kd-tree) used for nearest neighbors searching
* \note the tree has to be created as a spatial locator on \a cloud and \a indices
* \param tolerance the spatial cluster tolerance as a measure in L2 Euclidean space
* \param clusters the resultant clusters containing point indices (as a vector of PointIndices)
* \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
* \ingroup segmentation
*/
template <typename PointT, typename FunctorT> void
extractEuclideanClusters (
const PointCloud<PointT> &cloud, const Indices &indices,
Expand Down Expand Up @@ -202,6 +246,7 @@ namespace pcl
return;
}

eps_angle = std::max(std::abs(eps_angle), M_PI);
auto cos_eps_angle = std::cos(eps_angle);
auto normal_deviation_filter = [&](index_t i, index_t j, const Indices& nn_indices) -> bool {
double dot_p = normals[i].getNormalVector3fMap().dot(normals[nn_indices[j]].getNormalVector3fMap());
Expand Down Expand Up @@ -243,6 +288,7 @@ namespace pcl
if (indices.empty())
return;

eps_angle = std::max(std::abs(eps_angle), M_PI);
auto cos_eps_angle = std::cos(eps_angle);
auto normal_deviation_filter = [&](index_t i, index_t j, Indices& nn_indices) -> bool {
double dot_p =
Expand Down