Fixes remove_indices in UniformSampling #1902
Conversation
|
I do not understand the logic. Why do we add index to // If current point is closer, copy its index instead and mark previous index as removed
if (diff_cur < diff_prev)
{
removed_indices_.push_back (leaf.idx);
leaf.idx = (*indices_)[cp];
} |
There was a problem hiding this comment.
Sorry for the early approval. I didn't look deeply into the problem.
Based on https://travis-ci.org/PointCloudLibrary/pcl/jobs/247089340#L1522 you probably also need to use the full qualified name Filter<PointT>::removed_indices_, or declare an alias in the protected scope of the class, like in the lines 65-68.
| @@ -101,6 +109,9 @@ pcl::UniformSampling<PointT>::applyFilter (PointCloud &output) | |||
| if (leaf.idx == -1) | |||
| { | |||
| leaf.idx = (*indices_)[cp]; | |||
| if (extract_removed_indices_) { | |||
| removed_indices_->push_back((*indices_)[cp]]); | |||
There was a problem hiding this comment.
This makes no sense. There's no way at this point for you to know if the index in this leaf will be later on substituted in 124. If it ends up never getting replaced, the index should not be removed.
In line 124, like @taketwo pointed out, is where you're sure the index got replaced and dropped.
| @@ -72,7 +72,8 @@ namespace pcl | |||
| typedef boost::shared_ptr<const UniformSampling<PointT> > ConstPtr; | |||
|
|
|||
| /** \brief Empty constructor. */ | |||
| UniformSampling () : | |||
| UniformSampling (bool extract_removed_indices = false) : | |||
There was a problem hiding this comment.
This will break the ABI, which pushes things to 1.9.0
There was a problem hiding this comment.
Maybe we just have two constructors? One parameterless, one with compulsory bool.
There was a problem hiding this comment.
We will need to remember to delete it after ^^.
Let's think the other way around, maybe 1.8.1 simply doesn't make sense anymore, it's been more than year. Let's just adopt the new 1.8.0-dev designation and start working on a release with new features, etc... Under that context it's no problem to have the ABI break.
There was a problem hiding this comment.
On one hand, I like this idea. On the other hand, it pushes the next release (whichever number it will have) quite far in the future. What about releasing the current state as 1.8.1 (or 1.9.0) ASAP, and then moving on with merging this PR and other "1.9.0 milestone" features?
There was a problem hiding this comment.
SGTM 👍 Let me have a look at that change log and cry a little :D
There was a problem hiding this comment.
Haha :) Ok, and I'll work on "-dev".
fran6co
force-pushed
the
fix_uniform
branch
4 times, most recently
from
0265de7
to
4f59c5c
Compare
SergioRAgostinho
added
changelog: ABI break
Fixes #1826