# Critique my 3D Vector Class

## Recommended Posts

Ravyne    14300
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.
/******************************************************************************/
/*!
\file	vec3f.hpp
\date	6/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 bool	operator==(const Vec3f &lhs, const Vec3f &rhs);
inline bool	operator!=(const Vec3f &lhs, const Vec3f &rhs);
inline Vec3f	operator *(const Vec3f &lhs, const Vec3f &rhs);
inline Vec3f	operator /(const Vec3f &lhs, const Vec3f &rhs);
inline Vec3f	operator +(const Vec3f &lhs, const Vec3f &rhs);
inline Vec3f	operator -(const Vec3f &lhs, const Vec3f &rhs);
inline Vec3f	operator *(const Vec3f &lhs, const float rhs);
inline Vec3f	operator /(const Vec3f &lhs, const float rhs);
inline Vec3f	operator -(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 operators
friend bool	operator==(const Vec3f &lhs, const Vec3f &rhs);
friend bool	operator!=(const Vec3f &lhs, const Vec3f &rhs);

// self-modifying operators
inline void	operator =(const Vec3f &rhs);
inline void	operator*=(const Vec3f &rhs);
inline void	operator/=(const Vec3f &rhs);
inline void	operator+=(const Vec3f &rhs);
inline void	operator-=(const Vec3f &rhs);
inline void	operator*=(const float rhs);
inline void	operator/=(const float rhs);

inline void	normalize();

// three-opperand methods
static inline void	mul(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);
static inline void	div(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);
static inline void	add(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);
static inline void	sub(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);
static inline void	add(Vec3f &res, const Vec3f &lhs, const float rhs);
static inline void	sub(Vec3f &res, const Vec3f &lhs, const float rhs);

static inline void	cross(Vec3f &res, const Vec3f &lhs, const float rhs);

static inline Vec3f	cross(const Vec3f &lhs, const Vec3f &rhs);
static inline float	dot(const Vec3f &lhs, const Vec3f &rhs);
static inline float	rad(const Vec3f &src);	// Length^2
static inline float	abs(const Vec3f &src);	// Length

static inline Vec3f	norm()	const;

// temp-modifying operators
friend Vec3f	operator*(const Vec3f &lhs, const Vec3f &rhs);
friend Vec3f	operator/(const Vec3f &lhs, const Vec3f &rhs);
friend Vec3f	operator+(const Vec3f &lhs, const Vec3f &rhs);
friend Vec3f	operator-(const Vec3f &lhs, const Vec3f &rhs);
friend Vec3f	operator*(const Vec3f &lhs, const float rhs);
friend Vec3f	operator/(const Vec3f &lhs, const float rhs);
friend Vec3f	operator-(const Vec3f &src);

// usefull constants
static 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]

##### Share on other sites
jyk    2094
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'.

##### Share on other sites
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.

##### Share on other sites
jyk    2094
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...)

##### Share on other sites
skow    248
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.

##### Share on other sites
NickGravelyn    855
Quote:
Original post by jyk
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...)

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.

##### Share on other sites
Basiror    241
float X,Y,Z

1. I would use custom typedefs for all datatypes

e.g.: typedef float vector_float
Why? 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 on

I 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

##### Share on other sites
JohnBolton    1372

These functions should return a reference to *this, not void.
	// self-modifying operators	inline void		operator =(const Vec3f &rhs);	inline void		operator*=(const Vec3f &rhs);	inline void		operator/=(const Vec3f &rhs);	inline void		operator+=(const Vec3f &rhs);	inline void		operator-=(const Vec3f &rhs);	inline void		operator*=(const float rhs);	inline void		operator/=(const float rhs);
The more common name for this function is "normalize". It might also return a reference to *this.
	inline void		unitize();
These functions don't need to be members of the class.
	// three-operand methods	static void	mul(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);	static void	div(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);	static void	add(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);	static void	sub(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs);	static void	add(Vec3f &res, const Vec3f &lhs, const float rhs);	static void	sub(Vec3f &res, const Vec3f &lhs, const float rhs);	static void	cross(Vec3f &res, const Vec3f &lhs, const float rhs);
Why not name these what they are: length_squared() and length()?
	friend float	rad(const Vec3f &src);	// Length^2	friend float	abs(const Vec3f &src);	// Length
Since all your member data is public, there is no need for non-member functions to be friends.

##### Share on other sites
Brian Lawson    147
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.

##### Share on other sites
Ravyne    14300
Quote:
 Original post by JohnBoltonMy comments and suggestions:These functions should return a reference to *this, not void.  // self-modifying operators inline void operator =(const Vec3f &rhs); inline void operator*=(const Vec3f &rhs); inline void operator/=(const Vec3f &rhs); inline void operator+=(const Vec3f &rhs); inline void operator-=(const Vec3f &rhs); inline void operator*=(const float rhs); inline void operator/=(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 void unitize();

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 methods static void mul(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs); static void div(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs); static void add(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs); static void sub(Vec3f &res, const Vec3f &lhs, const Vec3f &rhs); static void add(Vec3f &res, const Vec3f &lhs, const float rhs); static void sub(Vec3f &res, const Vec3f &lhs, const float rhs); static void cross(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 float rad(const Vec3f &src); // Length^2 friend float abs(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.

##### Share on other sites
JohnBolton    1372
Quote:
Original post by Ravyne
Quote:
 Original post by JohnBoltonThese functions should return a reference to *this, not void.  inline void operator =(const Vec3f &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.

hmmm... Returning void instead of a reference to Vec3f will not result in meaningful optimization (if any at all), and you are not preventing ugly code, you are reducing the usability of your class. You don't like using the result of an assignment, but others might find it valuable. Code using the result of assignment is very common (something like this perhaps):
    while ( Vec3f::rad( v = GetVector() ) > 0 )    {        // use v ...    }
And since returning a reference to *this is the convention, your class would be considered broken by those that might depend on it.

Just my opinion, so disregard if you like.

##### Share on other sites
Promit    13246
Quote:
 Original post by RavyneThis is actually a concious decision on my part for the sake of optimization. [...] Admittedly, it does go against the norm though.
What incredibly naive and ignorant logic. First, it isn't an optimization. If the returned reference is not used, it will never be returned by the function. The optimizer is more than capable of handling such trivial cases. Second, going against the norm should generally be considered a grievous offense, unless there is a compelling reason. Since you lack a compelling reason here, you need to revise your functions to behave the way they're supposed to.

Oh, and because most people tend to screw this up: Implement + in terms of +=, not the other way around. The correct way to write operator+ is:
return Vec3f( *lhs ) += rhs;

##### Share on other sites
etothex    728
I would worry about the compiler (or the programmer) if a + or += operation wasn't inlined. And if it's inlined, the return reference is easily discarded.