2016-07-04 19:26 GMT+02:00 Andrey Gusev andrey.goosev@gmail.com:
+#ifndef __D3DX10MATH_INL__ +#define __D3DX10MATH_INL__
My copy of the file (from the latest DX SDK) uses __D3DXMATH_INL__ as header guard. I think we should do the same for compatibility.
+/* constructors & operators */
Nitpicking but this comment doesn't seem particularly useful.
+inline D3DXVECTOR2::D3DXVECTOR2(const FLOAT *pf)
Just use plain float.
+ if(!pf) return;
Space before '(', return on a separate line.
+inline D3DXVECTOR2::operator FLOAT* () +{ + return (FLOAT*)&x;
Please put a space between "float" and '*' in the cast. Probably do the same in the operator definition, although I don't really care in that regard.
+inline D3DXMATRIX D3DXMATRIX::operator - () const +{ + return D3DXMATRIX(-_11, -_12, -_13, -_14, + -_21, -_22, -_23, -_24, + -_31, -_32, -_33, -_34, + -_41, -_42, -_43, -_44); +}
This is almost identical to the MS's header. Please use 8 space indentation for line continuations. Probably drop the space between the "operator" keyword and the operator and between the operator and the opening parenthesis, in the operator declaration.
+inline D3DXCOLOR::operator UINT () const +{ + UINT _r = r >= 1.0f ? 0xff : r <= 0.0f ? 0x00 : (UINT)(r * 255.0f + 0.5f); + UINT _g = g >= 1.0f ? 0xff : g <= 0.0f ? 0x00 : (UINT)(g * 255.0f + 0.5f); + UINT _b = b >= 1.0f ? 0xff : b <= 0.0f ? 0x00 : (UINT)(b * 255.0f + 0.5f); + UINT _a = a >= 1.0f ? 0xff : a <= 0.0f ? 0x00 : (UINT)(a * 255.0f + 0.5f); + + return (_a << 24) | (_r << 16) | (_g << 8) | (_b << 0); +}
This one is also virtually identical. There seems to be room for writing a slightly different but equivalent implementation. FWIW identifiers starting with an underscore are supposed to be reserved, to some degree. It's generally better to avoid them.
+inline D3DXFLOAT16::D3DXFLOAT16() +{ +}
These default constructors are in (native) d3dx10math.h. I know that with d3dx9* headers we did put those in the .inl file but I think that was a bad idea.
+inline BOOL D3DXFLOAT16::operator == (const D3DXFLOAT16 &f) const +{
This one is also almost identical, implementation-wise. Now maybe there isn't that much room for variations here but still, at the very least replace the "== 0" with a '!'. If you can find other reasonable ways to differentiate, that's even better.
+inline D3DXCOLOR::D3DXCOLOR(UINT col) +{ + const FLOAT f = 1.0f / 255.0f; + r = f * (FLOAT)(unsigned char)(col >> 16); + g = f * (FLOAT)(unsigned char)(col >> 8); + b = f * (FLOAT)(unsigned char)(col >> 0); + a = f * (FLOAT)(unsigned char)(col >> 24); +}
Another one essentially identical. I would write it as something like:
inline D3DXCOLOR::D3DXCOLOR(UINT col) { float inv = 1.0f / 255.0f;
a = ((col >> 24) & 0xff) * inv; r = ((col >> 16) & 0xff) * inv; g = ((col >> 8) & 0xff) * inv; b = (col & 0xff) * inv; }
+static inline D3DXCOLOR* D3DXColorLerp(D3DXCOLOR *pout, const D3DXCOLOR *pc1, const D3DXCOLOR *pc2, FLOAT s)
Don't use those prefixed argument names: out, c1 and c2 work just fine. Also you want a space between D3DXCOLOR and '*' and no space between '*' and the function name.
+{ + if ( !pout || !pc1 || !pc2 ) return NULL;
Please no spaces after the '(' and before the ')'. Return on a separate line.
+ pout->r = (1-s) * (pc1->r) + s *(pc2->r);
You want spaces around the '-' operator and a space after the multiplication operator. No need for the parentheses around pc1->r and pc2->r. Regardless, MS's header uses the "optimized" linear interpolation formula, with just one multiplication. Your current implementation will generally give slightly different results (because of the limited precision of floats) so you should stick to the same formula instead.
This isn't an exhaustive list of issues. Many of the above comments apply to multiple places over the patch.
-
Matteo Bruni