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 #4139

Merged
merged 7 commits into from
Mar 22, 2022

Conversation

zishiwu123
Copy link
Contributor

@zishiwu123 zishiwu123 commented Mar 17, 2022

Fixes #3663

Discussion
I thought of two ways to instantiate ex, ey, and ez given the condition where ex instanceof Vec3 is true:

  1. Small premature optimization by setting ex = ex.x last so we don't have to make a clone of ex. A rule of thumb in software development is that "premature optimization is the root of all evil". However, I think it is okay to bend the rules in this situation since this is a game engine where performance matters. If a reviewer says otherwise, I will make changes based on their suggestions.
if (ex instanceof Vec3) {
    ez = ex.z;
    ey = ex.y;
    ex = ex.x;
}
  1. Clone ex and use the cloned Vec3 to instantiate ex, ey, and ez. Depending on the person, this option might be easier to understand than option 1. But there is a small performance penalty because this creates an extra Vec3 object.
if (ex instanceof Vec3) {
    const v = ex.clone();
    ex = v.x;
    ey = v.y;
    ez = v.z;
}

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@Maksims
Copy link
Contributor

Maksims commented Mar 17, 2022

No need to clone (it will lead to memory allocations, which then lead to minor/major GC stalls).
Code like this I believe is fine too, which eliminates the potential issue of someone changing the order of lines.

if (ex instanceof Vec3) {
    const vec = ex;
    ex = vec.x;
    ey = vec.y;
    ez = vec.z;
}

Also, it makes ey and ez function arguments optional.

@zishiwu123
Copy link
Contributor Author

Good idea @Maksims . Compound values are assigned by reference in JavaScript so it would not create a new copy of the object. Plus it's safer because the order in which ex, ey, and ez are assigned doesn't matter in your idea.

Thanks to @kungfooman for the suggestion to simplify use.
Thanks to @Maksims for pointing out no need to clone when
input is Vec3, just a reference is enough.
@zishiwu123
Copy link
Contributor Author

@kungfooman Good idea. I pushed another commit in this PR based on your suggestion.

@kungfooman
Copy link
Collaborator

@kungfooman Good idea. I pushed another commit in this PR based on your suggestion.

Why still check if its a Vec3, can't you just simplify the code completely now (removing all those instanceof lines)? By just always passing x, y, z to Quat#setFromEulerAngles and let only that method check if its a Vec3

Not sure if I'm missing something, I didn't test it. Good job!

@zishiwu123
Copy link
Contributor Author

zishiwu123 commented Mar 21, 2022

@kungfooman Another good point. The instanceof check in the 4 lines you mentioned are redundant. I removed them and the tests still pass.

No longer need to check if x instanceof Vec3 when x is passed
as input to setFromEulerAngles() pc.Quat function.
src/math/quat.js Outdated Show resolved Hide resolved
@willeastcott
Copy link
Contributor

This is great - thanks!! 🙏

@willeastcott willeastcott merged commit 919cbe8 into playcanvas:main Mar 22, 2022
@zishiwu123 zishiwu123 deleted the make-quat-from-vec3 branch March 25, 2022 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Vec3 Support for setFromEulerAngles() pc.Quat function
5 participants