Opinions on my 3D vector class

As for Speed: For all types of Vector/Matrix-Classes it's always usefull to use SSE-Instructions to speed things up. This will improve all your SIMD-Type Operations (Adding, Multiplying...) by quite a bit.

Whats the purpose of your accessors/mutators for the vector components? If you aren't doing any kind of checking or filtering of arguments, why have them.

Here's the code with a few comments added:

class __declspec(dllexport) Vector3
{

public:

// IIRC, any function defined in the class body is implicitly
// marked 'inline'.
inline Vector3()
{
/* blank for efficient allocation in batches*/
}

inline Vector3(float x, float y, float z) : x(x), y(y), z(z)
{
}


/****************************
*
* Operations
*
*****************************/



//addition

// Why return a constant reference here? (Also, why named functions
// rather than e.g. operator+=()? It looks like you have the operator
// versions later, so I'd just drop these.)
inline const Vector3& add(const Vector3 &other)
{
this->x += other.getX();
this->y += other.getY();
this->z += other.getZ();

return *this;
}

// Binary functions should (arguably) be implemented as non-member
// functions rather than member functions.
inline static Vector3 add(const Vector3 &left, const Vector3 &right)
{
return Vector3(left.getX() + right.getX(), left.getY() + right.getY(), left.getZ() + right.getZ());
}

inline Vector3 operator +(const Vector3 &right) const
{
return Vector3(this->x + right.getX(), this->y + right.getY(), this->z + right.getZ());
}

inline const Vector3& operator +=(const Vector3 &right)
{
this->x += right.getX();
this->y += right.getY();
this->z += right.getZ();

return *this;
}

//subtraction

inline const Vector3& subtract(const Vector3 &other)
{
this->x -= other.getX();
this->y -= other.getY();
this->z -= other.getZ();

return *this;
}

inline static Vector3 subtract(const Vector3 &left, const Vector3 &right)
{
return Vector3(left.x - right.getX(), left.y - right.getY(),left.z - right.getZ());
}

inline Vector3 operator-(const Vector3 &right) const
{
return Vector3(this->x - right.getX(), this->y - right.getY(), this->z - right.getZ());
}

inline const Vector3& operator -=(const Vector3 &right)
{
this->x -= right.getX();
this->y -= right.getY();
this->z -= right.getZ();

return *this;
}


/* comparison */

inline bool operator==(const Vector3 &right) const
{
return ( (this->x == right.getX()) && (this->y == right.getY()) && (this->z == right.getZ()) );
}



// scalar multiplication
inline const Vector3& multiply(float scalar)
{
this->x *= scalar;
this->y *= scalar;
this->z *= scalar;

return *this;
}

inline static Vector3 multiply(const Vector3 &vector, float scalar)
{
return Vector3(vector.x * scalar, vector.y * scalar, vector.z * scalar);
}

inline Vector3 operator *(float scalar) const
{
return Vector3(this->x * scalar, this->y * scalar, this-> z * scalar);

}

inline friend Vector3 operator*(float scalar, const Vector3& vector)
{
return Vector3(vector.x * scalar, vector.y * scalar, vector.z * scalar);
}

inline const Vector3& operator *=(float scalar)
{
this->x *= scalar;
this->y *= scalar;
this->z *= scalar;

return *this;
}


//Vector3 multiply(const Vector3 &other);
//static Vector3 multiplty(const Vector3 &left, const Vector3 &right);

// dot products



inline float dotProduct(const Vector3 &other) const
{
return (this->x * other.getX()) + (this->y * other.getY()) + (this->z * other.getZ());
}

// I agree with the previous poster that the accessors probably
// aren't doing much for you here.
inline static float dotProduct(const Vector3 &left, const Vector3 &right)
{
return (left.getX() * right.getX()) + (left.getY() * right.getY()) + (left.getZ() * right.getZ());
}

//cross product
inline static Vector3 crossProduct(const Vector3 &left, const Vector3 &right)
{

// [ i j k ]
// [ l.x l.y l.z]
// [ r.x r.y r.z]

return Vector3((left.getY() * right.getZ()) - (right.getY() * left.getZ()),
(left.getX() * right.getZ()) - (right.getX() * left.getZ()),
(left.getX() * right.getY()) - (right.getX() * left.getY()));

}


inline const Vector3& crossProduct(const Vector3 &other)
{
this->x = (this->y * other.getZ()) + (other.getY() * this->z);
this->y = (this->x * other.getZ()) + (other.getX() * this->z);
this->z = (this->x * other.getY()) + (other.getX() * this->y);

return *this;
}




inline float magnitude() const
{
return sqrt( this->magnitudeSquared() );

}

// To avoid code duplication, implement this as 'this dot this'.
inline float magnitudeSquared() const
{
return (this->x * this->x) + (this->y * this->y) + (this->z * this->z);

}

// A vector is unlikely to be exactly the zero vector under
// normal circumstances, and a vector doesn't have to be the
// zero vector for normalization to fail, so I don't think the
// check for 0 is of much value here. Also, setting the vector
// to zero is sort of an arbitrary choice, and is essentially a
// silent failure that could cause subtle bugs to go unnoticed.
// Better, IMO, to alert the caller when the magnitude is below
// a specified epsilon.
inline Vector3 normalize()
{
if(this->x != 0 || this->y != 0 || this->z != 0)
{
float mag = 1.0f/this->magnitude();
this->x *= mag;
this->y *= mag;
this->z *= mag;
}

else
this->setValues(0,0,0);

return *this;
}

inline static Vector3 normalize(const Vector3 &vector)
{
Vector3 temp(0,0,0);
if(vector.getX() != 0 || vector.getY() != 0 || vector.getZ() != 0)
{
float mag = 1.0f/vector.magnitude();
temp.setValues(vector.getZ() * mag,vector.getY() * mag,vector.getZ() * mag);
}

return temp;
}


inline Vector3 operator -() const
{
return Vector3(-this->x, -this->y, -this->z);
}

inline float getX() const
{
return this->x;
}

inline float getY() const
{
return this->y;
}

inline float getZ() const
{
return this->z;
}

inline void setX(float newX)
{
this->x = newX;
}

inline void setY(float newY)
{
this->y = newY;
}


inline void setZ(float newZ)
{
this->z = newZ;
}


inline void Vector3::setValues(float x, float y, float z)
{
this->x = x;
this->y = y;
this->z = z;

}

float x,y,z;

};

As for Speed: For all types of Vector/Matrix-Classes it's always usefull to use SSE-Instructions to speed things up. This will improve all your SIMD-Type Operations (Adding, Multiplying...) by quite a bit.

I was thinking of implementing them using SSE, but then I read this and reconsidered.

Whats the purpose of your accessors/mutators for the vector components? If you aren't doing any kind of checking or filtering of arguments, why have them.

Yeah they were left over from when I was trying to make everything really tidy interface wise. I left them in since they weren't causing any harm, but I've been meaning to get rid of them.

// IIRC, any function defined in the class body is implicitly
// marked 'inline'.

I'd like to make sure though, I try my best to not rely on default behavior.

// Why return a constant reference here? (Also, why named functions
// rather than e.g. operator+=()? It looks like you have the operator
// versions later, so I'd just drop these.)

I figured returning a reference would be more efficient than a copy?

// Binary functions should (arguably) be implemented as non-member
// functions rather than member functions.

Yeah I started adding friend functions to allow certain operands on the left hand side.

// A vector is unlikely to be exactly the zero vector under
// normal circumstances, and a vector doesn't have to be the
// zero vector for normalization to fail, so I don't think the
// check for 0 is of much value here. Also, setting the vector
// to zero is sort of an arbitrary choice, and is essentially a
// silent failure that could cause subtle bugs to go unnoticed.
// Better, IMO, to alert the caller when the magnitude is below
// a specified epsilon.

Now this is interesting. When else can normalization fail? And do you have a suggestion as an alternate way to signal that it has failed? It's kind of ugly, but it's the best way I could think of.

I'd like to make sure though, I try my best to not rely on default behavior.[/quote]
I'd question that approach. First, it might be worth checking if the standard has anything to say about this (I can't remember off the top of my head), because if the standard does specify that functions defined in the class body are implicitly marked inline (which seems to be confirmed here), then there's no reason to use the 'inline' keyword here.

Regardless, 'inline' is only a suggestion to the compiler. In any case, all those 'inlines' in the class body are just noise, IMO, and should probably be removed (again, IMO).

I figured returning a reference would be more efficient than a copy?[/quote]
My question was more in reference to the 'constant' part. (The convention is to return a non-constant reference from such functions to facilitate chaining.)

Now this is interesting. When else can normalization fail?[/quote]
Generally speaking, division by very small numbers can be problematic where floating-point representations are involved. (However, in this case the numerators would be of similar magnitude, so the problem may not be as pronounced.)

And do you have a suggestion as an alternate way to signal that it has failed? It's kind of ugly, but it's the best way I could think of.[/quote]
I don't know that there's one right answer as far as that goes. One solution however would be to implement a 'try normalize' function that writes the result to an output argument and returns a Boolean value indicating whether normalization succeeded. (Or, if the expectation is that all input vectors will be 'normalizable' in practice, you could just use an assertion.)

If you try to normalize the vector (smallest_possible_positive_float_value,0,0), it will fail.

Don't return by const reference, just return by reference.

Accessors (GetX, etc) are pointless here, since you're not maintaining any invariants. just make the stuff public.

The static methods (Add, etc.) are of dubious merit, at best.

[color=#880000]// Binary functions should (arguably) be implemented as non-member
[color=#880000]// functions rather than member functions.[/quote]


I wouldn't say this is arguable. It increases encapsulation -- however, to do so they must be non-friend in addition to non-member. 'friend' actually creates a stronger relationship than even inheritence, its basically only one step down from making everything public.


You should only need to implement the self-modifying operators (*=, /=, +=, -=) as member functions. With this, their non-modifying counterparts can be implemented in terms of their self-modifying counterparts as free functions within the same namespace -- They are every bit as much a part of the class as non-modifying member functions as you have now (Due to Koenig lookup), but better for encapsulation. It also enables the Return Value Optimization.

As a member function:
Vector3& operator+=(Vector3 const & rhs)
{
this.x += rhs.x;
this.y += rhs.y;
this.z += rhs.z;

return *this;
}

As a non-member, non-friend function in the same namespace:
Vector3 operator+(Vector3 lhs, Vector3 const &rhs) // Note that 'lhs' is passed by value, this is intentional.
{
return lhs += rhs;
}

If you want to enable SSE, you'll want to align your data and wrap it in a union with __m128 or a similar construct.

Also, SSE shouldn't be used for everything -- a simple vector add might actually be slower in SSE, due to the need to set up for SSE first. One place you will want to use it is when transforming a batch of vectors through a matrix -- SSE2, IIRC, can optimize this from a ~100 cycle per-call cost, down to 20 cycles per vector, amortized over any reasonable number of vectors, and down to 17 cycles for the common cases whereing the homogenous W component is set to 0 or 1 (vectors or points, respectively).

Vector3 operator+(Vector3 lhs, Vector3 const &rhs) // Note that 'lhs' is passed by value, this is intentional.
{
return lhs += rhs;
}

Why? To save a temporary? Just asking

Don't return by const reference, just return by reference.

Accessors (GetX, etc) are pointless here, since you're not maintaining any invariants. just make the stuff public.

The static methods (Add, etc.) are of dubious merit, at best.

[color="#880000"]// Binary functions should (arguably) be implemented as non-member
[color="#880000"]// functions rather than member functions.

I wouldn't say this is arguable. It increases encapsulation -- however, to do so they must be non-friend in addition to non-member. 'friend' actually creates a stronger relationship than even inheritence, its basically only one step down from making everything public.


You should only need to implement the self-modifying operators (*=, /=, +=, -=) as member functions. With this, their non-modifying counterparts can be implemented in terms of their self-modifying counterparts as free functions within the same namespace -- They are every bit as much a part of the class as non-modifying member functions as you have now (Due to Koenig lookup), but better for encapsulation. It also enables the Return Value Optimization.

As a member function:
Vector3& operator+=(Vector3 const & rhs)
{
this.x += rhs.x;
this.y += rhs.y;
this.z += rhs.z;

return *this;
}

As a non-member, non-friend function in the same namespace:
Vector3 operator+(Vector3 lhs, Vector3 const &rhs) // Note that 'lhs' is passed by value, this is intentional.
{
return lhs += rhs;
}

If you want to enable SSE, you'll want to align your data and wrap it in a union with __m128 or a similar construct.

Also, SSE shouldn't be used for everything -- a simple vector add might actually be slower in SSE, due to the need to set up for SSE first. One place you will want to use it is when transforming a batch of vectors through a matrix -- SSE2, IIRC, can optimize this from a ~100 cycle per-call cost, down to 20 cycles per vector, amortized over any reasonable number of vectors, and down to 17 cycles for the common cases whereing the homogenous W component is set to 0 or 1 (vectors or points, respectively).
[/quote]



I'll keep this all in mind. I've got a question, if I were to use SSE, how would I access say the y component of the 128-bit variable? Type casting seems like a bad idea, but I see no alternatives.

[quote name='Ravyne' timestamp='1297466170' post='4773105']
Vector3 operator+(Vector3 lhs, Vector3 const &rhs) // Note that 'lhs' is passed by value, this is intentional.
{
return lhs += rhs;
}

Why? To save a temporary? Just asking
[/quote]

Well, first and foremost, by using this pattern of implementing '+' in terms of '+=' (and similarly for the other operators) you reduce the amount of redundant code, because the act of adding a vector is now only defined in one place. It also has the benefit (and this is where the increased encapsulation I mentioned before comes in) that it reduces the footprint of functions which have direct access to your class internals (since I recommend everything be public because there are no invariants to maintain, it isn't increasing encapsulation in this case specifically, but it does whenever you have protected or private member variables or functions).

As to the operation of the above quoted code, yes, the gist is that you can save some temporaries by doing things in this way -- a good compiler doesn't always need to see this pattern to recognize when the return value optimization can be applied, but the above is generally recognized as most-likely to cause RVO to be induced. Plus, we get all the benefits mentioned above. Specifically, what happens above is that 'lhs' just gets re-used as the return value object, which would otherwise be constructed (and possibly copied, IIRC) at the point of return typically. The form above just makes the pattern very explicit and leaves no wiggle room for the compiler to get it wrong.

But, the important thing is that its not just better code, and not just (potentially) faster code -- its both of these things at once; a rare opportunity that should be grabbed when presented.

Thanks for the in-depth explanation. Seems I'll have to edit my vector class

cheers