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

Update the OpenMP implementations of normal and FPFH estimation #2278

Merged
merged 2 commits into from
Apr 18, 2018
Merged

Update the OpenMP implementations of normal and FPFH estimation #2278

merged 2 commits into from
Apr 18, 2018

Conversation

ThorstenHarter
Copy link
Contributor

The OpenMP implementations in the "features" module are missing bug fixes which have already been applied to the standard implementations some time ago.

This PR fixes two issues:

  • NormalEstimationOMP should set the normal-cloud to non-dense when the computation fails
  • FPFHEstimationOMP must skip invalid points or the kdtree-search will assert

@ThorstenHarter
Copy link
Contributor Author

The gcc build of normal_3d_omp.hpp failed, although the code is the same as in normal_3d.hpp (besides the #pragma omp and type of idx, but I have that already ruled out).
Visual Studio 2015 build works fine.
I'll have to review this file again tomorrow...

Merged the changes in 032f321 to the OMP version of the NormalEstimation
@ThorstenHarter
Copy link
Contributor Author

I have fixed the gcc/clang build and squashed my commits.
The AppVeyor build fails with a CMake error, I don't think that this is related to my changes.

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. Could you please make a minor style change? And then we are good to go.

@@ -62,7 +62,8 @@ pcl::FPFHEstimationOMP<PointInT, PointNT, PointOutT>::computeFeature (PointCloud
for (size_t idx = 0; idx < indices_->size (); ++idx)
{
int p_idx = (*indices_)[idx];
if (this->searchForNeighbors (p_idx, search_parameter_, nn_indices, nn_dists) == 0)
if (!isFinite((*input_)[p_idx]) ||
Copy link
Member

Choose a reason for hiding this comment

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

Please add a space between function name and braces to match PCL style. Also in the hunk below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Sergey, I have added a space after each of the two calls to isFinite.

Check for NaN/Inf to avoid assertion in kdtree search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants