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

Bipedal-locomotion-framework and iDynTree. A love that has come to an end #63

Closed
GiulioRomualdi opened this issue Jul 6, 2020 · 9 comments
Assignees
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@GiulioRomualdi
Copy link
Member

GiulioRomualdi commented Jul 6, 2020

Disclaimer The title of the issue is deliberately provocative.

In robotology/idyntree#707 I show the possibility to use Eigen in iDynTree public API. The main pro of using Eigen is enabling the linear algebra.
Discussing with @traversaro we realized that even if describing an iDynTree::VectorFixSize<> with Eigen is relatively simple robotology/idyntree#709, implementing the same approach for an iDynTree::VectorDynSize is way less trivial. Indeed in iDynTree, the concept of capacity is widely used. This concept does not have a corresponding in Eigen.

In order to solve #56 and other similar problems, we (@traversaro and I) drew 2 + 1 possible solutions:

  1. Nothing changes We keep the status quo.

  2. Introduce operator+() (and all the others) in iDynTree between matrices and vector

    • Pro it should be a relatively simple work
    • Cons every time an operation is computed new memory is allocated. 😢 I would avoid this.
  3. Avoid to use iDynTree in bipedal locomotion framework:

    • Pro: If all the methods and public attributes of the classes are Eigen It will be easy to use the library in other projects. Hopefully, the library will be used also in other labs since Eigen can be considered as numpy for Python. In the past, we already used Eigen instead of iDynTree in osqp-eigen.
    • Cons: It might be a long task.

In order to tackle the second point we should write a simple interface to iDynTree::KinDynComputations that takes as input Eigen Objects. The interface becomes useless as soon as the KinDynComputations class accepts iDynTree::Span

We should make a decision as fast as possible since the code in the repository is increasing day after day. (In theory, we should decide before merging #61 #62)

@dic-iit/dynamic-interaction-control

@GiulioRomualdi GiulioRomualdi added help wanted Extra attention is needed question Further information is requested labels Jul 6, 2020
@traversaro
Copy link
Collaborator

https://www.youtube.com/watch?v=4pBo-GL9SRg

@S-Dafarra
Copy link
Member

To be honest, it is not clear to me the outcome. Do you want to completely remove iDynTree as a dependency, or you just want to avoid using the iDynTree::VectorDynSize as much as possible? To be honest, removing iDynTree as a dependency is even trickier since right now we need Span (which can be fully exported by the way) while in the future we will need some library for dynamic computations 🤔

@GiulioRomualdi
Copy link
Member Author

The main idea is to keep iDynTree in private dependency (except for span) and use VectorXd, Eigen::Ref etc etc... in the public API.

Correct me if I'm mistaken @traversaro

@traversaro
Copy link
Collaborator

Yes, the idea is that Eigen::VectorXd or Eigen::Ref may be more convenient interface type then iDynTree::VectorDynSize now that alignment issues have been resolved. I don't see any benefit in trying to replace objects that do not have an equivalence in Eigen (such as iDynTree::Span, for which instead we need to wait for C++20 and std::span).

@diegoferigo
Copy link
Member

diegoferigo commented Jul 6, 2020

I think this should have been the original design of the public APIs :)

I am a big fan of STL-only APIs (e.g. robotology/gym-ignition#158 and the wearable interfaces for interfaces I recently desiged), they simplify system integration to a level that is priceless. The PIMPL idiom should also be a must. Of course, everything keeps working fine in the case only vectors are required. When there are APIs that need to accept or return matrices, a math-oriented library is often necessary. Now that Eigen solved its problems in public APIs, I will start advocating for this solution.

@GiulioRomualdi
Copy link
Member Author

Discussing with @traversaro and @S-Dafarra we decided to start using the Eigen in the public API.

For this reason, I will change the code of #61 to be in accordance with this new choice. I kindly ask @S-Dafarra to do the same for #62.

For the already existing code, I start the conversion in the "free time". I will open a PR for each existing component

@DanielePucci
Copy link
Member

CC @dic-iit/dynamic-interaction-control

@S-Dafarra
Copy link
Member

The choice of using Eigen in the public API of BipedalLocomotionFramework has already been assimilated through several PRs. I think we can close this issue now.

@traversaro
Copy link
Collaborator

I guess that then we can close robotology/idyntree#707 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants