Support multiple intrinsic orders in quat.fromEuler

[DCtheTall]

Fixes #403.

[DCtheTall]

@stefnotch please review this change when you get a chance 😄

[stefnotch]

gkjohnson has left some pretty good comments, please do take a look at them.

And, maybe the order should be XYZ intrinsic (even if I prefer XYZ extrinsic), because that's what three.js does by default?

[DCtheTall]

@gkjohnson I addressed your review comments, let me know if this is more what you had in mind.

[gkjohnson]

@DCtheTall Great thanks! A few thoughts:

• It's up to the maintainers of this repo but given that speed is such a big push for gl-matrix I suspect that having an internal function that uses a switch case to select which order to use might be undesirable (though I know I suggested in the other issue 😬) If it is okay then I think a single fromEuler function with an optional "order" argument that specifies some documented intrinsic or extrinsic order and defaults to the current behavior is cleaner. Either way I think it should just be one or the other.

• Given that an intrinsic rotation order is just the reverse of an extrinsic rotation (intrinsic XYZ rotation is the same as an extrinsic ZYX rotation) I don't think it's necessary to specify "Intrinsic" or "Extrinsic" in the function names. The library should just document what is being used per function and be consistent about it. If gl-matrix uses intrinsic XYZ order then a user who needs extrinsic XYZ order would just use the getEulerZYX method (assuming there are multiple methods).

Of course @stefnotch will have the final opinion on this but those are my 2 cents. Thanks for the improvements!

[DCtheTall]

@gkjohnson thank you for putting so much thought into this. I am also open to adding a new argument to fromEuler with a default value, since that is a simpler change, though all we'd be saving at that point is a function invocation since then the switch would be moved to the other function body.

I'll defer to @stefnotch and see what his thoughts are.

[stefnotch]

I suppose adding a new argument to 'fromEuler' would be reasonably clean.

(Sort of related #369 (comment) )

[DCtheTall]

@stefnotch

I suppose adding a new argument to 'fromEuler' would be reasonably clean.

Done.

Also decided to support all intrinsic orders while I was at it. I borrowed pretty shamelessly from Three.js's implementation but with the proper variable names for this library.

[DCtheTall]

@gkjohnson friendly ping

[gkjohnson]

@gkjohnson friendly ping

Thanks for the ping I didn't realize the ball was in my court. I just left a comment but after that's addressed I think the rest is up to the maintainers.

Thanks!

[DCtheTall]

@stefnotch

[stefnotch]

@DCtheTall Looks great, merged! Thank you and gkjohnson very much for all your work on this 🎉

[stefnotch]

After taking a look at it, I decided to slightly tweak a few bits and bobs, mainly using the documentation comment to validate the parameters and defining the default angle order in common.js.
3a321ea
0fc30ed

[magcius]

toLowerCase() causes an unavoidable string allocation in what is (possibly) a hot path. :/

I don't know if this is part of the API now, but I would prefer if people had to put their order argument in lower-case.

[magcius]

Falling back to zyx when specifying an order that isn't supported (e.g. xyx) feels like an API footgun, I would rather throw an error here.

[stefnotch]

@magcius Those are some quite valid concerns. Luckily, this isn't a part of the official API yet, which means that a PR that changes this behaviour would be very welcome.

[magcius]

I missed that these concerns were already addressed in 3a321ea, apologies.