I am working on a project that needs algebra operations. I have coded a Matrix class (only needed 4x4 matrix), and a Vector4 class. The classes working fine, but I would know if there are some bad practices that I use, and want you to help me improve this classes. Last time I encountered some problem when wanted to code something like this:

Matrix m;    // (4x4 matrix)
Vector4 v;   // (4-elements vector)

m(0) = v;    // replace first 4-elements matrix row by vector v
v = m(0);    // replace vector v by first 4-elements matrix row

I found some solution to get this to work:

Matrix.h

#pragma once

#include <cmath>

#include "Vector3.h"
#include "Vector4.h"

class Matrix
{
  public:

    Matrix();             
    Matrix(float,float,float,float,float,float,float,float,float,float,float,float,float,float,float,float);
    Matrix(float m[16]);
    Matrix(const Vector4& v1, const Vector4& v2, const Vector4& v3, const Vector4& v4);
    ~Matrix() {}

    const Matrix operator* ( const Matrix &m) const;

    float&  operator() (unsigned row, unsigned col);
    float   operator() (unsigned row, unsigned col) const;
    Vector4 operator() (unsigned row);
    Vector4 operator() (unsigned row) const;

    void translate(float x, float y, float z);
    void translate(const Vector3 &v);
    void rotate(float a, const Vector3& v);
    void transpose();

    const static Matrix IDENTITY;

    static Matrix createTranslation(const Vector3 &v);
    static Matrix createTranslation(float x, float y, float z);
    static Matrix createScale(const Vector3 &v);
    static Matrix createScale(float x, float y, float z);
    static Matrix createRotationX(float a);
    static Matrix createRotationY(float a);
    static Matrix createRotationZ(float a);
    static Matrix createRotation(float a, const Vector3& v);
    static Matrix createLookAt(const Vector3& eye, const Vector3& center, const Vector3& up);
    static Matrix createPerspective(float fovy, float aspect, float near, float far);

    // Euler angles //
    inline float getRoll() {return atan2(-(*this)(2,0),(*this)(0,0));}      // X //
    inline float getPitch() {return asin((*this)(1,0));}                    // Y //
    inline float getYaw() {return atan2(-(*this)(1,2),(*this)(1,1));}       // Z //

    void show();

  private:

    float mat[4][4];
};

Matrix.cpp

#include "Matrix.h"

#include <iostream>

#include "Math.h"

const Matrix Matrix::IDENTITY = Matrix(1,0,0,0,0,1,0,0,0,0,1,0,0,0,0,1);

Matrix::Matrix()
{
    *this = IDENTITY;
}

Matrix::Matrix(float _00,float _01,float _02,float _03,float _10,float _11,float _12,float _13,float _20,float _21,float _22,float _23,float _30,float _31,float _32,float _33)
{
    (*this)(0,0) = _00;
    (*this)(0,1) = _01;
    (*this)(0,2) = _02;
    (*this)(0,3) = _03;

    (*this)(1,0) = _10;
    (*this)(1,1) = _11;
    (*this)(1,2) = _12;
    (*this)(1,3) = _13;

    (*this)(2,0) = _20;
    (*this)(2,1) = _21;
    (*this)(2,2) = _22;
    (*this)(2,3) = _23;

    (*this)(3,0) = _30;
    (*this)(3,1) = _31;
    (*this)(3,2) = _32;
    (*this)(3,3) = _33;
}

Matrix::Matrix(float m[16])
{
    int index = 0;

    for ( int i = 0; i < 4; ++i)            
        for ( int j = 0; j < 4; ++j)    
        {
            (*this)(i,j) = m[index++];
        }
}

Matrix::Matrix(const Vector4& v1, const Vector4& v2, const Vector4& v3, const Vector4& v4)
{
    (*this)(0) = v1;
    (*this)(1) = v2;
    (*this)(2) = v3;
    (*this)(3) = v4;
}

float& Matrix::operator() (unsigned row, unsigned col)
{
    return mat[row][col];
}

float Matrix::operator() (unsigned row, unsigned col) const
{
    return mat[row][col];
}

// this one only needed for something like this: m(0) = Vector4(1,1,1,1) //
Vector4 Matrix::operator() (unsigned row)
{
    return Vector4 (&mat[row][0], &mat[row][1], &mat[row][2], &mat[row][3]);
}

Vector4 Matrix::operator() (unsigned row) const
{
    return Vector4 (mat[row][0], mat[row][1], mat[row][2], mat[row][3]);
}

const Matrix Matrix::operator* ( const Matrix &m) const
{
    Matrix a;
    float sum;

    for ( int i = 0; i < 4; ++i)            
        for ( int j = 0; j < 4; ++j)      
        {
            sum = 0;

            for ( int k = 0; k < 4; ++k)
                sum += (*this)(k,i) * m(j,k);

            a(j,i) = sum;
        }

    return a;
}

void Matrix::translate(float x, float y, float z)
{
    float xx = ( x * (*this)(0,0) + y * (*this)(1,0) + z * (*this)(2,0) );
    float yy = ( x * (*this)(0,1) + y * (*this)(1,1) + z * (*this)(2,1) );
    float zz = ( x * (*this)(0,2) + y * (*this)(1,2) + z * (*this)(2,2) );
    float ww = ( x * (*this)(0,3) + y * (*this)(1,3) + z * (*this)(2,3) );

    (*this)(3,0) += xx;
    (*this)(3,1) += yy;
    (*this)(3,2) += zz;
    (*this)(3,3) += ww;
}

void Matrix::translate(const Vector3 &v)
{
    this->translate(v.x, v.y, v.z);
}

void Matrix::transpose()
{
    Matrix trans;
    for( int i = 0 ; i < 4 ; ++i )
        for( int j = 0 ; j < 4 ; ++j )
            trans.mat[i][j] = this->mat[j][i];

    (*this) = trans;
}

void Matrix::show()
{
    std::cout << std::endl;
    for ( int i = 0; i < 4; ++i)            
    {
        for ( int j = 0; j < 4; ++j)       
            std::cout << this->mat[i][j] << " ";

        std::cout << std::endl;
    }
}

Matrix Matrix::createTranslation(const Vector3 &v)
{
    return Matrix
    (
          1,   0,   0,   0,
          0,   1,   0,   0,
          0,   0,   1,   0,
        v.x, v.y, v.z,   1
    );
}

Matrix Matrix::createTranslation(float x, float y, float z)
{
    return Matrix::createTranslation(Vector3(x,y,z));
}

Matrix Matrix::createScale(const Vector3 &v)
{
    return Matrix
    (
          v.x,   0,     0,   0,
          0,   v.y,     0,   0,
          0,     0,   v.z,   0,
          0,     0,     0,   1
    );
}

Matrix Matrix::createScale(float x, float y, float z)
{
    return Matrix::createScale(Vector3(x,y,z));
}

Matrix Matrix::createRotationX(float a)
{
    return Matrix
    (
          1,        0,       0,    0,
          0,    cos(a),  sin(a),   0,
          0,   -sin(a),  cos(a),   0,
          0,        0,       0,    1
    );
}

Matrix Matrix::createRotationY(float a)
{
    return Matrix
    (
        cos(a),   0,  -sin(a),    0,
            0,    1,       0,     0,
        sin(a),   0,   cos(a),    0,
            0,    0,       0,     1
    );
}

Matrix Matrix::createRotationZ(float a)
{
    return Matrix
    (
           cos(a),  sin(a),   0,    0,
          -sin(a),  cos(a),   0,    0,
               0,       0,    1,    0,
               0,       0,    0,    1
    );
}

Matrix Matrix::createRotation(float angle, const Vector3& v)
{
    float a = angle;
    float c = cos(a);
    float s = sin(a);

    Vector3 axis = v;
    axis.normalize();

    Vector3 temp = axis * (1.0f - c);

    Matrix Rotate;
    Rotate(0,0) = c + temp.x * axis.x;
    Rotate(0,1) = 0 + temp.x * axis.y + s * axis.z;
    Rotate(0,2) = 0 + temp.x * axis.z - s * axis.y;

    Rotate(1,0) = 0 + temp.y * axis.x - s * axis.z;
    Rotate(1,1) = c + temp.y * axis.y;
    Rotate(1,2) = 0 + temp.y * axis.z + s * axis.x;

    Rotate(2,0) = 0 + temp.z * axis.x + s * axis.y;
    Rotate(2,1) = 0 + temp.z * axis.y - s * axis.x;
    Rotate(2,2) = c + temp.z * axis.z;

    Matrix identity;

    Matrix m;
    m(0) = (identity)(0) * Rotate(0,0) + (identity)(1) * Rotate(0,1) + (identity)(2) * Rotate(0,2);
    m(1) = (identity)(0) * Rotate(1,0) + (identity)(1) * Rotate(1,1) + (identity)(2) * Rotate(1,2);
    m(2) = (identity)(0) * Rotate(2,0) + (identity)(1) * Rotate(2,1) + (identity)(2) * Rotate(2,2);
    m(3) = (identity)(3);

    return m;
}

