Criticize my custom vector class...

Started by
86 comments, last by Washu 17 years, 12 months ago
EDIT: I hope this is in the right section. I wasn't sure whether to post this here or at the Maths and Physics forum, but I realized how much this actually matters to game programming and how little mathematically it gets, so I thought it might be better off here. Feel free to correct this if needed though! I recently wrote this vector class (it's a structure actually) and I wonder what could be improved for speed and simplicity to use and what kind of functionality needs to be added in order to give the user everything he needs to do all kinds of vector calculations (to create averaged normals for lighting or for phyisical simulations etc). Currently it supports simple and direct access to all three elements of the vector (x, y and z), a function to add two vectors together (by using the plus-operator for simplicity), a function to calculate both the dot- and cross-product of two given vectors and a function that simply normalizes the vector. Here's the complete file:

#include <math.h>

float absolute ( float value )
// returns the absolute value for a given value
// and returns it
{
	// make the value positive by taking the
	// square root of the square and return it
	return ( ( float ) sqrt ( value * value ) );
}

struct cvector
// structure for three-dimensional vectors
{
	// define the elements for the vector
	float x, y, z;

	cvector operator + ( const cvector &other )
	// performs vector addition
	{
		cvector result;

		// add all elements to each other
		result.x = x + other.x;
		result.y = y + other.y;
		result.z = z + other.z;

		// and return the result
		return result;
	}

	cvector cross_product ( const cvector &other )
	// calculates the cross-product for two
	// given vectors
	{
		cvector result;

		// calculate all results
		result.x = ( y * other.z ) - ( z * other.y );
		result.y = ( z * other.x ) - ( x * other.z );
		result.z = ( x * other.y ) - ( y * other.x );

		// and return the resulting vector
		return result;
	}

	float dot_product ( const cvector &other )
	// calculates the dot-product for two given
	// vectors
	{
		// return the resulting value
		return ( x * other.x ) + ( y * other.y ) + ( z * other.z );
	}

	void normalize ( void )
	// normalizes the vector - that makes the vector
	// length equal one
	{
		float length;
		
		// calculate the length of the vector first
		length = absolute ( ( float ) sqrt ( ( x * x ) + ( y * y ) + ( z * z ) ) );

		// now divide each element of the vector
		// by the length of the vector
		x = x / length;
		y = y / length;
		z = z / length;
	}
};


So, this gives me the possibility all of this in my projects: // define a vector first cvector vector; // now give it some values vector.x = 3.0f; vector.y = 5.0f; vector.z = 2.0f; // we could now add another vector to it like this vector + another_vector; // or we can calculate both the dot- or cross-product vector.dot_product ( another_vector ); vector.cross_product ( another_vector ); // and last but not least we can normalize our vector vector.normalize ( ); I wrote my own function to return absolute values, because I only found one for integer values and not for float values. I am sure there are several tricks to avoid taking the square roots in my whole structure, but this is the first time I am writing a vector class, so please cope with my possible newbieness. ;)
Advertisement
Its looking great, but there is a lot you could do to make it better. Here is a little list of things I came up with:

- Make it a class. This may or may not be important to you, but for me, it makes more sense when a class has functions and a structure does not.

- Make differently sized vectors so that you can fulfill different needs. For example, CVector1 with an x value, CVector2 with an x and y value, CVector3 with an x, y, and z value, and a CVector4 with an x, y, z, and w value.

- Add multiple constructors so that you can easily create them. For example, have a few constructors like CVector(), CVector( float x, float y, float z ), CVector( CVector &v ), etc.

- Utility functions, such as GetLength(), Negate(), AngleWith( CVector &v ), Set( float x, float y, float z ), etc.

- Many more operators. +=, +, -=, -, *, *=, /, /=, for performing operations both with scalars and other vectors.

- I'm not sure if you are making your own matrix class as well, but if you are, think about adding some operators for multiplying with a matrix, and some functions such as RotateWith( CMatrix &m ), and InvRotateWith( CMatrix &m ).

- Finally, think about implementing a FastNormalize() function, when speed is critical. In this function, instead of using normal square root, use the following:
//-----------------------------------// Fast Square Root//-----------------------------------inline float rsqrtf( float v ){    // calculate the 1 / sqrt    float v_half = v * 0.5f;    int i = *( int* )&v;    i = 0x5f3759df - ( i >> 1 );    v = *( float* )&i;    return v * ( 1.5f - v_half * v * v );}


