Jump to content
  • Advertisement
Sign in to follow this  
Splinter of Chaos

Requesting peer review of 3D Vector class.

This topic is 3792 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Since the vector class is the foundation for most of the math I do, I want to make sure it has a good, intuitive interface. This wasn't hard until I ran across a problem: I wanted to modify the vector to have a specific magnitude for one calculation, but retain its pre-calculation data. It's a paradox, I know. What I did was make the magnitude changing function return a new Vector, but to keep the ability to change the magnitude without creating a new object, I overloaded Vector=float (well, T, since the Vector class is templated). Is this good, bad, or OK (passable)? Here's the source:
template< class T >
class Vector3
{
    public:
        T x, y, z; // The compisite forces.

        // METHODS //
        Vector3 ( T X=0.0f, T Y=0.0f, T Z=0.0f )
        {
            x=X;
            y=Y;
            z=Z;
        }

        // Normal methods
        const T xyTheta() const { return atan2( y, x ); }
        const T yzTheta() const { return atan2( y, z ); }
        const T xzTheta() const { return atan2( z, x ); }

        // UNTESTED! (But no need, so...)
        const Vector3 xyTheta( T newA ) const // newA = New Angle
        {
            const T M = magnitude();

            // If sin(A) = y/M
            //    y = M * sin(A)
            // newY = M * sin(newA)
            // newX = M * cos(newA)

            return Vector3
                   (
                       M * cos( newA ),
                       M * sin( newA )
                   );
        }

        // Get the magnitude of the vector.
        const T magnitude() const  { return sqrt( x*x + y*y + z*z ); }
        const T magnitude2() const { return       x*x + y*y + z*z;   }

        /* I decided that this should NOT return itself.
           Although I WAS treating it as a get(), I found that this
           function might be more apropriet to treat like an operator.
           (An operator returns a NEW onject.)

           Although this may not be intuitive, the overloaded
           operator=(T) will take it's place. This provides me
           with easy ways to get what a vector would look like a X magnitude
           without modifying it, but also an easy way to modify it.
        */
        const Vector3 magnitude( T newMagnitude ) const
        {
            // We'll call sqrt() on mag if it's not zero.
            T mag = magnitude2();

            // Avoid a divide by zero.
            if( !mag )
                return Vector3( 0.0f, 0.0f, 0.0f );

            // Find the rate of change.
            T k = newMagnitude / sqrt( mag );

            // Change x, y, and z.
            return *this * k;
        }

        T xyMagnitude() { return sqrt( x*x + y*y ); } const
        T yzMagnitude() { return sqrt( y*y + z*z ); } const
        T xzMagnitude() { return sqrt( x*x + z*z ); } const

        T yzMagnitude2() { return y*y + z*z; } const
        T xyMagnitude2() { return x*x + y*y; } const
        T xzMagnitude2() { return x*x + z*z; } const

        /* Although magnitude() doesn't modify itself, I though it
           more intuitive that clamp WOULD modify itself.
        */
        Vector3& clampMag( T max )
        {
            // I bet an extra multiplication is cheaper than sqrt(), so...
            if( this->magnitude2() > max*max )
                *this = max;

            return *this;
        }

        Vector3& normalize()
        {
            return *this /= magnitude();
        }

        // Math and arithmetic operators:
        const Vector3 operator + ( Vector3 right ) const
        {
            return Vector3( x + right.x, y + right.y, z + right.z );
        }

        const Vector3 operator - ( Vector3 right ) const
        {
            return Vector3( x - right.x, y - right.y, z - right.z );
        }

        const Vector3 operator * ( T right ) const
        {
            return Vector3( x*right, y*right, z*right );
        }

        const Vector3 operator * ( Vector3 right ) const
        {
            return Vector3( x*right.x, y*right.y, z*right.z );
        }

        const Vector3 operator / ( T right ) const
        {
            return Vector3( x/right, y/right, z/right );
        }

        // Unary operator.
        const Vector3 operator - () const
        {
            return Vector3( -x, -y, -z );
        }

        // Assignment operators.
        const Vector3& operator = ( PolarVector right )
        {
            x = right.x();
            y = right.y();
            return *this;
        }

        /* This sets the magnitude to the value of right.
           It might seem a little unintuitive, but I deemed it
           the more apropriet way to do this compaired with mag( newMag ).
        */
        const Vector3& operator = ( const T newMag )
        {
            T mag = magnitude2();

            // Avoid a divide by zero.
            if( !mag )
                return *this;

            // Find the rate of change.
            T k = newMag / sqrt( mag );

            // Change x, y, and z.
            return (*this) *= k;
        }

        Vector3& operator += ( Vector3 right )
        {
            x += right.x;
            y += right.y;
            z += right.z;
            return *this;
        }

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

        Vector3& operator *= ( T right )
        {
            x *= right;
            y *= right;
            z *= right;
            return *this;
        }

        Vector3& operator /= ( T right )
        {
            x /= right;
            y /= right;
            z /= right;
            return *this;
        }

        // IMPLICITE CONVERSIONS //
        /* True if either x, y, or z is non-zero.

           Good for inside if statements. I know implicite bool convertion
           might not be the BEST solution, but there's not GREAT solution
           I know of.
        */
        operator const bool() const
        {
            return x || y || z;
        }
};




v2
template< class T >
class Vector3
{

    public:
        T x, y, z; // The compisite forces.

        Vector3( const T X, const T Y, const T Z )
            : x(X), y(Y), z(Z)
        {
        }

        Vector3()
            : x(0), y(0), z(0)
        {
        }

        // Theta on different axies.
        T xyTheta() const { return atan2( y, x ); }
        T yzTheta() const { return atan2( y, z ); }
        T xzTheta() const { return atan2( z, x ); }

        // UNTESTED! (But no need, so...)
        Vector3 xyTheta( const T newA ) const // newA = New Angle
        {
            const T M = magnitude();

            // If sin(A) = y/M
            //    y = M * sin(A)
            // newY = M * sin(newA)
            // newX = M * cos(newA)

            return Vector3
                   (
                       M * cos( newA ),
                       M * sin( newA ),
                       0.0f
                   );
        }

        /* Get the magnitude of the vector. */
        const T magnitudeSqr() const { return x*x + y*y + z*z; }
        const T magnitude() const { return sqrt( magnitudeSqr() ); }

        T yzMagnitudeSqr() { return y*y + z*z; } const
        T xyMagnitudeSqr() { return x*x + y*y; } const
        T xzMagnitudeSqr() { return x*x + z*z; } const

        T xyMagnitude() const { return sqrt( yzMagnitudeSqr() ); }
        T yzMagnitude() const { return sqrt( xyMagnitudeSqr() ); }
        T xzMagnitude() const { return sqrt( xzMagnitudeSqr() ); }

        Vector3& normalize()
        {
            return *this /= magnitude();
        }

        /* This sets the magnitude to the value of right. */
        Vector3& magnitude( const T newMag )
        {
            T mag = magnitudeSqr();

            // Avoid a divide by zero.
            if( !mag )
                return *this;

            // Find the rate of change.
            T k = newMag / sqrt( mag );

            // Change x, y, and z.
            return (*this) *= k;
        }

        Vector3& clampMag( const T max )
        {
            // I bet an extra multiplication is cheaper than sqrt(), so...
            if( this->magnitudeSqr() > max*max )
                this->magnitude( max );

            return *this;
        }

        // Unary operator.
        Vector3 operator - () const
        {
            return Vector3( -x, -y, -z );
        }

        // Assignment operators.
        Vector3& operator = ( PolarVector right )
        {
            x = right.x();
            y = right.y();
            return *this;
        }

        template< class Y >
        Vector3& operator = ( Vector3<Y> right )
        {
            x = right.x;
            y = right.y;
            z = right.z;
            return *this;
        }

        template< class Y >
        Vector3& operator += ( Vector3<Y> right )
        {
            x += right.x;
            y += right.y;
            z += right.z;
            return *this;
        }

