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

Add CommonConversions and ManifConversions library #138

Merged
merged 2 commits into from
Nov 12, 2020

Conversation

prashanthr05
Copy link
Collaborator

@prashanthr05 prashanthr05 commented Nov 10, 2020

This PR includes basic aliases and conversions between Eigen/manif/iDynTree.

This PR includes conversions between Eigen/manif/iDynTree objects.
Currently, only conversions of transform objects are included.

@prashanthr05 prashanthr05 added the enhancement New feature or request label Nov 10, 2020
@prashanthr05 prashanthr05 self-assigned this Nov 10, 2020
@S-Dafarra
Copy link
Member

I think we have to pay attention to the definition of the Common library. This is most likely going to be a dependency for all the libraries inside the framework. Maybe it is worth separating the actual commonalities, from the conversion utilities.

What about creating a library, like Conversions, that inside links to a different library according to the specific library?

In this way, even if we depend on a new library, we can add conversion functions without the risk of making the new library a dependency for all the components. This is what we are doing already with yarp for example.

@traversaro @GiulioRomualdi what do you think?

@prashanthr05
Copy link
Collaborator Author

I think we have to pay attention to the definition of the Common library. This is most likely going to be a dependency for all the libraries inside the framework. Maybe it is worth separating the actual commonalities, from the conversion utilities.

What about creating a library, like Conversions, that inside links to a different library according to the specific library?

In this way, even if we depend on a new library, we can add conversion functions without the risk of making the new library a dependency for all the components. This is what we are doing already with yarp for example.

@traversaro @GiulioRomualdi what do you think?

Agreed. That would be a more scalable solution. Under the Conversions library, we might define different targets depending on the specific library we want to convert from. We might also define the associated aliases/typedefs with these targets.

@traversaro
Copy link
Collaborator

traversaro commented Nov 10, 2020

I think that conversions as separate library/libraries make sense. Furthermore, I am not sure it is good idea to have all those definitions, as I think they will complicate the readability of the API. For example, everyone knows what a Eigen::Matrix3d is, why defining a function as:

void functionThatTakesARotationMatrix(BipedalLocomotion::Common::Definitions::Rotation rot)

when I think it would be much more easy to read and use as:

void functionThatTakesARotationMatrix(Eigen::Matrix3d rot)

Also for conversions, I think that:

void Conversions::eigenRotTrans2ManifPose(Eigen::Ref<const Definitions::RotationMatrix> rotation,
                                          Eigen::Ref<const Definitions::Position> translation,
                                          Definitions::Pose& pose)

is not really clear, while:

void Conversions::eigenRotTrans2ManifPose(Eigen::Ref<const Eigen::Matrix3d> rotation,
                                          Eigen::Ref<const Eigen::Vector3d> translation,
                                          manif::SE3d& pose)

you get what this function is doing directly.

@prashanthr05 prashanthr05 changed the title Add Common library to maintain definitions/conversions Add CommonConversions and ManifConversions library Nov 10, 2020
@prashanthr05 prashanthr05 force-pushed the feature/util-definitions branch 2 times, most recently from 5d9decf to dbade28 Compare November 10, 2020 18:51
@prashanthr05
Copy link
Collaborator Author

I've made some relevant changes.

Copy link
Member

@GiulioRomualdi GiulioRomualdi left a comment

Choose a reason for hiding this comment

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

@prashanthr05, Thanks for iterating.
If @traversaro and @S-Dafarra agree we can proceed with the merge

@GiulioRomualdi
Copy link
Member

@prashanthr05 last comment, could you update the CHANGELOG.md?

@prashanthr05
Copy link
Collaborator Author

@prashanthr05 last comment, could you update the CHANGELOG.md?

Done in 1c17dc6.

Copy link
Member

@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.

I noticed that the versions are somehow duplicate. Are the versions returning void needed?

src/Conversions/CMakeLists.txt Outdated Show resolved Hide resolved
@S-Dafarra
Copy link
Member

I was thinking about the naming. Is it necessary to be so specific about the input types? We could just specify the output type, like toManifPose, then the input type is automatically deduced, similar to the toEigen of iDynTree.

@prashanthr05
Copy link
Collaborator Author

I was thinking about the naming. Is it necessary to be so specific about the input types? We could just specify the output type, like toManifPose, then the input type is automatically deduced, similar to the toEigen of iDynTree.

Thanks for this suggestion.

I have made the requested changes in b6eadd6

@GiulioRomualdi
Copy link
Member

@prashanthr05 let me know if you want to reorganize the commits. Otherwise, I can merge

@prashanthr05
Copy link
Collaborator Author

@GiulioRomualdi I have reorganized the commits. You could proceed with the merge after the CI passes, thank you!

@GiulioRomualdi
Copy link
Member

Merging! Thank you all!

@GiulioRomualdi GiulioRomualdi merged commit 67b78ab into ami-iit:master Nov 12, 2020
@prashanthr05 prashanthr05 deleted the feature/util-definitions branch November 12, 2020 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants