Unify pose; Support multi-shape Physx components in SapienPlanningWorld conversion #81
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This struct is intended to be used only for interfacing with Python. Internally, Pose is converted to and stored as Eigen::Isometry3 which is used by all computations.
Closed
Lexseal
reviewed
Apr 11, 2024
There was a problem hiding this comment.
Nice! I didn't think it would be such a big effort again. Two things I was wondering
- Floating point error accumulation can de-unitize the quaternion not sure if it will affect result
- How come
Pinocchioand some other modules are still rockingIsometry? If it was just missed, I can go ahead and unify all of them in my pass.
|
|
||
| /// @brief Overloading operator * for ``Pose<S> * Pose<S>`` | ||
| Pose<S> operator*(const Pose<S> &other) const { | ||
| return {q * other.p + p, q * other.q}; |
There was a problem hiding this comment.
I am not super sure if Eigen will keep the resulting quaternion as unit quaternion due to floating point error. This may potentially break your conjugate inverse from above?
There was a problem hiding this comment.
Not sure about that too. But I guessed that it won't matter too much as we are using double instead of float.
You are certainly welcomed to add unit-tests for this.
|
Meanwhile will fix the tests. Will be busy this afternoon with classes so might get these done later today |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
mplib.Posesimilar tosapien.Posebut usesfloat64instead.sapien.Poseand 4x4 np.ndarray transformation matrix tomplib.Posesapien.Pose/np.ndarrayto any method that requires amplib.Poseand implicit conversions will jump in so there's no need to callmplib.Pose()constructor explicitly. (Maybe add tests for these as well?)pyright's type checking will give error, which is expected and cannot be handled on MPlib side imo.@Lexseal Please review this first and make a separate pull request for updating all examples/tests.
Support for Halfspace is not added here. It will have a separate pull request.