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

Enable implicit conversion and change KinDynComputations holder in the iDynTree bindings #1037

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

GiulioRomualdi
Copy link
Member

As per the title, this PR changes the holder for KinDynComputations and enables the implicit conversion. The former allows the interoperability between iDynTree swig bindings and the blf pybind11 bindings. The latter simplifies calling methods that take as inputs vectors and/or matrices. For instance, now it is possible to call

import idyntree.swig as idyn

kindyn = idyn.KinDynComputations()
kindyn.setJointPos([1, 0, 1, 1, 1, 1, 1])

as you can notice it is not necessary to explicitly create an iDynTree::VectorDynSize
This latter modification should work also in Matlab and in theory, can simplify the implementation of the idyntree high level wrappers

@traversaro
Copy link
Member

traversaro commented Dec 13, 2022

Does:

vec =  iDynTree.VectorDynSize(7)
kindyn.setJointPos(vec)

still work?

@GiulioRomualdi
Copy link
Member Author

vec =  iDynTree.VectorDynSize(7)
kindyn.setJointPos(vec)

yes, just tested

@traversaro
Copy link
Member

traversaro commented Dec 13, 2022

Per Beyonce's Rule, you may add a test in https://github.com/robotology/idyntree/tree/master/bindings/python/tests if you want to make sure that this continue to work in the past.

This latter modification should work also in Matlab and in theory, can simplify the implementation of the idyntree high level wrappers

fyi @gabrielenava

@traversaro
Copy link
Member

How did you find out of the implicitconv option? I could not find any docs on it.

@traversaro
Copy link
Member

Things to do (not that you need to do it yourself @GiulioRomualdi, I just marking them down):

@traversaro
Copy link
Member

Based on #688, probably we can put the shared_ptr stuff in #ifndef SWIGMATLAB, as the problem should be only in our old swig matlab fork.

@traversaro traversaro closed this Dec 13, 2022
@traversaro traversaro reopened this Dec 13, 2022
@GiulioRomualdi
Copy link
Member Author

Based on #688, probably we can put the shared_ptr stuff in #ifndef SWIGMATLAB, as the problem should be only in our old swig matlab fork.

Done in e74239d

@traversaro
Copy link
Member

Based on #688, probably we can put the shared_ptr stuff in #ifndef SWIGMATLAB, as the problem should be only in our old swig matlab fork.

Done in e74239d

MATLAB binding generation still fails (see https://github.com/robotology/idyntree/actions/runs/3687003195/jobs/6240008478):

/home/runner/work/idyntree/idyntree/bindings/matlab/autogenerated/iDynTreeMATLAB_wrap.cxx: In function ‘int _wrap_new_Material__SWIG_1(int, mxArray**, int, mxArray**)’:
/home/runner/work/idyntree/idyntree/bindings/matlab/autogenerated/iDynTreeMATLAB_wrap.cxx:50371:7: error: ‘SWIG_CheckImplicit’ was not declared in this scope
50371 |   if (SWIG_CheckImplicit(SWIGTYPE_p_iDynTree__Material)) SWIG_fail;
      |       ^~~~~~~~~~~~~~~~~~
make[2]: *** [bindings/matlab/CMakeFiles/idyntreeOctaveMex.dir/build.make:76: bindings/matlab/CMakeFiles/idyntreeOctaveMex.dir/autogenerated/iDynTreeMATLAB_wrap.cxx.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:907: bindings/matlab/CMakeFiles/idyntreeOctaveMex.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

Based on https://github.com/search?q=repo%3Aswig%2Fswig%20SWIG_CheckImplicit&type=code, probably also implicitconv is also Python-only .

@GiulioRomualdi GiulioRomualdi force-pushed the swig/blf_simplification branch from e74239d to 9be0778 Compare December 13, 2022 16:05
@GiulioRomualdi
Copy link
Member Author

Let's see if 9be0778 fixes the problem

@traversaro
Copy link
Member

@traversaro
Copy link
Member

This latter modification should work also in Matlab and in theory, can simplify the implementation of the idyntree high level wrappers

fyi @gabrielenava

Actually we discovered that that option is not supported in MATLAB.

@traversaro
Copy link
Member

Now the generation work: https://github.com/robotology/idyntree/actions/runs/3687142976/jobs/6240337040 (the CI fails as nothing has changed, so actually we do not need to run generation).

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.

2 participants