I don't like the idea of changing the API of long-existing functions. 
I think a "clean" solution would be to provide a separate EulerAngles class which encapsulates conversion from and to Euler angles (to and from other RotationBase<>s).
O.t.o.h., I don't care too much for Euler angles anyways, so any solution that finds a majority is fine for me ...

See also bug 609 and bug 801.

Changing the behavior of the current method is not an option, but if another convension is so important, then I don't care adding another method. Respective unit tests will be required (see test/geo_eulerangles.cpp).

As talked in the forum[1], I had a proposal for alternative Euler angles implementation.

This also "fix" this bug, want to know if you think it matters for this bug.

[1] https://forum.kde.org/viewtopic.php?f=74&t=128502

In general, I like the implementation. Some suggestions/questions:
* I'd suggest naming the angles alpha-beta-gamma, instead of heading-pitch-roll. I think that makes it more evident which angle comes first. (And I think yaw-pitch-roll would be a more common naming, otherwise)
* Should we directly include this into the Geometry module? This would however require to more or less fix the API.
* Do we want this in 3.3? This would be a good opportunity to have it in a stable version soon.

Glancing over the implementation, I found several minor issues. If I find the time, I can look through it more thoroughly and give you comments.

(In reply to Christoph Hertzberg from comment #4)
> In general, I like the implementation. Some suggestions/questions:
> * I'd suggest naming the angles alpha-beta-gamma, instead of
> heading-pitch-roll. I think that makes it more evident which angle comes
> first. (And I think yaw-pitch-roll would be a more common naming, otherwise)
> * Should we directly include this into the Geometry module? This would
> however require to more or less fix the API.
> * Do we want this in 3.3? This would be a good opportunity to have it in a
> stable version soon.
> 
> Glancing over the implementation, I found several minor issues. If I find
> the time, I can look through it more thoroughly and give you comments.

Any update?
Do you want to give it a try as an unsupported module first?

- Tal

Sorry for late reply. In order not to interfere with 3.3, I'd put it first in unsupported to give it the time to mature.

As Christoph said, heading-pith-roll names should really be avoided (does not work well with all conventions like XYX). alpha, beta, gamma sounds good to me. 

Details:

- "Scalar angle(int) const"  should be  "const Vector& angles() const"
- then method "coeffs" could be removed.
- avoid one letter names like h, p, r -> use alpha(), beta(), gamma() ;)
- about the "fromRotation" methods:
 - replace the ones which are not static by operator= (as in Quaternion)
 - the others are actually static methods: use a capital F: FromRotation
- method "invert" should be removed.
- all public methods should be documented.
- use a macro to define both float and double EulerAnglesXYZ* typedefs
- AddConstIf -> not used
- no needs for NegateIfXor, compilers are extremely good at optimizing away branches based on compile time constants:
  if(IsHeadingOpposite-=IsEven) res.alpha() = -res.alpha();

After such changes, let's merge it in unsupported!

(In reply to Gael Guennebaud from comment #6)
> Sorry for late reply. In order not to interfere with 3.3, I'd put it first
> in unsupported to give it the time to mature.
> 
> As Christoph said, heading-pith-roll names should really be avoided (does
> not work well with all conventions like XYX). alpha, beta, gamma sounds good
> to me. 
> 
> Details:
> 
> - "Scalar angle(int) const"  should be  "const Vector& angles() const"
> - then method "coeffs" could be removed.
> - avoid one letter names like h, p, r -> use alpha(), beta(), gamma() ;)
> - about the "fromRotation" methods:
>  - replace the ones which are not static by operator= (as in Quaternion)
>  - the others are actually static methods: use a capital F: FromRotation
> - method "invert" should be removed.
> - all public methods should be documented.
> - use a macro to define both float and double EulerAnglesXYZ* typedefs
> - AddConstIf -> not used
> - no needs for NegateIfXor, compilers are extremely good at optimizing away
> branches based on compile time constants:
>   if(IsHeadingOpposite-=IsEven) res.alpha() = -res.alpha();
> 
> After such changes, let's merge it in unsupported!

Done it.

I only need to document it slightly better. What do you think?

This is getting better!

In addition to the doc (including some examples), I've spotted a few minor tweaks:

- The file unsupported/Eigen/src/EulerAngles/CMakeLists.txt needs to be updated

- EulerAngles(Scalar alpha...)  -> use const references for the arguments (important when using custom scalar types)

- the method toQuaternion is probably not needed as there is a conversion operator (let's keep the API as minimal as possible)

Finished it! (unless you find more issues)

Any update?

Sorry for taking that long. I've merge your fork:

https://bitbucket.org/eigen/eigen/commits/7c1025ae0fce

Great job.

The next step will to be integrate it in the Geometry module, once we'll get some feedbacks from other users.

(In reply to Gael Guennebaud from comment #11)
> Sorry for taking that long. I've merge your fork:
> 
> https://bitbucket.org/eigen/eigen/commits/7c1025ae0fce
> 
> Great job.
> 
> The next step will to be integrate it in the Geometry module, once we'll get
> some feedbacks from other users.

Thanks!

-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/947.