Start using the Eigen objects in the public API

[GiulioRomualdi]

I am pretty sure that all the iDynTree users know Eigen library. However, to be all aligned, Eigen is a C++ template library for linear algebra: matrices, vectors, numerical solvers, and related algorithms.

iDynTree widely uses Eigen to compute some operations. Thanks to the iDynTree::toEigen() function many iDynTree objects can be mapped in an Eigen::map and they can be almost treated as Eigen objects #510.

Using Eigen objects in the public API has been always discouraged because of the alignment problem. The problem is well known in the Eigen community, indeed it is documented in the documentation. In a nutshell the following code

class Foo
{
  ...
  Eigen::Vector4d v;
  ...
};
 
...
 
Foo *foo = new Foo;

causes a segmentation fault when the Advanced Vector Extension (AVX) instructions set is used to compile the library/application. Indeed running the code with the AVX option (-mavx) leads to the following assert

eigen_test: /usr/include/eigen3/Eigen/src/Core/DenseStorage.h:128: Eigen::internal::plain_array<T, Size, MatrixOrArrayOptions, 32>::plain_array() [with T = double; int Size = 4; int MatrixOrArrayOptions = 0]: Assertion `(internal::UIntPtr(eigen_unaligned_array_assert_workaround_gcc47(array)) & (31)) == 0 && "this assertion is explained here: " "http://eigen.tuxfamily.org/dox-devel/group__TopicUnalignedArrayAssert.html" " **** READ THIS WEB PAGE !!! ****"' failed.
Aborted (core dumped)

On the other hand, having objects that inherit directly from Eigen could be really useful (for instance we do not need to call iDynTree::toEigen() every time we need to compute an operation). Furthermore, we may avoid converting the iDynTree objects to Eigen objects when we use other libraries (e.g. osqp-eigen). Last but not least it may simplify the implementation of Templates function/classes where we require some basic algebraic operations (Please check ami-iit/bipedal-locomotion-framework#56).

Checking on Eigen website, I realized that the alignment "limitation" is completely solved if the library is compiled in [c++17] mode only with a sufficiently recent compiler (e.g., gcc>=7, clang>=5, MSVC>=19.12).
Ubuntu 18.04LTS ships gcc 7.5.0, while MSVC 19.12 is supported from VS2017, version 15.5.

I also wrote a simple snippet for testing the feature. (Please find the code here). Calling the following command the code will run without any error.

g++ -mavx -std=c++17 -o eigen_test -I/usr/include/eigen3/ ./eigen_test.cpp

while

g++ -mavx -std=c++11 -o eigen_test -I/usr/include/eigen3/ ./eigen_test.cpp

will generate a not working application.

What I am proposing is to start refactoring some core objects in iDynTree and inherit directly from Eigen. I know that this is a huge change in iDynTree (e.g. we have to ask for C++) and it may break some already existing code, but I believe that handling correctly the transition phase (that may last several months), we can do it.

@traversaro @robotology/iit-dynamic-interaction-control

[GiulioRomualdi]

Since I'm not able to wait I tried with the iDynTree::VectorFixSize. This is the result devel...GiulioRomualdi:refactoring/vector_fixsize

All the tests pass without any problems.

Thanks to the code I developed the following code compiles

    iDynTree::VectorFixSize<4> v1, v2;

    v1 << 1, 2, 3, 4;
    v2 << 5, 6, 7, 8;

    iDynTree::VectorFixSize<4> v3 = 2 * v1 + v2;

[traversaro]

That is great @GiulioRomualdi ! Just to be clear, the C++17 requirement is sufficient to avoid all the EIGEN_MAKE_ALIGNED_OPERATOR_NEW craziness is sufficient Eigen 3.3 that is shipped with Ubuntu 20.04 ? If that is true, I agree that we can using Eigen headers in public headers.

Just to be aligned, the main reasons (in decreasing order of importance) I introduced the rule "no non-STL includes in public headers" back in 2015 were (an extensive design rationale can be found in #29):

1. The need of manually adding EIGEN_MAKE_ALIGNED_OPERATOR_NEW (see http://eigen.tuxfamily.org/dox-devel/group__TopicStructHavingEigenMembers.html) for any Eigen-using structure was extremely difficult to enforce in practice, and was leading to subtle bugs in almost all code developed by not experienced developers.
2. The process of wrapping C++ code in SWIG for MATLAB and Python is not always straightforward, and I wanted to be sure that we had full control of the classes exposed in public headers that were, not make sure that we were able to deploy in a straightforward way any kind of workaround or solution necessary to get SWIG to work. In particular, something I was particularly unsure if it was possible to achieve with Eigen was to use SWIG-wrapped iDynTree object to permit to wrap in SWIG also downstream libraries that depended on iDynTree (see Introduce Python and Matlab Bindings  ami-iit/bipedal-locomotion-framework#53), that it is something that I know how to do with simple non-template objects such as iDynTree's one, while I was not sure how to do with Eigen.
3. Several distributions of Eigen did not included an Eigen3Config.cmake, so it was not trivial to include a find_dependency(Eigen3 CONFIG REQUIRED) in iDynTreeConfig.cmake .

Fast forward to 2020, the situation clearly improved:

1. According to the tests reported on this issue, this problem is solved by requiring C++17 . As C++17 does not require any ABI change (as it was instead with C++98/C++11 transition) I don't think it is a big problem to require it.
2. This is still the most uncertain part from my point of view, however since 2015 there are several examples of projects using Eigen3 with SWIG, see for example https://github.com/rdeits/swig-eigen-numpy (and the related issue Auto-generate Matlab bindings for our C++ libraries RobotLocomotion/drake#1267), so probably this problem can be solved relatively easily. On the other hand, in 5 years we never had the need to use iDynTree data structures as objects in the API of downstream libraries, so probably that is not an actual problem.
3. This has been completely solved, as all the distributions of Eigen3 that we support ship with a Eigen3Config.cmake file.

I still some general doubts of the fact that Eigen ABI can be drastically changed from one compilation unit to another by simple preprocessor definitions (see flexible-collision-library/fcl#96 (comment)) but the benefits are clearly more relevant then the downsides.

[GiulioRomualdi]

@traversaro

That is great @GiulioRomualdi ! Just to be clear, the C++17 requirement is sufficient to avoid all the EIGEN_MAKE_ALIGNED_OPERATOR_NEW craziness is sufficient Eigen 3.3 that is shipped with Ubuntu 20.04 ? If that is true, I agree that we can using Eigen headers in public headers.

Actually I tried with Ubuntu 18.04 and there the problem seems solved

[diegoferigo]

Thanks @GiulioRomualdi for starting the discussion about this topic and @traversaro for providing the overview of the history, very useful. I agree that directly using Eigen in our public APIs simplifies quite a lot many operations, especially when the data structures come from other libraries (thanks to Eigen maps).

My main concern is @traversaro's point 2. Particularly for what concerns Python, it would be great having also an eigen - numpy automatic conversion (another interesting project is stack-of-tasks/eigenpy, even though they use Boost Python). There are few differences in how eigen and numpy handle the vectors and many hidden caveats do exist. We should investigate how to do this conversion properly.

[GiulioRomualdi]

Shall we schedule a meeting on this?

[francesco-romano]

Hello guys.

I am not used to comment on iDynTree PRs, but this is a big change and, unfortunately, I feel obliged to leave a comment here.

I will not comment on "breaking change / break API/ABI" as sometimes this is needed and if handled correctly you can mitigate the pain for lots of users.
I am more concern about the change itself.
One of the big advantages of working with iDynTree objects is that they provide a nice separation between storage and algorithms: Vectors and Matrices are simply storage elements. If you want to do LA just use the almost-transparent Eigen::Map functionality.
Given the aforementioned point, I would have been against adding an Eigen::Vector as storage element in the class, as IMHO this would not have added anything on top of what is already provided.
But, inheriting from Eigen::Vector seems even worse to me. 90% of the time, inheritance is the wrong choice, even more from an Eigen type.

Maybe, if you can describe what issues do you currently have with iDynTree types, we can find a better way to handle this?

[GiulioRomualdi]

Hi @francesco-romano, nice to hear from you!
The main limitation that we have right now is the use of linear algebra in template functions/classes.

For instance, I would like to call the very same function with different types of objects (e.g. iDynTree, Eigen, CasADi...), since in iDynTree the linear algebra is not defined (you should use toEigen() for generating an Eigen::Map) this may have some limitations. Here an example ami-iit/bipedal-locomotion-framework#46 (comment)

If you have time, we can discuss into more detail by using one of the many web conferencing services. I think that also @traversaro may be interested in this.

[francesco-romano]

Happy to have a chat offline about this.

In general I think the real question is: why do you want to call the same function with different objects? If you are not in control of all the classes, I see this to be fragile. You can add static_assert of all the interest types but this would not scale.
So, I think you should tackle the problem from the opposite side. Anyway, let's talk offline

[traversaro]

As the need to have iDynTree objects that behave like Eigen object is not necessary anymore due to ami-iit/bipedal-locomotion-framework#63, I think we can close this issue and PR #709, what do you think @GiulioRomualdi ?

[GiulioRomualdi]

Yes we can close it.