Browse Source

Math: deprecate angle() for quaternions because it's wrong.

It should be returning twice the value, this is the half-angle. Sad that
this went unnoticed for so long, extra bad points for me to have a
complicated test but not actually verifying that the returned value
makes sense, sigh.

A *nasty* option would be to just fix it, but -- even though there's a
chance nonbody ever used it since nobody ever complained -- it would
introduce breakages to any code that fixed it and that's something to
definitely not do in a trusted codebase. So it's instead deprecated,
firing an annoying warning to whoever might have called it, and there's
a (temporary) replacement called halfAngle() that does exactly the same
but is named appropriately.

Then, once the deprecated angle() is removed (the usual deprecation
period, so a year or the span of two releases, whichever takes longer)
and enough time passes (so another year at least), I'll reintroduce it
with a correct return value.
pull/168/head
Vladimír Vondruš 3 years ago
parent
commit
1fa9862ad6
  1. 7
      doc/changelog.dox
  2. 2
      src/Magnum/Math/Complex.h
  3. 24
      src/Magnum/Math/Quaternion.h
  4. 32
      src/Magnum/Math/Test/QuaternionTest.cpp

7
doc/changelog.dox

@ -1068,6 +1068,13 @@ See also:
for consistency with @relativeref{Math::Color3,toSrgbInt()} and
@relativeref{Math::Color4,toSrgbAlphaInt()} and to prevent accidental type
mismatches
- @cpp Math::angle(const Quaternion<T>&, const Quaternion<T>&) @ce
historically returned a half-angle instead of the full angle, which is
incorrect. Because fixing it would break all current uses of it, it's
deprecated in favor of @ref Math::halfAngle(const Quaternion<T>&, const Quaternion<T>&),
which returns the same value but is named appropriately. This function will
get reintroduced with a correct output after enough time passes to avoid
breaking existing code.
- Markup styling for Emscripten application was switched to prefer using
CSS classes instead of the @cb{.css} #container @ce, @cb{.css} #sizer @ce,
@cb{.css} #expander @ce, @cb{.css} #listener @ce, @cb{.css} #canvas @ce,

2
src/Magnum/Math/Complex.h

