Update matlab bindings adding inverse-kinematics #633

lrapetti · 2020-01-16T09:53:14Z

With this PR bindings for inverse-kinematics are generated.

The commit has been done accordingly to the guidelines.

@traversaro is it ok to target master with the updated bindings or should devel be targeted instead?

traversaro · 2020-01-16T10:14:12Z

@traversaro is it ok to target master with the updated bindings or should devel be targeted instead?

Let's target devel as this is a new feature, thanks. I cleaned up the release process for 1.0, so I think a 1.1 release should be faster.

traversaro · 2020-01-16T10:17:07Z

Can you also update the Changelog at https://github.com/robotology/idyntree/blob/devel/CHANGELOG.md ? Thanks!

lrapetti · 2020-01-16T10:22:54Z

Sorry, by mistake I committed also the change in #631, I'll clean it up

lrapetti · 2020-01-16T10:28:26Z

Sorry, by mistake I committed also the change in #631, I'll clean it up

Fixed

Let's target devel as this is a new feature, thanks. I cleaned up the release process for 1.0, so I think a 1.1 release should be faster.

Target changed to devel

Can you also update the Changelog at https://github.com/robotology/idyntree/blob/devel/CHANGELOG.md ? Thanks!

Done d7e2035

traversaro · 2020-01-16T11:36:38Z

CHANGELOG.md

@@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [1.0.1] - 2020-01-14

+### Added
+- Added `bindings` for `InverseKinematics` (https://github.com/robotology/idyntree/pull/633)
+


Sorry, probably editing the ChangeLog is still confusing. The release 1.0.1 has been already done, so this change will go in a currently unreleased version, so it should be added in the ## [Unreleased] section, see https://keepachangelog.com/en/1.0.0/ for more details.

ah ok! Yes, actually in fact I was a bit confused and I was not sure about the position

It is now moved under Unreleased

traversaro · 2020-01-16T14:29:08Z

I just realized that unfortunately we can't merge the PR in its current form, sorry.

The main problem is that as it is now, we are breaking the following configuration (that is used, especially in Windows, in some repos, even if it is not tested by iDynTree's repo CI):

IDYNTREE_USES_IPOPT = OFF
IDYNTREE_USES_MATLAB = ON or IDYNTREE_USES_OCTAVE = ON

This is due to the fact that we commit the generated source code with the modified SWIG that supports matlab.
We faced this problem in the past also with other dependencies, in particular Irrlicht for the visualization library and Assimp for the solidshapes helpers library. In both cases, the solution was to compile a dummy version of the related libraries when the specified dependency is missing, that if used created an error at runtime, see:

In the case of InverseKinematics, I think the easiest option is to prepare a dummy InverseKinematics.cpp to compile when Ipopt is not available https://github.com/robotology/idyntree/blob/v1.0.1/src/inverse-kinematics/src/InverseKinematics.cpp , that in place of all the methods just prints an error that explains that IDYNTREE_USES_IPOPT should be enabled to use that class.
Sorry for the boring work, but without this or something equivalent it would be complicate to me to merge such a change.

lrapetti · 2020-01-16T17:20:19Z

In the case of InverseKinematics, I think the easiest option is to prepare a dummy InverseKinematics.cpp to compile when Ipopt is not available https://github.com/robotology/idyntree/blob/v1.0.1/src/inverse-kinematics/src/InverseKinematics.cpp , that in place of all the methods just prints an error that explains that IDYNTREE_USES_IPOPT should be enabled to use that class.
Sorry for the boring work, but without this or something equivalent it would be complicate to me to merge such a change.

@traversaro
Ok no worry, I can work this out. Does the "dummy class" need to implement all the actual methods of the class?

traversaro · 2020-01-17T16:33:17Z

Ok no worry, I can work this out. Does the "dummy class" need to implement all the actual methods of the class?

Yes, otherwise you would get a linking error.

lrapetti · 2020-01-17T16:41:04Z

With 0657826 I have implemented the dummy methods. However I was thinking that a possible cleaner solutions could be:

to define directly a dummy class InverseKinematicsDataDummy so we would move almost all the conditions in that class. So eventually in the future we may have a different solver working for windows and we can implemente the class for the other solver.
to use just a single if redefining all the methods. But I went for using multiple ifs because in this case if a new method is add this avoid forgetting adding it also in the NOT_USE_ASSIMP case.

ahoarau · 2020-01-17T17:00:23Z

src/CMakeLists.txt


 if (IDYNTREE_USES_IPOPT)
  set(IDYNTREE_BUILD_IK TRUE)
-  add_subdirectory(inverse-kinematics)
 endif()

 if (IDYNTREE_COMPILES_OPTIMALCONTROL)


IDYNTREE_USES_OPTIMALCONTROL instead ?

I think currently the way the option are used is: IDYNTREE_USES_{dependency_name} (e.g. for IDYNTREE_USES_YARP or IDYNTREE_USES_IPOPT), and IDYNTREE_COMPILES_{IDynTree component} (e.g. IDYNTREE_COMPILES_TESTS). So in this case I would say IDYNTREE_COMPILES_OPTIMALCONTROL is coherent with the rest of the code, even we might want to change and use always USE.

Anyway, this part did not change within this PR, so eventually the discussion might be moved elsewhere.

traversaro · 2020-01-18T21:29:51Z

But I went for using multiple ifs because in this case if a new method is add this avoid forgetting adding it also in the NOT_USE_ASSIMP case.

Yes, this make sense, thanks!

traversaro · 2020-01-18T22:23:48Z

src/inverse-kinematics/CMakeLists.txt

@@ -37,6 +37,7 @@ target_include_directories(${libraryname} SYSTEM PRIVATE ${EIGEN3_INCLUDE_DIR})
 target_link_libraries(${libraryname} idyntree-core idyntree-high-level ${IPOPT_LIBRARIES})

 target_compile_definitions(${libraryname} PRIVATE ${IPOPT_DEFINITIONS})
+target_compile_definitions(${libraryname} PRIVATE IDYNTREE_USES_IPOPT)


As you can see from the GitHub Actions failures, you need to also modify the CMakeLists.txt of this library to link IPOPT, pass the IDYNTREE_USES_IPOPT definition, add the private sources that depend on IPOPT only if IDYNTREE_USES_IPOPT is set to FALSE, similarly to what is done for assimp in the solid-shapes lib, see https://github.com/robotology/idyntree/blob/v1.0.1/src/solid-shapes/CMakeLists.txt . Note that as we require CMake 3.5 now, if necessary you can also use target_sources command, see https://github.com/robotology/idyntree/blob/v1.0.1/src/solid-shapes/CMakeLists.txt .

traversaro · 2020-01-18T22:24:40Z

See GitHub Actions failures and #633 (comment) .

lrapetti · 2020-01-23T09:39:53Z

See GitHub Actions failures and #633 (comment) .

it should be fixed in ee3d335.

lrapetti · 2020-01-23T09:43:48Z

src/inverse-kinematics/CMakeLists.txt

-
-add_library(${libraryname} ${IDYN_TREE_IK_HEADERS} ${IDYN_TREE_IK_SOURCES}
-                           ${PRIVATE_IDYN_TREE_IK_SOURCES} ${PRIVATE_IDYN_TREE_IK_HEADERS})
+if(IDYNTREE_USES_IPOPT)


I had this if since those classes are not compiled in the dummy class.
@traversaro please check if it looks sound for you or if you think there is a better way to handle it.

Using target_sources you can avoid to duplicate the add_library call, in any case what you did is ok, thanks!

lrapetti · 2020-01-23T11:57:18Z

Some tests, apparently not related to inverse-kinematics, are failing

traversaro · 2020-01-23T13:47:52Z

Some tests, apparently not related to inverse-kinematics, are failing

Yes, those are probably not related to the PR and caused by robotology/yarp#2189 .

traversaro · 2020-01-23T14:11:02Z

Some tests, apparently not related to inverse-kinematics, are failing

Failures are not related to PR, but it seems to be related to robotology/yarp#2189 .

traversaro · 2020-01-23T14:32:22Z

Can you rebase all the commits except ea370f3 or in genear clean the history? After that, the change can be merged.

lrapetti · 2020-01-23T14:59:51Z

Can you rebase all the commits except ea370f3 or in genear clean the history? After that, the change can be merged.

Done

traversaro · 2020-01-24T08:21:47Z

Can you rebase all the commits except ea370f3 or in genear clean the history? After that, the change can be merged.

Done

Thanks!

lrapetti mentioned this pull request Jan 16, 2020

Generate bindings for inverse-kinematics class #632

Closed

lrapetti changed the base branch from master to devel January 16, 2020 10:16

lrapetti force-pushed the add-inverse-kinematics-matlab-bindings branch from 0645894 to ea370f3 Compare January 16, 2020 10:24

traversaro reviewed Jan 16, 2020

View reviewed changes

lrapetti force-pushed the add-inverse-kinematics-matlab-bindings branch from d7e2035 to c297d56 Compare January 16, 2020 13:16

ahoarau reviewed Jan 17, 2020

View reviewed changes

traversaro reviewed Jan 18, 2020

View reviewed changes

traversaro mentioned this pull request Jan 18, 2020

GitHub Actions: add check of successful compilation if some dependencies are disabled #636

Merged

lrapetti commented Jan 23, 2020

View reviewed changes

lrapetti force-pushed the add-inverse-kinematics-matlab-bindings branch 2 times, most recently from e0a1166 to f16f240 Compare January 23, 2020 14:57

lrapetti added 3 commits January 23, 2020 15:58

[bindings] update matlab bindings

19e4328

Update CHANGELOG.md

bee5888

Compile dummy InverseKinematics when USE_IPOPT is OFF

ade4b8f

lrapetti force-pushed the add-inverse-kinematics-matlab-bindings branch from f16f240 to ade4b8f Compare January 23, 2020 14:59

traversaro merged commit b87350a into robotology:devel Jan 24, 2020

lrapetti deleted the add-inverse-kinematics-matlab-bindings branch January 24, 2020 09:22

lrapetti mentioned this pull request Jan 27, 2020

Expose iDynTree.InverseKinematics in Matlab bindings #498

Closed

traversaro mentioned this pull request Apr 29, 2020

Understand why iDynTree inverse kinematics crashes in Simulink and Matlab #684

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update matlab bindings adding inverse-kinematics #633

Update matlab bindings adding inverse-kinematics #633

lrapetti commented Jan 16, 2020

traversaro commented Jan 16, 2020

traversaro commented Jan 16, 2020

lrapetti commented Jan 16, 2020

lrapetti commented Jan 16, 2020

traversaro Jan 16, 2020

lrapetti Jan 16, 2020

lrapetti Jan 16, 2020

traversaro commented Jan 16, 2020

lrapetti commented Jan 16, 2020 •

edited

Loading

traversaro commented Jan 17, 2020

lrapetti commented Jan 17, 2020

ahoarau Jan 17, 2020

lrapetti Jan 18, 2020 •

edited

Loading

traversaro commented Jan 18, 2020

traversaro Jan 18, 2020

traversaro commented Jan 18, 2020

lrapetti commented Jan 23, 2020

lrapetti Jan 23, 2020

traversaro Jan 23, 2020

lrapetti commented Jan 23, 2020

traversaro commented Jan 23, 2020

traversaro commented Jan 23, 2020

traversaro commented Jan 23, 2020

lrapetti commented Jan 23, 2020

traversaro commented Jan 24, 2020

Update matlab bindings adding inverse-kinematics #633

Update matlab bindings adding inverse-kinematics #633

Conversation

lrapetti commented Jan 16, 2020

traversaro commented Jan 16, 2020

traversaro commented Jan 16, 2020

lrapetti commented Jan 16, 2020

lrapetti commented Jan 16, 2020

traversaro Jan 16, 2020

Choose a reason for hiding this comment

lrapetti Jan 16, 2020

Choose a reason for hiding this comment

lrapetti Jan 16, 2020

Choose a reason for hiding this comment

traversaro commented Jan 16, 2020

lrapetti commented Jan 16, 2020 • edited Loading

traversaro commented Jan 17, 2020

lrapetti commented Jan 17, 2020

ahoarau Jan 17, 2020

Choose a reason for hiding this comment

lrapetti Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

traversaro commented Jan 18, 2020

traversaro Jan 18, 2020

Choose a reason for hiding this comment

traversaro commented Jan 18, 2020

lrapetti commented Jan 23, 2020

lrapetti Jan 23, 2020

Choose a reason for hiding this comment

traversaro Jan 23, 2020

Choose a reason for hiding this comment

lrapetti commented Jan 23, 2020

traversaro commented Jan 23, 2020

traversaro commented Jan 23, 2020

traversaro commented Jan 23, 2020

lrapetti commented Jan 23, 2020

traversaro commented Jan 24, 2020

lrapetti commented Jan 16, 2020 •

edited

Loading

lrapetti Jan 18, 2020 •

edited

Loading