Re: d3dx9: Implementation of D3DXMatrixDecompose and tests

Hi, 

1. The order of
+    /*The components of the translation transformation vector are straigth in the fourth column of the transformation matrix*/
+    (*pouttranslation).x=(*pM).m[3][0];
+    (*pouttranslation).y=(*pM).m[3][1];
+    (*pouttranslation).z=(*pM).m[3][2];
+
+    /*Let's calculate rotation now*/
+    /*If some of the scale factors is zero, it is impossible to obtain the rotation*/
+    if (((*poutscale).x==0)||((*poutscale).y==0)||((*poutscale).z==0))
+    {
+        return D3DERR_INVALIDCALL;
+    }
should be reversed, as we want to return D3DERR_INVALIDCALL as soon as possible (if needed). Also, the whole code can be simplified to:
+    /*If some of the scale factors is zero, it is impossible to obtain the rotation*/
+    if ((poutscale->x==0)||(poutscale->y==0)||(poutscale->z==0))
+        return D3DERR_INVALIDCALL;
+
+    /*The components of the translation transformation vector are straigth in the fourth column of the transformation matrix*/
+    pouttranslation->x=pM->m[3][0];
+    pouttranslation->y=pM->m[3][1];
+    pouttranslation->z=pM->m[3][2];
+
+    /*Let's calculate rotation now*/

(Note the removed {} which saves two lines of code)

2. I haven't tested this, but if you haven't done so yet, you should check the function's behavior if one of the pointers is NULL. If it segfaults with native d3dx9_36, your code is okay, otherwise you'll need an additional check right at the beginning.

3. Also, there were some spelling mistakes:
+    /*The scale factors are the modules of the three vectors __of that__ lie in __the__ column of the 3x3 submatrix*/
+    /*The scale factors are the modules of the three vectors __which__ lie in __each__ column of the 3x3 submatrix*/

+    /*If some of the scale factors is zero, it is impossible to obtain the rotation*/
+    /*If __any scale factor__ is zero, it is impossible to obtain the rotation*/

+    /*Then we go for the biggest component in order to __minimise__ division error as commonly explained in the __literacy__.
+      We just calculate one component from the diagonal to avoid sign loss of the square root. The first sign is not important
+      because the rotations (x,y,z,w) and (-x,-y,-z,-w) are equivalent. But if we __calc ulated__ the other two components we would have
+      to choose and could be wrong.*/
+    /*Then we go for the biggest component in order to __minimize__ division error as commonly explained in the __literature__.
+      We just calculate one component from the diagonal to avoid sign loss of the square root. The first sign is not important
+      because the rotations (x,y,z,w) and (-x,-y,-z,-w) are equivalent. But if we __calculated__ the other two components we would have
+      to choose and could be wrong.*/

4. Regarding the test, you should perhaps explain in a one-line comment what's special about the tested numbers in comparison to the others (other sign? invalid numbers? etc.)

5. It's not bad, but for the future you may want  to split the implementation and the test in two patches so that you have a greater chance to get one of them in.


Best regards,
    Tony




Unbegrenzter Speicher, Top-Spamschutz, 120 SMS und eigene E-MailDomain inkl.
http://office.freenet.de/dienste/emailoffice/produktuebersicht/power/mail/in...