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

Added Euler angles rotation orders. #13538

Merged
merged 2 commits into from
Mar 14, 2018
Merged

Added Euler angles rotation orders. #13538

merged 2 commits into from
Mar 14, 2018

Conversation

thezwap
Copy link

@thezwap thezwap commented Mar 8, 2018

Only Tait-Bryan angles were supported previously.

@thezwap
Copy link
Author

thezwap commented Mar 8, 2018

Rotations such as ZXZ are very commonly used for calculating part rotations in multi-axis CNC machine controls. I used the maths from the same matlab script that the Tait-Bryan angles were taken from, in the original code (link in comment in Quaternion.js.)

@mrdoob
Copy link
Owner

mrdoob commented Mar 9, 2018

@WestLangley what do you think?

@mrdoob mrdoob added this to the rXX milestone Mar 9, 2018
@WestLangley
Copy link
Collaborator

@thezwap

  1. Did you test this code?

  2. Are the "proper" Euler angles you added of the intrinsic type?

  3. There has be no demand for these Euler types in three.js that I have been aware of. Would an examples/js/math/EulerUtils.js be acceptable, instead?

  4. What would be the minimal set of methods that you would need to add to three.js to satisfy your use case?

Were this to be integrated into core, for completeness, we would also have to add additional conversion methods for Euler to-and-from Quaternion and Euler to-and-from Matrix4.

@thezwap
Copy link
Author

thezwap commented Mar 11, 2018 via email

@WestLangley
Copy link
Collaborator

I haven’t tested all the other orders that I added

Please always test your PR before your file it.

as the documentation didn’t specify that only Tait-Bryan angles were supported

The supported rotation orders are specified in http://imac.local/three.js/docs/#api/math/Euler. Unfortunately, it does not appear that Object3D.rotation links to that page.

Personally, for my current use case, I’d need all of the methods I’ve added. Each machine control we work with uses a particular order in that set.

So, what are you doing as a workaround?

@thezwap
Copy link
Author

thezwap commented Mar 11, 2018

Please always test your PR before your file it.
Fair enough. I'll go through all of them and check they are correct. I've actually just noticed a flaw in my previous method of testing them, so I'll double-check.

The supported rotation orders are specified in http://imac.local/three.js/docs/#api/math/Euler.
Ok, I see that now. I think I did actually read that at one point, but misread that as being examples, rather than an exclusive list.

So, what are you doing as a workaround?
My workaround is the code I've written in a custom build of three.js

@thezwap
Copy link
Author

thezwap commented Mar 11, 2018

When I replied previously and said the maths that I used provided intrinsic rotations, I based that on the fact that reversing the order of rotations, converts intrinsic rotations to extrinsic, e.g. XYZ = ZY'X'' and vise versa. I made the assumption that the same applied to proper Euler angles, but now I'm doubting myself. I'm currently looking for information on this topic, and struggling to find a definitive answer. I'll comment again when I've got to the bottom of it.

@WestLangley
Copy link
Collaborator

So to clarify, all you need is for Quaternion.setFromEuler( euler ) to support "proper" Euler angles. Correct?

And you do not need Euler.setFromQuaternion( quaternion ) to support "proper" Euler angles. Correct?

And you do not need any of the other methods to support "proper" Euler angles?

@thezwap
Copy link
Author

thezwap commented Mar 11, 2018

Correct (on all three points.) Here is my usage:
var rotation = new THREE.Euler(rot1, rot2, rot3, 'ZXZ'); var vectorXdash = new THREE.Vector3(1, 0, 0); vectorXdash.applyEuler(rotation);

The applyEuler function in the Vector3, calls Quaternion.setFromEuler()

@WestLangley
Copy link
Collaborator

WestLangley commented Mar 11, 2018

Also, I expect the arguments to the constructor have a different meaning in your case.

In three.js,

var euler = new THREE.Euler( a, b, c, order );

means rotate around the x-axis by a -- regardless of the order -- whereas in your convention,

var euler = new THREE.Euler( a, b, c, order );

means rotate around the 1st-axis by a. This is important because the 1st axis is the same as the 3rd axis in your case.

Correct? If so, we would need to use a different class to represent "proper" Euler angles.

@WestLangley
Copy link
Collaborator

WestLangley commented Mar 12, 2018

So, as I said previously, if were are to support the 6 additional Euler orders, we would have to support all possible orders in all relevant methods, and I think we would need a ProperEuler class to do so.

This is why something like examples/js/utils/MathUtils.js could make sense, where we could define

THREE.MathUtils.setQuaternionFromProperEuler( q, a, b, c, order );

This method would implement your proposed code.

Or, we could create a static method:

Quaternion.setQuaternionFromProperEuler( q, a, b, c, order );

Edit: I think MathUtils is the best option for now.

@thezwap
Copy link
Author

thezwap commented Mar 12, 2018

Oh, I see now. That sounds like it makes sense. That would work fine for me, and hopefully others.

@WestLangley
Copy link
Collaborator

/ping @XanderLuciano
/ping @tentone

Do you have any opinions on this proposed feature?

@XanderLuciano
Copy link
Contributor

Overall, I'm in favor of adding support.

I like the recommendation of using THREE.MathUtils for now as I don't expect it to be commonly needed. Having a Quaternion.setFromProperEuler and Quaternion.setFromEuler would likely cause confusion, require an extra class, and support for additional utilities for no real reason. The resultant Quaternion is all that's needed.

I’ve started to see a few machine control apps built with threejs. See ncviewer.com as an example. That doesn’t do multi axis plane rotations yet as far as I know, but I’d be surprised if they weren’t on the roadmap.

NCViewer, to a certain degree, can do some multi axis rotations:

image

But I'm getting around the limitation by using multiple Quaternion.setFromAngleAxis to manually set the rotation order so to speak. This means instead of just creating one Quaternion I'm creating up to 3 (and to be honest, that's mostly because it was easier than figuring out the math to do it properly).

This is also something that Autodesk worked on relatively recently with our post processor (forum post about it), so to me it makes sense to add support for in Three.js as well. This way users won't have to figure out the math on their own if they want to work with Proper Euler angles.

Overall, it doesn't look like this would add much code and it would be useful to anyone working with simulating and driving multi axis CNC machines, multi axis 3D printers, and possibly even robotic arms (though I only have limited experience there).

@WestLangley
Copy link
Collaborator

Thanks, @XanderLuciano.

Let's do this: create examples/js/utils/MathUtils.js, and define

THREE.MathUtils.setQuaternionFromProperEuler( q, a, b, c, order );

There is no need to include a testbed, but it would be nice to see a link to one with a DAT.GUI supporting a, b, c, and order. A single AxesHelper for frame-of-reference, and one BoxGeometry controlled by the GUI would be sufficient to visually see that the code is working properly.

@thezwap Do you want to implement that? If not, I will be glad to.

@thezwap
Copy link
Author

thezwap commented Mar 12, 2018

I'm keen to have a go and see how far I get. If I get completely stuck, I'll let you know and hand it over or ask for advice. Haven't used dat.gui before, but it looks fairly straightforward.

@WestLangley
Copy link
Collaborator

@thezwap I needed this for something else, so here is a stub to get you started: https://jsfiddle.net/7aLfd0zd/

screen shot 2018-03-12 at 11 55 03 pm

@thezwap
Copy link
Author

thezwap commented Mar 13, 2018 via email

@WestLangley
Copy link
Collaborator

@thezwap As I expected, for performance reasons, the code should be written like this, instead:

q.set( c2 * s13, s2 * c1_3, s2 * s1_3, c2 * c13 );

rather than setting each component .x, .y, .z, .w separately. Otherwise, if q is the quaternion of an Object3D -- which it is in this case -- Object3D.rotation is updated four times.

/ping @mrdoob Ugh...

@thezwap
Copy link
Author

thezwap commented Mar 13, 2018

Ok. Here's the link to the updated fiddle: https://jsfiddle.net/7aLfd0zd/17/
And I've tested and commited MathUtils.js

I've made the change to use q.set, as you suggested. What do you think about it setting all of the quat. parameters to 0 if an unspecified order is given. Is that a bad idea?


var s1 = sin(a / 2);
var s2 = sin(b / 2);
var s3 = sin(c / 2);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four of these are not used.

@WestLangley
Copy link
Collaborator

Is that a bad idea?

Yep. A quaternion that represents a rotation has unit length. The zero rotation is set by: q.set( 0, 0, 0, 1 );

You could leave the quaternion unchanged if the order is invalid. I do not think you need the intermediate variables qx, qy, etc.

@thezwap
Copy link
Author

thezwap commented Mar 14, 2018

Updated fiddle: https://jsfiddle.net/thezwap/7aLfd0zd/26/

@WestLangley
Copy link
Collaborator

@thezwap Thanks!

@mrdoob This can be merged. I'll handle the formatting issues.

@thezwap
Copy link
Author

thezwap commented Mar 14, 2018

Thanks. That's my first pull request. Good to see how the process works.

@mrdoob mrdoob removed this from the rXX milestone Mar 14, 2018
@mrdoob mrdoob added this to the r91 milestone Mar 14, 2018
@mrdoob mrdoob merged commit 490f1c1 into mrdoob:dev Mar 14, 2018
@mrdoob
Copy link
Owner

mrdoob commented Mar 14, 2018

Thanks!

WestLangley added a commit that referenced this pull request Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants