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.

Sign up for GitHub

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

Jump to bottom

Add MatrixView and Span in InverseKinematics #822

Merged
merged 8 commits into from
Feb 8, 2021
Merged

Add MatrixView and Span in InverseKinematics #822

merged 8 commits into from
Feb 8, 2021

Conversation

GiulioRomualdi
Copy link
Member

This PR introduces the possibility to use MatrixView and Span as input in InverseKinematics class.

I also changed the signature of setProjectionAlongDirection and projectAlongDirection to take const references to vectors.

@GiulioRomualdi GiulioRomualdi self-assigned this Feb 4, 2021
@GiulioRomualdi
Copy link
Member Author

Python tests fail :(

@traversaro
Copy link
Member

I also changed the signature of setProjectionAlongDirection and projectAlongDirection to take const references to vectors.

Can you drop this change, i.e. the commit 5a57812 and similar changes in public APIs? It is an ABI-breaking change, and I think it would be great to land this change in iDynTree 3.x . Similar changes in private headers (such as changes in private/InverseKinematicsData.h) are ok instead. To avoid forgetting about this when we break ABI again, I added the issue #823 .

@GiulioRomualdi
Revert "Change the signature of ConvexHullProjectionConstraint::proje…
33fcbd4 
…ctAlongDirection() and ConvexHullProjectionConstraint::setProjectionAlongDirection() methods"

This reverts commit 5a57812.
@GiulioRomualdi
Update InverseKinematics to avoid API breaking
60cbf25
@GiulioRomualdi
Copy link
Member Author

Done! Let me know if it's ok.

I'm implementing the test

traversaro
traversaro reviewed Feb 5, 2021
View reviewed changes
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Perfect!

@traversaro
Copy link
Member

I'm implementing the test

Let me know if you are working on this, otherwise we can also merge it as it is for me.

@GiulioRomualdi
Copy link
Member Author

Let me know if you are working on this, otherwise we can also merge it as it is for me.

I try to write the test before Monday.

I would be sure that everything is working as expected before merging

@traversaro
Copy link
Member

Let me know if you are working on this, otherwise we can also merge it as it is for me.

I try to write the test before Monday.

I would be sure that everything is working as expected before merging

Ok!

@GiulioRomualdi
Copy link
Member Author

Thanks to the test I wrote in cecad9b I was able to spot a bug that has been fixed in cecad9b

@traversaro
Copy link
Member

Can we merge this? Squash and merge?

@GiulioRomualdi
Copy link
Member Author

Yes you can squash!

@traversaro traversaro merged commit 2aa71a2 into robotology:devel Feb 8, 2021
@GiulioRomualdi GiulioRomualdi deleted the IK/matrixview_span branch February 8, 2021 15:38
@diegoferigo diegoferigo mentioned this pull request Feb 9, 2021
Merged
@diegoferigo diegoferigo mentioned this pull request Apr 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traversaro traversaro traversaro approved these changes

Assignees

@GiulioRomualdi GiulioRomualdi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GiulioRomualdi @traversaro