Export the SSE definitions used compiling the lib

[SergioRAgostinho]

Addresses the first point in #1725.

[jspricke]

Why don't we export all definitions instead of only the ones for SSE?
(Sorry, a little short on time, otherwise I would check for myself.)

[taketwo]

It is unclear to me why compile definitions are necessary at all in this case. The file pcl_config.h.in is configured at the time of CMake generation for pcl, and it probably should have all the flags set correctly at that time instead of relying on -DDISABLE_<?> flags and such. Files that rely on these definitions should include the pcl_config.h header. This way, users don't need to use PCL_DEFINTIONS explicitly at all.

(from #1478)

[SergioRAgostinho]

Why don't we export all definitions instead of only the ones for SSE?

What other definitions exactly?

This is currently what I'm picking up when compiling PCL

CMAKE_CXX_FLAGS: -Wall -Wextra -Wno-unknown-pragmas -fno-strict-aliasing -Wno-format-extra-args -Wno-sign-compare -Wno-invalid-offsetof -Wno-conversion -march=native -msse4.2 -mfpmath=sse -Wabi -pthread -fopenmp
Defs: EIGEN_USE_NEW_STDVECTOR;EIGEN_YES_I_KNOW_SPARSE_MODULE_IS_NOT_STABLE_YET;vtkFiltersFlowPaths_AUTOINIT=1(vtkFiltersParallelFlowPaths);vtkIOExodus_AUTOINIT=1(vtkIOParallelExodus);vtkIOGeometry_AUTOINIT=1(vtkIOMPIParallel);vtkIOImage_AUTOINIT=1(vtkIOMPIImage);vtkIOSQL_AUTOINIT=2(vtkIOMySQL,vtkIOPostgreSQL);vtkRenderingContext2D_AUTOINIT=1(vtkRenderingContextOpenGL);vtkRenderingCore_AUTOINIT=4(vtkInteractionStyle,vtkRenderingFreeType,vtkRenderingFreeTypeOpenGL,vtkRenderingOpenGL);vtkRenderingFreeType_AUTOINIT=2(vtkRenderingFreeTypeFontConfig,vtkRenderingMatplotlib);vtkRenderingLIC_AUTOINIT=1(vtkRenderingParallelLIC);vtkRenderingVolume_AUTOINIT=1(vtkRenderingVolumeOpenGL)

This currently what I'm picking up when I link against PCL

-- PCL_DEFINITIONS: -march=native -msse4.2 -mfpmath=sse ;-DEIGEN_USE_NEW_STDVECTOR;-DEIGEN_YES_I_KNOW_SPARSE_MODULE_IS_NOT_STABLE_YET;-DFLANN_STATIC;-DDISABLE_OPENNI;-DDISABLE_OPENNI2;-DDISABLE_ENSENSO;-DDISABLE_DAVIDSDK;-DDISABLE_DSSDK;-DDISABLE_PCAP;-DDISABLE_PNG;-DDISABLE_LIBUSB_1_0;-DDISABLE_QHULL;-DDISABLE_RSSDK

Also agree, as discussed in #1478, that these disables should not be here.

[SergioRAgostinho]

By the way guys, I just grepped Eigen's source for EIGEN_USE_NEW_STDVECTOR and it comes out empty... It's doing nothing for us :D

[jspricke]

ok, then it should be fine. Regarding the -DEIGEN_..., yes they are really old and we should remove them.

[SergioRAgostinho]

As in, in this PR already?

The Sparse module became stable and officially supported in 3.1. I think our CI is still running 3.0.3
The new std vector was officially adopted in 2.0.9 so we're safe on this one.

[jspricke]

* Sérgio Agostinho <[email protected]> [2017-06-30 15:37]:

As in, in this PR already?

As it's related to this PR I would be ok with that, as long as it's a separate commit.

The Sparse module became stable and officially supported in 3.1. I think our CI is still running 3.0.3

It's really sad that Travis is still on 12.04, but this flag doesn't really hurt us, so we can leave it in.

The new std vector was officially adopted in 2.0.9 so we're safe on this one.

Sound like a good candidate to drop :).

[SergioRAgostinho]

It's really sad that Travis is still on 12.04, but this flag doesn't really hurt us, so we can leave it in.

So... Travis already supports 14.04. We're just using 12.04 because it's the one set by default.

[taketwo]

BTW, as far as I can tell, the only usage of Eigen sparse module in PCL is the code that I contributed (RandomWalker segmentation), and it is explicitly enabled only for Eigen >= 3.1, where the macro is not needed.

(Disclaimer: I did not add this macro, it existed long before I checked in RandomWalker).

[SergioRAgostinho]

Searching 3427 files for "Eigen/Sparse"

/Users/neglective/Development/3rdparty/pcl/features/include/pcl/features/eigen.h:
   47  #include <Eigen/StdVector>
   48  #include <Eigen/Geometry>
   49: #include <Eigen/Sparse>
   50  
   51  #endif    // PCL_FEATURES_EIGEN_H_

/Users/neglective/Development/3rdparty/pcl/segmentation/include/pcl/segmentation/impl/random_walker.hpp:
   41  #include <boost/bimap.hpp>
   42  
   43: #include <Eigen/Sparse>
   44  
   45  namespace pcl

2 matches across 2 files

Searching 3427 files for "Eigen::Sparse"

/Users/neglective/Development/3rdparty/pcl/segmentation/CMakeLists.txt:
   79         )
   80      endif()
   81:     # Random walker requires Eigen::Sparse module that is available since 3.1.0
   82      if(NOT ("${EIGEN_VERSION}" VERSION_LESS 3.1.0))
   83      list(APPEND incs
   ..
  115         )
  116      endif()
  117:     # Random walker requires Eigen::Sparse module that is available since 3.1.0
  118      if(NOT ("${EIGEN_VERSION}" VERSION_LESS 3.1.0))
  119      list(APPEND impl_incs

/Users/neglective/Development/3rdparty/pcl/segmentation/include/pcl/segmentation/impl/random_walker.hpp:
   78            typedef typename boost::property_map<Graph, boost::vertex_index_t>::type VertexIndexMap;
   79            typedef boost::iterator_property_map<typename std::vector<Weight>::iterator, VertexIndexMap> VertexDegreeMap;
   80:           typedef Eigen::SparseMatrix<Weight> SparseMatrix;
   81            typedef Eigen::Matrix<Weight, Eigen::Dynamic, Eigen::Dynamic> Matrix;
   82            typedef Eigen::Matrix<Weight, Eigen::Dynamic, 1> Vector;

/Users/neglective/Development/3rdparty/pcl/test/segmentation/CMakeLists.txt:
   10  
   11  if (build)
   12:   # Random walker requires Eigen::Sparse module that is available since 3.1.0
   13    if(NOT ("${EIGEN_VERSION}" VERSION_LESS 3.1.0))
   14      PCL_ADD_TEST(random_walker test_random_walker

/Users/neglective/Development/3rdparty/pcl/test/segmentation/test_random_walker.cpp:
   68  typedef boost::property_map<Graph, boost::edge_weight_t>::type EdgeWeightMap;
   69  typedef boost::property_map<Graph, boost::vertex_color_t>::type VertexColorMap;
   70: typedef Eigen::SparseMatrix<Weight> SparseMatrix;
   71  typedef Eigen::Matrix<Weight, Eigen::Dynamic, Eigen::Dynamic> Matrix;
   72  typedef Eigen::Matrix<Weight, Eigen::Dynamic, 1> Vector;

5 matches across 4 files

[taketwo]

Exactly. So besides RandomWalker there is this include in "pcl/features/eigen.h", but sparse matrices are not used anywhere in feature module. Also, looking at the history, this include used to reside in the "pcl/features/principal_curvatures.h" since the beginning of the repo. So no explanation why it is there. I'd say let's get rid of the include and the macro!

[SergioRAgostinho]

[..] and the macro!

I'm confused: do you mean the if(NOT ("${EIGEN_VERSION}" VERSION_LESS 3.1.0)) or the -DEIGEN_YES_I_KNOW_SPARSE_MODULE_IS_NOT_STABLE_YET definition?

[taketwo]

I meant DEIGEN_YES_I_KNOW_SPARSE_MODULE_IS_NOT_STABLE_YET.

[SergioRAgostinho]

Should be good to merge now.