Feedback on my vector class implementations - GameDev.Net Discussion Forums

Add Forum to Favorites | Send Topic To a Friend | View Forum FAQ | Track this topic

Last Thread Next Thread

Feedback on my vector class implementations

zeeli Member since: 10/30/2005

Posted - 4/3/2009 5:13:54 AM

Hi,
I'd like to get some feedback on my 2d and 3d vector class implementations.

Vector3.h

#ifndef VECTOR3_H
#define VECTOR3_H

#include <cmath>

template<typename T>
class Vector3 {
public:
	Vector3() : m_tX(0), m_tY(0), m_tZ(0) {}
	Vector3(T tX, T tY, T tZ) : m_tX(tX), m_tY(tY),m_tZ(tZ) {}
	Vector3(const Vector3<T> &rhs) {
		if(*this == rhs) {
			return *this;
		}

		*this = rhs;
	}

	~Vector3() {}

	inline bool operator ==(const Vector3<T> &rhs) {
		return (m_tX == rhs.m_tX && m_tY == rhs.m_tY && m_tZ == rhs.m_tZ);
	}

	inline bool operator !=(const Vector3<T> &rhs) {
		return (m_tX != rhs.m_tX && m_tY != rhs.m_tY && m_tZ != rhs.m_tZ);
	}

	inline bool operator <(const Vector3<T> &rhs) {
		return (m_tX < rhs.m_tX && m_tY < rhs.m_tY && m_tZ < rhs.m_tZ);
	}

	inline bool operator <=(const Vector3<T> &rhs) {
		return (m_tX <= rhs.m_tX && m_tY <= rhs.m_tY && m_tZ <= rhs.m_tZ);
	}

	inline bool operator >(const Vector3<T> &rhs) {
		return (m_tX > rhs.m_tX && m_tY > rhs.m_tY && m_tZ > rhs.m_tZ);
	}

	inline bool operator >=(const Vector2<T> &rhs) {
		return (m_tX >= rhs.m_tX && m_tY >= rhs.m_tY && m_tZ >= rhs.m_tZ);
	}

	inline Vector2<T> &operator =(const Vector2<T> &rhs) {
		if(*this == rhs) {
			return *this;
		}

		*this = rhs;

		return *this;
	}

	inline Vector3<T> &operator +(const Vector3<T> &rhs) {
		Vector3<T> vec;
		vec.m_tX = m_tX + rhs.m_tX;
		vec.m_tY = m_tY + rhs.m_tY;
		vec.m_tZ = m_tZ + rhs.m_tZ;

		return vec;
	}

	inline Vector3<T> &operator +=(const Vector3<T> &rhs) {
		Vector3<T> vec;
		vec.m_tX = m_tX += rhs.m_tX;
		vec.m_tY = m_tY += rhs.m_tY;
		vec.m_tZ = m_tZ += rhs.m_tZ;

		return vec;
	}

	inline Vector3<T> &operator -(const Vector3<T> &rhs) {
		Vector3<T> vec;
		vec.m_tX = m_tX - rhs.m_tX;
		vec.m_tY = m_tY - rhs.m_tY;
		vec.m_tZ = m_tZ - rhs.m_tZ;

		return vec;
	}

	inline Vector3<T> &operator -=(const Vector3<T> &rhs) {
		Vector3<T> vec;
		vec.m_tX = m_tX -= rhs.m_tX;
		vec.m_tY = m_tY -= rhs.m_tY;
		vec.m_tZ = m_tZ -= rhs.m_tZ;

		return vec;
	}

	inline Vector3<T> &operator *(T tScalar) {
		Vector3<T> vec;
		vec.m_tX = m_tX * tScalar;
		vec.m_tY = m_tY * tScalar;
		vec.m_tZ = m_tZ * tScalar;

		return vec;
	}

	inline T magnitude() {
		return sqrt((m_tX * m_tX) + (m_tY * m_tY) + (m_tZ * m_tZ));
	}

	inline void normalize() {
		T magnitude = magnitude();

		m_tX = m_tX / magnitude;
		m_tY = m_tY / magnitude;
		m_tZ = m_tZ / magnitude;
	}

	inline Vector3<T> cross(const Vector3<T> &rhs) {
		Vector2<T> vec;
		vec.m_tX = (m_tY * rhs.m_tZ) - (m_tZ * rhs.m_tY);
		vec.m_tY = (m_tZ * rhs.m_tX) - (m_tX * rhs.m_tZ);
		vec.m_tZ = (m_tX * rhs.m_tY) - (m_tY * rhs.m_tX);

		return vec;
	}

	inline T dot(const Vector3<T> &rhs) {
		return (m_tX * rhs.m_tX + m_tY * rhs.m_tY + m_tZ * rhs.m_tZ);
	}

	inline T x() {
		return m_tX;
	}

	inline T y() {
		return m_tY;
	}

	inline T z() {
		return m_tZ;
	}
private:
	T m_tX, m_tY, m_tZ;
};

typedef Vector3<float> vector3f;
typedef Vector3<double> vector3d;


#endif //VECTOR3_H

Vector2.h

#ifndef VECTOR2_H
#define VECTOR2_H

#include <cmath>

template<typename T>
class Vector2 {
	public:
		Vector2() : m_tX(0), m_tY(0) {}
		Vector2(T tX, T tY) : m_tX(tX), m_tY(tY) {}
		Vector2(const Vector2<T> &rhs) {
			if(*this == rhs) {
				return *this;
			}

			*this = rhs;
		}

		~Vector2() {}

		inline bool operator ==(const Vector2<T> &rhs) {
			return (m_tX == rhs.m_tX && m_tY == rhs.m_tY);
		}

		inline bool operator !=(const Vector2<T> &rhs) {
			return (m_tX != rhs.m_tX && m_tY != rhs.m_tY);
		}

		inline bool operator <(const Vector2<T> &rhs) {
			return (m_tX < rhs.m_tX && m_tY < rhs.m_tY);
		}

		inline bool operator <=(const Vector2<T> &rhs) {
			return (m_tX <= rhs.m_tX && m_tY <= rhs.m_tY);
		}

		inline bool operator >(const Vector2<T> &rhs) {
			return (m_tX > rhs.m_tX && m_tY > rhs.m_tY);
		}

		inline bool operator >=(const Vector2<T> &rhs) {
			return (m_tX >= rhs.m_tX && m_tY >= rhs.m_tY);
		}

		inline Vector2<T> &operator =(const Vector2<T> &rhs) {
			if(*this == rhs) {
				return *this;
			}

			*this = rhs;

			return *this;
		}

		inline Vector2<T> &operator +(const Vector2<T> &rhs) {
			Vector2<T> vec;
			vec.m_tX = m_tX + rhs.m_tX;
			vec.m_tY = m_tY + rhs.m_tY;

			return vec;
		}

		inline Vector2<T> &operator +=(const Vector2<T> &rhs) {
			Vector2<T> vec;
			vec.m_tX = m_tX += rhs.m_tX;
			vec.m_tY = m_tY += rhs.m_tY;

			return vec;
		}

		inline Vector2<T> &operator -(const Vector2<T> &rhs) {
			Vector2<T> vec;
			vec.m_tX = m_tX - rhs.m_tX;
			vec.m_tY = m_tY - rhs.m_tY;

			return vec;
		}

		inline Vector2<T> &operator -=(const Vector2<T> &rhs) {
			Vector2<T> vec;
			vec.m_tX = m_tX -= rhs.m_tX;
			vec.m_tY = m_tY -= rhs.m_tY;

			return vec;
		}

		inline Vector2<T> &operator *(T tScalar) {
			Vector2<T> vec;
			vec.m_tX = m_tX * tScalar;
			vec.m_tY = m_tY * tScalar;

			return vec;
		}

		inline T magnitude() {
			return sqrt((m_tX * m_tX) + (m_tY * m_tY));
		}

		inline void normalize() {
			T magnitude = magnitude();

			m_tX = m_tX / magnitude;
			m_tY = m_tY / magnitude;
		}

		inline T dot(const Vector2<T> &rhs) {
			return (m_tX * rhs.m_tX + m_tY * rhs.m_tY);
		}

		inline T x() {
			return m_tX;
		}

		inline T y() {
			return m_tY;
		}
	private:
		T m_tX, m_tY;
};

#endif //VECTOR2_H

Please tell me if you find any flaws or things that should be improved.

Best regards,
zEeLi

User Rating: 1019 | Rate This User

Report this Post to a Moderator | Link

grekster Member since: 4/23/2004 From: Whitley Bay, United Kingdom

Posted - 4/3/2009 5:27:32 AM

You have some typos in your code, some of the Vector3 methods reference Vector2.

User Rating: 1353 | Rate This User

Report this Post to a Moderator | Link

ToohrVyk Member since: 9/12/2002 From: Paris, France

Posted - 4/3/2009 5:28:45 AM

Your comparison operators are incorrect. How does (0,1) compare to (1,0) ? If you really insist on comparing vectors, use lexicographical order.

You seem to be returning from a constructor, which is meaningless. You also implement your copy constructor in terms of the assignment operator. The end result is that your copy constructor is both longer and slower than the default, compiler-provided copy constructor.

Even worse, you implement your assignment operator in terms of itself! And it's not exception-safe!

Perhaps you should compile and use your class before asking for feedback, so that you don't publish code that just doesn't work?

Blog — Facebook

User Rating: 1977 | Rate This User

Report this Post to a Moderator | Link

zeeli Member since: 10/30/2005

Posted - 4/3/2009 5:34:55 AM

Thank you both.

User Rating: 1019 | Rate This User

Report this Post to a Moderator | Link

rozz666 Member since: 5/27/2007 From: Lodz, Poland

Posted - 4/3/2009 5:37:34 AM

What's THAT?!

Vector3(const Vector3<T> &rhs) {
		if(*this == rhs) {
			return *this;
		}

		*this = rhs;
	}

Constructors don't return anything.

~~Vector3(const Vector3<T>& rhs) : m_x(rhs.m_x), m_y(rhs.m_y), m_z(rhs.m_z) { }~~

Provide a constructor from different vector types.
Remove the empty destructor declaration, the default is already provided.
Make Vector3 a struct. There's no need to ide anything. Make m_x, m_y and m_z public instead of providing accessors.
The <, <=, >=, > operators don't make sense for vectors.
Make all binary operators, except =, +=, etc. non-member to avoid unpleasant suprises.
You've missed std:: before sqrt.
In your normalize function check for magnitude == 0 or near 0.
Make cross, dot, normalize etc. non-member.

The same applies for Vector2.

Edit: The default constructor already provides what's needed.

User Rating: 1055 | Rate This User

Report this Post to a Moderator | Link

zeeli Member since: 10/30/2005

Posted - 4/3/2009 5:56:26 AM

Quote:
Original post by rozz666

Make all binary operators, except =, +=, etc. non-member to avoid unpleasant suprises.

What would those be?

User Rating: 1019 | Rate This User

Report this Post to a Moderator | Link

rozz666 Member since: 5/27/2007 From: Lodz, Poland

Posted - 4/3/2009 6:19:46 AM

Quote:
Original post by zeeli

Quote:
Original post by rozz666

Make all binary operators, except =, +=, etc. non-member to avoid unpleasant suprises.

What would those be?

Vector3<int> a;
Vector3<float> b;
Vector3<float> c;

c = a + b;

What's the type of a + b?
If + is a member function, it's Vector3<int>.
You have to ways to handle this.
1) Implement type promotion for your vectors.
2) Make + non-member. a + b would fail to compile.

User Rating: 1055 | Rate This User

Report this Post to a Moderator | Link

rozz666 Member since: 5/27/2007 From: Lodz, Poland

Posted - 4/3/2009 6:24:19 AM

The other problem may be:

