Kdtree #5
Open
Kdtree #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kunaltyagi
reviewed
Jul 13, 2020
| { | ||
| using Self = KdTreeFLANN<PointT, KdTreeFLANN<PointT>>; |
There was a problem hiding this comment.
Self is a bad name for parent class
There was a problem hiding this comment.
Oh, yeah didn't notice I was referencing to base class. WIll update it to 'Base.
|
|
||
| namespace pcl | ||
| { | ||
| /** \brief KdTree represents the base spatial locator class for kd-tree implementations. | ||
| * \author Radu B Rusu, Bastian Steder, Michael Dixon | ||
| * \ingroup kdtree | ||
| */ | ||
| template <typename PointT> | ||
| template <typename PointT, typename Derived> |
There was a problem hiding this comment.
Not supplying a default argument can break user's code.
Comment on lines
+237
to
+240
| return (exec, nearestKSearch (input_->points[index], k, k_indices, k_sqr_distances)); | ||
| } | ||
| assert (index >= 0 && index < static_cast<int> (indices_->size ()) && "Out-of-bounds error in nearestKSearch!"); | ||
| return (nearestKSearch (input_->points[(*indices_)[index]], k, k_indices, k_sqr_distances)); | ||
| return (exec, nearestKSearch (input_->points[(*indices_)[index]], k, k_indices, k_sqr_distances)); |
There was a problem hiding this comment.
Are you trying to return Python tuples? :o
kunaltyagi
reviewed
Jul 13, 2020
| std::vector<int> &k_indices, std::vector<float> &k_sqr_distances) const | ||
| { | ||
| PointT p; | ||
| copyPoint (point, p); | ||
| return (nearestKSearch (p, k, k_indices, k_sqr_distances)); | ||
| return (nearestKSearch (exec, p, k, k_indices, k_sqr_distances)); |
There was a problem hiding this comment.
Please remove parens after return
kunaltyagi
reviewed
Jul 13, 2020
| int | ||
| nearestKSearch (const Executor &exec, const PointT &p_q, int k, | ||
| std::vector<int> &k_indices, std::vector<float> &k_sqr_distances) const { | ||
| return static_cast<Derived&>(*this).nearestKSearch(exec, p_q, k, k_indices, k_sqr_distances); |
There was a problem hiding this comment.
This is potentially confusing IMO. You're hiding the function in the derived class without overloading. Some note somewhere detailing what's happening and more importantly why it's happening the way it is
shrijitsingh99
force-pushed
the
executor-integration
branch
2 times, most recently
from
1f11251 to
caabb6f
Compare
shrijitsingh99
force-pushed
the
executor-integration
branch
7 times, most recently
from
6d88bfe to
7c3b82c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO: