mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.43k stars 35.36k forks source link

Euler3 class equivalent to Blender's #3404

Closed bhouston closed 9 years ago

bhouston commented 11 years ago

Many math libraries have a EulerRotation class that stores x,y,z rotations along with an order. There are a number of useful functions for manipulating and converting between Euler angles that can also go on this class. Right now Three.js does not have a Euler class rather we just pass around a Vector3 along with an order.

Besides the obvious, some useful Euler3 functions are:

This is what I am aiming for:

http://www.blender.org/documentation/blender_python_api_2_59_2/mathutils.html#mathutils.Euler

I've written up a draft here that I can complete if there is interest:

https://github.com/bhouston/three.js/blob/euler3/src/math/Euler3.js

mrdoob commented 11 years ago

This is interesting. Should make things a bit more clear. What do you think @WestLangley?

WestLangley commented 11 years ago
  1. From a design standpoint, I like it -- carrying the order along with the vector -- but this is going to break every example that exists to date. (ugh!) So, I'll leave it up to you... I'm not sure I see what would be "more clear" by doing this, but I do like it.
  2. The resulting new property names: object.rotation becomes object.euler, and object.eulerOrder is removed.
  3. Don't forget about the global THREE.Object3D.defaultEulerOrder = 'XYZ'.
  4. Why is it THREE.Euler3? I think THREE.Euler is fine.
  5. The reorder( newOrder ) function is easy -- just convert to quaternion and back -- and keep them intrinsic. Do not venture into supporting extrinsic rotations unless there is a compelling reason to do so. Maintain one way of doing things, please.
  6. inverse() - easy
  7. clamp() - I do not see a compelling reason for this one, but it is not much code. What is the use case?
  8. alternativeSolution() - I really do not see a compelling reason for this one. What is the use case?
  9. Also, if you are going to cut and paste code from elsewhere in the library, please do not forget to give attribution to those of us who came before and invested many hours of work developing these routines in the first place. :-)
mrdoob commented 11 years ago

I'm not sure I see what would be "more clear" by doing this, but I do like it.

Well, the rotation specific methods that we have in Vector3 would then move to Euler and keep things tidier. I can't think of side effects or use cases where rotation specific methods would be handy on a Vector3. There is also some euler related pollution in Object3D.

The resulting new property names: object.rotation becomes object.euler, and object.eulerOrder is removed.

I think it can/should still be object.rotation. It's just that it won't be of Vector3 type anymore, it would be Euler instead. But, yes, object.eulerOrder will get deprecated and also THREE.Object3D.defaultEulerOrder = 'XYZ' could then be moved to THREE.Euler.default = 'XYZ' or so...

Also, if you are going to cut and paste code from elsewhere in the library, please do not forget to give attribution to those of us who came before and invested many hours of work developing these routines in the first place. :-)

This is something I think about from time to time...

There is a lack of support for credit retaining for things like this. And I'm starting to see the "credits" at the top of files more as people responsible of the file rather than credits per se (I wonder if people would add themselves in the header if they saw it like this).

The only "solution" I can see for situations like this would be to write down where the code was copy pasted from in the git commit.

WestLangley commented 11 years ago

I think it can/should still be object.rotation.

I do not agree. To me, one of two things should happen:

a. object.rotation becomes object.euler. Euler and Quaternion are two representations for a rotation. Having the Euler representation called rotation is just not right. This is a good opportunity to fix it.

b. create a new THREE.Rotation class and leave it as object.rotation. Then in the new class, have properties euler, quaternion, and use.Quaternion. Axis-angle is also a representation for a rotation, and that could be removed from Vector4 and put into it's own class. I would not include it in the new Rotation class.

I prefer (b), I think.

bhouston commented 11 years ago

I prefer @WestLangley's option (a). I find the name "euler" more descriptive than "rotation" as a variable name. "Rotation" is generic and to me could refer to either a Matrix3, Quaternion or an Euler.

With regards to option (b), I don't like a multi-function Rotation class because you never know really what it is unless query its status every time you are about to use it (e.g. it may or may not have extrinsic rotations in it.) Also Rotation classes generally (where I have seen them) are either Quaternions underneath or Eulers underneath they are not both.

But there is option (c) which is a mix between option (a) and option (b). Rename Euler to Rotation and have our rotation class be Euler angles all the time. Then we can use the name "rotation" as a variable name as it maps to the class name. I have seen this pattern before, for example the 3DS MAX's SDK has a RotationValue class that is just an Euler: http://docs.autodesk.com/3DSMAX/15/ENU/3ds-Max-SDK-Programmer-Guide/index.html?url=cpp_ref/class_rotation_value.html,topicNumber=cpp_ref_class_rotation_value_html

mrdoob commented 11 years ago

What about...

d. Allowing object.rotation be both. By default it would be object.rotation = new THREE.Euler() but you could change it to object.rotation = new THREE.Quaternion(). So instead of checking for object.useQuaternion we would be checking for the type of object.rotation?

arodic commented 11 years ago

I agree with doob's proposal. object.rotation makes more sense since position and scale are not named by their type. Also it won't break examples.

WestLangley commented 11 years ago

have our rotation class be Euler angles all the time.

No, we do not want to do that. It is better to have the rotation logic behind-the-scenes as quaternion-based. It is much faster.

The only reason for keeping Euler is @mrdoob wants the user to be able to specify something like euler.x += 0.01.

Actually, Euler angles confuse users, too. :-) It would be good to also be able to specify axisAngle.angle += 0.01.

So the multi-function rotation class, if we were to use it, would be quaternion-based, and just allow for varying methods of specifying the rotation.

If we do not implement a Rotation class, the I prefer using the term euler for Euler rotation. Yes, it breaks a lot of examples, and I agree that is a good reason for keeping the rotation term.

bhouston commented 11 years ago

@WestLangley wrote: "It is better to have the rotation logic behind-the-scenes as quaternion-based. It is much faster. ... So the multi-function rotation class, if we were to use it, would be quaternion-based, and just allow for varying methods of specifying the rotation."

For our purposes this would defeat most of the reasons for an Euler class. The main benefits in an animation system of using Euler angles is that you can have multiple revolutions specified (e.g., euler.x = 4 * PI) This isn't allowed with Quaternions which merely store an orientation. Thus with Euler angles you can make a car wheel spin rapidly just from interpolation between two Euler angles where as with pure Quaternions that isn't exactly possible without a bit of extra conversion. It would be a shame to lose this main advantage of Euler angles.

bhouston commented 11 years ago

I also support having object.rotation be a polymorphic variable that can either be Euler or Quaternion. This gets the majority of the benefits of @WestLangley's proposed multi-function rotation class without actually creating a new multi-function class.

WestLangley commented 11 years ago

@bhouston I think you misunderstood me. I did not suggest eliminating the Euler class -- but adding a Rotation class.

bhouston commented 11 years ago

I've updated my PR #3406 such that Object3D.rotation can be either an Euler or Quaternion. I have also removed Object3D.quaternion/useQuaternion.

I noticed two places were it was assumed that Object3D.quaternion was used Object3D.useQuaternion was not checked first -- not sure if these are potential bugs or not:

https://github.com/mrdoob/three.js/pull/3406/files#L8R263

https://github.com/mrdoob/three.js/pull/3406/files#L3R877

But my changes didn't introduce these two potential bugs.

WestLangley commented 11 years ago

I'm not digging the polymorphic thing. To me, this is total confusion.

object.rotation.setFromQuaternion( quaternion );

rotation must be an Euler angle

object.rotation.setFromRotationMatrix( matrix );

I'm not sure what rotation is

object.rotation.setFromRotationMatrix( matrix, rotation.order );

It's an Euler angle

object.rotation.multiply( quaternion );

It must be a quaternion

object.rotation.setFromRotationMatrix( lookAtMatrix );

Could be either

v1.applyQuaternion( object.quaternion ) is now v1.applyQuaternion( object.rotation )

Huh?

object.rotation.set( 0, 0, 0, 1 );

Is this correct or not? It depends on what rotation is.

bhouston commented 11 years ago

Now this dates me a bit but we can look to SGI's OpenInventor as a potential guide. (ThreeJS is basically an OpenInventor-style framework.)

I notice here in their SoTransform node that they use SbRotation which is a quaternion. They do not provide alternatives in their base node implementation.

https://github.com/aumuell/open-inventor/blob/master/lib/database/include/Inventor/nodes/SoTransform.h

What they do provide is the ability to create animated nodes such as an SoRotor which basically spins its child nodes at a regular rate, thus it is an incremental animated rotation:

https://github.com/aumuell/open-inventor/blob/master/lib/database/include/Inventor/nodes/SoRotor.h

