-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Implemented roll function #7093
Conversation
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.
Thanks for taking this on! I'm also not super familiar with quaternions yet, so to start off I just have some questions about how the method works.
For tests: maybe we could try adding one that does a roll with numbers we can verify by hand, like a 90 degree roll? I think what we would expect to see afterwards is that if you start with a [0, 1, 0] up vector, then after the roll, you'd get something like [1, 0, 0] (maybe with a minus sign in there, the handedness of our vectors always messes me up, haha.)
src/webgl/p5.Camera.js
Outdated
const axisQuaternion = p5.Quat.fromAxisAngle( | ||
this._renderer._pInst._toRadians(amount), | ||
local.z[0], local.z[1], local.z[2]); | ||
const upQuat = new p5.Quat(0, this.upX, this.upY, this.upZ); |
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.
Would you mind explaining a bit how this works? Previously when I've seen a vector multiplied by a quaternion, it has some slightly different math. In the demo @SableRaf linked, they do it like this:
rotate( p ) {
// ( w*w - dot( v, v )) * p + 2 * dot( p, v ) * v + 2 * w * cross( v, p )
return p5.Vector.mult( p, this.w*this.w - this.v.dot(this.v) )
.add( p5.Vector.mult( this.v, 2 * p.dot(this.v) ) )
.add( p5.Vector.mult( this.v, 2 * this.w ).cross( p ) );
}
...and gl-matrix has an implementation here: https://glmatrix.net/docs/vec3.js.html#line522
Currently, it looks like this is also trying to do a quaternion-vector multiply, but right now it's turning the up vector into a quaternion first and then doing a quat-quat multiply. I'm not super familiar with quaternions, so if this is a technique people use, maybe it's worth adding a comment linking to a resource explaining it a bit?
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.
Would you mind explaining a bit how this works? Previously when I've seen a vector multiplied by a quaternion, it has some slightly different math. In the demo @SableRaf linked, they do it like this:
Yeah, I actually referred to the below link.
https://danceswithcode.net/engineeringnotes/quaternions/quaternions.html
So Initially we have the axis over which the vector has to be rotated and the angle with which the vector has to be rotated.
Based on this information I used the below formula to get a quaternion for the axis and angle
And then for rotation first I converted the vector to quaternion and then used the rotation formula (15b) to get the new vector as mentioned in the below figure.
This is the code in which I have applied the rotation formula (15b)
https://github.com/rohanjulka19/p5.js/blob/fb5289a485bad54046efbdb389bd9d9c5f52fdcc/src/webgl/p5.Quat.js#L46
Now I also didn't understood @SableRaf's logic but based on the below stackexchange link it seems like that is a more simplified version of the formula mentioned in the above figure (15b).
https://gamedev.stackexchange.com/a/50545
So I guess will change the code and use the simplified version instead as it requires less operations. Also will add the test and the above stack link in comments for future reference.
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.
Thanks for the explanation! that makes sense to me now. Mind maybe also linking that stackexchange answer in a comment too?
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.
Don't let my "logic" confuse things for you 😅 I really only have very surface knowledge of quaternions.
Hi @davepagurek ,
Also as you highlighted have added the links to the stackexchange answer and the doc I referred to, in the comments. Additionaly, I had to add a new function called https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/EPSILON |
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.
Thanks for adding the tests and comments @rohanjulka19! I think this is looking good.
The last thing I want to maybe iterate on is the documentation. I left a comment on the method for p5.Vector
, but also can we check is whether, when you run grunt yui:dev
to launch a local version of the p5.js reference, the new quaternion class is visible in the public reference? If so, we may want to either mark it as private for now or iterate on some of the documentation. (I think it's also fine for now if we want to make it temporarily private, and then open a separate issue for documentation.)
Ok have made quaternion class private for the time being. It won't show in docs now. |
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.
Thanks for the updates, looks good!
@all-contributors please add @rohanjulka19 for code |
I've put up a pull request to add @rohanjulka19! 🎉 |
@all-contributors please add @haroon10725 for code |
I've put up a pull request to add @haroon10725! 🎉 |
Resolves #6760
Changes:
Addressed the changes suggested by @SableRaf and @davepagurek mentioned in the issue #6819 for implementation of roll function highlighted below. Now quaternions are used to calculate the new up vector after rotation of camera on the z-axis.
#6819 (comment)
#6819 (comment).
Right now not really sure about the code structure and correctness (still don't completely understand quaternions and their working) that's why haven't added unit tests.
Screenshots of the change:
Screen.Recording.2024-06-13.at.7.55.16.PM.mov
PR Checklist
npm run lint
passes