Vector3<float> a(1.5, 1.5, 1.5);
Vector3<int> b(1, 1, 1);

a == b -> false
b == a -> true

User Rating: 1055 | Rate This User

Report this Post to a Moderator | Link

rozz666 Member since: 5/27/2007 From: Lodz, Poland

Posted - 4/3/2009 6:35:48 AM

Some reading...

User Rating: 1055 | Rate This User

Report this Post to a Moderator | Link

jyk GDNet+ Member since: 10/23/2003

Posted - 4/3/2009 12:28:09 PM

Quote:
The <, <=, >=, > operators don't make sense for vectors.

That isn't necessarily true (IMO). The < operator (implemented as a lexicographical comparison) can be useful for storing vectors in associative containers, among other things.

Quote:
Make Vector3 a struct. There's no need to ide anything. Make m_x, m_y and m_z public instead of providing accessors.

@The OP: Making the member data public is a valid choice, but be aware that it's not the only 'right way' to do it - keeping the data private is valid as well. (I did notice though that you don't provide any way to set the values of individual elements.)

Here are a few more observations (some of these may have already been pointed out):

1. It looks like some of your assignment operators (e.g. +=) return a reference to a temporary. What you want is return *this.

2. You'll probably want a 'squared length' function as well as 'length'. Implement the squared length function as return dot(*this, *this), and the length (or magnitude) function as return std::sqrt(squared_length()); this will reduce code duplication.

3. Your != conditional should use the logical operator ||, not &&.

4. Expressions of the form scalar * vector are not currently possible. You'll need to add a binary non-member operator to fix this (and, as has already been mentioned, the other binary operators in your classes should really be made non-member as well).

5. Functions defined within the class body are implicitly marked inline, so you don't need the inline keyword.

6. What's the 't' in (e.g.) m_tX for?

[ Configurable Math Library ]

User Rating: 1986 | Rate This User

Report this Post to a Moderator | Link

zeeli Member since: 10/30/2005

Posted - 4/3/2009 4:30:15 PM

Quote:
Original post by jyk

Quote:
The <, <=, >=, > operators don't make sense for vectors.
That isn't necessarily true (IMO). The < operator (implemented as a lexicographical comparison) can be useful for storing vectors in associative containers, among other things.
Quote:
Make Vector3 a struct. There's no need to ide anything. Make m_x, m_y and m_z public instead of providing accessors.
@The OP: Making the member data public is a valid choice, but be aware that it's not the only 'right way' to do it - keeping the data private is valid as well. (I did notice though that you don't provide any way to set the values of individual elements.)

Here are a few more observations (some of these may have already been pointed out):

1. It looks like some of your assignment operators (e.g. +=) return a reference to a temporary. What you want is return *this.

2. You'll probably want a 'squared length' function as well as 'length'. Implement the squared length function as return dot(*this, *this), and the length (or magnitude) function as return std::sqrt(squared_length()); this will reduce code duplication.

3. Your != conditional should use the logical operator ||, not &&.

4. Expressions of the form scalar * vector are not currently possible. You'll need to add a binary non-member operator to fix this (and, as has already been mentioned, the other binary operators in your classes should really be made non-member as well).

5. Functions defined within the class body are implicitly marked inline, so you don't need the inline keyword.

6. What's the 't' in (e.g.) m_tX for?

Thank you for your time mate! The t is for variable type T

User Rating: 1019 | Rate This User

Report this Post to a Moderator | Link

jyk GDNet+ Member since: 10/23/2003

Posted - 4/3/2009 7:37:25 PM

Quote:
The t is for variable type T

I suggest dropping the 't', as it just clutters up the code without adding any useful information (IMO). For further arguments for and against this type of notation, Google or search the forum archives for 'hungarian notation'.

User Rating: 1986 | Rate This User

Report this Post to a Moderator | Link

All times are ET (US)

Last Thread Next Thread

Forum Rules:

You may not post new threads
You may not post replies
You may not edit your posts
You may not use HTML in your posts

Jump To: