-
Notifications
You must be signed in to change notification settings - Fork 712
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
Cleanup and improvement for fromVectors() #491
base: master
Are you sure you want to change the base?
Conversation
Is the project still alive? |
|
||
// Close to PI - antiparallel check | ||
if (dot < -0.999999) { | ||
this.setFromAxisAngle(Math.abs(ux) > Math.abs(uz) ? new Vec3(-uy, ux, 0) : new Vec3(0, -uz, uy), Math.PI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why provoke GC? Previous solution is GC free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code is cleaner this way and it only handles the (hopefully) super rare edge case this way. But I mean, we could also have an initialized Vec3 somewhere floating around for such operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, rare or not, I'm following 100% GC free principle. This allows me to set breakpoints in Vec/Quat constructors to see where allocations are happening under stress testing, so I can make sure everything is as optimized as it gets (memory-wise). But yea, everyone has different ideas (as usual).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea good way of seeing it for sure. Should I change the commit to be GC free to be merged or don't you see the necessity?
Not exactly this repo, but I'm still using and updating the code for myself. I was actually wondering just some days ago how |
It should maybe not be called |
Hi Stefan,
thanks for Cannon.js! As I am the author of Quaternion.js ( https://github.com/infusion/Quaternion.js ), I was interested in your implementation and figured, your
fromVectors()
method could be improved quite a bit. Besides this I added a note where the calculation fornormalizeFast()
comes from and removed the unused vectors used before for multiplication.Hope this is helpful.
Robert