Hi, Here are some comments on math3d in the hope that it helps.
Here I'm talking about class QGenericMatrix, but from what i've seen, much of that applies to the other classes. http://qt.gitorious.org/qt/qt/blobs/master/src/gui/math3d/qgenericmatrix.h 1. "operator()(int, int)" const should return a "const T&", not a T, for consistency with the non-const operator and in cases where the user is interested in the address where a coefficient is stored (if the user wants direct access to implement his own optimized operation). 2. Not sure what's the need for constData() const when you already have data() const. Maybe explain in documentation? (Or remove if useless). 3. You allow arbitrary datatype T, yet you do memcpy() on the array which only works for usual number types and wouldn't work for custom types T having a nontrivial constructor. So you need to make a choice, either use memcpy() or allow arbitrary T (or say clearly that T must be either float or double... maybe guard that in a "static assertion" kind of check) 4. (This one is a nitpick, feel free to ignore) The array is stored as a T[][], frankly you're better off storing as a T[] and doing the offset computation explicitly, for 2 reasons: 1) the T[][] forces you to write weird looking syntax such as in operator+= and data(); 2) if you ever want to support dynamically-sized matrices, it'll suddenly make a big difference and the only right way will be T[], otherwise you'd be allocating a dynamic array for every column. 5. operator== and operator!= should never be used in practice, of course, as they do an exact comparision of floating point values which is meaningless. I understand that you may have added these operators just so that QGenericMatrix's can be used in certain containers. So that's ok, but please document clearly the fact that they should never be used. By the way if you want proper fuzzy compares for matrices, see what we do in Eigen, e.g. isApprox(). 6. Same as 5. in isIdentity() etc. I also have a specific question about QQuaternion: 7. what is the geometric significance of QQuaternion::vector() and setVector()? They seem to be working as if the quaternion was interpreted as a homogeneous vector with 4 components; I wonder what can be a use case for that ?? Even so, there's a discrepancy: vector() returns a vector with the 4th component set to 1, but setVector doesn't touch the 4th component? And a remark about QMatrix4x4: 8. Having to do "QMatrix4x4 m(1);" to get an uninitialized matrix is a rather strange API! As shows the comment that you added everytime you use that ;) Rather make this constructor take a special enum value QMatrix4x4::Uninitialized, or make that an empty struct, etc! Cheers, Benoit _______________________________________________ Qt4-preview-feedback mailing list [email protected] http://lists.trolltech.com/mailman/listinfo/qt4-preview-feedback
