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

Let Matrix3.prototype.fromAngleVector accept a Vector3 #2

Open
rhulha opened this issue Nov 23, 2015 · 8 comments
Open

Let Matrix3.prototype.fromAngleVector accept a Vector3 #2

rhulha opened this issue Nov 23, 2015 · 8 comments

Comments

@rhulha
Copy link

rhulha commented Nov 23, 2015

Currently, Matrix3x3.prototype.fromAngles takes three separate parameters: x, y, z. It would be nice if there was an additional function, or added functionality to the current 'fromAngles', to accept a Vector3 Object as well:

var rot = new goo.Matrix3x3();
entity.transformComponent.transform.rotation.toAngles(rot); // notice a Vector3 passed in here...
rot.y += ctx.world.tpf;
// this would be nice!
entity.transformComponent.transform.rotation.fromAngleVector(rot); // we pass in the Vector3 here as well.

Currently, we have to pass in each part of the Vector3:
entity.transformComponent.transform.rotation.fromAngles(rot[0]. rot[1], rot[2]);

@schteppe
Copy link
Member

Hello, I've been thinking about this since it was first suggested (a long time ago), and I think a good approach would be to add an Euler angles class, like in Three.js. If we implemented it in Goo, it would look something like this:

var angles = new Euler({ order: 'YZX' });
angles.setFromRotationMatrix(entity.transformComponent.transform.rotation);
angles.y += world.tpf;
entity.transformComponent.transform.rotation.setFromEuler(angles);

This approach has some advantages over the vector approach:

  • It follows the "method operates on the host object" pattern
  • Euler instances can have an "order" property, which makes it possible to switch order depending on use case. In your case, you could make the rotation work for Z axis if you switch order.
  • The order property would emphasize that Euler angles actually have an order, which is good if you're new to euler angles and rotations.

What do you think?

@rhulha
Copy link
Author

rhulha commented Nov 24, 2015

An optional Euler class sounds great, but why not simply add parameter detection to the function like GooJS does in other places ?

@schteppe
Copy link
Member

We're trying to move away from polymorphism / parameter detection I think, to improve performance... Especially on these things that you might want to call every frame. Right @rherlitz ?

@rherlitz
Copy link
Contributor

We'll consider the options there, but get it in somehow :)

@rhulha
Copy link
Author

rhulha commented Nov 25, 2015

One needs to strike a balance between performance and ease of use.
In my book ease of use comes first, but your mileage may vary ;-)

@hccampos
Copy link
Contributor

I think it is not that hard to have both ease of use and performance. Just have a method with a different name or the euler class like @schteppe mentioned. And eventually if you add some kind of type system it might even be possible to inline stuff.

@rhulha
Copy link
Author

rhulha commented Nov 30, 2015

I believe in ease of use and the Principle of least astonishment

The current API violates that principle and adding support for Vector3 would fix it.
Only adding support for a new class Euler would not fix it, unless all other methods add support for it as well and remove support for Vector3.
That however would break backwards compatibility I fear. (or ?)

So the only way to be backwards compatible and consistent with the rest of the API would be to add support for Vector3.

@hccampos
Copy link
Contributor

Yea, we need to think through it pretty well. One thing is certain, it should all be consistent. But I very much hate overloads in such a performance critical lib and having a bunch of different ways to do things. When you start adding typeof to your code things get really messy really quick. I vote for going with the Euler solution (even if we need to deprecate stuff) or adding a method fromAnglesVectoror something similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants