   Critique my 3D Vector Class
  Hello, I'm looking to get some feedback on my vector class, which is part of a larger library of math, graphics, input and platform code. I'm confident of the math and my implimentation is well optimized so I'm not posting the function definitions. What I'd like to know is if I've missed anything, have too much, better choices for function types (external friend vs. static method vs. standard method) and other critiques centered more around design.A few non-standard details of note:1)The "mul" method and standard '*' operation represent compoment-wise multiplication of elements, that is, for two vectors X is multiplied by X and returned in X. This is repeated for Y and Z and is usefull for non-uniform scaling of the components.2)The "div" method and standard '/' operation represents component-wise division of elements. It is equivilant to multiplication by a vector comprised of the inverse of each component of one of the multiplication factors.3)The "rad" method represents the squared length of the vector. The name "rad" comes from the fact that this is the radicand in the vector length computation -- sqrt(X^2 + Y^2 + Z^2)4)The "abs" method represents the length of the vector.5)The "unitize" method turns the calling vector into a unit vector, while the "unit" method returns a new vector that is the unit vector of the calling vector./******************************************************************************//*!\filevec3f.hpp\date6/14/2005\brief  This is the interface file for the Vec3f class, a 3-element,single-precision(float) vector class.*******************************************************************************/#ifndef FUEL_VEC3F_H#define FUEL_VEC3F_H#include <math.h>namespace Fuel { namespace Math{class Vec3f;inline booloperator==(const Vec3f &lhs, const Vec3f &rhs);inline booloperator!=(const Vec3f &lhs, const Vec3f &rhs);inline Vec3foperator *(const Vec3f &lhs, const Vec3f &rhs);inline Vec3foperator /(const Vec3f &lhs, const Vec3f &rhs);inline Vec3foperator +(const Vec3f &lhs, const Vec3f &rhs);inline Vec3foperator -(const Vec3f &lhs, const Vec3f &rhs);inline Vec3foperator *(const Vec3f &lhs, const float rhs);inline Vec3foperator /(const Vec3f &lhs, const float rhs);inline Vec3foperator -(const Vec3f &src);/******************************************************************************//*!\class Vec3f\brief  The Vec3f class impliments a 3-element, single-precision(float) vector class.    Operations include:- Default and Copy constructors- Parameterized constructor (X, Y, Z)- Parameterized constructor (from v1 to v2)- Equality and inequality operators- Assignment operator- Multiplication, Division, Addition and subtraction of vectors.- Multiplication and Division by scalars.- Cross product- Dot product- Length^2 and length functions- unitization functions*******************************************************************************/class Vec3f{public:Vec3f(){ }Vec3f(float x, float y, float z) : X(x), Y(y), Z(z){ }Vec3f(const Vec3f &src);Vec3f(const Vec3f &from, const Vec3f &to);// equality operatorsfriend booloperator==(const Vec3f &lhs, const Vec3f &rhs);friend booloperator!=(const Vec3f &lhs, const Vec3f &rhs);// self-modifying operatorsinline voidoperator =(const Vec3f &rhs);inline voidoperator*=(const Vec3f &rhs);inline voidoperator/=(const Vec3f &rhs);inline voidoperator+=(const Vec3f &rhs);inline voidoperator-=(const Vec3f &rhs);inline voidoperator*=(const float rhs);inline voidoperator/=(const float rhs);inline voidnormalize();// three-opperand methodsstatic inline voidmul(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static inline voiddiv(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static inline voidadd(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static inline voidsub(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static inline voidadd(Vec3f &res, const Vec3f &lhs, const float rhs);static inline voidsub(Vec3f &res, const Vec3f &lhs, const float rhs);static inline voidcross(Vec3f &res, const Vec3f &lhs, const float rhs);static inline Vec3fcross(const Vec3f &lhs, const Vec3f &rhs);static inline floatdot(const Vec3f &lhs, const Vec3f &rhs);static inline floatrad(const Vec3f &src);// Length^2static inline floatabs(const Vec3f &src);// Lengthstatic inline Vec3fnorm()const;// temp-modifying operatorsfriend Vec3foperator*(const Vec3f &lhs, const Vec3f &rhs);friend Vec3foperator/(const Vec3f &lhs, const Vec3f &rhs);friend Vec3foperator+(const Vec3f &lhs, const Vec3f &rhs);friend Vec3foperator-(const Vec3f &lhs, const Vec3f &rhs);friend Vec3foperator*(const Vec3f &lhs, const float rhs);friend Vec3foperator/(const Vec3f &lhs, const float rhs);friend Vec3foperator-(const Vec3f &src);// usefull constantsstatic const Vec3f ZEROVECTOR;static const Vec3f UNIT_POS_X;static const Vec3f UNIT_POS_Y;static const Vec3f UNIT_POS_Z;static const Vec3f UNIT_NEG_X;static const Vec3f UNIT_NEG_Y;static const Vec3f UNIT_NEG_Z;float X, Y, Z;};One thing I've considered adding is the box-product. How useful is it for graphics and other game calculations? Are there any more useful calculations that I've missed?I also have math.h included in the header file because it is needed for the inline implimentation of the abs (length) function. Aside from the fact that I should rather include cmath, is it a terrible practice to include the math header directly in my own header? If so, is it terrible enough to warrant making the length function non-inlined to avoid the issue alltogether?[Edited by - Ravyne on June 6, 2006 4:10:56 PM]
  I'm not going to dig it up now, but if you look through the last 10-15 pages of the 'game programming' forum, you'll find a similar thread; many of the common issues regarding vector and other math classes were discussed in that thread.As you might guess, these sorts of discussions often generate more heat than light. There are countless issues involved in writing a math class, many of which elicit strong opinions, so you're unlikely to find concensus. You've also chosen to go against convention in a couple of areas, which may throw people off a bit.There's only one thing I'm going to comment on, as it's a bit of a pet issue for me, and that is non-obvious operator overloading. If you're the only person who will ever use this class, that's one thing, but otherwise I'd recommend not using operator*() or operator/() for component-wise multiplication and division; it's not obvious or standard, and makes code that uses the class hard to read for others. Perhaps a modulate() function instead?As for the box product, yes, it's used in a few common algorithms. I'd never heard it called the 'box product' until now though, so you might go with the more common name 'triple product'.   ;  I dont have comments on the code, but I do wonder why the heck someone would write their own math classes, like vectors.. the only reason i can see is for learning puposes.   ;Quote:Original post by Matt AufderheideI dont have comments on the code, but I do wonder why the heck someone would write their own math classes, like vectors.. the only reason i can see is for learning puposes.I can think of a few reasons, but in any case, do you have a particular alternative in mind? (Just curious...)   ;Quote:Original post by Matt AufderheideI dont have comments on the code, but I do wonder why the heck someone would write their own math classes, like vectors.. the only reason i can see is for learning puposes.Vector classes are used so much with in graphics programming I find it hard to believe why people wouldn’t write their own custom classes.  I myself like an entirely OO driver form of vectors, and I have many custom functions that aren’t in traditional vector classes that I find useful in the code I write such as nearest tangents, closest points on a line, projections etc.   ;Quote:Original post by jykQuote:Original post by Matt AufderheideI dont have comments on the code, but I do wonder why the heck someone would write their own math classes, like vectors.. the only reason i can see is for learning puposes.I can think of a few reasons, but in any case, do you have a particular alternative in mind? (Just curious...)He probably would recommend the DirectX math classes (assuming you're using DirectX). Otherwise I bet there are open source math libraries out there. Personally I agree with custom writing math classes and functions.   ;  float X,Y,Z1. I would use custom typedefs for all datatypese.g.: typedef float vector_floatWhy? once you probably want to switch to 64 bit platforms and suddenly you get confronted with the use of standard float everywhere. This is not an issue as long as you only work on one and the same platform though.Imagine you switch to a 64bit platform and your binary files(levels,models ...) store 32 bit floats.Now you wish you had used custom typedefs.In the math library I usually use a seperate typedef to make use of the standard datatypes but for IO I use special typedefs to make sure I won t run into trouble later onI dunne why you call your top namespace Fuel, it rather use a more generic name for the basecode (I assume this vector class shall be part of your growing basecode)It would be interesting to see you implementation and not only the declarations   ;  My comments and suggestions:These functions should return a reference to *this, not void. // self-modifying operatorsinline voidoperator =(const Vec3f &rhs);inline voidoperator*=(const Vec3f &rhs);inline voidoperator/=(const Vec3f &rhs);inline voidoperator+=(const Vec3f &rhs);inline voidoperator-=(const Vec3f &rhs);inline voidoperator*=(const float rhs);inline voidoperator/=(const float rhs); The more common name for this function is "normalize". It might also return a reference to *this. inline voidunitize(); These functions don't need to be members of the class. // three-operand methodsstatic voidmul(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voiddiv(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voidadd(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voidsub(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voidadd(Vec3f &res, const Vec3f &lhs, const float rhs);static voidsub(Vec3f &res, const Vec3f &lhs, const float rhs);static voidcross(Vec3f &res, const Vec3f &lhs, const float rhs); Why not name these what they are: length_squared() and length()? friend floatrad(const Vec3f &src);// Length^2friend floatabs(const Vec3f &src);// Length Since all your member data is public, there is no need for non-member functions to be friends.   ;Quote:Original post by NickGravelynHe probably would recommend the DirectX math classes (assuming you're using DirectX). Otherwise I bet there are open source math libraries out there. Personally I agree with custom writing math classes and functions.The DirectX math library is a great place to start for basic and even some more advanced functionality.  The only drawback to it, is that it's in a lib and the functions can't be inlined.  Some of the basic functions are inlined...but some of the very frequently called functions like D3DXMatrixMultiply, D3DXVec3Transform (and all of the vector transform funcs in general), D3DXMatrixInverse, etc, are all implemented in the lib which prevents them from being inlined.We ran a test once that timed something like 100,000 matrix multiplies -- first round using the simple standard c-style loop over the indicies and the multiply and add; second round using the D3DXMatrixMultiply func.At first, I was astounded by the results because as you may or may not know, some of the D3DX math functions are SSE and SSE2 optimized -- anyway, the simple c-style home brew implementation blew the D3DX verion out of the water.The consensus here was that it wasn't the actual implementation that was the problem, but rather the fact that the c-style home brew was able to be inlined by the compiler whereas the D3DX version was not.  100,000 function calls apparently add up significantly and begin to outweigh the benefits of having SSE or SSE2 optimized code.Pretty interesting -- and food for thought.   ;Quote:Original post by JohnBoltonMy comments and suggestions:These functions should return a reference to *this, not void. // self-modifying operatorsinline voidoperator =(const Vec3f &rhs);inline voidoperator*=(const Vec3f &rhs);inline voidoperator/=(const Vec3f &rhs);inline voidoperator+=(const Vec3f &rhs);inline voidoperator-=(const Vec3f &rhs);inline voidoperator*=(const float rhs);inline voidoperator/=(const float rhs); This is actually a concious decision on my part for the sake of optimization. The returned reference is rarely used and, although technically 'correct' (or at least adhering to common practice,) promotes ugly code like a = b += c + d. So, to my mind, I've gained some optimization and prevented ugly code, and the fact that a direct reference to the variable in the next statement should be no less efficient seals the deal. Admittedly, it does go against the norm though.Quote:The more common name for this function is "normalize". It might also return a reference to *this. inline voidunitize(); Agreed and changed. I think my initial theory behind "unitize" was to prevent confusion between it and a normal vector, which probably made sense to me at the time, but doesn't hold water anymore.Quote:These functions don't need to be members of the class. // three-operand methodsstatic voidmul(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voiddiv(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voidadd(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voidsub(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);static voidadd(Vec3f &res, const Vec3f &lhs, const float rhs);static voidsub(Vec3f &res, const Vec3f &lhs, const float rhs);static voidcross(Vec3f &res, const Vec3f &lhs, const float rhs); They don't, its simply to help keep namespace polution to a minimum.Quote:Why not name these what they are: length_squared() and length()? friend floatrad(const Vec3f &src);// Length^2friend floatabs(const Vec3f &src);// Length Its just my personal preference that function names be kept to a minimum. abs() is a fairly common representation of the length, as it models after the absolute-value/vector magnitude operator relationship in Linear algebra (as I'm sure you're aware, just laying out my logic) rad is just something conveniantly short that made sense to me.Quote:Since all your member data is public, there is no need for non-member functions to be friends.True enough, but I may someday change that, and for now it isn't hurting anything. It also expresses the close relationship between the functions and class, marking them as part of the official API and not something added on by a third party.In fact, I've left the non-member opporators as friends, and changed the non-member methods to static methods (again, to avoid polluting the namespace.