Skip to content
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

Template Matrix class #1297

Open
wants to merge 29 commits into
base: development
Choose a base branch
from

Conversation

marauder2k7
Copy link
Contributor

Adds a template class for matrices.

NOTE: Templated class has not replaced MatrixF yet.

initial implemenation of templated classes :
Matrix class first.
setColumn
setRow
isIdentity

only a few functions left
fix comment, torque is already column major, even though doc says its row major
add mul functions and operators
code conformity changes
added functions for normalize and affineInverse
@dottools
Copy link
Contributor

The thing that stands out to me immediately that needs work is the inconsistent brace coding format style.
Should match the cleaner brace coding format style that is already present in the mMatrix.h and mMatrix.cpp files.
Example, from awful bracing style:

template<typename DATA_TYPE, U32 rows, U32 cols>
inline bool Matrix<DATA_TYPE, rows, cols>::isIdentity() const {
   for (U32 i = 0; i < rows; i++) {
      for (U32 j = 0; j < cols; j++) {
         if (j == i) {
            if((*this)(i, j) != static_cast<DATA_TYPE>(1)) {
               return false;
            }
         }
         else {
            if((*this)(i, j) != static_cast<DATA_TYPE>(0)) {
               return false;
            }
         }
      }
   }
   
   return true;
}

To proper bracing style that promotes readability:

template<typename DATA_TYPE, U32 rows, U32 cols>
inline bool Matrix<DATA_TYPE, rows, cols>::isIdentity() const
{
   for (U32 i = 0; i < rows; i++)
   {
      for (U32 j = 0; j < cols; j++)
      {
         if (j == i)
         {
            if((*this)(i, j) != static_cast<DATA_TYPE>(1))
            {
               return false;
            }
         }
         else
         {
            if((*this)(i, j) != static_cast<DATA_TYPE>(0))
            {
               return false;
            }
         }
      }
   }

   return true;
}

There's several rather large inline functions in mMatrix.h that should be moved as normal class functions to mMatrix.cpp. Keep in mind that even though modern processors are fast, they're still constrained to the same instruction cache line sizes that existed decades prior and thus heavily duplicated machine generated code (inlined function) is slower and less efficient than multiple branched function calls to the same code memory location (normal function).

I'm also very concerned with all the if () checks for row and col, and having to use nested loops. This templated matrix class is going to perform horribly compared to the per hand made matrix sized functions. The compiler won't be able to do opportunistic SIMD instruction generation with this class at all.

apparently templated classes need all functions to be inline, otherwise unresolved symbols
macro for switching between matrixf and templated
few functions that were missed
backup closest working example, no errors or warnings from compile, matrices arent correct though yet.
most working example
@marauder2k7
Copy link
Contributor Author

marauder2k7 commented Jul 28, 2024

The thing that stands out to me immediately that needs work is the inconsistent brace coding format style. Should match the cleaner brace coding format style that is already present in the mMatrix.h and mMatrix.cpp files. Example, from awful bracing style:

template<typename DATA_TYPE, U32 rows, U32 cols>
inline bool Matrix<DATA_TYPE, rows, cols>::isIdentity() const {
   for (U32 i = 0; i < rows; i++) {
      for (U32 j = 0; j < cols; j++) {
         if (j == i) {
            if((*this)(i, j) != static_cast<DATA_TYPE>(1)) {
               return false;
            }
         }
         else {
            if((*this)(i, j) != static_cast<DATA_TYPE>(0)) {
               return false;
            }
         }
      }
   }
   
   return true;
}

To proper bracing style that promotes readability:

template<typename DATA_TYPE, U32 rows, U32 cols>
inline bool Matrix<DATA_TYPE, rows, cols>::isIdentity() const
{
   for (U32 i = 0; i < rows; i++)
   {
      for (U32 j = 0; j < cols; j++)
      {
         if (j == i)
         {
            if((*this)(i, j) != static_cast<DATA_TYPE>(1))
            {
               return false;
            }
         }
         else
         {
            if((*this)(i, j) != static_cast<DATA_TYPE>(0))
            {
               return false;
            }
         }
      }
   }

   return true;
}

There's several rather large inline functions in mMatrix.h that should be moved as normal class functions to mMatrix.cpp. Keep in mind that even though modern processors are fast, they're still constrained to the same instruction cache line sizes that existed decades prior and thus heavily duplicated machine generated code (inlined function) is slower and less efficient than multiple branched function calls to the same code memory location (normal function).

I'm also very concerned with all the if () checks for row and col, and having to use nested loops. This templated matrix class is going to perform horribly compared to the per hand made matrix sized functions. The compiler won't be able to do opportunistic SIMD instruction generation with this class at all.

templated class, since c++ 11 and up all functions need to be inline otherwise you get unresolved externals errors.

Due to the limited tests ive already done it is proving to be more performant than matrixf and the math_c functions

The reason for the if() checks on rows and cols is because this class is designed to be used with any size matrix you want, there will be implementations of 3x3 3x2 4x3 3x4 that we know of so far, for example the instancing stuff uses alot of 4x3 matrices, atm we have to take these in as 4x4, doesnt sound like much but theres over 70 of them in an array for the instancing shader

silence issues from macos clang
bracket lines
change functions to match mmath_c to figure out where the issue is.
committed tests for matrix class
so far all tests are matching between templated and stock matrixf class
more tests that match between template and matrixf
change mul tests to use more real world examples
fixed inverse function, was not returning correctly.
test box multiplication
test transformPlane
added more unit tests to match values between templated and matrix

tests showed discrepancies in affineInverse, fixed the function to return what is expected.
more unit tests revealed more discrepancies fixes applied.
implemented the rest of the tests
euler single dimension angle tests now pass, missed 1.0f in z
further tests showed issues with inverse function, now to better match what was originally happening, the inverse only happens on the 3x3 portion of the matrix and translation is handled separately.

Frustum test now uses more real world examples of a projection matrix. Test for the full unproject stack of math to test its result as unproject was where the issue about inverse originated
added #if block around inverse methods to track down shadow bug

uses old inverse method as default for now.
fix invertTo function
unitTest to make sure matrix calling invertTo does not get changed.
reimplemented gauss jordan.
returning identity no longer necessary as fullinverse is its own algo
add default destructor
invertTo should always just be const
return loop to * operator, explicit will not allow for scaling of rows and cols
change matrix unit tests to use POINT_EPSILON macro for testing precision
remove old matrix test class, was not used anyway
@marauder2k7
Copy link
Contributor Author

This PR is now fully ready for reviews and tests

to use the templated class add #define USE_TEMPLATE_MATRIX to the torqueConfig.h file

@dottools
Copy link
Contributor

dottools commented Aug 1, 2024

Should add #cmakedefine USE_TEMPLATE_MATRIX with a comment in Tools/CMake/torqueConfig.h.in so that it can be a CMake build option.

@marauder2k7
Copy link
Contributor Author

marauder2k7 commented Aug 1, 2024

Should add #cmakedefine USE_TEMPLATE_MATRIX with a comment in Tools/CMake/torqueConfig.h.in so that it can be a CMake build option.

i was going to completely remove matrixF if this passed the review phase

Added the cmake option, but it should be advices, if this gets committed into main while still in its review phase due to the number of classes effected by it and so on, its best to remove that cmake option

cmake option to enable the templated matrix class, this is still in review phase, if it gets merged into main, probably best to remove this options just in case someone activates it accidentally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants