3
\$\begingroup\$

I've created this 3D vector class and would like it to be reviewed. I wonder if there are still some improvements that can be made. This is a header only implementation. 

#pragma once
#include <string>

template<class T> class vec3
{
public:
    union {
        T f[3];

        struct {
            T x, y, z;
        };
    };

    vec3(vec2<T> p, T z = 0) :
        x(p.x),
        y(p.y),
        z(z)
    {
    }
    vec3(T x = 0, T y = 0, T z = 0) :
        x(x),
        y(y),
        z(z)
    {
    }

    template <typename _L> operator vec2<_L>() const { return vec3<_L>(x, y); };
    template <typename _L> operator vec3<_L>() const { return vec3<_L>(x, y, z); };

    vec3 operator-() const {
        return vec3<T>(-x, -y, -z);
    };

    bool operator==(const vec3& other) const {
        return (x == other.x && y == other.y && z == other.z);
    };
    bool operator!=(const vec3& other) const {
        return !operator==(other);
    };

    bool operator<(const vec3& other) const {
        if (x < other.x)
            return true;
        if (x > other.x)
            return false;
        //x == other.x
        if (y < other.y)
            return true;
        if (y > other.y)
            return false;
        //y == other.y
        if (z < other.z)
            return true;
        if (z > other.z)
            return false;
        //z == other.z
        return false;
    };
    bool operator>(const vec3& other) const {
        return other < *this;
    };
    bool operator<=(const vec3& other) const {
        return !operator>(other);
    };
    bool operator>=(const vec3& other) const {
        return !operator<(other);
    };

    vec3& operator*= (const vec3& other) {
        x *= other.x;
        y *= other.y;
        z *= other.z;
        return *this;
    };
    vec3 operator* (const vec3& other) const {
        return vec3<T>(x * other.x, y * other.y, z * other.z);
    };

    vec3& operator/= (const vec3& other) {
        x /= other.x;
        y /= other.y;
        z /= other.z;
        return *this;
    };
    vec3 operator/ (const vec3& other) const {
        return vec3<T>(x / other.x, y / other.y, z / other.z);
    };

    vec3& operator+= (const vec3& other) {
        x += other.x;
        y += other.y;
        z += other.z;
        return *this;
    };
    vec3 operator+ (const vec3& other) const {
        return vec3<T>(x + other.x, y + other.y, z + other.z);
    };

    vec3& operator-= (const vec3& other) {
        x -= other.x;
        y -= other.y;
        z -= other.z;
        return *this;
    };
    vec3 operator- (const vec3& other) const {
        return vec3<T>(x - other.x, y - other.y, z - other.z);
    };

    vec3& operator*= (const T f) {
        x *= f;
        y *= f;
        z *= f;
        return *this;
    };
    vec3 operator* (const T f) const {
        return vec3<T>(x*f, y*f, z*f);
    };

    vec3& operator/= (const T f) {
        x /= f;
        y /= f;
        z /= f;
        return *this;
    };
    vec3 operator/ (const T f) const {
        return vec3<T>(x / f, y / f, z / f);
    };

    vec3& operator+= (const T f) {
        x += f;
        y += f;
        z += f;
        return *this;
    };
    vec3 operator+ (const T f) const {
        return vec3<T>(x + f, y + f, z + f);
    };

    vec3& operator-= (const T f) {
        x -= f;
        y -= f;
        z -= f;
        return *this;
    };
    vec3 operator- (const T f) const {
        return vec3<T>(x - f, y - f, z - f);
    };

    /* Returns the magnitude of this vector. */
    T magnitude() const {
        return sqrt(sqrMagnitude());
    };
    /* Returns the squared magnitude of this vector. */
    T sqrMagnitude() const {
        return x*x + y*y + z*z;
    };

    /* Returns the inverse of this vector (1 / vector). */
    vec3& invert() {
        return ((*this) = 1 / (*this));
    }
    /* Returns a copy of this vector inverted (1 / vector). */
    vec3 inverse() const {
        return (1 / (*this));
    }

    /* Normalizes this vector. */
    vec3& normalize() {
        return ((*this) *= 1 / magnitude());
    };
    /* Returns a copy of this vector normalized. */
    vec3 normalized() const {
        return ((*this) * 1 / magnitude());
    };

    /* Return the dot product of this vector and 'other'. */
    T dot(const vec3& other) const {
        return x*other.x + y*other.y + z*other.z;
    };
    /* Return the dot product of vector a and b. */
    static T dot(const vec3& a, const vec3& b) {
        return a.x*b.x + a.y*b.y + a.z*b.z;
    };

    /* Return the cross product of this vector and 'other'. */
    vec3 cross(const vec3& other) const {
        return vec3<T>(y * other.z - z * other.y, z * other.x - x * other.z, x * other.y - y * other.x);
    };
    /* Return the cross product of vector a and b. */
    static vec3 cross(const vec3& a, const vec3& b) {
        return vec3<T>(a.y * b.z - a.z * b.y, a.z * b.x - a.x * b.z, a.x * b.y - a.y * b.x);
    };

    /* Returns the distance from this vector to 'to'. */
    T distance(const vec3& to) const {
        return (*this - to).magnitude();
    };
    /* Returns the distance between vector a and b. */
    static T distance(const vec3& a, const vec3& b) {
        return (a - b).magnitude();
    };

    /* Returns a linearly interpolated vector between this vector and 'other' based on t. */
    vec3 lerp(const vec3& other, const float t) const {
        return ((1.0f - t)*(*this) + t*other);
    };
    /* Returns a linearly interpolated vector between vector a and b based on t. */
    static vec3 lerp(const vec3& a, const vec3& b, const float t) {
        return ((1.0f - t)*a + t*b);
    };

    /* Returns a spherically interpolated vector between this vector and 'other' based on t. */
    vec3 slerp(const vec3& other, float t) const {
        float dotproduct = dot(other);

        t /= 2;

        float theta = acos(dotproduct);
        if (theta < 0.0f)
            theta = -theta;

        float st = sin(theta);
        float coeff1 = sin((1 - t)*theta) / st;
        float coeff2 = sin(t*theta) / st;

        return (coeff1*(*this) + coeff2*other).normalize();
    };
    /* Returns a spherically interpolated vector between vector a and b based on t. */
    static vec3 slerp(const vec3& a, const vec3& b, float t) {
        float dotproduct = dot(a, b);

        t /= 2;

        float theta = acos(dotproduct);
        if (theta < 0.0f)
            theta = -theta;

        float st = sin(theta);
        float coeff1 = sin((1 - t)*theta) / st;
        float coeff2 = sin(t*theta) / st;

        return (coeff1*a + coeff2*b).normalize();
    };

    /* Calculate the angle between two vectors in radians. */
    T angle(const vec3& to) {
        float dot = (*this).dot(to);
        dot /= ((*this).magnitude() * to.magnitude());
        return acos(dot);
    };
    /* Calculate the angle between two vectors in radians. */
    static T angle(const vec3& from, const vec3& to) {
        float dot = from.dot(to);
        dot /= (from.magnitude() * to.magnitude());
        return acos(dot);
    };

    /* Returns a structured string representation of this vector. */
    std::string toString() const {
        return "[x: " + std::to_string(x) + ", y: " + std::to_string(y) + ", z: " + std::to_string(z) + "]";
    };

    friend vec3<T> operator* (const T d, const vec3& vec) {
        return vec3<T>(d * vec.x, d * vec.y, d * vec.z);
    };

    friend vec3<T> operator/ (const T d, const vec3& vec) {
        return vec3<T>(d / vec.x, d / vec.y, d / vec.z);
    };

    friend vec3<T> operator+ (const T d, const vec3& vec) {
        return vec3<T>(d + vec.x, d + vec.y, d + vec.z);
    };

    friend vec3<T> operator- (const T d, const vec3& vec) {
        return vec3<T>(d - vec.x, d - vec.y, d - vec.z);
    };

    static const vec3 left;
    static const vec3 right;
    static const vec3 up;
    static const vec3 down;
    static const vec3 forward;
    static const vec3 back;
    static const vec3 zero;
    static const vec3 one;
};

template <typename T> const vec3<T> vec3<T>::left(-1, 0, 0);
template <typename T> const vec3<T> vec3<T>::right(1, 0, 0);
template <typename T> const vec3<T> vec3<T>::up(0, 1, 0);
template <typename T> const vec3<T> vec3<T>::down(0, -1, 0);
template <typename T> const vec3<T> vec3<T>::forward(0, 0, 1);
template <typename T> const vec3<T> vec3<T>::back(0, 0, -1);
template <typename T> const vec3<T> vec3<T>::zero(0, 0, 0);
template <typename T> const vec3<T> vec3<T>::one(1, 1, 1);
Improve this question
\$\endgroup\$

3 Answers 3

Reset to default
3
\$\begingroup\$

I see a number of things that may help you improve your program.

Include all required headers

The template has calls to acos and sin but doesn't include the <cmath> header which is required. The code should have this line:

#include <cmath>

Use namespaces

The previously mentioned acos and sin should use the namespace prefix as std::acos and std::sin.

Don't use anonymous structs

While they are standard in C11, they are not allowed in C++. There are compiler extensions that can enable them but they don't conform to the standard. In this case, you could either name it or use the next suggestion.

Eliminate the union

Within the array is never used. This suggests that the union could simply be eliminated. If you think that users of the code are going to want to refer to the elements as an array, you could provide an operator[] implementation for that with cleaner syntax anyway.

Eliminate vec2

The vec3 class does not seem to rely on a vec2 for existence or functionality, so it should not be a part of the class definition. Right now, it's not possible to use the class unless one has a vec2 also available, but that dependency is also not #included per the first suggestion.

Don't use #pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions. By definition, all #pragma are non-standard. For portable code, you should use the standard include guards. Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Think about unsigned numbers

It may be that you intended for this class to use exclusively floating point numbers, but there isn't any checking of that, so the result is that this line:

std::cout << vec3<unsigned>::left.toString() << '\n';

compiles and runs just fine, but gives a peculiar result:

[x: 4294967295, y: 0, z: 0]

I would recommend adding a static_assert to the class:

static_assert(std::is_signed<T>{}, "Error: vec3 type must be signed");

Use appropriate floating point types

If I am using a vec3<double>, I might not be happy that angle is internally using only a float instead. Consider using either T or double where float is currently used within the class member functions.

Wrap it in a namespace

To make sure your vec3 doesn't collide with another in some other library, you might want to wrap the whole thing up in your own namespace.

Improve this answer
\$\endgroup\$
2
\$\begingroup\$ 
template <typename _L> operator vec2<_L>() const { return vec3<_L>(x, y); };

Two issues with this line:

  • You're using a reserved naming convention. The Standard Library and compiler-related extensions reserve names starting with _ followed by an uppercase letter for internal use in macros, private types or even global variables. See the anser at: What are the rules about using an underscore in a C++ identifier?

  • Probably a typo, but if you're casting to vec2, then I suppose the intended was to return a vec2? If you never actually use that method in the code, it will never get instantiated, because this is a template class, so the compiler will not report this kind of typos that it would otherwise flag in non-template code.

This comparison operator seems very convoluted and, to some extent, questionable:

bool operator<(const vec3& other) const {
    if (x < other.x)
        return true;
    if (x > other.x)
        return false;
    //x == other.x
    if (y < other.y)
        return true;
    if (y > other.y)
        return false;
    //y == other.y
    if (z < other.z)
        return true;
    if (z > other.z)
        return false;
    //z == other.z
    return false;
};

If a single element of this vector is less than other, you return true. I have doubts if this is a good definition of operator <. The usual would be to only consider this < other if all members of this are less than other. In some cases it might also make more sense to test the lengths/magnitudes of the vectors.

bool operator < (const vec3& other) const
{
    return (x < other.x) && (y < other.y) && (z < other.z);

    // or alternatively:
    // return sqrMagnitude() < other.sqrMagnitude();
}

The difference between invert and inverse is not clear without looking at the implementation. They actually do the same thing, only difference is that invert mutates this. If you really think there's a need for the existence of the two methods, consider renaming inverse to inverted, being consistent with the other notation used on normalize-normalized.

But actually, inverting a vector is usually associated with changing its direction, that is, negating the vector. Your invert function performs 1/vec, which is an operation usually associated with the reciprocal of a value. I suggest a complete name change for that method.

Final nitpicking, don't add ; to the end of each function. It is unnecessary and if you try to compile on Clang or GCC with maxed-out warning levels, you'll get a torrent of warnings for the extra semicolons.

Improve this answer
\$\endgroup\$
1
\$\begingroup\$

Don't repeat yourself. In many place you have 

T distance(const vec3& to) const {
    return (*this - to).magnitude();
};

static T distance(const vec3& a, const vec3& b) {
    return (a - b).magnitude();
};

you can define one function using the other.

static T distance(const vec3& a, const vec3& b) {
    return a.distance(b);
};

Also don't define operator that don't make sense. There is no unique way to sort 3d vector. Not much people will use your < operator.

You have the operator vec3 * vec3 and the dot product that do the same thing. Having two way to do the same operation confuse people.

You can check glm for inspiration. http://glm.g-truc.net/0.9.0/api/a00104_source.html :)

Keep on coding.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.