Segmentation Euclidan Clustering #4
Conversation
| @@ -45,6 +45,13 @@ | |||
|
|
|||
| namespace pcl | |||
| { | |||
| template <typename PointT, typename FunctorT> void | |||
There was a problem hiding this comment.
Missing docstring.
| clusters.push_back (r); // We could avoid a copy by working directly in the vector | ||
| } | ||
| } | ||
| auto lambda = [&](int &i, int &j, std::vector<int>& nn_indices) -> bool { |
There was a problem hiding this comment.
nn_indices can be const and no need for i and j should be passed by value. You should switch the index type to index_t and std::vector<int> to simply Indices.
There was a problem hiding this comment.
I'm also questioning if you should be passing indices as an argument to the lambda as well instead of capturing it.
| auto lambda = [&](int &i, int &j, std::vector<int>& nn_indices) -> bool { | ||
| //processed[nn_indices[j]] = true; | ||
| // [-1;1] | ||
| double dot_p = normals.points[i].normal[0] * normals.points[nn_indices[j]].normal[0] + |
There was a problem hiding this comment.
- Don't access
pointsdirectly.normals.points[i].normal[0]->normals[i].normal[0] - Use the
getVectorMapmethods and Eigen to compute the dot product. One things to consider:normals[i].normalis composed of 4 components and theoretically the last one should be set to zero. I'm not sure how well we enforce this. If this condition is true, you can use all 4 components of the vector the dot product will computed employing SIMD instructions. A very quick operation in a hot-loop. 🤑 However if this is not true, then your dot product is incorrect. I'm not sure of the performance benefit between:
a. Unconditionally, set the last component of the normal to 0 and use a 4 component vector map to get the dot product, which will use SIMD. Implies changing the users' point-cloud without their knowledge
b. Get a 3 component vector map and not care about SIMD.
There was a problem hiding this comment.
This would be a good place for executor usage, we could even do dot product using omp (though it would be only benificial if the cloud size is quite large).
I think we might encounter this problem multiple places when providing SIMD alternatives. We should just give a warning to the user and set the last element to 0.
Leaving it to the user whether he wants performance or preserve the last element.
We can also consider preserving the last element of the user is fine with the slight performance penalty.
There was a problem hiding this comment.
I think we might encounter this problem multiple places when providing SIMD alternatives. We should just give a warning to the user and set the last element to 0.
We don't provide explicit access to the 4th component of the normals, suggesting it should not be touched by users. I would set it to zero with no warning.
segmentation/include/pcl/segmentation/impl/extract_clusters.hpp
Outdated
Show resolved
Hide resolved
segmentation/include/pcl/segmentation/impl/extract_clusters.hpp
Outdated
Show resolved
Hide resolved
| @@ -86,30 +87,39 @@ pcl::extractEuclideanClusters (const PointCloud<PointT> &cloud, | |||
| if (nn_indices[j] == -1 || processed[nn_indices[j]]) // Has this point been processed before ? | |||
There was a problem hiding this comment.
You're not allowed to rely on index being -1. Does radiusSearch provide overloads which do not rely on this behavior?
There was a problem hiding this comment.
I am not sure if we even need nn_indices[j] == -1. I did some tracing and couldn't find setting of nn_indices[j] to -1. The commit history also doesn't give insight on why it was abruptly added in between since it seems to link to a PR before usage of GitHub.
segmentation/include/pcl/segmentation/impl/extract_clusters.hpp
Outdated
Show resolved
Hide resolved
shrijitsingh99
force-pushed
the
executor-integration
branch
2 times, most recently
from
1f11251 to
caabb6f
Compare
shrijitsingh99
force-pushed
the
executor-integration
branch
7 times, most recently
from
6d88bfe to
7c3b82c
Compare
No description provided.