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

Affine3d and PTransformd.inverse() support #5

Closed
ahundt opened this issue Nov 8, 2016 · 3 comments
Closed

Affine3d and PTransformd.inverse() support #5

ahundt opened this issue Nov 8, 2016 · 3 comments

Comments

@ahundt
Copy link

ahundt commented Nov 8, 2016

I might create a pull request for initializing a PTransformd with an Affine3d, and adding .inverse() to PTransformd since that will make it consistent with its own functions and the equivalents in Eigen.

  1. Is there interest, that is can I expect a reasonable implementation and pull request like this would be merged?

  2. The Affine3d rotation component is inverted per the SpaceVecAlg convention. Should this be handled here, or perhaps in a separate conversion header?

@haudren
Copy link
Collaborator

haudren commented Nov 9, 2016

You can submit a PR to initialize a PTransformd from an Affine3d, it is always good to have a better interoperability with Eigen.

For your second point, no it should not: the inverse() convention is a RBDyn convention. Thus it should not be handled here (and it is of course strictly a no-go to modify the default behaviour).

@ahundt
Copy link
Author

ahundt commented Nov 10, 2016

To verify, for item 1 I then assume it should not perform an inverse in the constructor.

For item 2 would a conversion.h in RBDyn with documentation of why it exists be appropriate, and provides functions that perform the inversion for various appropriate types like Affine3d (though templated for the general versions), quaternion, matrixXd etc?

I also understand that switching conventions there would be a big transition like the one from Eigen2 to Eigen3 so it most likely isn't practical or worthwhile to make changes everywhere across many libraries, which would be necessary for such a low level change!

@haudren
Copy link
Collaborator

haudren commented Jun 20, 2017

This will be fixed (at last!) by #15 . Please have a look if you have any remarks / questions.

@haudren haudren closed this as completed Jun 20, 2017
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

No branches or pull requests

2 participants