Improvements to OUR-CVFH Estimation, including mapping from clusters to features and doc fixes #738
Conversation
|
You should rebase your branch on PCL master to avoid the "Merge branch" commit |
|
I don't think these debugging lines are particularly informative. They probably only apply to my particular problem. I'll rebase when I get to work. Thanks for looking over my code. =) |
|
Okay. Rebased. Good trick! Didn't know I could do that, thanks. |
| @@ -310,7 +310,7 @@ pcl::OURCVFHEstimation<PointInT, PointNT, PointOutT>::sgurf (Eigen::Vector3f & c | |||
|
|
|||
| if ((min_axis / max_axis) > axis_ratio_) | |||
| { | |||
| PCL_WARN("Both axis are equally easy/difficult to disambiguate\n"); | |||
| PCL_WARN("Both axes are equally easy/difficult to disambiguate\n"); | |||
There was a problem hiding this comment.
There should be a space before the parenthesis
There was a problem hiding this comment.
I believe this is a macro, so a space is not permitted, but it's also not my code — I just updated the error message.
There was a problem hiding this comment.
Space is not permitted in macro definitions, but it is okay to put it when a macro is called.
Generally, our policy is not to fix space issues in lines we don't touch with updates, and do fix in lines we touch. In this case the line in question is being updated anyways, so it would be nice to fix the space as well.
|
I have now adjusted the spacing in |
| @@ -262,6 +262,15 @@ namespace pcl | |||
| { | |||
| indices = clusters_; | |||
| } | |||
|
|
|||
| /** \brief Gets the number of non-disambiguable axes that correspond to each centroid | |||
| * \param[out] vector mapping each centroid to the number of signatures | |||
There was a problem hiding this comment.
Parameter description should contain the name of the parameter, i.e. in this case the line should be:
\param[out] cluster_axes vector mapping each centroid to the number of signatures
|
I spotted one Doxygen-related issue, but otherwise everything looks good. Could you please squash the fixup commits so we have a neat history? |
|
Oops. Apparently I don't know how to rebase properly. Working on it. |
centroid clusters to VFH signatures -- which occurs when axes can't be disambiguated. Also, ensured that the width and height are set properly on output when compute, computeFeature, and computeRFAndShapeDistribution are called.
|
BAM. 'Tis done. |
|
Thanks, merging this. |
Improvements to OUR-CVFH Estimation, including mapping from clusters to features and doc fixes
Fixes #737 by adding a
vector<short> cluster_axes_toOURCVFHEstimation. This protected field consists of a count of the number of signatures that have been computed per cluster. It is set bycomputeRFAndShapeDistributionat the same time ascompute's output. When all axes are properly disambiguated, it will consist only of the value 1 (one entry per cluster). When some cluster i has multiple SGURF solutions, instead of 1, the vector will contain the number of SGURFs in location i.Also fixes a typo in the documentation and another in the warning (plural of axis is axes, so "both axes" not "both axis").