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

Introduce Eigen::Vector to describe the iDynTree::VectorFixSize #709

Conversation

GiulioRomualdi
Copy link
Member

This PR is in view of #707.

In details the with this PR DynTree::VectorFixSize will inherit from Eigen::Matrix<double, VecSize, 1>, consequentially, the methods already implemented in Eigen (e.g. data()) have been removed.

@S-Dafarra
Copy link
Contributor

S-Dafarra commented Jul 1, 2020

Probably it is also worth changing the corresponding toEigen methods in the EigenHelpers. Probably we can just deprecate them and simply return the vector itself.

@traversaro
Copy link
Member

Probably it is also worth changing the corresponding toEigen methods in the EigenHelpers. Probably we can just deprecate them and simply return the vector itself.

Given that the Eigen::Map may have tricky semantics differences from Eigen::Matrix, I would keep the existing toEigen function. If you want to get the actual vector and you know you are using iDynTree 2, you can just avoid using the toEigen at all.

@traversaro
Copy link
Member

To enable C++17, let's use target_compile_features with the cxx_std_17 meta-feature. Using target_compile_options is just a legacy from the past.

Copy link
Contributor

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't know what it could be the effect of this for Matlab and Python bindings though.

@GiulioRomualdi
Copy link
Member Author

Before merging we should understand if the MATLAB bindings are generated without any errors. See here

@GiulioRomualdi
Copy link
Member Author

When I try to generate the MatLab bindings I obtain the following output

/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,3,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,3,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,4,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,4,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,6,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,6,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,10,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,10,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,16,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,16,1 >' using %template.

I think it's expected right? @traversaro

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Jul 1, 2020

This commit 6ea2d4b introduced c++17. I think that as a conequence it breaks the compilation on macOS https://github.com/robotology/idyntree/pull/709/checks?check_run_id=827232152#step:15:934

Indeed random_shuffle was deprecated in C++14 and completely removed in C++17. See here

@traversaro
Copy link
Member

When I try to generate the MatLab bindings I obtain the following output

/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,3,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,3,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,4,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,4,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,6,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,6,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,10,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,10,1 >' using %template.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Nothing known about base class 'Eigen::Matrix< double,16,1 >'. Ignored.
/home/gromualdi/robot-code/robotology-superbuild/robotology/iDynTree/src/core/include/iDynTree/Core/VectorFixSize.h:1: Warning 401: Maybe you forgot to instantiate 'Eigen::Matrix< double,16,1 >' using %template.

I think it's expected right? @traversaro

Yes, that basically means that the base class is ignored.

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Jul 1, 2020

Yes, that basically means that the base class is ignored.

However, there are functions like std::string VectorFixSize<VecSize>::toString() where a function implemented in Eigen is used (e.g. this->operator[]). The fact that Eigen is simplicity skipped may cause problems is the function toString() is called in a language (i.e. Python/MatLab) where Eigen is not supported. Am I miss something?

The same story also for VectorFixSize<VecSize>::setVal(const unsigned int index, const double new_el)

@traversaro
Copy link
Member

Yes, that basically means that the base class is ignored.

However, there are functions like std::string VectorFixSize<VecSize>::toString() where a function implemented in Eigen is used (e.g. this->operator[]). The fact that Eigen is simplicity skipped may cause problems is the function toString() is called in a language (i.e. Python/MatLab) where Eigen is not supported. Am I miss something?

With "the base class is ignored in SWIG" we meant that SWIG does not try to generate glue code to call the methods of the base class from Python/MATLAB. However, the glue code to call the methods of the child class (VectorFixSize) are correctly generated, and once the glue code calls the C++ method of the child class, then the C++ code is execute normally, so calling then the method of the base class.

@GiulioRomualdi
Copy link
Member Author

@traversaro shall I push also the autogenerated files of the matlab bindings?

@traversaro
Copy link
Member

@traversaro shall I push also the autogenerated files of the matlab bindings?

If you don't use them, I do not think is necessary, I think a check just to make sure they continue to work is ok.

@GiulioRomualdi
Copy link
Member Author

Close in view of #707 (comment)

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.

None yet

4 participants