Please verify my vector Class Is Correct So Far

Math and Physics Programming

Started by Darobat January 01, 2005 11:30 AM

6 comments, last by Zakwayda 19 years, 3 months ago

100

Author

January 01, 2005 11:30 AM

Greetings I am working on a program that requires my vector class, but so far it doesn't work. I'm wondering if it is a problem with the vector class's functions, how I use them or something else. Here is the vector class:

// vector.h
#ifndef _VECTOR_H_
#define _VECTOR_H_

#include <cmath>

class CVector3D
{
public:
	CVector3D();
	CVector3D(float x, float y, float z);
	float x, y, z;
	float length;

	CVector3D &operator-(CVector3D &ls);
	CVector3D &operator=(CVector3D &ls);

	void Set(float x, float y, float z);	// Set the value of the vector
	void CalculateLength();
	void Normalize();
	CVector3D CrossProudct(CVector3D v);
};

#endif

// vector.cpp
#include "vector.h"

CVector3D::CVector3D()
{
}

CVector3D::CVector3D(float _x, float _y, float _z)
{
	Set(_x, _y, _z);
}

CVector3D &CVector3D::operator-(CVector3D &rs)
{
	CVector3D *returnVal = new CVector3D;
	CVector3D &result = *returnVal;
	result.x = x - rs.x;
	result.y = y - rs.y;
	result.z = z - rs.z;
	return result;
}

CVector3D &CVector3D::operator=(CVector3D& rs)
{
	x = rs.x;
	y = rs.y;
	z = rs.z;
	CalculateLength();
	return *this;
}

void CVector3D::Set(float _x, float _y, float _z)
{
	x = _x;
	y = _y;
	z = _z;
	CalculateLength();
}

void CVector3D::CalculateLength()
{
	length = sqrt(pow(x, 2) + pow(y, 2) + pow(z, 2));
}

void CVector3D::Normalize()
{
	CalculateLength();
	if(length > 0.0f)
	{
		x /= length;
		y /= length;
		z /= length;
	}
}

// Finds the cross product of u X v
CVector3D CVector3D::CrossProudct(CVector3D v)
{
	CVector3D result;
	result.x = y * v.z - z * v.y;
	result.y = z * v.x - x * v.z;
	result.z = x * v.y - y * v.x;
	CalculateLength();
	return result;
}

Thanks

--------------------C++ Home - Check it out!Lol... - Amazing video

Webbster

122

January 01, 2005 11:47 AM

Looks fine as far as I can tell. You may want to consider inlining certain functions as it will ever so slightly improve operating speed. Especially in a vector class which is likely to be used a lot in a 3d application.

johnb

352

January 01, 2005 12:12 PM

Using new like that inside e.g. your operator- is a bad idea: it means the calling code needs to take care of deleting the object when done with it. Not only is this extra work but it means code like this

result = a + (b - c);

won't work.

Your "CrossProudct" shows how it should be done, i.e. return by value a variable you create in the routine. Returning a pointer may seem more efficient but you need to create storage at some point and a good compiler will take care of minimising unnecessary copying.

Having a length field which you update is an interesting idea but it should be used consistently, otherwise again calling code needs to keep track of it. E.g. you should update it during Set, subtraction and to 1 during normalisation.

As for inlining, just put the function bodies inside the class definition in the header. IMHO this is clearer and easier to read than splitting it over two source files, and it automatically inlines the functions.

John BlackburneProgrammer, The Pitbull Syndicate

OrenGL

228

January 01, 2005 12:37 PM

Operator- is a member function that should operate on the class itself. It should change the value or the class x,y,z variables and should not create a new vector. Useing "new" like you did is a really really really bad Idea. you can write it like:

const CVector3D &CVector3D::operator-(CVector3D &rs){
...
return *this;
}

why use length? It is somthing that will not be used for all vectors, this class can be used as a point in space too.

In normalize when you have the '/' operator 3 times, you are better off finding out what one_over_length is and multipliing by that three times.

Don't use Set, the x,y,z are public and you should have a copy constructor anyway.

One last thing, I find it a bit anoying that the variable names in the function definition of the constructor are (A) the same names as member variables and (B) diffrent in the body.

let us see what you come up with again when your done ;)

Don't shoot! I'm with the science team.....

Darobat

100

Author

January 01, 2005 12:55 PM

Thanks for the tips!

#ifndef _VECTOR_H_#define _VECTOR_H_#include <cmath>class CVector3D{public:	CVector3D() {};	CVector3D(float _x, float _y, float _z) : x(_x), z(_z), y(_y) {};	float x, y, z;	const CVector3D operator-(const CVector3D &rs)	{		x -= rs.x;		y -= rs.y;		z -= rs.z;		return *this;	}	CVector3D &operator=(const CVector3D &rs)	{		x = rs.x;		y = rs.y;		z = rs.z;		Length();		return *this;	}	const float Length() const {return sqrt(pow(x, 2) + pow(y, 2) + pow(z, 2));}	void Normalize()	{			if(Length() > 0.0f)		{			float ratio = 1 / Length();			x *= ratio;			y *= ratio;			z *= ratio;		}	}	const CVector3D CrossProudct(const CVector3D v) const	{		CVector3D result;		result.x = y * v.z - z * v.y;		result.y = z * v.x - x * v.z;		result.z = x * v.y - y * v.x;		return result;	}};#endif

K, It is now completly inlined and in one file. I changed the things you poined out. Tell me what you think. :)

--------------------C++ Home - Check it out!Lol... - Amazing video

Ravyne

14,306

January 01, 2005 01:18 PM

Quote:Original post by OrenGL
Operator- is a member function that should operate on the class itself. It should change the value or the class x,y,z variables and should not create a new vector. Useing "new" like you did is a really really really bad Idea. you can write it like:

const CVector3D &CVector3D::operator-(CVector3D &rs){
...
return *this;
}

NO! Operator - should return by value. Operator -= should operate on the class itself.

Quote:Original post by OrenGL
why use length? It is somthing that will not be used for all vectors, this class can be used as a point in space too.

In normalize when you have the '/' operator 3 times, you are better off finding out what one_over_length is and multipliing by that three times.

Agreed, I would simply have a length function which returns the length of the vector as needed rather than keep it at all times. You generally don't need the length that terribly often, its probably equal if not less work to calculate it when needed.

Heres the header for my vector class:

#ifndef VECTOR3_H#define VECTOR3_H#include "point3.h"class Vector3{public:	Vector3()				: X(0), Y(0), Z(0)	{ }	Vector3(float x, float y, float z)	: X(x), Y(y), Z(z)	{ }	Vector3(const Vector3&);	Vector3(const Point3&, const Point3&);	float		Dot(const Vector3&);	float		Cross(const Vector3&);	float		Len();	float		SqrLen();	Vector3		Normal();	void		Normalize();	Vector3& operator=(const Vector3&);	Vector3 operator+(const Vector3&);	Vector3 operator-(const Vector3&);	Vector3 operator*(const float);	Vector3 operator/(const float);	float X;	float Y;	float Z;};#endif //VECTOR3_H

[EDIT] come to think of it, Normalize() should be void rather than Vector3&

[Edited by - Ravyne on January 1, 2005 1:18:07 PM]

throw table_exception("(? ???)? ? ???");

johnb

352

January 01, 2005 01:19 PM

Quote:Original post by OrenGL
Operator- is a member function that should operate on the class itself. It should change the value or the class x,y,z variables and should not create a new vector.

This is describing something else, -=. You can have both. E.g. from a 3D vector library I'm working with:

_3D& operator-=(const _3D& right)			{x -= right.x; y -= right.y; z -= right.z; return *this;}_3D operator-(const _3D& right) const		{_3D tmp = *this; return tmp -= right;}

Both operators, -= and -, are defined, with - using the -= definition. Other operators, such as + and * a scalar, are similarly defined.

