NormalSpaceSampling - fix bucket assignment, remove use of raw distribution pointer, unit-test rewriting

[SergioRAgostinho]

Cleaning up some code in preparation for the random mixin. The use of bind here was intentional, to leverage the copy bind does under the hood. lamdbas bro

[SergioRAgostinho]

I wrote the follow approach to address both your comments and one of the tests is failing 🤦‍♂️ . I remember this happening before. Have a look

Edit:
The test was written poorly. If I apply the modulus like before it passes with no issues.

diff --git a/filters/include/pcl/filters/impl/normal_space.hpp b/filters/include/pcl/filters/impl/normal_space.hpp
index 1bf720719..123ddb58d 100644
--- a/filters/include/pcl/filters/impl/normal_space.hpp
+++ b/filters/include/pcl/filters/impl/normal_space.hpp
@@ -59,11 +59,7 @@ pcl::NormalSpaceSampling<PointT, NormalT>::initCompute ()
     return false;
   }
 
-  boost::mt19937 rng (static_cast<unsigned int> (seed_));
-  boost::uniform_int<unsigned int> uniform_distrib (0, unsigned (input_->size ()));
-  delete rng_uniform_distribution_;
-  rng_uniform_distribution_ = new boost::variate_generator<boost::mt19937, boost::uniform_int<unsigned int> > (rng, uniform_distrib);
-
+  rng_.seed (seed_);
   return (true);
 }
 
@@ -213,11 +209,12 @@ pcl::NormalSpaceSampling<PointT, NormalT>::applyFilter (std::vector<int> &indice
 
       unsigned int pos = 0;
       unsigned int random_index = 0;
+      boost::uniform_int<unsigned> rng_uniform_distribution (0u, M - 1u);
 
       // Picking up a sample at random from jth bin
       do
       {
-        random_index = static_cast<unsigned int> ((*rng_uniform_distribution_) () % M);
+        random_index = rng_uniform_distribution (rng_);
         pos = start_index[j] + random_index;
       } while (is_sampled_flag.test (pos));
 
diff --git a/filters/include/pcl/filters/normal_space.h b/filters/include/pcl/filters/normal_space.h
index 5f7505d9c..34280f113 100644
--- a/filters/include/pcl/filters/normal_space.h
+++ b/filters/include/pcl/filters/normal_space.h
@@ -39,6 +39,7 @@
 
 #include <pcl/filters/boost.h>
 #include <pcl/filters/filter_indices.h>
+
 #include <ctime>
 #include <climits>
 
@@ -77,17 +78,10 @@ namespace pcl
         , binsy_ ()
         , binsz_ ()
         , input_normals_ ()
-        , rng_uniform_distribution_ (nullptr)
       {
         filter_name_ = "NormalSpaceSampling";
       }
 
-      /** \brief Destructor. */
-      ~NormalSpaceSampling ()
-      {
-        delete rng_uniform_distribution_;
-      }
-
       /** \brief Set number of indices to be sampled.
         * \param[in] sample the number of sample indices
         */
@@ -189,8 +183,7 @@ namespace pcl
       bool
       isEntireBinSampled (boost::dynamic_bitset<> &array, unsigned int start_index, unsigned int length);
 
-      /** \brief Uniform random distribution. */
-      boost::variate_generator<boost::mt19937, boost::uniform_int<std::uint32_t> > *rng_uniform_distribution_;
+      boost::mt19937 rng_;
   };
 }

[kunaltyagi]

Any change in sampling strategy will result in indices being picked in a different order. This test appears to be testing the indices after a random sampling.

[kunaltyagi]

🚀 pending CI. Sorry for the delay, this fell through the cracks till you linked it in another PR

[QiMingZhenFan]

@SergioRAgostinho Sorry to bother you. I'm quite confused by the function pcl::NormalSpaceSampling<PointT, NormalT>::findBin (const float *normal), in which you calculate index in each dimension by
const unsigned ix = static_cast<unsigned> (std::round (0.5f * (binsx_ - 1.f) * (normal[0] + 1.f)));
const unsigned iy = static_cast<unsigned> (std::round (0.5f * (binsy_ - 1.f) * (normal[1] + 1.f)));
const unsigned iz = static_cast<unsigned> (std::round (0.5f * (binsz_ - 1.f) * (normal[2] + 1.f)));.

My wonder is why binsx_/binsy_/binsz_ needs to be subtracted by 1.0?

Take ix as an example. In my opinion, std::round should be replaced by std::floor and (binsx_ - 1.f) should be (binsx_) instead.
That is to say, the code should be const unsigned ix = static_cast<unsigned> (std::floor (0.5f * (binsx_) * (normal[0] + 1.f)));

Or if we insist on using std::round, the equation should be const unsigned ix = static_cast<unsigned> (std::round (0.5f * (binsx_) * (normal[0] + 1.f) - 0.5f));

Could you please explain how the formula at the top was derived? Thanks!

[mvieth]

@QiMingZhenFan Do you believe that there is a bug in the code, or are you just trying to understand the code? If you suspect a bug, what is the effect?

[QiMingZhenFan]

@QiMingZhenFan Do you believe that there is a bug in the code, or are you just trying to understand the code? If you suspect a bug, what is the effect?

@mvieth Thanks for the reply!

Yes, I suspect it's a bug, which may lead to a wrong division. That is, a normal vector may be assigned to the wrong bin.

Let's take ix as an example. Suppose binsx_ is 5, then ix must be one of [0, 1, 2, 3, 4].

Let normal[0] be 0.24, then follow the equation in this PR, ix = std::round (0.5f * (5 - 1.f) * (0.24 + 1.f)) = std::round (0.5f * (4) * (1.24)) = std::round (2.48) = 2.

But the picture following shows that this normal[0]=0.24 should be assigned to bin 3.

[mvieth]

@QiMingZhenFan Oh yes, you are right. With the current code, the outer two intervals are only half as wide as the other intervals. Would you like to create a pull request to fix this code?

[QiMingZhenFan]

@mvieth Sure. I'll do it today.