        template< class Y >
        Vector3& operator -= ( Vector3<Y> right )
        {
            x -= right.x;
            y -= right.y;
            z -= right.z;
            return *this;
        }

        template< class Y >
        Vector3& operator *= ( Y right )
        {
            x *= right;
            y *= right;
            z *= right;
            return *this;
        }

        template< class Y >
        Vector3& operator /= ( Y right )
        {
            x /= right;
            y /= right;
            z /= right;
            return *this;
        }

        /* TODO get help on this. How does B multiply?
        static Vector3 resolute( Vector3 A, Vector3 B )
        {
            // According to wiki: C =  (A*B)/mag(B)^2 * B
            return B * ( product(B)/B.magnitude2() );
            // Won't work: bulshit errors.
            // But don't need.
        }
        */

        // IMPLICITE CONVERSIONS //
        /* True if either x, y, or z is non-zero.

           Good for inside if statements.

           I know there there are potential problems.
           I'll watch out for them and adjust WHEN they occure.
        */
        operator const bool() const
        {
            return x || y || z;
        }
};

// MATH OPERATION WITH VECTORS //
/* Having them return const takes out nonsense operations like (a*b)=c */
template< class T > inline
const Vector3<T> operator + ( const Vector3<T>& a, const Vector3<T>& b )
{
    return Vector3<T>( a ) += b;
}

template< class T > inline
const Vector3<T> operator - ( const Vector3<T>& a, const Vector3<T>& b )
{
    return Vector3<T>( a ) -= b;
}

/* Dot product.

   I think * for dot product and ^ for cross is intuitive ENOUGH.
*/
template< class T, class Y > inline
const T operator * ( const Vector3<T>& a, const Vector3<Y>& b )
{
    return Vector3<T>( a ) *= b;
}

// Scaler operators:
template< class T, class Y > inline
const Vector3<T> operator * ( const Vector3<T>& a, const Y b )
{
    return Vector3<T>( a ) *= b;
}
template< class T, class Y > inline
const Vector3<T> operator * ( const T b, const Vector3<Y>& a )
{
    return a * b;
}

template< class T, class Y > inline
const Vector3<T> operator / ( const Vector3<T>& a, const Y b )
{
    return Vector3<T>( a ) /= b;
}

template< class T, class Y > inline
const Vector3<T> magnitude( const Vector3<T>& vect, const Y newMagnitude )
{
    return Vector3<T>( vect ).magnitude( newMagnitude );
}

I'd appreciate any advice anyone has on my class, but the special issues are as posted at the top. Thank you, EDIT: I almost forgot to post the code that made me feel these nuances were necessary:
// If there was any input... (input is a vector representing directional input.
if( input = speed ) // Speed is a pre-defined float
{
    // Vector=float does nothing if the magnitude is zero (it'll return false).
    // But, we'll get the proper magnitude otherwise, AND it'll return true!
    // ( Sacraficing clarity for wittyness :D )

    // If a shot can be fired...
    if(    keyStates[ SDLK_a] // AWESOME mode invoked by 'a'
        || ticks-timeOfLastShot > delay
        || SDL_JoystickGetButton( joystick , 5 )   // AWESOME mode invoked by triggers.
        || SDL_JoystickGetButton( joystick , 4 ) )
    {
        // Looks dangerous, but I have things set up so I don't need to worry
        // about memory allocation/deallocation. I really do.
        new Bullet
        (
            player->pos + input.magnitude( 15 ), // Spawn point. Input does NOT take the magnitude of 15 because of changes I made to Vector3
            input,                               // Initial velocity (why I set input's magnitude to speed).
            GLVector3( 0.0f, 0.0f, input.xyTheta() * R_TO_D ) // The rotation vector.
        );

        timeOfLastShot = ticks;

        // The player gets kicked back.
        player->vel = input.magnitude( speed );

        rot.z = input.xyTheta() * R_TO_D;



[Edited by - Splinter of Chaos on July 22, 2008 12:06:56 AM]

Share this post


Link to post
Share on other sites
Advertisement
I might be going crazy here but your code is a bit non-obvious to follow due to your change.

Wouldn't this do exactly the same thing? It wouldn't modify the original, it returns a copy and the addition works as the operator is defined to work.

float inputMag = input.magnitude();
// Avoid a divide by zero.
Vector3 dtVec(0.0f, 0.0f, 0.0f);
if( inputMag )
dtVec = input * (15.0f / inputMag);
.
.
.
player->pos + dtVec;




In general it's not a good idea to overload something like "magnitude" whilst subverting its accepted behaviour. Instead leave your "magnitude" method as the mathematics would suggest that it behaves and implement a specific and descriptively named method that does your alternative action.

Andy

EDIT: tidied up my code example, realised that you were trying to guard against a few things like div/0 etc.

Share this post


Link to post
Share on other sites
Guest Deventer
Hi Splinter. Hope this helps you out! =)

You have quite a lot of code duplication.
e.g:
const T magnitude2() const { return x*x + y*y + z*z; }


could be written as
const T magnitude2() const { return sqrt( magnitude(); ) }



You also don't pass Vectors by reference.
e.g:
Vector3& operator += ( Vector3 right )
{
x += right.x;
y += right.y;
z += right.z;
return *this;
}


should be written as
Vector3& operator += ( const Vector3& right )
{
x += right.x;
y += right.y;
z += right.z;
return *this;
}



operator +, operator - and friends are binary operators which can (and should) be written as non-member functions, I'm also not convinced they should return new const vectors:
Vector3 operator + ( const Vector3 & left, const Vector 3 & right )
{
return Vector3( left ) += right; // Avoid code duplication
}



lastly (from me),
what exactly does this do?:
const Vector3 operator * ( Vector3 right ) const
{
return Vector3( x*right.x, y*right.y, z*right.z );
}


It looks like a Dot Product, but it returns a Vector3, which isn't right... But the main point I'm trying to make here is that perhaps this isn't suitable as an operator. Its intention is not clear.

Note: I haven't tested this code, it's intended to show how improvements can be made, but not to replace your actual code unless it has been checked over first.

Edit: Just realised my post sounded a bit terse.

Share this post


Link to post
Share on other sites
Something that caught my eye.

const T magnitude2() const { return       x*x + y*y + z*z;   }

Is badly named. I suspect you meant this to be magnitudeSqaured() const.


        const T magnitudeSquared() const  { return ( x*x + y*y + z*z ); }
const T magnitude() const { return sqrt( magnitudeSquared() ); }


Also, if you are returning by value, don't bother making the return type const. There is no benefit from it being non-const.

Share this post


Link to post
Share on other sites
Quote:
Original post by Deventer
You also don't pass Vectors by reference.


I actually wasn't quite sure about whether they should or not. Supposedly, it's less efficient to pass basic types by pointer (which is how the reference would be implemented) than by value. I figured that there's no way for me to really know, so I assumed by value would be OK here. If you still think I should change it, I think I will.

And yeah, the last one's the dot product, but you're right, it's probably inaccurate. I've never had that level of math.

As for the code duplication: it doesn't matter, really. Since both functions are inline, the assembly generated won't be more efficient and it would actually take longer to type since I can't just copy/paste.

But, thanks.

Quote:
Original post by yaustar
        const T magnitudeSquared() const  { return ( x*x + y*y + z*z ); }
const T magnitude() const { return sqrt( magnitudeSquared() ); }


I found, though a little research, that the suffix two was conventional enough for me to use it meaning squared.

No advantage from const returning? Fine. I only really did it because I was told to use const as much as possible, anyway. I don't like it, personally.

Share this post


Link to post
Share on other sites
Guest Deventer
Quote:
Original post by Splinter of ChaosAnd yeah, the last one's the dot product, but you're right, it's probably inaccurate. I've never had that level of math.

As for the code duplication: it doesn't matter, really. Since both functions are inline, the assembly generated won't be more efficient and it would actually take longer to type since I can't just copy/paste.

Might I suggest you use:
T DotProduct( Vector3 & left, Vector3 & right )
{
return left.x * right.x + left.y * right.y + left.z * right.z;
}

for your DotProduct.

With regards to the code duplication, it's not about efficiency, it's about correctness and reducing the number of ways your code can go wrong (by reducing the amount of code).

What if you had made a typo in the initial implementation and copied/pasted that typo into another block of code?
If you then noticed you had made the typo in one function, you would then need to go back and edit it in two (or more) places rather than just one.

Share this post


Link to post
Share on other sites
Quote:
Original post by Splinter of Chaos
Quote:
Original post by Deventer
You also don't pass Vectors by reference.


I actually wasn't quite sure about whether they should or not. Supposedly, it's less efficient to pass basic types by pointer (which is how the reference would be implemented) than by value. I figured that there's no way for me to really know, so I assumed by value would be OK here. If you still think I should change it, I think I will.


The answer is in your quote "less efficient to pass basic types by pointer" because the vector is not a basic type ;) Also passing by reference can sometimes be optimised away by the compiler, especially if it supports the SSE instruction optimisations, however passing by value will require that the copy constructor is called which can limit the optimisations a compiler can perform. In general, you want to reference non-POD datatypes to avoid them being copied.

Also your constructor will be better off using initialisation lists rather than assignment.

template< class T >
class Vector3
{
public:
T x, y, z; // The compisite forces.

// METHODS //
// Vector3 ( T X=0.0f, T Y=0.0f, T Z=0.0f )
// {
// x=X;
// y=Y;
// z=Z;
// }
// see the following link for why.
// http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.6
Vector3 ( T X=0.0f, T Y=0.0f, T Z=0.0f ) : x(X), y(Y), z(Z)
{;}



Andy

Share this post


Link to post
Share on other sites
I was about to make an argument on how I'd gain no efficiency and make the code slightly harder to read, but then I said hypothetically "it would only really matter if the client used an object like a big int class..." I guess just for sake of that hypothetical, I'll do it.

Thanks for the suggestions so far. Didn't get my main issue resolved, but no one seems to have a problem with it and I'm bettering things I never would have thought to ask about. Thanks.

Share this post


Link to post
Share on other sites
Heya -- a couple comments/suggestions:

  • I'd recommend against making this class templated. Right now there's only a specific subset of operations that make sense, depending on what values get fed in for T. As an example, if someone makes a Vector3<int>, all of the theta functions as well magnitude/normalization won't work.

  • There's no real reason to return "const T" or "const Vector3". It prevents certain nonsense operations from compiling ("Vector3<float>(a, b, c).xyTheta().x = b;") but they were nonsense operations to begin with.

  • I'd recommend very strongly against supporting any non-explicit default constructor, like your "Vector3 ( T X=0.0f, T Y=0.0f, T Z=0.0f )". This gives the compiler freedom to do all sorts of things that you generally don't want it doing. As an example, with that constructor, this is valid code:

    Vector3<float> v, a;
    a = 1.0f + v;


    Do you know what that does by reading the code? Are you sure that it's what you'd expect/want for it to do? On a related note:

  • I'd also recommend very strongly against supporting a bool conversion operator.

    The fact that a bool conversion exists is the reason why this isn't valid code:

    Vector3<float> v, a;
    a = v + 1.0f;


    It's invalid because the compiler can't decide whether you want to say "Vector = bool + float" or "Vector = Vector + float", when in truth you probably want neither.

    When you're trying to convert it to a bool, what are you really asking? Is this vector zeroed/small/something else? Why not convert that to a specific conditional function and call that -- it'll be more clear in the calling code as well.

  • Similarly, I think supporting an operator= is going to lead to heartache. As with the above two points, you're giving the compiler permission to do things behind the scenes that you probably want to control explicitly. As an example, this is valid code:

    Vector<float> v;
    v = v.magnitude() > 1.0f;


    The compiler knows that that > operator will return a bool, and that it can convert a bool to a float, and so this will wind up setting the magnitude of v to either 1.0f or 0.0f, very non-intuitively. (If you remove this operator, the "clampMag()" function will continue to compile, but now it will be generating incorrect code because of the default constructor parameters.)

  • I'll second what's already been mentioned -- generally when you have a math class like this you want to support operators working regardless of how their arguments are arranged. ("Vector * float" should give the same results as "float * Vector".) The easiest way to do this is to declare inline friend functions. Also, again seconding what's been mentioned, you probably want those operators to "const Vector&" and return a standard "Vector".


This isn't a full list of comments, but I hope there's some useful information and food for thought in there.

Share this post


Link to post
Share on other sites
Okay, here are some hard truths:

0. Understand the role of your class
Your Vector3 class is responsible for representing a mathematical vector. Leave it at that. Give it the operators, but don't make it responsible for calculations pertaining to its self (this works in combination with point 2, you'll be moving that functionality). I'm glad you chose PolarVector as a seperate class, and didn't try and make a homogeneous vector that handles both cases.


1. The best code you write is code you've never written
xyTheta and friends... are they used? Is there a need for them? You've even written comments over one saying it's not used. Save yourself some time, don't write them. Hell, even remove them. If you ever change something about your class, you'll have to update them to reflect this change. Since you're not using them (probably), this can only cause you more time, not less. And if you ever do find yourself needing them, they're not that hard to write, right?


2. Prefer non-member functions
This is a design issue, and not an issue about performance or whatever. By pushing out functions to be non-member (when it is possible), you reduce the coupling of your code, and consolidate functionality. So prefer magnitude(const Vector3&) to Vector3::magnitude (templating removed for brevity). The only exception I would make for a vector class is to have a normalize member function to do what you're doing (so, leave it in), and have a non-member normalize function that doesn't modify the vector, but returns a normalized copy of said vector. You'll find people grok this idiom very easily.


2.5 Make operators friend non-member
This has a practical issue as well as a design issue. You can have my_vector * 3.0f, but you can't have 3.0f * my_vector. Why? Because in your class, you can't define an operator * that takes a T as its LHS (left-hand-side). So for multiplication, I'd be writing (templates removed for brevity):
class Vector3
{
...
friend Vector3 operator * (const Vector3& lhs, const T& rhs);
friend Vector3 operator * (const T& lhs, const Vector3& rhs);
};

Vector3 operator * (const Vector3& lhs, const T& rhs)
{
return Vector3(lhs.x * rhs, lhs.y * rhs, lhs.z * rhs);
}

Vector3 operator * (const T& lhs, const Vector3& rhs)
{
return rhs * lhs; // no need to write this again
}


Note: There are some operators for which this doesn't (and shouldn't) work: They are unary operators (to simplify). Assignment operators, negation operators, etc.


3. Prefer overloading to default arguments
Vector3<float> my_vector3(4);

Whoops. That doesn't really make sense, taking just one numerical value. Does it initialise x, or does it set x, y, and z to the same number? It's a Vector3. To have the behaviour you want, write two constructors: the default constructor, and one that takes the three parameters, with no default arguments. Then people can't misunderstand what it does.


4. Be intuitive
Your magnitude function that returns a Vector3 is non-intuitive. You even say so. This is a bad thing. People who use your code (you!) expect certain behaviour. I, and everyone on the planet (who use vectors), expect a magnitude function to return the magnitude of said vector. As previously stated, magnitude2 should be renamed to magnitude_squared to better communicate what it does. Your cast operator to bool should be deleted; there's no logical boolean representation of a vector. The GREAT solution is a simple comparison to a Vector3(0, 0, 0), ie if (my_vector == Vector3(0, 0, 0)).

Remove the assignment operator from T (that magnitude thing you keep going on about). It will bite you so hard.

Remove the operator * that seems to be doing a dot product. Make a non-member function called dot_product(const Vector3& lhs, const Vector3& rhs) that does it for you. You'll trip yourself up if you don't.


5. General small things
  • Use initialisation lists
  • Use const references for parameters
  • Take your T as a const reference, too (what if T is not a primitive?)
  • Assert against floating-point exceptions (ie, dividing by zero)



I hope that helps! :D

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!