@ -72,7 +72,7 @@ To avoid numerical issues when two complex numbers are very close to each
other, the dot product is clamped to the @f$ [-1, +1] @f$ range before being
passed to @f$ \arccos @f$.
@see @ref Complex::isNormalized(),
@ref angle(const Quaternion<T>&, const Quaternion<T>&),
@ref halfAngle(const Quaternion<T>&, const Quaternion<T>&),
@ref angle(const Vector<size, FloatingPoint>&, const Vector<size, FloatingPoint>&)
*/
template<class T> inline Rad<T> angle(const Complex<T>& normalizedA, const Complex<T>& normalizedB) {

24
src/Magnum/Math/Quaternion.h

@ -27,7 +27,7 @@
*/
/** @file
* @brief Class @ref Magnum::Math::Quaternion, function @ref Magnum::Math::dot(), @ref Magnum::Math::angle(), @ref Magnum::Math::lerp(), @ref Magnum::Math::slerp()
* @brief Class @ref Magnum::Math::Quaternion, function @ref Magnum::Math::dot(), @ref Magnum::Math::halfAngle(), @ref Magnum::Math::lerp(), @ref Magnum::Math::slerp()
*/
#ifndef CORRADE_NO_DEBUG
@ -59,7 +59,7 @@ template<class T> inline T dot(const Quaternion<T>& a, const Quaternion<T>& b) {
}
/** @relatesalso Quaternion
@brief Angle between normalized quaternions
@brief Half-angle between normalized quaternions
Expects that both quaternions are normalized. @f[
\theta = \arccos \left( \frac{p \cdot q}{|p| |q|} \right) = \arccos(p \cdot q)
@ -72,12 +72,28 @@ passed to @f$ \arccos @f$.
@ref angle(const Complex<T>&, const Complex<T>&),
@ref angle(const Vector<size, FloatingPoint>&, const Vector<size, FloatingPoint>&)
*/
template<class T> inline Rad<T> angle(const Quaternion<T>& normalizedA, const Quaternion<T>& normalizedB) {
template<class T> inline Rad<T> halfAngle(const Quaternion<T>& normalizedA, const Quaternion<T>& normalizedB) {
CORRADE_DEBUG_ASSERT(normalizedA.isNormalized() && normalizedB.isNormalized(),
"Math::angle(): quaternions" << normalizedA << "and" << normalizedB << "are not normalized", {});
"Math::halfAngle(): quaternions" << normalizedA << "and" << normalizedB << "are not normalized", {});
return Rad<T>{std::acos(clamp(dot(normalizedA, normalizedB), T(-1), T(1)))};
}
#ifdef MAGNUM_BUILD_DEPRECATED
/**
@brief @copybrief halfAngle(const Quaternion<T>&, const Quaternion<T>&)
@m_deprecated_since_latest This function historically returned a half-angle
instead of the full angle, which is incorrect. Because fixing it would
break all current uses of it, it's deprecated in favor of
@ref halfAngle(const Quaternion<T>&, const Quaternion<T>&), which returns
the same value but is named appropriately. This function will get
reintroduced with a correct output after enough time passes to avoid
breaking existing code.
*/
template<class T> CORRADE_DEPRECATED("use halfAngle() instead") inline Rad<T> angle(const Quaternion<T>& normalizedA, const Quaternion<T>& normalizedB) {
return halfAngle(normalizedA, normalizedB);
}
#endif
/** @relatesalso Quaternion
@brief Linear interpolation of two quaternions
@param normalizedA First quaternion

32
src/Magnum/Math/Test/QuaternionTest.cpp

@ -544,20 +544,26 @@ void QuaternionTest::angle() {
auto b = Quaternion({4.0f, -3.0f, 2.0f}, -1.0f).normalized();
/* Verify also that the angle is the same as angle between 4D vectors */
CORRADE_COMPARE(Math::angle(a, b), Math::angle(
CORRADE_COMPARE(Math::halfAngle(a, b), Math::angle(
Vector4{1.0f, 2.0f, -3.0f, -4.0f}.normalized(),
Vector4{4.0f, -3.0f, 2.0f, -1.0f}.normalized()));
CORRADE_COMPARE(Math::angle(a, b), 1.704528_radf);
CORRADE_COMPARE(Math::angle(-a, -b), 1.704528_radf);
CORRADE_COMPARE(Math::angle(-a, b), Rad(180.0_degf) - 1.704528_radf);
CORRADE_COMPARE(Math::angle(a, -b), Rad(180.0_degf) - 1.704528_radf);
CORRADE_COMPARE(Math::halfAngle(a, b), 1.704528_radf);
CORRADE_COMPARE(Math::halfAngle(-a, -b), 1.704528_radf);
CORRADE_COMPARE(Math::halfAngle(-a, b), Rad(180.0_degf) - 1.704528_radf);
CORRADE_COMPARE(Math::halfAngle(a, -b), Rad(180.0_degf) - 1.704528_radf);
/* Same / opposite. Well, almost. It's interesting how imprecise
normalization can get. */
CORRADE_COMPARE_WITH(Math::angle(a, a), 0.0_radf,
CORRADE_COMPARE_WITH(Math::halfAngle(a, a), 0.0_radf,
Corrade::TestSuite::Compare::around(0.0005_radf));
CORRADE_COMPARE_WITH(Math::angle(a, -a), 180.0_degf,
CORRADE_COMPARE_WITH(Math::halfAngle(a, -a), 180.0_degf,
Corrade::TestSuite::Compare::around(0.0005_radf));
/* Trivial case, to verify it's actually returning the right value (hah) */
CORRADE_COMPARE(Math::halfAngle(
Quaternion::rotation(Deg(20.0f), Vector3::xAxis()),
Quaternion::rotation(Deg(70.0f), Vector3::xAxis())),
25.0_degf);
}
void QuaternionTest::angleNormalizedButOver1() {
@ -567,8 +573,8 @@ void QuaternionTest::angleNormalizedButOver1() {
Quaternion a{{1.0f + Math::TypeTraits<Float>::epsilon()/2, 0.0f, 0.0f}, 0.0f};
CORRADE_VERIFY(a.isNormalized());
CORRADE_COMPARE(Math::angle(a, a), 0.0_radf);
CORRADE_COMPARE(Math::angle(a, -a), 180.0_degf);
CORRADE_COMPARE(Math::halfAngle(a, a), 0.0_radf);
CORRADE_COMPARE(Math::halfAngle(a, -a), 180.0_degf);
}
void QuaternionTest::angleNotNormalized() {
@ -577,12 +583,12 @@ void QuaternionTest::angleNotNormalized() {
std::ostringstream out;
Error redirectError{&out};
Math::angle(Quaternion{{1.0f, 2.0f, -3.0f}, -4.0f}.normalized(), {{4.0f, -3.0f, 2.0f}, -1.0f});
Math::angle({{1.0f, 2.0f, -3.0f}, -4.0f}, Quaternion{{4.0f, -3.0f, 2.0f}, -1.0f}.normalized());
Math::halfAngle(Quaternion{{1.0f, 2.0f, -3.0f}, -4.0f}.normalized(), {{4.0f, -3.0f, 2.0f}, -1.0f});
Math::halfAngle({{1.0f, 2.0f, -3.0f}, -4.0f}, Quaternion{{4.0f, -3.0f, 2.0f}, -1.0f}.normalized());
CORRADE_COMPARE(out.str(),
"Math::angle(): quaternions Quaternion({0.182574, 0.365148, -0.547723}, -0.730297) and Quaternion({4, -3, 2}, -1) are not normalized\n"
"Math::angle(): quaternions Quaternion({1, 2, -3}, -4) and Quaternion({0.730297, -0.547723, 0.365148}, -0.182574) are not normalized\n");
"Math::halfAngle(): quaternions Quaternion({0.182574, 0.365148, -0.547723}, -0.730297) and Quaternion({4, -3, 2}, -1) are not normalized\n"
"Math::halfAngle(): quaternions Quaternion({1, 2, -3}, -4) and Quaternion({0.730297, -0.547723, 0.365148}, -0.182574) are not normalized\n");
}
void QuaternionTest::matrix() {

Loading…
Cancel
Save