Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compilation of test_Icosphere with Eigen 3.4 #1613

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Oct 10, 2021

While debugging a build failure in the dartsim conda-forge package (see conda-forge/dartsim-feedstock#16), I noticed the following build failure when building dart with Eigen 3.4 :

(dart_new_eigen) traversaro@IITICUBLAP257:~/dart/build_new_eigen$ ninja test_Icosphere
[1/2] Building CXX object unittests/unit/CMakeFiles/test_Icosphere.dir/test_Icosphere.cpp.o
FAILED: unittests/unit/CMakeFiles/test_Icosphere.dir/test_Icosphere.cpp.o
/home/traversaro/mambaforge/envs/dart_new_eigen/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_SYSTEM_DYN_LINK -DBOOST_TEST_DYN_LINK -I/home/traversaro/dart/unittests -I/home/traversaro/dart -I/home/traversaro/dart/build_new_eigen -isystem /home/traversaro/dart/unittests/gtest -isystem /home/traversaro/dart/unittests/gtest/include -isystem /home/traversaro/mambaforge/envs/dart_new_eigen/include/eigen3 -fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /home/traversaro/mambaforge/envs/dart_new_eigen/include -Wall -Wextra -fPIC -O3 -DNDEBUG -MD -MT unittests/unit/CMakeFiles/test_Icosphere.dir/test_Icosphere.cpp.o -MF unittests/unit/CMakeFiles/test_Icosphere.dir/test_Icosphere.cpp.o.d -o unittests/unit/CMakeFiles/test_Icosphere.dir/test_Icosphere.cpp.o -c /home/traversaro/dart/unittests/unit/test_Icosphere.cpp
In file included from /home/traversaro/dart/dart/math/Icosphere.hpp:104,
                 from /home/traversaro/dart/unittests/unit/test_Icosphere.cpp:35:
/home/traversaro/dart/dart/math/detail/Icosphere-impl.hpp: In instantiation of 'static std::pair<std::vector<typename dart::math::TriMesh<S>::Vector3>, std::vector<typename dart::math::TriMesh<S>::Triangle> > dart::math::Icosphere<S_>::computeIcosahedron(dart::math::Icosphere<S_>::S) [with S_ = double; typename dart::math::TriMesh<S>::Triangle = Eigen::Matrix<long unsigned int, 3, 1, 0, 3, 1>; typename dart::math::TriMesh<S>::Vector3 = Eigen::Matrix<double, 3, 1>; dart::math::Icosphere<S_>::S = double]':
/home/traversaro/dart/dart/math/detail/Icosphere-impl.hpp:133:67:   required from 'void dart::math::Icosphere<S_>::build() [with S_ = double]'
/home/traversaro/dart/dart/math/detail/Icosphere-impl.hpp:109:3:   required from 'dart::math::Icosphere<S_>::Icosphere(dart::math::Icosphere<S_>::S, std::size_t) [with S_ = double; dart::math::Icosphere<S_>::S = double; std::size_t = long unsigned int]'
/home/traversaro/dart/unittests/unit/test_Icosphere.cpp:50:59:   required from here
/home/traversaro/dart/dart/math/detail/Icosphere-impl.hpp:77:24: error: converting to 'Eigen::Matrix<double, 3, 1>' from initializer list would use explicit constructor 'Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols>::Matrix(const std::initializer_list<std::initializer_list<typename Eigen::internal::traits<Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols> >::Scalar> >&) [with _Scalar = double; int _Rows = 3; int _Cols = 1; int _Options = 0; int _MaxRows = 3; int _MaxCols = 1; typename Eigen::internal::traits<Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols> >::Scalar = double]'
   77 |   std::vector<Vector3> vertices = {{{-x, 0, z},
      |                        ^~~~~~~~
/home/traversaro/dart/dart/math/detail/Icosphere-impl.hpp:90:32: error: converting to 'Eigen::Matrix<long unsigned int, 3, 1, 0, 3, 1>' from initializer list would use explicit constructor 'Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols>::Matrix(const std::initializer_list<std::initializer_list<typename Eigen::internal::traits<Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols> >::Scalar> >&) [with _Scalar = long unsigned int; int _Rows = 3; int _Cols = 1; int _Options = 0; int _MaxRows = 3; int _MaxCols = 1; typename Eigen::internal::traits<Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols> >::Scalar = long unsigned int]'
   90 |   static std::vector<Triangle> triangles
      |                                ^~~~~~~~~
ninja: build stopped: subcommand failed.

Building exactly the same code with exactly the same dependency except for changing Eigen from 3.4.0 to 3.3.9 was working fine, so I am quite sure that this problem is related to Eigen 3.4.0 . I am not sure about the exact nature of the error as I am not familiar with initializations of std containers of Eigen vectors, but according to my tests it seems that the error is fixed by removing a pair of curly bracket solved the problem while ensuring that everything still compiles fine on Eigen 3.3.9 , and this is proposed in this PR.

However, things to check accurately:

  • Make sure that all relevant Eigen versions/compilers pairs are covered by CI, eitherwise we may break them.
  • Check if the format that I used for the initializer list after the change still make sense

Before creating a pull request

  • Document new methods and classes
  • Format new code files using clang-format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@traversaro
Copy link
Contributor Author

Check if the format that I used for the initializer list after the change still make sense

I double checked with clang-format9 and it seems correct.

@jslee02
Copy link
Member

jslee02 commented Oct 19, 2021

Yeah, I see the same error building on archilinux. Thanks for the fix!

@jslee02 jslee02 merged commit e8f84a8 into dartsim:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants