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 euler conversion from Quaternion #397

Closed
wants to merge 1 commit into from

Conversation

@Melix19
Copy link
Contributor

Melix19 commented Nov 18, 2019

I think that this method could be helpful in some situation using magnum, so I decided to make a PR.
I took the algorithm from QT (that it's based on another algorithm as specified in their comments) and adapted to magnum using a Vector3 as return.

It's obvious that this method should not be abused because of the problems that eulers can have (and I think that this type of conversion isn't even always 100% perfect), so maybe we could specify that in the doc?

@mosra mosra added this to the 2019.1c milestone Nov 19, 2019
@mosra mosra added this to TODO in Math and algorithms via automation Nov 19, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 19, 2019

Codecov Report

Merging #397 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   72.56%   72.59%   +0.02%     
==========================================
  Files         354      354              
  Lines       18847    18866      +19     
==========================================
+ Hits        13676    13695      +19     
  Misses       5171     5171
Impacted Files Coverage Δ
src/Magnum/Math/Quaternion.h 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3a1f74...4ea2719. Read the comment docs.

Copy link
Owner

mosra left a comment

Hi, thanks a lot! This feature is requested quite often lately (cc: @alanjfs) :)

But I can't accept it without tests, I'm afraid -- a test case for this would go into Math/Test/QuaternionTest.cpp and it needs to go through all branches of the new function (when you push, codecov will check the coverage for you and report it in the comment above). You can build & run the tests locally by enabling BUILD_TESTS.

What could work, I think, would be having a bunch of values that each triggers a different code path (how many different cases is there? four? more?) and then also a dedicated check for the normalization assert. One way that could be easy to do is checking that the values round-trip, for example:

{ 
    Quaternion a{0.35f, 0.134f, 0.37f, 0.02f}; // i don't know, something
    Math::Vector3<Rad> b{1.534_radf, 0.78_radf, 2.02_radf}; // i guess?

    CORRADE_COMPARE(a.toEuler(), b);
    CORRADE_COMPARE(b,
        Quaternion::rotate(b.z(), Vector3::zAxis())*
        Quaternion::rotate(b.y(), Vector3::yAxis())*
        Quaternion::rotate(b.x(), Vector3::xAxis()));
} { 
    Quaternion a{...};
    Math::Vector3<Rad> b{...};

     // ... and so on to ensure all branches are tested
}

Also, sorry for the excessive number of comments compared to your other PRs, but math additions need extra care :)

src/Magnum/Math/Quaternion.h Outdated Show resolved Hide resolved
src/Magnum/Math/Quaternion.h Outdated Show resolved Hide resolved
src/Magnum/Math/Quaternion.h Outdated Show resolved Hide resolved
src/Magnum/Math/Quaternion.h Outdated Show resolved Hide resolved
src/Magnum/Math/Quaternion.h Outdated Show resolved Hide resolved
src/Magnum/Math/Quaternion.h Outdated Show resolved Hide resolved
@Melix19 Melix19 force-pushed the Melix19:quaternion_to_euler branch from c3d314b to b5786dc Nov 19, 2019
@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Nov 19, 2019

Finally had time for this!

So, I changed the algorithm to the one found in wikipedia, the reason is that the Qt's one was giving a "dancing" result when converting from/to eulers in every frame (I know that this is never a good idea anyway, but it was just for testing):
Dancing rect

With this new algorithm is basically perfect.
Then, for the tests I just added a random quaternion and compared the euler result to a result given from this online converter given the same quaternion.

If there's nothing else to do, I think that it can be merged :D

@alanjfs

This comment has been minimized.

Copy link
Contributor

alanjfs commented Nov 20, 2019

Woho! Good timing! I also need this. :D

Copy link
Owner

mosra left a comment

Nice! Thanks for going the extra mile and finding a better algorithm. This one is much less branching, I like it.

Apart from the one thing and uncovered line below, can you change the commit message to say toEuler instead of toRotation? Then I'm happy to merge :)

src/Magnum/Math/Quaternion.h Outdated Show resolved Hide resolved
@Melix19 Melix19 force-pushed the Melix19:quaternion_to_euler branch from b5786dc to 4ea2719 Nov 22, 2019
@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Nov 22, 2019

Sorry for the delay.
I added another test with other values trying to cover the remaining line, but for some reason it gives me the wrong result. For reference:

    Quaternion a2 = Quaternion({0.70f, 0.0f, 0.70f}, 0.0f).normalized();
    Math::Vector3<Rad> b2{0.0_radf, -1.57079_radf, -3.14159_radf};

    CORRADE_COMPARE(a2.toEuler(), b2);
    Values a2.toEuler() and b2 are not the same, actual is
    Vector(Rad(0), Rad(-1.57045), Rad(0))
    but expected
    Vector(Rad(0), Rad(-1.57079), Rad(-3.14159))

So, I changed the algorithm again, using directly the one that this online converter uses that in my tests it seems the perfect one. The licence is MIT, so it should be good.

Hopefully the coverage is 100% now.

T m23 = rotMatrix[1][2];
T m33 = rotMatrix[2][2];

euler.y() = Rad<T>(std::asin(-Math::min(Math::max(m13, T(-1.0)), T(1.0))));

This comment has been minimized.

Copy link
@Melix19

Melix19 Nov 22, 2019

Author Contributor

Oh, and here I tried to use the Math::clamp() instead of the Math::min(Math::max()), but for some reason it says that it does not exist (?)

This comment has been minimized.

Copy link
@mosra

mosra Nov 23, 2019

Owner

It's defined in Functions.h that are not included here. This is fine :)

@mosra
mosra approved these changes Nov 23, 2019
Copy link
Owner

mosra left a comment

Yay! 🎉

T m23 = rotMatrix[1][2];
T m33 = rotMatrix[2][2];

euler.y() = Rad<T>(std::asin(-Math::min(Math::max(m13, T(-1.0)), T(1.0))));

This comment has been minimized.

Copy link
@mosra

mosra Nov 23, 2019

Owner

It's defined in Functions.h that are not included here. This is fine :)

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Nov 23, 2019

Merged as 0972898, thank you! Interesting that there's so many algos out there but only some of them work as expected :D

@mosra mosra closed this Nov 23, 2019
Math and algorithms automation moved this from TODO to Done Nov 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.