Hello everyone,
opened 01:23PM - 09 Mar 18 UTC
closed 04:19PM - 09 Mar 18 UTC
This isn't really a bug, just a confusing API -
In the API for `Quaternion.t… oEulerAngles` the method signature is
`toEulerAngles(order?: string): Vector3;` but looking at the source code, the order parameter has a default of `public toEulerAngles(order = "YZX"): Vector3 {`, suggesting that the order can be passed as string to define the desired output order, however looking at the actual implementation, the order param is not used at all.
```ts
/**
* Returns a new Vector3 set with the Euler angles translated from the current Quaternion.
*/
public toEulerAngles(order = "YZX"): Vector3 {
var result = Vector3.Zero();
this.toEulerAnglesToRef(result, order);
return result;
}
/**
* Sets the passed vector3 "result" with the Euler angles translated from the current Quaternion.
* Returns the current Quaternion.
*/
public toEulerAnglesToRef(result: Vector3, order = "YZX"): Quaternion {
var qz = this.z;
var qx = this.x;
var qy = this.y;
var qw = this.w;
var sqw = qw * qw;
var sqz = qz * qz;
var sqx = qx * qx;
var sqy = qy * qy;
var zAxisY = qy * qz - qx * qw;
var limit = .4999999;
if (zAxisY < -limit) {
result.y = 2 * Math.atan2(qy, qw);
result.x = Math.PI / 2;
result.z = 0;
} else if (zAxisY > limit) {
result.y = 2 * Math.atan2(qy, qw);
result.x = -Math.PI / 2;
result.z = 0;
} else {
result.z = Math.atan2(2.0 * (qx * qy + qz * qw), (-sqz - sqx + sqy + sqw));
result.x = Math.asin(-2.0 * (qz * qy - qx * qw));
result.y = Math.atan2(2.0 * (qz * qx + qy * qw), (sqz - sqx - sqy + sqw));
}
return this;
}
```
I'd suggest either the order should be removed entirely, or actually implemented.
as I spent a good few minutes figuring out what is wrong with my code before noticing that the parameter doesn’t do anything.
Are there any plans to implement it? Should I try myself and make a PR?
Yup it has been kept as a TODO so if you are willing to contribute it ? let s do it
2 Likes
ok, I’ll look into it on a weekend, hopefully it won’t be too difficult
3 Likes
So, I tried, but didn’t manage to finish it. It requires quite a bit of testing to be sure every order is working correctly.
Until then it would probably be smart to change the order in function comments, like @sebavan suggested?
@Deltakosh any objections to even simply remove it ??? which I d consider the safest
otherwise, I might simply move the order we use in the function comments as I can totally understand the frustration.