Use random generator from C++11 instead of from Boost in module filters

[SunBlack]

Separation from #2956 due to the need to adapt tests.

[taketwo]

From the CI output looks like metslib needs Boost random header to be included explicitly. I'd suggest to add it for now, even though we will probably get rid of it later (#2954).

[SunBlack]

Interesting that changes in module filters module affects recognition. As already mentioned in #2956 I suggest to first merge #2956 and after this all other PRs related to this. This would fix include error, too (see here)

[taketwo]

#2956 was merged, could you please rebase this one?

[SergioRAgostinho]

This looks ok to me, so I assume it's time to have a look at the outputs from the unit tests just to confirm if updating values is the way to go.

[SergioRAgostinho]

Proposal to remove these useless lines.

pcl/test/filters/test_sampling.cpp

Lines 127 to 132 in 711b121

	EXPECT_EQ (1412, (*walls_indices)[0]);
	EXPECT_EQ (1943, (*walls_indices)[walls_indices->size () / 4]);
	EXPECT_EQ (2771, (*walls_indices)[walls_indices->size () / 2]);
	EXPECT_EQ (3215, (*walls_indices)[walls_indices->size () * 3 / 4]);
	EXPECT_EQ (2503, (*walls_indices)[walls_indices->size () - 1]);

[taketwo]

Yep, these lines are not very helpful. But can we add at least some test for the output walls_indices? Otherwise we are not really testing anything. I will need to have a deeper look.

[Neumann-A]

Word of warning: MSVC random implementation is rather slow.
https://developercommunity.visualstudio.com/content/problem/86909/stdnormal-distribution-four-times-slower-than-the.html

[kunaltyagi]

MSVC is rather slow

But they could have just copied Boost 😛 IIRC, some instances have already undergone the transformation. Correct me if I'm wrong, but it seems that the code paths are performance critical.

[Neumann-A]

word of advice: Instead of depending directly on boost::random or std::random use/introduce pcl::random which is just an alias for either one of those which can be switched using a preprocessor definition like USE_BOOST_RANDOM. This way you can easily switch between implementations without changing a lot of code all in your codebase. Maybe there will be a std::random2 somewhere in the future

[kunaltyagi]

Maybe there will be a std::random2 somewhere in the future

I wouldn't hold my breath. See std::regex for proof.

As for macro flags, that's sorta what I did with pcl::shared_ptr, but I fear compile times might increase if we put all these in a common file or have lots of files with just these compile-time switches if we use a different file for a different alias.

[Neumann-A]

but I fear compile times might increase

runtime is more important than compile time.......if your change introduces a 2.5x speed degredation on windows people will probably be not happy with it ;) And this applies to cl as well as clang-cl users if VS STL is used.

[stale]

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

[mvieth]

So, I just benchmarked VoxelGridCovariance::getDisplayCloud with gcc/libstdc++, and it takes 2.8 times longer if std::mt19937 and std::normal_distribution are used, and 1.6 times longer if only boost::mt19937 is replaced with std::mt19937. Boost uses the Ziggurat method while libstdc++ (gcc) and libc++ (llvm) and MSVC STL all three seem to use the Marsaglia method, which may explain the speed difference partially (but not completely).
So I would vote to leave boost::mt19937 and boost:normal_distribution untouched in voxel_grid_covariance.hpp for now, and close this pull request. normal_space.h[pp] has been adapted by #3862
@larshg What do you think?

[larshg]

Lets keep using boost for now.