div 0 compute3DCentroid

[greenbrettmichael]

div 0 safety for centroids calculated on clouds with only NAN/inf values
I would be happy to see why the unit test at https://github.com/PointCloudLibrary/pcl/blob/master/test/common/test_centroid.cpp#L77 did not catch this, but I was unable to get the googletest runner to start for test_centroid (it is only running 174 tests for me currently)
closes #2480

[taketwo]

The test you referenced only checks the output of the function (number of processed points), not the values in the computed centroid. As such I don't think we can expect it to fail.

pcl/common/include/pcl/common/centroid.h

Lines 56 to 63 in 72f41b6

	/** \brief Compute the 3D (X-Y-Z) centroid of a set of points and return it as a 3D vector.
	* \param[in] cloud_iterator an iterator over the input point cloud
	* \param[out] centroid the output centroid
	* \return number of valid points used to determine the centroid. In case of dense point clouds, this is the same as the size of input cloud.
	* \note if return value is 0, the centroid is not changed, thus not valid.
	* The last component of the vector is set to 1, this allows to transform the centroid vector with 4x4 matrices.
	* \ingroup common
	*/

Pay attention to the note that the centroid is not changed if no points were processed. This is not how the actual implementation behaves (it unconditionally sets centroid to zero in the very beginning). So we have a decision to make here: either we change implementation to match the documentation or the other way round. I'd probably go with the latter (shouldn't break existing code). Opinions?

[SergioRAgostinho]

Here's my thoughts:

1. At some point the user will always have to check if it is supplying 0 points to all of these functions, outside computeCentroid and friends, because regardless of the value we send out of the centroid variable, the only way to trust the result is to confirm the number of points.
2. This might be one of the few cases where I'm prone to follow what the documentation is saying because it will save us some very minor computational cycles because it's trash anyway. If I was to write this function from scratch, I would just allow the values to go to inf/nan.

[taketwo]

OK, I actually agree with you, let's follow docstring.

[greenbrettmichael]

@SergioRAgostinho @taketwo that is a good catch. I have responded to the feedback given.

[taketwo]

Could you elaborate what is this cloud[indices[i]] you are referring to?

[greenbrettmichael]

@taketwo the comment was C+V'd from https://github.com/PointCloudLibrary/pcl/blob/master/common/include/pcl/common/impl/centroid.hpp#L559 where it made sense. I can delete the comments as I don't think they add any value in these functions, or simplify to the params they are actually taking place of in their respective functions

[taketwo]

Even in that function I don't quite understand what the comment wants to say.
But anyway. I'd propose we try to take advantage of SSE optimizations. We can use Eigen4f for accumulator, and execute additions as accu += cloud_iterator->getVector4fMap(). Each addition will (hopefully) be performed as a single SSE2 instruction.

[greenbrettmichael]

@taketwo sounds good, I can add that on Sunday, as well as address the CI failure

Edit: need to delay to 12/12 due to scheduling

[stale]

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

[stale]

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

[mvieth]

Superseded by #5181