Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork56.4k
Add to/from Euler Angles in quaternion#19098
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| { | ||
| Quat<T> q1 = Quat<T>::createFromXRot(angles(0) /2.); | ||
| Quat<T> q2 = Quat<T>::createFromYRot(angles(1) /2.); | ||
| Quat<T> q3 = Quat<T>::createFromZRot(angles(2) /2.); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
/ 2.
* 0.5f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK! Thanks for your reviewing code for us.
Uh oh!
There was an error while loading.Please reload this page.
| Quat<T> q1 = Quat<T>::createFromXRot(angles(0) /2.); | ||
| Quat<T> q2 = Quat<T>::createFromYRot(angles(1) /2.); | ||
| Quat<T> q3 = Quat<T>::createFromZRot(angles(2) /2.); | ||
| q = q1 * q2 * q3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lets reduce amount of generated binary code.
Make private function (detail namespace should be fine)
template <typename T>Quat<T> createQuatFromRot(int axis, T angle){ if (i == 0) return Quat<T>::createFromXRot(angle * 0.5f); if (i == 1) return Quat<T>::createFromXRot(angle * 0.5f); if (i == 2) return Quat<T>::createFromXRot(angle * 0.5f); CV_Assert(0);}Fill axis1,axis2,axis3 (preferable fromstatic const int rotationAxis[][3] = { { i1, i2, i3 }, { i1, i2, i3}, ... }; table):
int axis1, axis2, axis3; // values are 0/1/2 for X/Y/Zenum EulerAnglesType{ ...#ifndef CV_DOXYGEN EULER_ANGLES_MAX_VALUE#endif };+ static assert for table size using EULER_ANGLES_MAX_VALUE+ runtime assert: eulerAnglesType < EULER_ANGLES_MAX_VALUEFinally, use this common code in epilog:
Quat<T> q1 = Quat<T>::createQuatFromRot(axis1, angles(0));Quat<T> q2 = Quat<T>::createQuatFromRot(axis2, angles(1));Quat<T> q3 = Quat<T>::createQuatFromRot(axis3, angles(2));q = q1 * q2 * q3;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, I got it and I will update the implementation oftoEulerAngle() in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@alalek The table size of rotationAxis is fixed (24,4). Is it necessary to addstatic_assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Static assert is a compilation guard if someone would change/update corresponding enum values.
| if (abs(R(0,2) -1) < CV_QUAT_CONVERT_THRESHOLD) | ||
| { | ||
| std::cout <<"WARNING: Gimbal Lock will occur. Euler angles are non-unique." << std::endl; | ||
| std::cout <<"For intrinsic rotations, we set the third angle to zero, and for external rotation, we set the first angle to zero." << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
std::cout
Avoid that. There isCV_LOG_WARNING(). However messages probably should be reduced to INFO level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done!
| * | ||
| * The three elemental rotations may be [extrinsic](https://en.wikipedia.org/wiki/Euler_angles#Definition_by_extrinsic_rotations) | ||
| * (rotations about the axes *xyz* of the original coordinate system, which is assumed to remain motionless), | ||
| * or [intrinsic](https://en.wikipedia.org/wiki/Euler_angles#Definition_by_intrinsic_rotations)(rotations about the axes of the rotating coordinate system *XYZ*, solidary with the moving body, which changes its orientation after each elemental rotation). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please fix the links to paragraphs in that wiki article
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, we will update doc.
| * Using intrinsic rotations with Euler angles type XYZ as an example, | ||
| * \f$\theta_1 \f$, \f$\theta_2 \f$, \f$\theta_3 \f$ are three angles for Euler angles, the rotation matrix R can be calculated by:\f[R =X(\theta_1)Y(\theta_2)Z(\theta_3) | ||
| * ={\begin{bmatrix}\cos\theta_{2}\cos\theta_{3}&-\cos\theta_{2}\sin\theta_{3}&\sin\theta_{2}\\\cos\theta_{1}\sin\theta_{3}+\cos\theta_{3}\sin\theta_{1}\sin\theta_{2}&\cos\theta_{1}\cos\theta_{3}-\sin\theta_{1}\sin\theta_{2}\sin\theta_{3}&-\cos\theta_{2}\sin\theta_{1}\\\sin\theta_{1}\sin\theta_{3}-\cos\theta_{1}\cos\theta_{3}\sin\theta_{2}&\cos\theta_{3}\sin\theta_{1}+\cos\theta_{1}\sin\theta_{2}\sin\theta_{3}&\cos\theta_{1}\cos_{2}\end{bmatrix}}\f] | ||
| * Rotation matrix M and R are equal. As long as \f$ s_{2} \neq 1 \f$, by comparing each element of two matrices ,the solution is\f$\begin{cases} \theta_1 = \arctan2(-m_{23},m_{33})\\\theta_2 = arcsin(m_{13}) \\\theta_3 = \arctan2(-m_{12},m_{11}) \end{cases}\f$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please clarify what iss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, thanks!s_2 meansin\theta_2, I will fix the issue.
Uh oh!
There was an error while loading.Please reload this page.
alalek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please add commits with code optimizations from here:https://github.com/alalek/opencv/commits/pr19098_r
| * @param assumeUnit if QUAT_ASSUME_UNIT, this quaternion assume to be a unit quaternion and | ||
| * this function will save some computations. Otherwise, this function willnormalized this | ||
| * quaternion at first thentodo the transformation. | ||
| * this function will save some computations. Otherwise, this function willnormaliz this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, thanks for your commits! We have added these codes and removed the typo.
| namespace | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
namespace detail { should be better
| namespace | ||
| { | ||
| template<typename T> | ||
| Quat<T>createFromAxisRot(int axis,const T theta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It make sense to addstatic to keep this local:
template <typename T> staticQuat<T> createFromAxisRot(int axis, const T theta)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you! I got it！
| Quat<T> q3 =createFromAxisRot(rotationAxis[eulerAnglesType][2],angles(2)); | ||
| return q1 * q2 * q3; | ||
| } | ||
| elseif (rotationAxis[eulerAnglesType][3]){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
}else // (rotationAxis[eulerAnglesType][3]){and removeCV_Assert(0).
Uh oh!
There was an error while loading.Please reload this page.
| {R(2,2),0,R(1,0),R(1,1), CV_PI,R(1,0),R(0,0),R(2,0),R(2,1),R(2,2),R(0,2), -R(1,2),1},//EXT_ZXZ | ||
| {R(2,2),0,R(1,0),R(0,0), CV_PI,R(1,0),R(0,0),R(2,1), -R(2,0),R(2,2),R(1,2),R(0,2),1}//EXT_ZYZ | ||
| }; | ||
| if(!rotationR[eulerAnglesType][12]){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Before thisif build dynamic table:
T rotationR[12] through decoding in a loop using the indexes above.
And apply replacementrotationR[eulerAnglesType] =>rotationR for code below (it should not have large changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK! We really appreciate your help.
| * For Proper Euler angles,the valid range of \f$\theta_2\f$ is in [0, π]. The solutions of Euler angles are unique in condition of \f$ \theta_2 \in (0, π)\f$ . If \f$\theta_2 =0 \f$ or \f$\theta_2 =π \f$, | ||
| * there are infinite solutions and gimbal lock will occur. | ||
| */ | ||
| enum EulerAnglesType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is better to avoid placing enums in template classes.
I see these options:
- base non-template class with such constants (
QuatEnum?) - external enum with proper prefixes (like
QUAT_ASSUME_NOT_UNIT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, we will create classQuatEnum to place enumEulerAnglesType.
| Quatd qEuler1 =Quatd::createFromEulerAngles(test_angle, Quatd::INT_XYZ); | ||
| Quatd qEuler2 =Quatd::createFromEulerAngles(test_angle, Quatd::INT_XZY); | ||
| Quatd qEuler3 =Quatd::createFromEulerAngles(test_angle, Quatd::INT_XZX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider using arrays and for loops to process/check values. LikeqEuler[24]
This should reduce amount of code.
Use error messages to help with problem investigations. See proposed commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, we use for loops and arrayqEuler[24] in test codes to reduce duplicate code.
| TEST_F(QuatTest, EulerAngles){ | ||
| Vec3d test_angle={0.523598,0.78539,1.04719}; | ||
| Quatd qEuler0 =Quatd(0,0,0,0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not transform these huge lists (here and below) to loops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, thanks, I solved this problem by using the for loop method and arrayqEuler.
alalek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good progress!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…test_quaternion.cpp
alalek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me 👍
* add to/from Euler Angles* restruct codes* quat: optimize implementation* cleanup debug code* correct spelling errors* create QuatEnum for enum EulerAnglesType* use for loop for test_quaternion* drop template from isIntAngleType & add minimal error information in test_quaternion.cppCo-authored-by: ShanChenqi <shanchenqi@huawei.com>Co-authored-by: Alexander Alekhin <alexander.a.alekhin@gmail.com>
We finally support all types of Euler angles to/from the quaternion.
Pull Request Readiness Checklist
See details athttps://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.