void Matrix::rotate(float angle, const Vector3& v)
{
    float a = angle;
    float c = cos(a);
    float s = sin(a);

    Vector3 axis = v;
    axis.normalize();

    Vector3 temp = axis * (1.0f - c);

    Matrix Rotate;
    Rotate(0,0) = c + temp.x * axis.x;
    Rotate(0,1) = 0 + temp.x * axis.y + s * axis.z;
    Rotate(0,2) = 0 + temp.x * axis.z - s * axis.y;

    Rotate(1,0) = 0 + temp.y * axis.x - s * axis.z;
    Rotate(1,1) = c + temp.y * axis.y;
    Rotate(1,2) = 0 + temp.y * axis.z + s * axis.x;

    Rotate(2,0) = 0 + temp.z * axis.x + s * axis.y;
    Rotate(2,1) = 0 + temp.z * axis.y - s * axis.x;
    Rotate(2,2) = c + temp.z * axis.z;

    Matrix m;
    m(0) = (*this)(0) * Rotate(0,0) + (*this)(1) * Rotate(0,1) + (*this)(2) * Rotate(0,2);
    m(1) = (*this)(0) * Rotate(1,0) + (*this)(1) * Rotate(1,1) + (*this)(2) * Rotate(1,2);
    m(2) = (*this)(0) * Rotate(2,0) + (*this)(1) * Rotate(2,1) + (*this)(2) * Rotate(2,2);
    m(3) = (*this)(3);

    *this = m;
}

Matrix Matrix::createLookAt(const Vector3& eye, const Vector3& center, const Vector3& up)
{
    Vector3 f = center - eye;
    f.normalize();
    Vector3 u = up;
    u.normalize();
    Vector3 s = Vector3::cross(f, u);
    s.normalize();
    u = Vector3::cross(s, f);

    Matrix Result;

    Result(0,0) = s.x;
    Result(1,0) = s.y;
    Result(2,0) = s.z;
    Result(0,1) = u.x;
    Result(1,1) = u.y;
    Result(2,1) = u.z;
    Result(0,2) = -f.x;
    Result(1,2) = -f.y;
    Result(2,2) = -f.z;
    Result(3,0) = -Vector3::dot(s, eye);
    Result(3,1) = -Vector3::dot(u, eye);
    Result(3,2) = Vector3::dot(f, eye);

    Result(3,3) = 1.0f;

    return Result;
}

Matrix Matrix::createPerspective(float fovy, float aspect, float near, float far)
{
    float angle = (fovy / 180.0f) * PI;
    float f = 1.0f / tan( angle * 0.5f );

    return Matrix
    (
        f/aspect,       0,                       0,      0,
               0,       f,                       0,      0,
               0,       0,   (far+near)/(near-far),     -1,
               0,       0,   2*far*near/(near-far),      0
    );
}

Vector4.h

#pragma once

#include "Vector3.h"

#include <iostream>

class Vector4 : public Vector3
{
  public:

    Vector4();
    Vector4(float x, float y, float z, float w);
    Vector4(float x, float y, float z);
    Vector4(float* x, float* y, float* z, float* w);
    ~Vector4() {}

    Vector4 operator= ( const Vector4& v);
    const Vector4 operator* ( const float &scalar) const;
    const Vector4 operator+ (const Vector4 &v) const;

    inline void show() {std::cout << this->x << " " << this->y << " " << this->z << " " << this->w << std::endl;}

  private:

    float w;

    // only to allow do that: m(0) = Vector4(1,1,1,1) //
    float* px;
    float* py;
    float* pz;
    float* pw;

    bool pointer;  // to check what constructor was called //
};

Vector4.cpp

#include "Vector4.h"

Vector4::Vector4()
{
    x = 0.0f;
    y = 0.0f;
    z = 0.0f;
    w = 1.0f;
    pointer = false;
}

Vector4::Vector4 (float x, float y, float z, float w)
{
    this->x = x; this->y = y; this->z = z; this->w = w;
    pointer = false;
}

Vector4::Vector4 (float x, float y, float z)
{
    this->x = x; this->y = y; this->z = z; this->w = 1.0f;
    pointer = false;
}

Vector4::Vector4(float* x, float* y, float* z, float* w)
{
    this->px = x; this->py = y; this->pz = z; this->pw = w;
    this->x = *x; this->y = *y; this->z = *z; this->w = *w;
    pointer = true;
}

Vector4 Vector4::operator= ( const Vector4& v)
{
    if ( pointer )
    {
        *px = x = v.x;
        *py = y = v.y;
        *pz = z = v.z;
        *pw = w = v.w;
    }
    else
    {
        x = v.x;
        y = v.y;
        z = v.z;
        w = v.w;
    }
}

const Vector4 Vector4::operator* ( const float &scalar) const
{
    return Vector4(x*scalar, y*scalar, z*scalar, w*scalar);
}

const Vector4 Vector4::operator+ (const Vector4 &v) const
{
    Vector4 vec;
    vec.x = this->x + v.x;
    vec.y = this->y + v.y;
    vec.z = this->z + v.z;
    vec.w = this->w + v.w;
    return vec;
}

You can see that Vector4 has some funny methods. Is there a better solution to write that code?

  • 1
    How come w is 1.0f by default instead of 0.0f like the other components? – Aaron Franke Aug 5 at 4:17
  • Hi @AaronFranke, right now I don't remember why the default value for w is 1.0 as I was programming this 5 years ago, but probably you have right and it should also be 0.0 – Tom Aug 5 at 19:38
up vote 3 down vote accepted

Don't write the destructor if you don't need it:

~Matrix() {}

This is ugly:

Vector4::Vector4 (float x, float y, float z, float w)
{
    this->x = x; this->y = y; this->z = z; this->w = w;
    pointer = false;
}

// try:

Vector4::Vector4 (float x, float y, float z, float w)
    : x(x), y(y), z(z), w(w)
    , pointer(false)
{}

Don't bother to use the inline keyword.

inline float getRoll() {return atan2(-(*this)(2,0),(*this)(0,0));}      // X //
inline float getPitch() {return asin((*this)(1,0));}                    // Y //
inline float getYaw() {return atan2(-(*this)(1,2),(*this)(1,1));}       // Z //

The code is automatically declared as inline inside the class. The keyword has no real meaning in terms of the compiler in-lining your code. Humans are useless at understanding when in-lining should (should not be done). So let the machine work it out.

Rather than just being able to dump to stdard out. You may want to pramertize that with any stream. Also provide a standard operator<<() that calls it.

void show(std::ostream& out = std::cout);

std::ostream& operator<<(std::ostream& stream, Matrix const& m)
{
    m.show(stream);
    return stream;
}

I don't like the duality of Vector4.
You have controlled it by allowing very limited access into the class. But with a slight tweak you can make the pointer representation the normal without worrying about leaking (any more than you do currently).

  1. If you pass it pointer to the constructor use those.
  2. If you pass in values save them locally but set the pointers up to point at the internal values.
  3. When doing operations always use them via the pointers (as the pointers point at the correct place always).

Look like this now:

Vector4::Vector4()
    :  x(0f),  y(0f),  z(0f),  w(0f)
    , px(&x), py(&y), pz(&z), pw(&w)
{}

Vector4::Vector4 (float ix, float iy, float iz, float iw = 1.0f)
    :  x(ix),  y(iy),  z(iz),  w(iw)
    , px(&x), py(&y), pz(&z), pw(&w)
{}

Vector4::Vector4(float* ix, float* iy, float* iz, float* iw)
    :  x(0f),  y(0f),  z(0f),  w(0f)
    , px(ix), py(iy), pz(iz), pw(iw)
{}

// This should now always work as expected.
Vector4 Vector4::operator= ( const Vector4& v)
{
    *px = *v.px;
    *py = *v.py;
    *pz = *v.pz;
    *pw = *v.pw;
}

I would go one step further.
Consider converting the pointers into references (this only works if the pointers should never be NULL). But it seems like that is the case and will make the code even more readable.

For the Matrix, I’d consider replacing the 16 float constructor with an initializer_list constructor, and adding a copy constructor.

I agree that your Vector4 class is rather ugly. Two ways of accomplishing what you want:

  1. Define a MatrixRow proxy class (e.g. storing a reference to the Matrix and a row number) with a Vector4 assignment operator.
  2. Fundamentally reorganize your Matrix and Vector4 classes to make them views into an underlying, reference counted, storage object, so the row Vector4 shares the storage object of the Matrix, along with an offset into it. The exact details of this can be rather intricate, but I hope you get the general idea.
  • Thank you for your answer, I thought about an initializer_list, but how can I be sure that the list has exactly 16 elements in compile-time? ...or maybe I just don't need to know the list size at the compile-time – Tom Jun 22 '13 at 17:34
  • That’s a good question. Not sure whether such a constraint can be enforced, or whether one would just zero-pad / truncate otherwise. – microtherion Jun 22 '13 at 19:08

Small additional point that was not mentioned in the other answers: since your classes have non trivial constructors, they are not considered as POD (http://en.cppreference.com/w/cpp/concept/PODType), which by itself is not much of a problem. But, that means that your IDENTITY constant will not be stored in the static data section. It will be initialised at the start of the program. And unfortunately, the order of global constant initialisation in C++ is not defined. So if in another module you define another constant which relies on IDENTITY to be constructed, it may use it before it's defined, and lead to some weird behaviour.

I ran into that problem myself a few days ago, and lost 2h trying to figure out what was happening. So I'd just thought I share.

  • Thank you for sharing your knowledge, these are definitely useful information – Tom Aug 7 '17 at 7:31

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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