This also shows another feature of such libraries. Use const wherever it makes sense. This helps the compiler optimise the code and catches some errors in code using the library.

John BlackburneProgrammer, The Pitbull Syndicate

Zakwayda

2,389

January 01, 2005 01:40 PM

When I was working on my math library (well, actually I'm always working on my math library), I found it helpful to look at as many examples (good or bad) as possible. So here's an older version of my Vector3 class:

// --------------------------------------------------------------------------------------// FILE: Vector3.h// AUTHOR: Jesse Krebs// --------------------------------------------------------------------------------------#ifndef VECTOR3_H#define VECTOR3_H#include "MathUtil.h"class Vector3{public:	Vector3() {}	Vector3(const Vector3& v) {Set(v);}	Vector3(float v[]) {Set(v);}	Vector3(float x, float y, float z) {Set(x, y, z);}	void SetX(float x) {m_x = x;}	void SetY(float y) {m_y = y;}	void SetZ(float z) {m_z = z;}		void Zero() {Set(0.0f, 0.0f, 0.0f);}	void Set(const Vector3& v) {*this = v;}	void Set(float v[]) {assert(v); m_x = v[0]; m_y = v[1]; m_z = v[2];}	void Set(float x, float y, float z) {m_x = x; m_y = y; m_z = z;}		inline float operator[](int i) const;		inline bool operator==(const Vector3& v) const; // No epsilon	inline bool operator!=(const Vector3& v) const; // No epsilon	inline Vector3  operator-() const;	inline Vector3& operator=(const Vector3& v);	inline Vector3& operator+=(const Vector3& v);	inline Vector3& operator-=(const Vector3& v);	inline Vector3& operator*=(float scale);	inline Vector3& operator/=(float scale);		inline friend Vector3 operator+(const Vector3& v1, const Vector3& v2);	inline friend Vector3 operator-(const Vector3& v1, const Vector3& v2);	inline friend Vector3 operator*(float scale, const Vector3& v);	inline friend Vector3 operator*(const Vector3& v, float scale);	inline friend Vector3 operator/(const Vector3& v, float scale);		inline void Minimize(const Vector3& v);	inline void Maximize(const Vector3& v);	inline float Normalize();	inline float Length() const;	inline float LengthSquared() const;	inline float Dot(const Vector3& v) const;	inline Vector3 Cross(const Vector3& v) const;		inline Vector3 UnitCross(const Vector3& v) const;	inline Vector3 Absolute() const;	inline Vector3 Reflect(const Vector3& v) const;	private:		float m_x;	float m_y;	float m_z;};// --------------------------------------------------------------------------------------inline float Vector3::operator[](int i) const{	assert(i >= 0 && i < 3);	return (&m_x);}// --------------------------------------------------------------------------------------inline bool Vector3::operator==(const Vector3& v) const{	return m_x == v.m_x && m_y == v.m_y && m_z == v.m_z;}// --------------------------------------------------------------------------------------inline bool Vector3::operator!=(const Vector3& v) const{	return m_x != v.m_x || m_y != v.m_y || m_z != v.m_z;}// --------------------------------------------------------------------------------------inline Vector3 Vector3::operator-() const{	return Vector3(-m_x, -m_y, -m_z);}// --------------------------------------------------------------------------------------inline Vector3& Vector3::operator=(const Vector3& v){	m_x = v.m_x;	m_y = v.m_y;	m_z = v.m_z;	return *this;}// --------------------------------------------------------------------------------------inline Vector3& Vector3::operator+=(const Vector3& v){	m_x += v.m_x;	m_y += v.m_y;	m_z += v.m_z;	return *this;}// --------------------------------------------------------------------------------------inline Vector3& Vector3::operator-=(const Vector3& v){	m_x -= v.m_x;	m_y -= v.m_y;	m_z -= v.m_z;	return *this;}// --------------------------------------------------------------------------------------inline Vector3& Vector3::operator*=(float scale){	m_x *= scale;	m_y *= scale;	m_z *= scale;	return *this;}// --------------------------------------------------------------------------------------inline Vector3& Vector3::operator/=(float scale){	assert(fabsf(scale) > Math::EPSILON);	float inverse = 1.0f / scale;	m_x *= inverse;	m_y *= inverse;	m_z *= inverse;	return *this;}// --------------------------------------------------------------------------------------inline Vector3 operator+(const Vector3& v1, const Vector3& v2){	return Vector3(v1.m_x + v2.m_x, v1.m_y + v2.m_y, v1.m_z + v2.m_z);}// --------------------------------------------------------------------------------------inline Vector3 operator-(const Vector3& v1, const Vector3& v2){	return Vector3(v1.m_x - v2.m_x, v1.m_y - v2.m_y, v1.m_z - v2.m_z);}// --------------------------------------------------------------------------------------inline Vector3 operator*(float scale, const Vector3& v){	return Vector3(scale * v.m_x, scale * v.m_y, scale * v.m_z);}// --------------------------------------------------------------------------------------inline Vector3 operator*(const Vector3& v, float scale){	return Vector3(v.m_x * scale, v.m_y * scale, v.m_z * scale);}// --------------------------------------------------------------------------------------inline Vector3 operator/(const Vector3& v, float scale){	assert(fabsf(scale) > Math::EPSILON);	float inverse = 1.0f / scale;	return Vector3(v.m_x * inverse, v.m_y * inverse, v.m_z * inverse);}// --------------------------------------------------------------------------------------void Vector3::Minimize(const Vector3& v){	Set(Math::Min(m_x, v.m_x), Math::Min(m_y, v.m_y), Math::Min(m_z, v.m_z));}// --------------------------------------------------------------------------------------void Vector3::Maximize(const Vector3& v){	Set(Math::Max(m_x, v.m_x), Math::Max(m_y, v.m_y), Math::Max(m_z, v.m_z));}// --------------------------------------------------------------------------------------inline float Vector3::Normalize(){	float length = Length();	*this /= length;	return length;}// --------------------------------------------------------------------------------------inline float Vector3::Length() const{	return Math::Sqrt(m_x * m_x + m_y * m_y + m_z * m_z);}// --------------------------------------------------------------------------------------inline float Vector3::LengthSquared() const{	return m_x * m_x + m_y * m_y + m_z * m_z;}// --------------------------------------------------------------------------------------inline float Vector3::Dot(const Vector3& v) const{	return m_x * v.m_x + m_y * v.m_y + m_z * v.m_z;}// --------------------------------------------------------------------------------------inline Vector3 Vector3::Cross(const Vector3& v) const{	return Vector3(m_y * v.m_z - m_z * v.m_y,				   m_z * v.m_x - m_x * v.m_z,				   m_x * v.m_y - m_y * v.m_x);}// --------------------------------------------------------------------------------------inline Vector3 Vector3::UnitCross(const Vector3& v) const{	Vector3 cross = Cross(v);	cross.Normalize();	return cross;}// --------------------------------------------------------------------------------------inline Vector3 Vector3::Absolute() const{	return Vector3(fabsf(m_x), fabsf(m_y), fabsf(m_z));}// --------------------------------------------------------------------------------------inline Vector3 Vector3::Reflect(const Vector3& v) const{	return v - 2.0f * Dot(v) * *this;}// --------------------------------------------------------------------------------------#endif

I deleted some stuff for this post, so I may have messed something up.

Also, I don't think you have to put the 'inlines' in the class declaration, but I didn't know that at the time.

There are a few things here - public vs. private data, member vs. friend dot and cross functions - that people differ in opinion about. So remember that this is just one way of doing things.

Edit: a couple more things. I think my newer version uses member initializer lists where appropriate. Also, I've read that prefixing variables with underscores is not recommended, as it may cause conflicts with some compilers.

Please verify my vector Class Is Correct So Far

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Please verify my vector Class Is Correct So Far

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines