chore(core): added Matrix4.fromTranslationQuaternion #345
Conversation
modules/core/src/classes/matrix4.ts
Outdated
| getTranslation(result: NumericArray = [-0, -0, -0]): NumericArray { | ||
| getTranslation(): Vector3; | ||
| getTranslation<T extends NumericArray>(result: T): T; | ||
| getTranslation(result = new Vector3()): Vector3 { |
There was a problem hiding this comment.
Should we change the return type here in the implementation, as it is not always a Vector3?
There was a problem hiding this comment.
Well, I guess we can keep the NumericArray return type in the default case, that still allows to chain calls if a Vector3 is provided as input... I'll update that !
There was a problem hiding this comment.
Thanks. My understanding when using overloaded signatures is that the "implementation" signature doesn't become visible to the user. Only the overloaded signatures without implementation become part of the public API.
If I am correct, the change I am proposing is more a matter of making the code a little clearer to maintain.
| @@ -42,6 +42,11 @@ test('Matrix4#fromQuaternion', (t) => { | |||
| t.end(); | |||
| }); | |||
|
|
|||
| test('Matrix4#fromTranslationQuaternion', (t) => { | |||
| tapeEquals(t, new Matrix4().fromTranslationQuaternion([0, 0, 0], [0, 0, 0, 1]), IDENTITY_MATRIX); | |||
There was a problem hiding this comment.
Appreciate the test case. Maybe add one test case with non-zero vectors to test the non-trivial case?
There was a problem hiding this comment.
Yes, a litlle bit lazy here !!! Just inspired myself from the test case Matrix4#fromQuaternion. I'll do my best to update that !
There was a problem hiding this comment.
Thanks, admit that our existing tests are not always that good. But that seems like a poor reason for keeping a bad habit in new code....
…mproved Matrix4#fromTranslationQuaternion test case
Hi again ! Sorry, I just realized that this repo has moved to visgl/math.gl. |
Sorry I din't notice that this was in the uber-web repo. Yes, that would be good. (We are having a discussion with Uber to see if we can solve this split but for now we need to work in the vis.gl fork. ) |
Added Matrix4.fromTranslationQuaternion.
And updated Matrix4.getScale, getTranslation, getRotation and getRotationMatrix3 to return same type as input. This allows the following kind of construct:
new Matrix4().getRotationMatrix3().invert()