-
Notifications
You must be signed in to change notification settings - Fork 718
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
quat2 implementation #231
quat2 implementation #231
Conversation
Added a fromRotation and improved the multiplication by a quat
Getting the new functions...
Now it only uses 1 array to represent a dual quaternion
I finally finished optimizing all of the code. Here is the code Should I remove some of the useless comments. (The previous implementations of a function...) |
Further discussion of this PR at: #221 I think this is a valuable addition to the library. Was about to start implementing these functions myself and was glad to find this PR and issue |
@stefnotch I left some comments on some
It is a best practice to remove any dead comments, or code from a final PR. |
src/gl-matrix/mat4.js
Outdated
@@ -946,6 +946,38 @@ export function fromRotationTranslation(out, q, v) { | |||
return out; | |||
} | |||
|
|||
//TODO FIX ME |
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.
Whats the Issue here?
src/gl-matrix/vec3.js
Outdated
@@ -520,31 +520,43 @@ export function transformMat3(out, a, m) { | |||
return out; | |||
} | |||
|
|||
//TODO Fix me (upgrade the function signature) |
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.
TODO whats the issue with the signature?
Great to see that somebody is interested! I'll go and fix those as soon as possible. (Should be done by next Sunday. Hopefully sooner.) I'm not sure about those comments, they might be related to the fact that the library switched their module system at some point. |
docs/quat2.js.html
Outdated
//https://github.com/mosra/magnum/blob/master/src/Magnum/Math/DualQuaternion.h#L364 | ||
//http://mosra.cz/blog/magnum-doc/classMagnum_1_1Math_1_1DualQuaternion.html#a6dcad20c0e9c7c9361b9c291ee42b181 | ||
/** | ||
* @class Dual Quaternion<br>Format: [[real],[dual]]<br>Make sure to have normalized dual quaternions, otherwise the functions may not work as intended. |
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.
confused here isn't the format [real, dual]
? maybe this needs to be updated
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.
Yeah, you are right
docs/quat2.html
Outdated
|
||
<h2><span class="attribs"><span class="type-signature"></span></span>quat2<span class="signature">()</span><span class="type-signature"></span></h2> | ||
|
||
<div class="class-description">Dual Quaternion<br>Format: [[real],[dual]]<br>Make sure to have normalized dual quaternions, otherwise the functions may not work as intended.</div> |
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.
Should the format be [real, dual]
here?
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.
Yep
Ok, here is what I still have to do:
Should I implement those 2 functions?
|
great work on this! I need some clarification on what you are asking.
Are you saying you haven't implemented these two functions? or wrote tests for them? If that's the case I think
Either way, I don't think either |
I have not implemented these 2 functions yet. Ok, I guess I'll implement lookAt! forEach would be similar to vec3.forEach. I guess I won't implement it for now and add it at a later date if there is any demand for it. |
Feel free to implement it.
(Hopefully)
@kevzettler @toji @sinisterchipmunk @chinedufn |
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.
@stefnotch thanks for all your work here this is looking great! I left a review with some inline comments where I had concerns. Please address at your convience.
docs/mat4.js.html
Outdated
@@ -974,6 +974,38 @@ <h1 class="page-title">Source: mat4.js</h1> | |||
return out; | |||
} | |||
|
|||
//TODO FIX ME |
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.
Is this outstanding?
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.
That's a documentation file. I didn't rebuild the docs. (Should probably do it)
docs/vec3.js.html
Outdated
@@ -548,31 +548,43 @@ <h1 class="page-title">Source: vec3.js</h1> | |||
return out; | |||
} | |||
|
|||
//TODO Fix me (upgrade the function signature) |
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.
Is this new? why does the signature need to change?
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.
Fixed. (Only had to rebuild the docs.)
spec/gl-matrix/quat2-spec.js
Outdated
describe("translate", function() { | ||
var matrixA = mat4.create(), matOut = mat4.create(), quatOut = quat2.create(); | ||
beforeEach(function() { | ||
//quat2A only seems to work when created using this function? |
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.
Any idea whats up here?
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.
No clue. I'm also confused.
It might be possible that quat2A isn't really a valid dual quaternion, however my maths isn't anywhere near good enough to figure out if that is, in fact, the case.
@@ -3,6 +3,11 @@ const assert = require('assert'); | |||
|
|||
|
|||
global.expect = function(e) { | |||
|
|||
function expected(e, message, a) { |
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 don't see this used anywhere below
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.
It's a convenience function which is only used in the same file.
e.g. https://github.com/stefnotch/gl-matrix/blob/master/spec/helpers/spec-helper.js#L86
src/gl-matrix/mat4.js
Outdated
export function fromQuat2(out, a) { | ||
//var normalizedA = quat2.create(); | ||
//quat2.normalize(normalizedA, a); | ||
//quat2.getTranslation(translation, normalizedA); |
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.
remove unused commented code
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.
Oh, my bad, I thought I had removed all of those comments.
@@ -522,29 +522,40 @@ export function transformMat3(out, a, m) { | |||
|
|||
/** | |||
* Transforms the vec3 with a quat | |||
* Can also be used for dual quaternions. (Multiply it with the real part) | |||
* | |||
* @param {vec3} out the receiving vector | |||
* @param {vec3} a the vector to transform | |||
* @param {quat} q quaternion to transform with | |||
* @returns {vec3} out | |||
*/ | |||
export function transformQuat(out, a, q) { |
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 don't think we should rewrite this function to handle parameter overriding for both quat
and quat2
types. The gl-matrix
library aims towards a 'functional programming' style of 'pure functions' which aims for static type style fixed function signatures. I would propose a new transformQuat2
method that explicitly handles the quat2
behavior.
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'm perfectly aware of it.
This thing works because a dual quaternion is represented by a flat array with 2 quaternions.
[x0, y0, z0, w0, x1, y1, z1, w1]
The first part is a perfectly normal, normalized quaternion which represents a rotation. The second part is a quaternion which represents a translation.
And, the function only looks at the first 4 elements of whatever array gets passed to it. In our case, it's the dual quaternion's rotation. It ignores the translation part, which is perfectly fine, because a vector doesn't really have a translation. (When you have a vector that represents a point, it's actually a vector from the origin to the point's coordinates.)
However, this comment might also cause some confusion. e.g.
Why doesn't this translate my vector?
transformQuat(out, [0,0,0], [no rotation, a translation])
(The answer would be: Because a vector only has a "magnitude (or length) and direction" --Wikipedia)
So, I guess it can be removed? If a person actually wants to rotate his vectors with a dual quaternion's rotation part, he probably knows his stuff and doesn't need this comment?
@@ -330,7 +330,7 @@ describe("vec2", function() { | |||
|
|||
beforeEach(function() { result = vec2.distance(vecA, vecB); }); | |||
|
|||
it("should return the distance", function() { expect(result).toBeCloseTo(2.828427); }); | |||
it("should return the distance", function() { expect(result).toBeEqualish(2.828427); }); |
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 the change on assertions here?
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 removed toBeCloseTo
because it wasn't working properly and toBeEqualish
does the exact same thing.
@stefnotch thanks for addressing comments. This lgtm, additional review from one of the maintainer's cc @toji @sinisterchipmunk is needed. To re-emphasize the value prop here, I feel this is a relevant and powerful addition to the |
So far, over a month has passed by and nothing has happened. |
ping @toji @sinisterchipmunk |
Note to self: Make sure that the quat2 code doesn't mess up the other code. |
@chinedufn @kevzettler @Beeblerox Dual quaternions are now a part of gl-matrix! 😃 Please report any bugs that you find. I'm also open to all suggestions. |
I implemented dual quaternions, which are extremely useful for gpu skinning. CryEngine uses them!
Dual quaternions are very similar to matrices. They represent a rotation and translation using 2 quaternions.
Pros:
+They only use 8 floats (rotation, translation) instead of 16 floats.
+They do not drift over time and do not introduce unwanted scaling and shearing.
+Interpolating them is easier and they do not suffer from the candy wrapper effect. The top one uses linear blend skinning with matrices and the bottom one uses dual quaternion skinning.
Cons:
-They cannot represent shearing.
-GLSL does not support them out of the box. (You have to write the multiplication code yourself; it is almost as fast as a matrix multiplication.)
I also improved the vec3.translateQuat.
Here is a fixed benchmark: https://jsperf.com/quaternion-transform-vec3-implementations-fixed (The benchmark's code was wrong)
If you feel like this doesn't fit into the library, feel free to not accept the pull request.