739462 - Update Tilt Math.jsm vector and matrix algebra

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Developer Tools: 3D View, defect, P4)

Description

[Victor Porof [:vporof][:vp]]

Some improvements were added to the original glMatrix library [0].
From what I skimmed, the functions which need some attention are:
vec3.multiply
vec3.dist (also add a vec3.distSquared?)
mat4.fromRotationTranslation
quat4.inverse
quat4.conjugate


It would be nice to update this stuff.
Also, maybe we can optimize the matrix multiplication to be in O(n^2.807) time.

[0] https://github.com/toji/gl-matrix/blame/master/gl-matrix.js

Comment 1

[Abhishek Potnis [:avp]]

Hi Victor,
I am interested on working on this bug. Could you please guide me on getting started with this bug.

Thanks.

Comment 2

[Victor Porof [:vporof][:vp]]

Hey, thanks for the interest.
Our current math functions used for Tilt, while working, are getting a bit stale. They're based on a library called glMatrix, which can be found on github (see comment #1). https://github.com/toji/gl-matrix/blob/master/gl-matrix.js

The library on github seems pretty stable at the moment (last commit 2 months ago). Your task is pretty much browsing that library, finding out what functions in glMatrix have changed or are missing from our implementation in browser/devtools/tilt/TiltMath.jsm, and add/modify them.

Some tests will also be required to be written, for the new functions, or if some existing checks start failing, to change them. See browser/devtools/tilt/test/browser_tilt_math_NN.js for how these tests look like.

For bonus points, it would be great to experiment with our mat4.multiply implementation to use Strassen's O(n^2.807) algorithm, instead of the classic Eight-Recursion O(n^3) algorithm. Since we only need 4x4 matrix multiplication in there, no need to handle recursion cases. A straight-forward unroll should work.

Comment 3

[Abhishek Potnis [:avp]]

Attachment: Added multiply, dist, distSquared and fromRotationTranslation functions to TiltMath.jsm

I have added multiply, dist, distSquared and fromRotationTranslation functions to TiltMath.jsm, still some more left to add.

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 658081 [details] [diff] [review]
Added multiply, dist, distSquared and fromRotationTranslation functions to TiltMath.jsm

Review of attachment 658081 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Abhishek,
Looks good so far. Keep 'em coming :)
Only a few style nits, but overall this looks that it's heading in the right direction.

::: browser/devtools/tilt/TiltMath.jsm
@@ +133,5 @@
>      return aDest;
>    },
>  
> +   /**
> +   * Performs a vector multiplication

Please align the /** with the * from the next line.
Comments should look like this:
/**
 * Comment
 */

@@ +145,5 @@
> +   *                if not specified result is written to the first operand
> +   *
> +   * @return {Array} the destination vec3 if specified, first operand otherwise
> +   */
> +  multiply: function (aVec, aVec2, aDest)

Need to have a named function expression here.

@@ +148,5 @@
> +   */
> +  multiply: function (aVec, aVec2, aDest)
> +  {
> +    if (!aDest) {
> +      aDest=aVec;

Space between = and the operands.

@@ +382,5 @@
> +   *
> +   * @return {Array} Distance between aVec and aVec2
> +   */
> +  dist: function (aVec, aVec2)
> +  {

Same here, we're going to need a named function expression.

@@ +385,5 @@
> +  dist: function (aVec, aVec2)
> +  {
> +    let x = aVec2[0] - aVec[0];
> +    let y = aVec2[1] - aVec[1];
> +    let z = aVec2[2] - aVec[2];         

No trailing whitespace please :)

@@ +399,5 @@
> +   *                second vector
> +   *
> +   * @return {Array} Distance between aVec and aVec2
> +   */
> +  distSquared: function (aVec, aVec2)

Named function expression.

@@ +404,5 @@
> +  {
> +    let x = aVec2[0] - aVec[0];
> +    let y = aVec2[1] - aVec[1];
> +    let z = aVec2[2] - aVec[2];         
> +    return (x*x + y*y + z*z);

Let's try to keep a consistent style throughout the file.
Space between * and operands.

@@ +1679,5 @@
> +   *                if not specified result is written to the first operand
> +   *
> +   * @return {Array} the destination quat4 if specified, first operand otherwise
> +   */
> +  fromRotationTranslation: function (aQuat, aVec, aDest)

Named function expression.

@@ +1688,5 @@
> +
> +    let x = aQuat[0]; 
> +    let y = aQuat[1]; 
> +    let z = aQuat[2]; 
> +    let w = aQuat[3];

No trailing whitespace.

Comment 5

[Abhishek Potnis [:avp]]

Attachment: Added/modified the functions in TiltMath.jsm

I have added/modified the above listed functions,left with the strassen's matrix multiplication though.....will add it in my next patch !

Comment 6

[Victor Porof [:vporof][:vp]]

New component triage. Filter on HORSE MASKS.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Victor and Abhishek, what's the status of this bug? Is it still being worked on or is there anything we could do to pull in more help to resolve it? Thanks!

Comment 8

[Victor Porof [:vporof][:vp]]

(In reply to Liz Henry :lizzard from comment #7)
> Hi Victor and Abhishek, what's the status of this bug? Is it still being
> worked on or is there anything we could do to pull in more help to resolve
> it? Thanks!

Progress on this bug has been OK, but we're going to fix this during an upcoming Tilt overhaul.