Thats about all that I can think of. Don't think that everything I have listed is required for a good vector class. If yours does what it needs, then stick with it. No need to go adding functionality that you will never use.
Mike Popoloski | Journal | SlimDX
For some reason, i think that square root will work much slower than something like this:
float absloute (float value) {    return (value < 0) ? (-1 * value) : value;}
It's also a bit silly to be calling absolute() in your normalize function, since you will never get a negative number from the sqrt of the sum of the squares.
Quote:Original post by ussnewjersey4
- Many more operators. ... *, *=, /, /=...

What is the type of the expression (vector1 * vector2)?

Because vectors have both a scalar product (dot-product) and a vector product (cross-product), I would tend to say overloading the multiplication and division operators is not the best idea. Until all keyboards have "·" and "×" symbols and the C++ standard incorporates them as operators, best to use regular functions. (I also think many of the functions so far would be better off as non-member functions.)
{[JohnE, Chief Architect and Senior Programmer, Twilight Dragon Media{[+++{GCC/MinGW}+++{Code::Blocks IDE}+++{wxWidgets Cross-Platform Native UI Framework}+++
You really want to avoid the sqrt.. but you seem to know that.. lookup SIMD ..

//here is a faster absolute value_inline static float FastAbs(float f) {	int i=((*(int*)&f)&0x7fffffff);	return (*(float*)&i);}


divides are a little slower so
your normalize should get the inverse of length and multiply
float iLenght = 1.0f / length;
x *= iLenght;
y *= iLenght;
z *= iLenght;


other functions you could add..
Length // return the magnitude of a vector
LenghtSquared // return mag squared
Min // returns the minimun xyz of two vectors
Max // same but maximum
Reflect // reflect the vector from a givin normal
Project // project the vector from a given axis

.. those are some of the things I find helpful
Much of that is very inefficient I'm afriad.

First of all you do not need to call absolute around the sqrt because sqrt always returns the positive answer. Actually you must have realised this because you're relying on it in your absolute function.

Secondly, division is much slower than multiplication, so you're better to perform division only once to get the scale factor, then 3 multiplications for the x, y, and z components.

sqrt btw is even slower than division. You'd be much better off using a simple:
if (test < 0) return -test; else return test;
However I can't see the need for that in a vector class anyway as it only relates to a single number.
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms
Why on earth would you implement your own absolute value function when there exists a standard one? std::abs. Furtheremore, there is already an inner product implementation that is also standard (see std::inner_product).

Finally: I would personally implement your oeprator+, operator-, unary operator+, unary operator-, and operator * (scaling) as non-member functions in terms of assignment versions of those operators that are member functions. Cross product can also be a non-member function (which makes a lot more sense) Your class naming is also wildly wacky, as a vector is not a 3 tuple.

That's really just the tip of the iceberg.

In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.

Thank you to everybody who replied so quickly first of all.

I have now changed the following:

- Added a function to calculate a square root the quick way (I am not sure if this function is really correct, setting i to different values in the second and third line seems a bit strange -- any clear-up would be much appreciated):

inline float square_root ( float value )// quickly calculates and returns the square// root for a given value{   	int i;		// calculate the square root as fast	// as possible	i = *( int* )&value;	i = 0x5f3759df - ( i >> 1 );		// and return the square root        return *( float* )&i * ( 1.5f - ( ( *( float* )&i ) * 0.5f ) * ( *( float* )&i ) * ( *( float* )&i ) );}


- Changed my function to return the absolute value:

inline float absolute ( float value )// returns the absolute value for a given value// and returns it{	// calculate the absolute value the	// fast way and return it	return ( value < 0 ) ? ( -1 * value ) : value;}


- Turned the whole thing into a class using private members for the three elements and having the functions public (as well as having a new function to set/change the elements of the vector).

- Added those three constructors to initialize to default values (0.0f for each element), to given values and to the values of another given vector:

cvector ( )// initializes to default{	x = 0.0f;	y = 0.0f;	z = 0.0f;}cvector ( float new_x, float new_y, float new_z )// initializes to given values{	x = new_x;	y = new_y;	z = new_z;}cvector ( const cvector &other )// initializes to a given vector{	x = other.x;	y = other.y;	z = other.z;}


- Changed the normalize function for the vector to not use the obsolete absolute-function anymore and made sure to replace those three divisions with multiplications, so that it now looks like this:

void normalize ( void )// normalizes the vector - that makes the vector// length equal one{	float length;				// calculate the length of the vector first	length = 1.0f / square_root ( ( x * x ) + ( y * y ) + ( z * z ) );	// now divide each element of the vector	// by the length of the vector	x *= length;	y *= length;	z *= length;}


Some more notes:

- I don't think I'll add functions to calculate degrees between two vectors or functions to reflect the vector around a given normal or anything just yet, because that's a tad specific for the moment. I am really nor sure whether I'll ever use this functionality and don't want to waste my time on it now.

- I'd like to make the vector class be able to take anything from one (x) up until three (x, y and z) or even four (x, y, z and w) elements (or even infinite elements), but I am really not sure if there's a dynamic way (except creating one class named cvector1 for vectors with one element and one named cvector3 for vectors with three elements for example). Additionally I don't know whether I'll use anything other than three-dimensional vectors later on.

- I don't worry about matrices yet, so there's no need to include functions that mess around with vectors and matrices yet.

- I did notice that my way to calculate the dot- or cross-product by using functions instead of operators is not really the most appealing, but -- as TDragon said -- I can't find good operators really (I think "<" for dot- and ">" for cross-product would be highly misleading and a bit random).

Anyways, thanks for the comments and opinions so far. Keep those and the suggestions for improvement comming please!

Last but not least, here's the new complete listing:

#include <math.h>inline float square_root ( float value )// quickly calculates and returns the square// root for a given value{   	int i;		// calculate the square root as fast	// as possible	i = *( int* )&value;	i = 0x5f3759df - ( i >> 1 );		// and return the square root        return *( float* )&i * ( 1.5f - ( ( *( float* )&i ) * 0.5f ) * ( *( float* )&i ) * ( *( float* )&i ) );}inline float absolute ( float value )// returns the absolute value for a given value// and returns it{	// calculate the absolute value the	// fast way and return it	return ( value < 0 ) ? ( -1 * value ) : value;}class cvector// class for vectors{	private :		// define the elements for the vector		float x, y, z;	public :		cvector ( )		// initializes to default		{			x = 0.0f;			y = 0.0f;			z = 0.0f;		}		cvector ( float new_x, float new_y, float new_z )		// initializes to given values		{			x = new_x;			y = new_y;			z = new_z;		}		cvector ( const cvector &other )		// initializes to a given vector		{			x = other.x;			y = other.y;			z = other.z;		}		cvector set ( float new_x, float new_y, float new_z )		// sets values for the vector		{			x = new_x;			y = new_y;			z = new_z;		}		cvector operator + ( const cvector &other )		// performs vector addition		{			cvector result;			// add all elements to each other			result.x = x + other.x;			result.y = y + other.y;			result.z = z + other.z;			// and return the result			return result;		}		cvector cross_product ( const cvector &other )		// calculates the cross-product for two		// given vectors		{			cvector result;			// calculate all results			result.x = ( y * other.z ) - ( z * other.y );			result.y = ( z * other.x ) - ( x * other.z );			result.z = ( x * other.y ) - ( y * other.x );			// and return the resulting vector			return result;		}		float dot_product ( const cvector &other )		// calculates the dot-product for two given		// vectors		{			// return the resulting value			return ( x * other.x ) + ( y * other.y ) + ( z * other.z );		}		void normalize ( void )		// normalizes the vector - that makes the vector		// length equal one		{			float length;						// calculate the length of the vector first			length = 1.0f / square_root ( ( x * x ) + ( y * y ) + ( z * z ) );			// now divide each element of the vector			// by the length of the vector			x *= length;			y *= length;			z *= length;		}};
Use std::abs and std::sqrt. Don't write your own versions. Especially for the sqrt, which will be slower than the library one.
SlimDX | Ventspace Blog | Twitter | Diverse teams make better games. I am currently hiring capable C++ engine developers in Baltimore, MD.

This topic is closed to new replies.

Advertisement