I do think that @WestLangley is right that this is overly complex. I'd advocate dropping Euler as a means of specifying rotation on Object3D and instead just always stick to Quaternions as the single representation, we can make it easier to update the Quaternion using an Euler as an incremental though, that may be useful...

mrdoob commented 11 years ago

Agreed. I wasn't too convinced about the idea of object.rotation being able to be Euler or Quaternion myself either. I could already imagine all the people on stackoverflow complaining that their code isn't working.

I also agree that removing euler and going quaternion only seems like the best bet. I'm just curious of how easy we can make them. How would the code be for just having an object rotating on it's Y axis?

WestLangley commented 11 years ago

In other words, remove object.eulerOrder, and object.useQuaternion.

Then either

(a) remove object.quaternion and let object.rotation = new THREE.Quaternion(), or

(b) remove object.rotation and keep object.quaternion.

I'd definitely do (b). Having rotation suddenly become a Quaternion would be a nightmare.

Access Routines. The renderers access the object.matrix directly. The properties object.position, object.quaternion, and object.scale are just used to specify object.matrix.

What we need, now, are easy ways to specify or increment a Quaternion. We already have several.

Quaternion.setFromEuler( euler )
Quaternion.setFromAxisAngle( axis, angle )
Quaternion.setFromRotationMatrix( matrix )

We also have object.rotateOnAxis( axis, angle ), which does a local (incremental) rotation. I recently proposed in #3211 a global-axis version of the same, but it was rejected.

One final point. I'd say clear of routines that "rotate a quaternion". I'd think in terms of "rotating an object".

mrdoob commented 11 years ago

If we were to move to .quaternion I would propose to have something like this to minimize breakage:

THREE.Rotation = function ( quaternion ) {

    this.euler = { x: 0, y: 0, z: 0 };
    this.quaternion = quaternion;

};

THREE.Rotation.prototype = {
    get x () {
        return this.euler.x;
    },
    set x ( value ) {
        this.euler.x = value;
        this.quaternion.setFromEuler( this.euler );
    },
    get y () {
        return this.euler.y;
    },
    set y ( value ) {
        this.euler.y = value;
        this.quaternion.setFromEuler( this.euler );
    },
    get z () {
        return this.euler.z;
    },
    set z ( value ) {
        this.euler.z = value;
        this.quaternion.setFromEuler( this.euler );
    }
};

Then in Object3D...

this.quaternion = new THREE.Quaternion();
this.rotation = new THREE.Rotation( this.quaternion );
bhouston commented 11 years ago

@mrdoob @WestLangley I've updated my pull request so that it includes THREE.Rotation as described above and also I've made the suggested changes to Object3D. I've also updated quite a few examples.

WestLangley commented 11 years ago

@bhouston Can you please respond to my questions posted above?

clamp() - I do not see a compelling reason for this one. What is the use case?

alternativeSolution() - I really do not see a compelling reason for this one. What is the use case?

bhouston commented 11 years ago

@WestLangley Sorry, I missed the questions.

Euler.clamp() is a fairly common part of Euler classes. It is part of Blender's, Maya's, and 3DS Max's Euler classes. It converts from potentially multiple revolution Euler representation to one that is just representative of an orientation. Thus you can view it as a means of discarding revolutions information if you don't need it.

Euler.alternativeSolution(). I don't actually yet have a use case for this, rather it just seemed interesting. I've removed it.

WestLangley commented 11 years ago

@bhouston I have also seen elsewhere routines for adding and subtracting Euler angles, and for "multiplying" them. We do not provide those routines... but we could...

They also let the user clamp Euler angle to a user-defined range. We do not support that either. But we could... Instead, the proposed routine hardwires the clamp range.

The point is, there are a lot of routines we could add.

Clamping the Euler angles can easily be done by the user, if he needs it.

The side benefit is there is no need to add fpModulus to the math library.

I agree this is a subjective decision, and if @mrdoob wants it, it will be added.

For the record, my vote is "no" on this one.

But the remainder of your contributions here looks real fine, IMHO. :-)

bhouston commented 11 years ago

@WestLangley, thanks for the review, it is appreciated! I have just done another push that removes Euler.clamp() and Math.fpModulus(). :-)

WestLangley commented 11 years ago

@bhouston Hehe. You are so agreeable. I am not used to it. :-)

dubejf commented 9 years ago

It seems like everything has been implemented!