Adding skeleton for planners #43

S-Dafarra · 2020-05-07T08:15:33Z

This PR is the first with the aim of adding planners to the bipedal-locomotion-framework.

While doing this, I spotted a few issues in the CMake configurations:

the option IS_INTERFACE does not need anything afterward (see the example in https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html)
the file FindEigen3.cmake should not be necessary anymore as the supported OSs already ship a decent version of this file.
iDynTree is a strong dependency right now. Basically all the targets depend on it, either directly or indirectly (via GenericContainer for example).
I tried to avoid using ${iDynTree_LIBRARIES}, linking only the needed target, in this case iDynTree::idyntree-core. This opened for a new issue: Estimators was depending on Eigen, but this dependency was not correctly handled. Some target in ${iDynTree_LIBRARIES} is including it publicly, so this issue did not appear before. Hence,
- I added the possibility to specify the Eigen dependency in the add_bipedal_locomotion_library function
- I set this dependency for the Estimators part.

In addition, I wanted to rename vector_data to container_data. Initially, I was planning to add a new container and the meta-function vector_data was coming in handy. Hence, I changed its name. The attempt of adding the new container failed, but I liked the new name, so I left it there 😁.

The actual purpose of this PR is then to add two header files:

Removing findEigen3. Setting iDynTree as required as everything depends on it. Linking only the iDynTree core target. The option IS_INTERFACE does not need the ON afterwards.

Testing that GenericVector supports also a vector of iDynTree::Transforms.

S-Dafarra · 2020-05-07T08:30:17Z

I just noticed that the RecursiveLeastSquareTest depends on the EigenHelpers, but it should not be able to find Eigen. Both idyntree-core and Estimators include it privately, yet it compiles and runs. @traversaro any hint?

Edit: I guess that being the test compiled with Estimators, we are sure that the compiler knows where to find Eigen, even if this property is not propagated from the library to the test. I wonder then if it is better to add the Eigen includes also in the tests 🤔

traversaro · 2020-05-07T09:18:39Z

I just noticed that the RecursiveLeastSquareTest depends on the EigenHelpers, but it should not be able to find Eigen. Both idyntree-core and Estimators include it privately, yet it compiles and runs. @traversaro any hint?

That is intentional, even if probably documentation is lacking on the iDynTree side. EigenHelpers is an header only, and it was always meant to be a private header. At some point someone asked me to install it, and I was ok with it with the pact that iDynTree was not handling the CMake part of finding Eigen3, even because the idea was that a project was using EigenHelpers , it was already finding Eigen3 on its own. I think that in general this is a nice pattern (I wanted to move also the YARP and iCub conversions function to it, eventually) but documentation is indeed lacking. If for now you could just link or find Eigen3 explicitly in RecursiveLeastSquareTest that would be great.

…areTest.

It also change temporarily the module path in order to find packages that may not be available with find_package. See robotology/idyntree#642 (comment)

S-Dafarra · 2020-05-07T12:20:48Z

If for now you could just link or find Eigen3 explicitly in RecursiveLeastSquareTest that would be great.

No problem for the documentation, I actually mainly tried to understand from the CMake code 😁 I have added the dependency in 2ad6611.

In 690cb30 I added the dependencies in the config file. I also modified the InstallBasicPackageFiles.cmake file to modify the module path temporarily. This should address what described in robotology/idyntree#642 (comment). I tested it locally and a simple CMakeLists.txt like

project(testBipedalLocomotion CXX)
cmake_minimum_required(VERSION 3.5)

find_package(BipedalLocomotionFramework)

add_executable(test main.cpp)
target_link_libraries(test BipedalLocomotion::Framework)

works nicely. (I tried adding IPOPT locally as dependency too)

GiulioRomualdi

LGTM

S-Dafarra · 2020-05-11T12:08:06Z

Thanks @GiulioRomualdi, if you agree I will merge!

traversaro · 2020-05-11T12:17:51Z

CMakeLists.txt

+if (FRAMEWORK_HAS_YARP)
+  list(APPEND FRAMEWORK_PUBLIC_DEPENDENCIES YARP)
+endif()
+
+if (FRAMEWORK_HAS_Eigen3)
+  list(APPEND FRAMEWORK_PRIVATE_DEPENDENCIES Eigen3)
+endif()


Actually this would be a bit more robust if it depends on YARP only if the components that depends in a public way are enabled, not only if YARP or Eigen3 is found in the system.

Actually if you do not use Eigen3 as an inclusion in the public headers, I don't think it make sense to have it as a private dependency as it is an header-only dependency.

Actually this would be a bit more robust if it depends on YARP only if the components that depends in a public way are enabled

I thought about that at the beginning, but then I did not find any way to automatize this check. In principle, I would need to determine a set of dependencies depending on the compiled targets. Initially, I thought of adding an entry in the add_bipedal_library function (something like DEPENDENCIES YARP iDynTree), but this would be a sort of duplicate of the targets to link and it is error-prone since it does not throw any error if you miss some of them.
The other option was to change the if condition to check directly if some of the components having YARP as dependency are enabled (like what you suggest). But then, every time we add a new component we would need to change also those if, with a high likelihood of forgetting one from time to time. Hence, I thought that the safest and easiest option was to propagate all the dependencies that were already found in the system.

Actually if you do not use Eigen3 as an inclusion in the public headers, I don't think it make sense to have it as a private dependency as it is an header-only dependency.

Even if we compile statically?

The other option was to change the if condition to check directly if some of the components having YARP as dependency are enabled (like what you suggest). But then, every time we add a new component we would need to change also those if, with a high likelihood of forgetting one from time to time. Hence, I thought that the safest and easiest option was to propagate all the dependencies that were already found in the system.

That make sense, but can we use FRAMEWORK_USE_YARP instead of FRAMEWORK_HAS_YARP? So a user can explicitly disable it.

Actually if you do not use Eigen3 as an inclusion in the public headers, I don't think it make sense to have it as a private dependency as it is an header-only dependency.

Even if we compile statically?

An header only library is only used at compile time, and not at the linking time, so you safely link a static library that used internally Eigen3 completely ignoring the fact that Eigen3 has been used.

An header only library is only used at compile time, and not at the linking time, so you safely link a static library that used internally Eigen3 completely ignoring the fact that Eigen3 has been used.

It makes sense, I will remove it, thanks!

That make sense, but can we use FRAMEWORK_USE_YARP instead of FRAMEWORK_HAS_YARP? So a user can explicitly disable it.

Deal! Should I use the same flag also here in the following lines? https://github.com/dic-iit/bipedal-locomotion-framework/blob/690cb30180554b5f8c83c40a18ffadd3573d9488/cmake/BipedalLocomotionFrameworkFindDependencies.cmake#L131-L141

Using USE and removed Eigen as a private dependency (in the config file) in 30e2f06

S-Dafarra · 2020-05-13T07:48:21Z

If you agree, I'll merge.

GiulioRomualdi · 2020-05-13T09:19:29Z

You can merge

S-Dafarra added 3 commits May 6, 2020 20:55

Better handling of the Eigen3 dependency.

6fd0923

Removing findEigen3. Setting iDynTree as required as everything depends on it. Linking only the iDynTree core target. The option IS_INTERFACE does not need the ON afterwards.

Renamed macro vector_data to container_data.

8e3f0c8

Testing that GenericVector supports also a vector of iDynTree::Transforms.

Including Contact and Advanceable headers.

8b8b751

S-Dafarra added this to the Iteration 43 - End of Research Increment milestone May 7, 2020

S-Dafarra requested a review from GiulioRomualdi May 7, 2020 08:15

S-Dafarra self-assigned this May 7, 2020

S-Dafarra added 2 commits May 7, 2020 14:00

Setting Eigen dependency for tests and adding it in RecursiveLeastSqu…

2ad6611

…areTest.

Setting the dependencies in the config file.

690cb30

It also change temporarily the module path in order to find packages that may not be available with find_package. See robotology/idyntree#642 (comment)

S-Dafarra requested a review from traversaro May 7, 2020 12:21

GiulioRomualdi approved these changes May 11, 2020

View reviewed changes

traversaro approved these changes May 11, 2020

View reviewed changes

traversaro reviewed May 11, 2020

View reviewed changes

Using USE instead of HAS when listing dependencies.

30e2f06

S-Dafarra merged commit abee1b3 into ami-iit:master May 13, 2020

prashanthr05 mentioned this pull request Nov 12, 2020

Make compilation of Contact library independent from Planners library #141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding skeleton for planners #43

Adding skeleton for planners #43

S-Dafarra commented May 7, 2020

S-Dafarra commented May 7, 2020 •

edited

Loading

traversaro commented May 7, 2020

S-Dafarra commented May 7, 2020 •

edited

Loading

GiulioRomualdi left a comment

S-Dafarra commented May 11, 2020

traversaro May 11, 2020

traversaro May 11, 2020

S-Dafarra May 11, 2020 •

edited

Loading

traversaro May 11, 2020

S-Dafarra May 11, 2020

S-Dafarra May 13, 2020

S-Dafarra commented May 13, 2020

GiulioRomualdi commented May 13, 2020

Adding skeleton for planners #43

Adding skeleton for planners #43

Conversation

S-Dafarra commented May 7, 2020

S-Dafarra commented May 7, 2020 • edited Loading

traversaro commented May 7, 2020

S-Dafarra commented May 7, 2020 • edited Loading

GiulioRomualdi left a comment

Choose a reason for hiding this comment

S-Dafarra commented May 11, 2020

traversaro May 11, 2020

Choose a reason for hiding this comment

traversaro May 11, 2020

Choose a reason for hiding this comment

S-Dafarra May 11, 2020 • edited Loading

Choose a reason for hiding this comment

traversaro May 11, 2020

Choose a reason for hiding this comment

S-Dafarra May 11, 2020

Choose a reason for hiding this comment

S-Dafarra May 13, 2020

Choose a reason for hiding this comment

S-Dafarra commented May 13, 2020

GiulioRomualdi commented May 13, 2020

S-Dafarra commented May 7, 2020 •

edited

Loading

S-Dafarra commented May 7, 2020 •

edited

Loading

S-Dafarra May 11, 2020 •

edited

Loading