pcl::ISSKeypoint3D: Fixed segfault with search surface enabled.

[pmdifran]

Changed to index surface_ with search query results, intead of input_.

Prevents segfault when surface_ is set with setSearchSurface.

[ghost]

This fails at line 440, When input_->size() < surface_->size():

Line 440: if (third_eigen_value_[index] < third_eigen_value_[j])

This is because for the max checks, we should be searching input_, not surface_. This search happens at Line 431.

Line 431: this->searchForNeighbors (index, non_max_radius_, nn_indices, nn_distances); should be searching input_, and not surface_, in this case.

[mvieth]

I don't fully understand the problem yet, especially how your last comment relates to the changes you have made. Considering that searchForNeighbors searches in input_, isn't it correct that nn_indices is used with input_? Ideally, you would add a simple test in test/keypoints/test_iss_3d.cpp that fails without the fix, but passes with the fix.

[ghost]

@mvieth sorry for the confusion. This is for computing ISSKeypoints on a subset of the data, whilst using the full cloud resolution to calculate the descriptors. I can write a test to go along with this. To clarify:

Yes - searchForNeighbors uses (*input_)[index] as the query point for the search, but it is still searching surface_.

Note that third_eigen_value_ is the size of input_. Because searchForNeighbors is returning indices corresponding to surface_, we get segfault on Line 440.

The search at Line 431 should be getting nearest neighbors in input_ instead (i.e. the indices of points that we have shape descriptors for). See snippet below.

https://github.com/PointCloudLibrary/pcl/blob/00a55faaa332c6ca1ebb7619ef6139e993464826/keypoints/include/pcl/keypoints/impl/iss_3d.hpp#L416-L444

[mvieth]

Ah yes, you are right, searchForNeighbors is searching in surface_. So your change in line 194 looks correct to me. But the problem in lines 431, 440, ... you are describing, is a separate problem, not related to the change you made in getScatterMatrix, right?

[kunaltyagi]

Essentially, the code is making an assumption that either:

• surface and input clouds are the same
• surface and input clouds are similar (in number of points)

Are these assumptions invalid?

A simple fix to segfault would be a nice warning when the input cloud is smaller than the surface cloud (seg-fault land)

[ghost]

I think that these assumptions really limit the ISSKeypoint detection, and yes - are invalid for someone using this method.

What if I want to ensure that keypoints are a minimum distance from each other?

It doesn't make sense to compute them at the full cloud resolution, and then apply a voxel filter - if we could alternatively filter input_ from the start (and save ourselves from a bunch of unnecessary computations).

To fix segfault at Line 440, could we:

• check if surface_ == input_
• if they differ, construct a KdTree for input_, with the same type as KdTreePtr
• Use this KdTree to search for input neighbors within the non_max_radius_ (i.e. Line 431 above)

[kunaltyagi]

@pmdifran Let's add a test which will detect the segfault?

Use different KdTree to search for input neighbors within the non_max_radius_

Sounds reasonable. Thoughts @mvieth ?

[mvieth]

Sounds generally reasonable, but I have some doubts how much sense this makes in the (ISS) keypoint selection: in the scenario you @pmdifran are describing, you would have a full point cloud and set it as surface_, and take some evenly spaced points from that, e.g. by voxel filter, and use that as input_, right? This would already be a keypoint selection in itself. If you then use ISSKeypoint3D, the algorithm has much fewer points to choose from (only the ones in input_), and will (if I see it correctly) give you keypoints of lower "quality", not the optimal keypoints as if the algorithm had total freedom of choice. You should also consider that the non_max_radius_ ensures that the keypoints have a certain distance from each other. If I understood you correctly, your main concern is reduced speed (due to the additional computations) - do you have an estimate of how long the keypoint selection takes for you (typically), how large your point clouds are, how much smaller than surface_ you would want input_ to be? I would also see some other opportunities for increasing speed, e.g. in getScatterMatrix

[ghost]

@mvieth those are some good points. My overall concern is that ISSKeypoints doesn't really support everything that the keypoints API promises.

In my scenario, the clouds I use are typically 50-80M points. I wanted to reduce amount of computations, while ensuring a maximum spacing of the keypoints.

I haven't looked through getScatterMatrix, but I can see if there's any optimizations we could make there!

[mvieth]

My overall concern is that ISSKeypoints doesn't really support everything that the keypoints API promises.

I am not super familiar with the keypoints module, but it could be possible that the distinction between surface and input does not make sense for every keypoint type. Of course it should be clear to the user what is possible and what is not, and there should be warnings and a graceful exit before any errors happen.

In my scenario, the clouds I use are typically 50-80M points. I wanted to reduce amount of computations, while ensuring a maximum spacing of the keypoints.

If you would like to investigate further where the most computation time is spent, you could e.g. try valgrind's callgrind (valgrind --tool=callgrind ./name_of_the_executable, at least on Linux) together with kcachegrind to do that.

I haven't looked through getScatterMatrix, but I can see if there's any optimizations we could make there!

Optimizations could be using the symmetry of the covariance matrix, and using float instead of double

[ghost]

Pushed a fix for segfault during the maximum check, and it seems to be working with input_ != surface_.

You are right, that this interface might not be needed for all keypoint types. I only noticed this issue while I was playing around with ISSKeyPoints. I am happy to do some testing though.

[mvieth]

Could you add a test in test_iss_3d.cpp to verify that your changes are correct?

[ghost]

Added some tests.

If you're happy with this - I can add the input_search_tree_ as an ISSKeypoints3D member, and set it up during initCompute.

Adding it to the class prevents us from constructing the KdTree in two separate function calls (getBoundaryPoints and detectKeypoints).