Remove magic numbers from organized_segmentation_demo app

[aPonza]

Fixes #3106

I'm working on ROS kinetic and with a realsense, so I only know this compiles correctly and works in a ROS environment (so cloud_cb mainly), but I can't test it at runtime without #2214.

[SergioRAgostinho]

Quick comment about bc996a7, tldr: remove it.

While the use of make_shared is in general considered good C++ practice you have to keep in mind we have a lot custom allocators in place, for instance PointCloud. make_shared does not support that. See this post for an example on the topic. Hence our practice of initializing the shared pointers directly with new. If you runtime tested your code, it would likely segfault.

[aPonza]

Done, but why not use allocate_shared? I didn't quite understand why that wouldn't be viable. Using new is exception unsafe and allocates the control block separately iirc.

[SergioRAgostinho]

Done, but why not use allocate_shared? I didn't quite understand why that wouldn't be viable. Using new is exception unsafe and allocates the control block separately iirc.

Thanks. That would be the optimal case in terms of correctness. It has at least one downside in terms of ease of use: it requires the users to know if the object has a special allocator or not. In reality, the best option for me would be to explore the idea of having a thin wrapper pcl::make_shared<T> which would lift the need for the user to have this knowledge. I haven't tried this proof of concept yet so I'm unaware of how feasible it is.

[aPonza]

It's a bit of a rabbit-hole, but check this relevant discussion. I arrived there through a discussion in drake (which is also linked there). Point being, it might all not be needed anyways.

[SergioRAgostinho]

Well, it´s good to know that will be valid after C++17!

Anyway, I started playing with the thin-wrapper this morning and managed to make something based of this post. The biggest technical aspects are solved and proven to be feasible.

#include <Eigen/Core>

#include <iostream>
#include <memory>
#include <type_traits>

#define PCL_MAKE_ALIGNED_OPERATOR_NEW \
  EIGEN_MAKE_ALIGNED_OPERATOR_NEW \
  using custom_allocator = void;


struct CustomAllocatedT {
  PCL_MAKE_ALIGNED_OPERATOR_NEW
};

struct DefaultT {};



template<typename T>
class has_custom_allocator
{
    template<typename U, typename = typename U::custom_allocator> static char test (unsigned);
    template<typename U> static int32_t test (...);
  public:
    static constexpr bool value = (sizeof(test<T>(0)) == sizeof(char));
};


template<typename T, typename... Args>
std::enable_if_t<has_custom_allocator<T>::value,std::shared_ptr<T>>
make_shared (Args&&... args)
{
  std::cout << "I'm a custom allocated type" << std::endl;
  return std::allocate_shared<T> (Eigen::aligned_allocator<T> (), std::forward<Args> (args)...);
}

template<typename T, typename... Args>
std::enable_if_t<!has_custom_allocator<T>::value,std::shared_ptr<T>>
make_shared (Args&&... args)
{
  std::cout << "I'm a default allocated type" << std::endl;
  return std::make_shared<T> (std::forward<Args> (args)...);
}


int
main()
{

  DefaultT dobj;
  CustomAllocatedT cobj;

  std::cout << "Is DefaulT custom allocated: " << has_custom_allocator<DefaultT>::value << std::endl;
  std::cout << "Is CustomAllocatedT custom allocated: " << has_custom_allocator<CustomAllocatedT>::value << std::endl;
  std::shared_ptr<DefaultT> dptr = make_shared<DefaultT> (dobj);
  std::shared_ptr<CustomAllocatedT> cptr = make_shared<CustomAllocatedT> (cobj);
  return 0;
}

It outputs:

$ ./make_shared 
Is DefaulT custom allocated: 0
Is CustomAllocatedT custom allocated: 1
I'm a default allocated type
I'm a custom allocated type

[aPonza]

I was trying using std::uses_allocator but yours is more generic. Nice!

[taketwo]

I'm not convinced that explicitly setting parameters to certain values (which happen to be the defaults) is necessarily an issue and has to be fixed. Yes, they appear like magic numbers, but even if you delete them, behind the curtain they are still used. In my opinion, a better approach would be to leave this explicit parameter setting and add a comment explaining where they come from. That is, according to Alex, "to perform well for tabletop objects as imaged by a primesense sensor".

[SergioRAgostinho]

That is a valid point. It can happen that if the default changes in the future, suddenly the example/tutorial produces bad results. What's your opinion @aPonza ?

[aPonza]

What taketwo is saying is the reason I wanted to comment the lines out in the first place. His solution is better though. I'll go ahead and fix it.