Skip to main content
GameDev.net gamedev.net
🔒 Locked

Criticize my custom vector class...

Started by d h k May 4, 2006 at 2:19 PM 86 replies 11.8k views
Original Post
d h k
d h k
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. ;)
Mike.Popoloski
Mike.Popoloski
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
Paulius Maruska
Paulius Maruska
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;}
Dave Hunt
Dave Hunt
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.
TDragon
TDragon
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.)
MTclip
MTclip
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
iMalc
iMalc
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.
Washu
Washu
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.
d h k
d h k
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;		}};
Promit
Promit
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.
Dave Hunt
Dave Hunt
As for the "fast" square-root, I don't think you need to worry about that. There have been many discussions here and elsewhere regarding this. On modern hardware, the built-in sqrtf() (with intrinsics enabled) performs at least as well as the "fast" approximations.
CTar
CTar
Quote:
Original post by ussnewjersey4
- 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.

What you are saying doesn't make sense, the struct keyword in C++ defines a class with public access and inheritance by default. It was only in C you couldn't use functions when using the struct keyword.

Quote:
Original post by ussnewjersey4
- 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.

Or even better, an N-dimensional vector where the number of elements is specified as a template argument.

Quote:
Original post by TDragon
Quote:
Original post by ussnewjersey4
- Many more operators. ... *, *=, /, /=...


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


He stated that the OP should define them for vectors and scalars, and using the operator* with a scalar is a good idea. I agree though that for dot- and cross-product you should use a normal function.

Quote:

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


He should use the standard std::abs, not that compiler-dependent, slow function.
Lets look at your function, you:

  1. Take address of float

  2. Cast to int*

  3. Dereference pointer

  4. Perform and-operation

  5. Take address of int

  6. Cast to float*

  7. Dereference pointer



Where a simple function (x>0?x:-x) is one compare-instruction, there will be 50 % change chance of a jump-instruction and 50 % chance of a negate instruction. So the worst case should be 3 instructions (of course it's inlined, so no ret) and best case 1 instruction. Also your function depends on the float's format.


To the OP:
I would get rid of all those comments, I know / divides, I know a function called normalize normalizes something, I know what it means to normalize a vector, I don't need to be told that return returns the resulting value etc. This is stuff someone could figure out faster by looking at your code than at your comments.
alnite
alnite
Quote:
Original post by CTar
To the OP:
I would get rid of all those comments, I know / divides, I know a function called normalize normalizes something, I know what it means to normalize a vector, I don't need to be told that return returns the resulting value etc. This is stuff someone could figure out faster by looking at your code than at your comments.

Some of those comments are redundant, but not all of them. Although everybody knows what a vector does, they are actually necessary for documentation purposes.
nilkn
nilkn
Quote:
Original post by CTar
Quote:
Original post by ussnewjersey4
- 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.

What you are saying doesn't make sense, the struct keyword in C++ defines a class with public access and inheritance by default. It was only in C you couldn't use functions when using the struct keyword.


In my experience, many people, when using C++, use structs exclusively for classes which contain only public data, and no methods at all. It's simply a convention. I think this is what ussnewjersey4 was getting at.
Paulius Maruska
Paulius Maruska
Quote:
Original post by nilkn
In my experience, many people, when using C++, use structs exclusively for classes which contain only public data, and no methods at all. It's simply a convention. I think this is what ussnewjersey4 was getting at.

Yeah, i'm one of those people... :)
Although i do add a few constructors for the structs...
Promit
Promit
Quote:
Original post by CTar
He should use the standard std::abs, not that compiler-dependent, slow function.
Lets look at your function, you:

  1. Take address of float

  2. Cast to int*

  3. Dereference pointer

  4. Perform and-operation

  5. Take address of int

  6. Cast to float*

  7. Dereference pointer


Most of those aren't ops that will appear in the generated code. Coming into the function there's a float on the stack. Step 1 loads that into an int register, which covers the first three in your list. The and operation only assumes that the float is conformant to IEEE 754/IEC 559. The vast majority of platforms (certainly the ones you'll be writing games for that have a hardware FPU) do, and if you're really paranoid you can (static) assert on std::numeric_limits::is_iec559, which will catch the problem instantly when you begin to port to a new platform. Lastly the last three ops are covered by returning a float value, which on many platforms will simply be accomplished by loading it off the stack into whatever integer register is used for return values to begin with.

That said, use std::abs. Note that x86 in particular provides the FABS instruction, which the compiler will replace the abs call with when you enable intrinsics.
SlimDX | Ventspace Blog | Twitter | Diverse teams make better games. I am currently hiring capable C++ engine developers in Baltimore, MD.
Mike.Popoloski
Mike.Popoloski
Quote:

In my experience, many people, when using C++, use structs exclusively for classes which contain only public data, and no methods at all. It's simply a convention. I think this is what ussnewjersey4 was getting at.

Yes, I was just expressing my own design patterns. I know that all structs are just classes that are in the public view by default, but I like it better when they are separated.

One thing, I'm not sure how fast the "Fast Square Root" function actually is, I just borrowed it from some other code (I don't remember where). But if you do use it, know that it is calculating 1/sqrt, so its actually inverse square root.
Mike Popoloski | Journal | SlimDX
deffer
deffer
1. Why didn't you make x,y,x members public? I'm sure people using such a simple class like 3D vector would want a simplicity of use (I do).

2. Const-correctness in member functions (+,-,dot,cross).

3. I would make all member functions non-member. dot, cross, normalize, length - those are functions, and its nice to see them in code looking as mathematical expressions. for ex.:
float cosAng = dot(norm(v1), norm(v2));

4. I would make normalize as:
cvector normalized(void) const;
or, as a non-member function:
cvector normalized(const cvector &) const;

Normalization of a vector would look like this:
v1 = norm(v1);
Compiler will optimize away any assignments (it did in my vector class).


Again: those are mathematical expressions that you'll be inserting into the code. Writing them in OO manner would obfuscate them. IMHO.
Mike.Popoloski
Mike.Popoloski
Just as an example, here is my own 4D vector class. Its probably not the best, but it certainly gets the job done.

//-----------------------------------------------------------------------------// 4D Vector//-----------------------------------------------------------------------------class ATLVector4{public:	// constructors	ATLVector4();	ATLVector4(float X, float Y, float Z, float W = 1.0f );	ATLVector4( const ATLVector4 &v );	// methods	inline void Set( float X, float Y, float Z, float W = 1.0f );	inline float GetLength();	inline float GetSqrLength() const;	inline void  Negate();	inline void  Normalize();	inline float AngleWith( ATLVector4 &v );	inline void  RotateWith( const ATLMatrix &m );	inline void  InvRotateWith( const ATLMatrix &m );	inline void  Difference( const ATLVector4 &u, const ATLVector4 &v );	inline void  Cross( const ATLVector4 &u, const ATLVector4 &v );	// operators	void		operator += ( const ATLVector4 &v );	void		operator -= ( const ATLVector4 &v );	void		operator *= ( float f );	void		operator /= ( float f );	void		operator += ( float f );	void		operator -= ( float f );	float		operator *	( const ATLVector4 &v ) const;	ATLVector4	operator *	( float f ) const;	ATLVector4	operator /	( float f ) const;	ATLVector4	operator +	( float f ) const;	ATLVector4	operator -	( float f ) const;	ATLQuaternion   operator *( const ATLQuaternion &q ) const;	ATLVector4	operator *	( const ATLMatrix &m ) const;	ATLVector4	operator +	( const ATLVector4 &v ) const;	ATLVector4	operator -	( const ATLVector4 &v ) const;	ATLVector4     &operator = ( const ATLVector4 &v );	// variables	float x,y,z,w;};
Mike Popoloski | Journal | SlimDX
johnb
johnb
Quote:
Original post by deffer
1. Why didn't you make x,y,x members public? I'm sure people using such a simple class like 3D vector would want a simplicity of use (I do).


You should't do this for such a class. You might for example one day soon want to implement it using SSE or AltiVec on appropriate hardware, using a 128 bit register to store the values. This will be impossible if code's been written that assumes vector components are accessible as individual floats.

Better instead to add accessor functions which hide the internal representation, e.g.

// ...    float GetX() const {return x;}    void SetX(float _x) {x = _x;}// etc. ...
John BlackburneProgrammer, The Pitbull Syndicate

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.