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.

Sign up for GitHub

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

Jump to bottom

KdTree: handle 0 or negative k for nearestKSearch #4430

Merged
merged 3 commits into from
Nov 5, 2020
Merged

KdTree: handle 0 or negative k for nearestKSearch #4430

merged 3 commits into from
Nov 5, 2020

Conversation

JStech
Copy link
Contributor

@JStech JStech commented Oct 1, 2020

While working on #4414 I mistakenly searched for 0 nearest neighbors, and the method segfaulted. This handles that situation better, as well as if a negative k is passed (the argument is a signed integer). Also, my editor automatically removes trailing whitespace, so that's in here too. I could revert all those changes if desired.

@JStech JStech force-pushed the kdtree_knn_when_k_is_0 branch from bbddb3c to b752b90 Compare October 1, 2020 03:56
kunaltyagi
kunaltyagi reviewed Oct 1, 2020
View reviewed changes
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Please split this PR in 2 commits at the very least: 1 with ws changes and one with logic change and one for just logic changes

kdtree/include/pcl/kdtree/impl/kdtree_flann.hpp Outdated
std::vector<float> &k_distances) const
{
assert (point_representation_->isValid (point) && "Invalid (NaN, Inf) point coordinates given to nearestKSearch!");
assert ((k >= 0) && "Invalid number of neighbors");
Copy link
Member

Choose a reason for hiding this comment

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

Why not just accept unsigned int as input? See #4350 (only the changes in octree module)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

@JStech JStech force-pushed the kdtree_knn_when_k_is_0 branch from b752b90 to 127c6bb Compare October 8, 2020 05:03
John Stechschulte added 2 commits October 7, 2020 23:20
removing trailing whitespace
5f80d04 
more trailing whitespace
handle 0 or negative k
8713422 
on second thought, k is better unsigned

more unsigned int ks
@JStech JStech force-pushed the kdtree_knn_when_k_is_0 branch from a984ed7 to 8713422 Compare October 8, 2020 05:21
@JStech
Copy link
Contributor Author

JStech commented Oct 8, 2020

I switched to unsigned ints, and separated the whitespace changes from the actual changes. Let me know if you have any more suggestions.

@kunaltyagi
Copy link
Member

Why did you choose unsigned int over uindex_t?

@mvieth
Copy link
Member

mvieth commented Oct 8, 2020

Why did you choose unsigned int over uindex_t?

I think that's a good choice because uindex_t could create the false impression that k is an index, but it is actually a number.

@mvieth
Copy link
Member

mvieth commented Oct 8, 2020

Seems like total_nr_points_ also needs to be an unsigned data type (to make the macOS clang build pass)

@JStech
Copy link
Contributor Author

JStech commented Oct 8, 2020

Yes, I chose unsigned int because k is not an index, and I don't think we should plan for it to be arbitrarily large (4 billion nearest neighbors ought to be enough for anybody).

I did make total_nr_points_ a uindex_t, and I was able to eliminate several casts as a result. I'll let CI tell me if I messed it up.

@mvieth mvieth added module: kdtree needs: code review Specify why not closed/merged yet labels Oct 15, 2020
@JStech
Copy link
Contributor Author

JStech commented Oct 18, 2020

I think this is ready to go. It passed CI.

@kunaltyagi kunaltyagi added changelog: API break Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation labels Oct 19, 2020
mvieth
mvieth approved these changes Nov 5, 2020
View reviewed changes
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

It would probably make sense to change k from int to unsigned int in all nearestKSearch functions (search module) in the future.

@kunaltyagi kunaltyagi merged commit 1c04491 into PointCloudLibrary:master Nov 5, 2020
@huonggiangdhtn
Copy link

Neighbors within radius search at (0.144053 0.15847 0.434392) with radius=0.01 0.141197 0.161906 0.436549 (squared distance: 2.46188e-05) 0.143541 0.163574 0.433012 (squared distance: 2.82178e-05) 0.0257284 -0.115956 0.444 (squared distance: 6.19848e-05)

I use this function to find nearest point with radius, kdtree.radiusSearch (searchPoint, radius, pointIdxRadiusSearch, pointRadiusSquaredDistance). However this function return error with one of these coordinate < 0 like the third row in above example. Can anyone know how to fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kunaltyagi kunaltyagi kunaltyagi approved these changes

@mvieth mvieth mvieth approved these changes

Assignees
No one assigned
Labels
changelog: API break Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation module: kdtree needs: code review Specify why not closed/merged yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JStech @kunaltyagi @mvieth @huonggiangdhtn