Looking for more knowledge on how to make my C++ Vector2 class better

Question

This is my first attempt ever to build a Vector2 class. I scoured the net for anything that might make this class more efficient but know i'm to the point where i'm ready to share. This way I can get advice on how I can improve it further. I wanted to do this before I make my Vector3 and Vector4 class.

Vector2.h

//VECTOR2 H
#ifndef VECTOR2_H
#define VECTOR2_H

//INCLUDES
#include <math.h>

//DEFINE TYPES
typedef float float32;

//VECTOR2 CLASS
class Vector2
{
public:
//MEMBERS
float32 x;
float32 y;

//CONSTRUCTORS
Vector2(void);
Vector2(float32 xValue, float32 yValue);
Vector2(const Vector2 & v);
Vector2(const Vector2 * v);

//DECONSTRUCTOR
~Vector2(void);

//METHODS
void Set(float32 xValue, float32 yValue);

float32 Length() const;
float32 LengthSquared() const;
float32 Distance(const Vector2 & v) const;
float32 DistanceSquared(const Vector2 & v) const;
float32 Dot(const Vector2 & v) const;
float32 Cross(const Vector2 & v) const;

Vector2 & Normal();
Vector2 & Normalize();

//ASSINGMENT AND EQUALITY OPERATIONS
inline Vector2 & Vector2::operator = (const Vector2 & v) { x = v.x; y = v.y; return *this; }
inline Vector2 & Vector2::operator = (const float32 & f) { x = f; y = f; return *this; }
inline Vector2 & Vector2::operator - (void) { x = -x; y = -y; return *this; }
inline bool Vector2::operator == (const Vector2 & v) const { return (x == v.x) && (y == v.y); }
inline bool Vector2::operator != (const Vector2 & v) const { return (x != v.x) || (y != v.y); }

//VECTOR2 TO VECTOR2 OPERATIONS
inline const Vector2 Vector2::operator + (const Vector2 & v) const { return Vector2(x + v.x, y + v.y); }
inline const Vector2 Vector2::operator - (const Vector2 & v) const { return Vector2(x - v.x, y - v.y); }
inline const Vector2 Vector2::operator * (const Vector2 & v) const { return Vector2(x * v.x, y * v.y); }
inline const Vector2 Vector2::operator / (const Vector2 & v) const { return Vector2(x / v.x, y / v.y); }

//VECTOR2 TO THIS OPERATIONS
inline Vector2 & Vector2::operator += (const Vector2 & v) { x += v.x; y += v.y; return *this; }
inline Vector2 & Vector2::operator -= (const Vector2 & v) { x -= v.x; y -= v.y; return *this; }
inline Vector2 & Vector2::operator *= (const Vector2 & v) { x *= v.x; y *= v.y; return *this; }
inline Vector2 & Vector2::operator /= (const Vector2 & v) { x /= v.x; y /= v.y; return *this; }

//SCALER TO VECTOR2 OPERATIONS
inline const Vector2 Vector2::operator + (float32 v) const { return Vector2(x + v, y + v); }
inline const Vector2 Vector2::operator - (float32 v) const { return Vector2(x - v, y - v); }
inline const Vector2 Vector2::operator * (float32 v) const { return Vector2(x * v, y * v); }
inline const Vector2 Vector2::operator / (float32 v) const { return Vector2(x / v, y / v); }

//SCALER TO THIS OPERATIONS
inline Vector2 & Vector2::operator += (float32 v) { x += v; y += v; return *this; }
inline Vector2 & Vector2::operator -= (float32 v) { x -= v; y -= v; return *this; }
inline Vector2 & Vector2::operator *= (float32 v) { x *= v; y *= v; return *this; }
inline Vector2 & Vector2::operator /= (float32 v) { x /= v; y /= v; return *this; }
};

#endif
//ENDFILE

Vector2.cpp

//VECTOR2 CPP
#include "Vector2.h"

//CONSTRUCTORS
Vector2::Vector2(void) : x(0), y(0) { }
Vector2::Vector2(float32 xValue, float32 yValue) : x(xValue), y(yValue) { }
Vector2::Vector2(const Vector2 & v) : x(v.x), y(v.y) { }
Vector2::Vector2(const Vector2 * v) : x(v->x), y(v->y) { }

//DECONSTRUCTOR
Vector2::~Vector2(void) { }

//METHODS
void Vector2::Set(float32 xValue, float32 yValue) { x = xValue; y = yValue; }

float32 Vector2::Length() const { return sqrt(x * x + y * y); }
float32 Vector2::LengthSquared() const { return x * x + y * y; }
float32 Vector2::Distance(const Vector2 & v) const { return sqrt(((x - v.x) * (x -     v.x)) + ((y - v.y) * (y - v.y))); }
float32 Vector2::DistanceSquared(const Vector2 & v) const { return ((x - v.x) * (x -     v.x)) + ((y - v.y) * (y - v.y)); }
float32 Vector2::Dot(const Vector2 & v) const { return x * v.x + y * v.y; }
float32 Vector2::Cross(const Vector2 & v) const { return x * v.y + y * v.x; }

Vector2 & Vector2::Normal() { Set(-y, x); return *this; }
Vector2 & Vector2::Normalize()
{
if(Length() != 0)
{
    float32 length = LengthSquared();
    x /= length; y /= length;
    return *this;
}

x = y = 0;
return *this;
}

//ENDFILE

Thanks in advanced to anyone that helps :)

First Edit:

Changed Cross to OrthoVector

Vector2 & Vector2::Ortho() { Set(-y, x); return *this; }

Changed Normal Code to actually get the Normal of the Vector

Vector2 & Vector2::Normal() { float32 len = Length(); Set(x / len, y / len); return *this; }

Second Edit:

Changed the normal code again to check for division by zero

Vector2 & Vector2::Normal()
{
if(Length() != 0)
{
    float32 len = Length();
    x /= len; y /= len;
    return *this;
}

x = y = 0;
return *this;
}

Got rid of my Pointer Constructor and my Empty Deconstructor as well.

Answer 1

Vector is a relatively common name (as is vector 2/3 etc). So you may need to make your include guards a bit more unique. I always put my stuff into my own namespace (which happens to match a domain I own to make things unique). I then include the namespace as part of the include guard. Alternatively you can generate a GUID that will also make sure it is unique.

//VECTOR2 H
#ifndef VECTOR2_H
#define VECTOR2_H

Mine would look like:

#ifndef BOBS_DOMAIN_VECTOR_2_H
#define BOBS_DOMAIN_VECTOR_2_H

namespace BobsDomain {

Is it valid to create a Vector 2 with zero values? If so does that mean x/y default to 0.0 in which case the following 2

Vector2(void);
Vector2(float32 xValue, float32 yValue);

Could be redefined as (Though then it is possible to create a Vector2 with a single value (which may not be desirable).

Vector2(float32 xValue = 0.0, float32 yValue = 0.0);

Does your default copy constructor do anything special?

Vector2(const Vector2 & v);

If not then I would let the compiler generated version by the one you use.

Not sure you want to be able to construct from a pointer. Very few classes allow this (apart from samart pointers). Does this mean you are taking ownership of the pointer or you will just make a copy of the vector.

Vector2(const Vector2 * v);

I would drop this constructor.
Then the following code

Vector2* data = /* Get Vector2 */;
Vector2  copy(data);

Has to be modified like this (not a major change).

Vector2* data =           /* Get Vector2 */;
Vector2  copy(*data);     // Notice the extra star.
                          // But this is not a big cost and simplifies the interface
                          // considerably as we do not need to wory about ownership
                          // semantics.

Don't put the void when you have an empty parameter list.
Also if the destructor does not do anything then use the compiler generated version.

~Vector2(void);

Why do you have set method when the members are public.

void Set(float32 xValue, float32 yValue);

All operators that have an assignment version are easier to define if you define them in terms of the assignment operator. It keeps the meaning consistent.

What I mean is: operator+ is easy to define in terms of operator+=

Vector2  const operator+ (Vector2 const& rhs) const {Vector2 result(*this); return result += rhs;}
Vector2&       operator+=(Vector2 const& rhs)       {x += v.x; y += v.y; return *this;}

Since we are defining all the other operators you may want to define the comparison operator.

bool operator<(Vector const& rhs)      {return (x < rhs.x) || ((x == rhs.x) && (y < rhs.y));}

Now you can use the Vector2 as the key in a std::map.

Answer 2

Cross product is meaningless in the context that you have created it in for 2d vectors. If you are going for the orthogonal vector, it is simply "(x, y) -> (y, -x)".

Your "Normal" function is actually returning the orthogonal vector. "Normal" is vectorland means "Vector of unit length in the same direction".

To calculate the real normal, it is

  ai + bj         double mag = sqrt((a*a) + (b*b));
-----------  or   Vector2 normal(a / mag, b / mag);
| ai + bj |

Answer 3

One small thing, in places where you would use the Pythagorean formula, instead of this:

C = sqrt(A*A,B*B)

You can use this:

C = hypot(A,B)

Answer 4

One fundamental property of vectors you haven't represented: a scalar times a vector is a vector.

I wouldn't actually implement your vector * vector and vector / vector operations at all ... those are not commonly useful.

Actually there is a meaningful cross product in two dimensions, but the return value is a scalar, not a vector.

float32 Cross(const Vector2& a, const Vector2& b) {
    return a.x * b.y - a.y * b.x;
}

This brings up another point: I prefer free functions to member functions when the meaning is roughly symmetric in two variables, like Dot, Cross, or even operator+. Besides aesthetics, there's the fact that operator+(const X& a, const Y& b) can use implicit conversions on both operands but X::operator+(const Y& b) const can only use implicit conversions on b.

Answer 5

Provide alternatives to methods returning copies.

On such classes, I usually defines a setAdd and setSub method that fill the current vector with the result of an addition (resp subtraction).

void setAdd(const Vector2& v1, const Vector2& v2)
{
    x = v1.x + v2.x;
    y = v1.y + v2.y;
}

using this kind of method in place of operator+ spare a Vector2 copy. In certain cases this as a visible impact on code performance (see Havok Physics math lib).

Handle more use cases in normalize

When normalizing a vector you often do further computation using its previous norm, and you often need to set it to a desired norm. It's easy to modify the Normalize method for those use cases.

float32 Vector2::Normalize(float32 desiredNorm = 1.f)
{
    float32 len = Length();
    if(len < EPSILON)
    {
         x = y = 0.f;
         return 0.f;
    }
    else
    {
        float32 mult = desiredNorm/len;
        x *= mult; y *= mult;
        return len;
    }
 }

NB. I have also make the following changes:

• Only one computation of Length(), square root computation are expensive.
• Use a less-than-epsilon instead of a different-to-0 which can lead to various problem because of floating point precision, the value of EPSILON should be small (0.00001 for example).

Provide a truncate method

I've found this very usefull to define a truncate method that enforce an upper-bound on the vector's norm.

void Truncate(float32 upperBound)
{
    float32 sqrLen = SqrLength();
    if(sqrLen > upperBound * upperBound)
    {
        float32 mult = upperBound/sqrt(sqrLen);
        x *= mult; y *= mult;
    }
}

NB.

• You should probably assert that upperBound is positive.
• Again I'm trying to minimize the number of square root computed.