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

Add Vec3 Support for setFromEulerAngles() pc.Quat function #3663

Closed
eproasim opened this issue Nov 4, 2021 · 5 comments · Fixed by #4139
Closed

Add Vec3 Support for setFromEulerAngles() pc.Quat function #3663

eproasim opened this issue Nov 4, 2021 · 5 comments · Fixed by #4139
Labels

Comments

@eproasim
Copy link

eproasim commented Nov 4, 2021

Currently when setting a Quat from Euler Angles it only accepts three separate number arguments. It would be very convenient to be able to just pass a Vec3 directly.

https://developer.playcanvas.com/en/api/pc.Quat.html#setFromEulerAngles

@yaustar yaustar added the good first issue Good for newcomers label Mar 11, 2022
@zishiwu123
Copy link
Contributor

zishiwu123 commented Mar 16, 2022

Hi, I'm interested in tackling this. I have 2 different ideas for how to implement it and wanted to get your opinion on which way is preferred?

One, we can overload the x argument to the constructor of Quat by checking if x instanceof Vec3. This is the same pattern that currently enables Quat to take in an array of length 4. The good thing about this approach is that it is consistent with the existing code. The bad thing is that it can grow to be messy if you add more types for x. Similar to these lines:

engine/src/math/quat.js

Lines 19 to 44 in 9a0697c

constructor(x = 0, y = 0, z = 0, w = 1) {
if (x.length === 4) {
/**
* The x component of the quaternion.
*
* @type {number}
*/
this.x = x[0];
/**
* The y component of the quaternion.
*
* @type {number}
*/
this.y = x[1];
/**
* The z component of the quaternion.
*
* @type {number}
*/
this.z = x[2];
/**
* The w component of the quaternion.
*
* @type {number}
*/
this.w = x[3];

Two, the alternative is to create a separate static method with the class Quat for constructing Quat instances out of a Vec argument as described in this article. This is the JavaScript equivalent to writing an overloaded constructor except we have to call an explicit function pc.Quat.fromVec3(v) instead of just pc.Quat(v). However, it seems we'd still have to do the type checking from the first idea anyway so perhaps the first idea is better?

static fromVec3(v) {
    this.x = v[0];
    this.y = v[1];
    this.z = v[2];

    return this;
}

@mvaligursky
Copy link
Contributor

I think I'd prefer the option 1 as it's more consistent with the rest of the code base.
Thanks for looking into it!

@willeastcott
Copy link
Contributor

willeastcott commented Mar 16, 2022

It is more consistent. But it's also the source of several (now visible) errors output in VS Code. e.g.:

image

I do experience some regret at overloading function parameters like this now and if I could turn by time, I probably wouldn't have made that decision.

@zishiwu123
Copy link
Contributor

zishiwu123 commented Mar 16, 2022

Clarification:

  • option 1 is to overload the ex argument of function setFromEulerAngles in Quat, not to overload the constructor of Quat
  • option 2 is to create a new function setFromEulerAnglesVec3 as JavaScript does not seem to allow for function overloading

@zishiwu123
Copy link
Contributor

zishiwu123 commented Mar 17, 2022

My bad. I meant to reference 3663 but accidentally typed 3633 in my PR. Then when I tried to explain this in 3633, I accidentally had the # along with 3633 so got a reference to 3633 to here, even though they are not related to each other.

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

Successfully merging a pull request may close this issue.

5 participants