On Thu, Oct 22, 2009 at 3:15 PM, Thomas Klimpel <Thomas.Klimpel@synopsys.com> wrote:
So I decided to review the library so that I can also comment on the positive things. I noticed that my review is not free from unfriendly remarks, but there are hopefully enough friendly words to compensate for these.
Thanks for your time, I do appreciate it.
A possible improvement might be more explicit about the requirements of the value types of the matrices.
I realize that this page got buried in the documentation, even I had trouble finding it yet I know I wrote it. :) I'll have to fix this but here's a direct link: http://www.revergestudios.com/boost-la/scalar_requirements.html This area definitely needs work, because I'm not 100% sure those requirements are entirely correct. They could be wrong because while I have been using a previous version of this library for quite a while, I've not implemented a custom scalar type that's compatible with the library, so that part has never been tested.
- What is your evaluation of the implementation?
The implementation looks quite polished. I was impressed by the complete suite of unit-tests and the code generators. I've seen that there were even code generators for matrix_determinant and matrix_inverse. However, I think that these two code generators are false friends, because they implement an analytic formula that is neither efficient nor robust against rounding errors.
For determinants, I wanted to have at least the 2x2 3x3 and 4x4 versions done efficiently because those are used in 2D and 3D graphics. The formula is certainly not efficient, but (perhaps a bit naively?) I relied on compiler optimizations: the generated functions pull all scalars in local variables, which should untie the optimizer to figure out which series of multiplication/addition operations are identical and can be computed only once.
The traits system of the library probably even allows to adapt the raw T[Rows][Cols] type without using the la::mat<T, Rows, Cols> wrapper class.
The wrappers are necessary to support return-by-value operations (you can't copy an array.) That said, none of the supported operations require their *arguments* to be copyable, so you can use any of them with plain C arrays out of the box. Other non-copyable types are used by the view proxy system (in fact those types can't even be instantiated much less copied.)
Providing wrappers for T[D1][D2][D3] and T[D1][D2][D3][D4] , ..., T[D1][D2][D3][D4][D5][D6][D7][D8][D9] might also be useful sometimes (and the code generators might help here...), but this is the question of the scope of the library again.
I'm not sure why we're talking about 3D arrays, those can't possibly represent vectors or matrices. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode