Skip to content
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.

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

add robust covariance matrix calculation to normal_3d.h #3169

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
61 changes: 61 additions & 0 deletions features/include/pcl/features/normal_3d.h
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,67 @@ namespace pcl
}
}

/** \brief Compute the normalized 3x3 covariance matrix and the centroid of a given set of points in a single loop.
* Normalized means that every entry has been divided by the number of valid entries in the point cloud.
* For small number of points, or if you want explicitly the sample-variance, scale the covariance matrix
* with n / (n-1), where n is the number of points used to calculate the covariance matrix and is returned by this function.
* Prior to the covariance matrix calculation the center point of the neighborhood is subtracted from all points.
* This is done to avoid numerical issues due to loss of significance.
* \note This method does not check whether the cloud is dense because it expects finite neighborhoods.
* \param[in] cloud the input point cloud
* \param[in] indices subset of points given by their indices
* \param[out] covariance_matrix the resultant 3x3 covariance matrix
* \param[out] centroid the centroid of the set of points in the cloud
* \return size of the input cloud.
* \ingroup common
*/
inline unsigned int
Copy link
Member

Choose a reason for hiding this comment

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

This is a big function. This inline is uncalled for.

computeMeanAndCovarianceMatrixDemeaned (const pcl::PointCloud<PointInT> &cloud,
const std::vector<int> &indices,
Eigen::Matrix<float, 3, 3> &covariance_matrix,
Eigen::Matrix<float, 4, 1> &centroid)
{
// create the buffer on the stack which is much faster than using cloud[indices[i]] and centroid as a buffer
Eigen::Matrix<float, 1, 9, Eigen::RowMajor> accu = Eigen::Matrix<float, 1, 9, Eigen::RowMajor>::Zero ();
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to explicitly specify row/col major in a vector. Also, you're accessing all element individually in the code bellow. A std::array would be just fine.

Copy link
Member

Choose a reason for hiding this comment

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

This (and the previous inline) all comes directly from the original implementation. If we want to change, then I'd change both.

Copy link
Member

Choose a reason for hiding this comment

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

Let's update both then.

Copy link
Member

Choose a reason for hiding this comment

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

We need a decision first whether we'll keep both :)

size_t point_count;
PointInT center = cloud[indices[0]];
Copy link
Member

Choose a reason for hiding this comment

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

Take a const reference the point.

Copy link
Contributor

Choose a reason for hiding this comment

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

The case if indices is an empty vector, e.g. no neighbors, must be considered.

Copy link
Member

Choose a reason for hiding this comment

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

Add that extra logic then. My original inline comment is still applicable.

PointInT p;
Copy link
Member

Choose a reason for hiding this comment

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

Under normal circumstances p should be declared inside the for-loop.


point_count = indices.size ();
for (const int &index : indices)
{
//const PointT& point = cloud[*iIt];
p.x = cloud[index].x - center.x;
p.y = cloud[index].y - center.y;
p.z = cloud[index].z - center.z;
Copy link
Member

Choose a reason for hiding this comment

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

Any chance this can be vectorized with?

inline pcl::Vector4fMap getVector4fMap () { return (pcl::Vector4fMap (data)); } \

p.getVector4fMap () = center.getVector4fMap ();

Copy link
Member

Choose a reason for hiding this comment

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

We experimented with this and also 3fMap version and there was no difference in performance.

accu [0] += p.x * p.x;
accu [1] += p.x * p.y;
accu [2] += p.x * p.z;
accu [3] += p.y * p.y;
accu [4] += p.z * p.z;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, I think there is a bug in this line, shouldn't it rather be:
accu [4] += p.y * p.z; ?

accu [5] += p.z * p.z;
accu [6] += p.x;
accu [7] += p.y;
accu [8] += p.z;
}

accu /= static_cast<float> (point_count);
centroid[0] = accu[6] + center.x;
centroid[1] = accu[7] + center.y;
centroid[2] = accu[8] + center.z;
centroid[3] = 1;
covariance_matrix.coeffRef (0) = accu [0] - accu [6] * accu [6];
covariance_matrix.coeffRef (1) = accu [1] - accu [6] * accu [7];
covariance_matrix.coeffRef (2) = accu [2] - accu [6] * accu [8];
covariance_matrix.coeffRef (4) = accu [3] - accu [7] * accu [7];
covariance_matrix.coeffRef (5) = accu [4] - accu [7] * accu [8];
covariance_matrix.coeffRef (8) = accu [5] - accu [8] * accu [8];
covariance_matrix.coeffRef (3) = covariance_matrix.coeff (1);
covariance_matrix.coeffRef (6) = covariance_matrix.coeff (2);
covariance_matrix.coeffRef (7) = covariance_matrix.coeff (5);
return (static_cast<unsigned int> (point_count));
}

protected:
/** \brief Estimate normals for all points given in <setInputCloud (), setIndices ()> using the surface in
* setSearchSurface () and the spatial locator in setSearchMethod ()
Expand Down