4x4 matrix implementation in C++

I've been doing some 3D graphics in OpenGL lately and I needed a way to work with 4x4 matrices. My implementation supports the following operations:

• Matrix-matrix addition.

• Matrix-matrix subtraction.

• Matrix-matrix multiplication.

• Transformation matrix creation.

• View matrix creation.

• Perspective projection matrix creation.

• Orthographic projection matrix creation.

matrix.h

```
#ifndef MATRIX_H_
#define MATRIX_H_
#define _USE_MATH_DEFINES

#include
#include
#include
#include
#include
#include
#include
#include
#include
#include "SWOGLL.h"
#include "vector4.h"

struct Matrix4x4
{
GLfloat m_elements[16];

Matrix4x4();
Matrix4x4(GLfloat elements[]);
GLfloat* GetElementsPointer();
static Matrix4x4 CreateScale(Vector4 scale);
static Matrix4x4 CreateTranslation(Vector4 translation);
static Matrix4x4 CreateRotationX(GLfloat angle);
static Matrix4x4 CreateRotationY(GLfloat angle);
static Matrix4x4 CreateRotationZ(GLfloat angle);
static Matrix4x4 CreateView(Vector4 forward, Vector4 up, Vector4 right, Vector4 position);
static Matrix4x4 CreatePerspectiveProjection(GLfloat width, GLfloat height, GLfloat fov, GLfloat nearPlane, GLfloat farPlane);
static Matrix4x4 CreateOrthographicProjection(GLfloat left, GLfloat right, GLfloat top, GLfloat bottom, GLfloat farPlane, GLfloat nearPlane);

inline Matrix4x4& operator+=(const Matrix4x4& rhs)
{
for(int i = 0; i m_elements[i] += rhs.m_elements[i];
}
return *this;
}

inline Matrix4x4& operator-=(const Matrix4x4& rhs)
{
for(int i = 0; i m_elements[i] -= rhs.m_elements[i];
}
return *this;
}

inline Matrix4x4& operator*=(const Matrix4x4& rhs)
{
this->m_elements[0] = this->m_elements[0] rhs.m_elements[0] + this->m_elements[1] rhs.m_elements[4] + this->m_elements[2] rhs.m_elements[8] + this->m_elements[3] rhs.m_elements[12];
this->m_elements[1] = this->m_elements

All in all I really like your code. It's good and easy to read, and there's nothing really wrong with it AFAICS. In the following I'm writing down whatever's coming to my mind what could theoretically be improved/changed. Therefore don't take that as a list of things to change, but rather as points to think about.

Too much of this

Whenever you refer the data members of your class, you use this:

this->m_elements[i] = elements[i];

I think you do this to clearly separate between local variables / function parameters / global variables and the data members of the class. I've done this, too. But I think it's a reasonable assumption to make that somebody reading your code knows that data members can be accessed without this, and to know (at least roughly) the rules regarding name resolution.

And, moreover, you already have something in place to distinguish between data members and "the rest": That m_ prefix. I read that as member_elements, or alternatively as matrix_elements.

Having both just means more to type and more to read. And the long lines suffer somewhat from that "more". Compare:

this->m_elements[0] = this->m_elements[0] * rhs.m_elements[0] + this->m_elements[1] * rhs.m_elements[4] + this->m_elements[2] * rhs.m_elements[8] + this->m_elements[3] * rhs.m_elements[12];

// No this
m_elements[0] = m_elements[0] * rhs.m_elements[0] + m_elements[1] * rhs.m_elements[4] + m_elements[2] * rhs.m_elements[8] + m_elements[3] * rhs.m_elements[12];

This point is highly debatable though, so if that's just "your style" then feel free to ignore it ;)

C style arrays are out of fashion

Instead of

GLfloat m_elements[16];

you could (should?) use

std::array m_elements;

The benefit is more information - the array size - for you (and the compiler) in some circumstances. A C style array "decays" (is implicitly converted) to a pointer to its first element in most situations. This conversion loses the information about the array size.

I doubt that this has any benefit to you in your particular case, though, because you can use ...

range based for

... even with plain C arrays, too. This saves you from accidentally wrong indexing and other, often hard to find bugs:

// your code
for(int i = 0; i m_elements[i] *= rhs;
}

// with range based loop
for(GLfloat & element : m_elements)
{
    element *= rhs;
}

You can also go one step further and use std::for_each, possibly making use of execution policies when (if) you switch to C++17.

Getting rid of more "manual" for loops

Using std::transform you can get rid of more of these manual indexing loops:

// your code
for(int i = 0; i m_elements[i] += rhs.m_elements[i];
}

// using std::transform
std::transform(std::begin(m_elements), std::end(m_elements),
               std::begin(rhs.m_elements),
               std::begin(m_elements),
               [] (GLfloat const & l, GLfloat const & r) { return l + r; });

Whether that's better in terms of readability is ... eh ... let's say "debatable", too. And of course it changes semantics: operator+ and operator= are now used, instead of operator+=.

Instead you could also keep the manual indexing, but make sure you run over the correct range:

size_t const number_elements = 
    std::distance(std::begin(m_elements), std::end(m_elements));
// or with std::array use
//  m_elements.size();
for(size_t i = 0; i < number_elements; i++)
{
    m_elements[i] += rhs.m_elements[i];
}

Oh, and I like indexing with size_t as that's what's returned from the various size() member functions and may protect you from fallacies with the "usual arithmetic conversions" between unsigned and signed integer types. And of course when dealing with large amounts of data, int might be too narrow to hold possible indices. But of course, this doesn't apply here.

memset can be dangerous, better avoid it

Instead of memset you could use the C++ (almost) equivalent std::fill. memset is a bad idea when using anything but plain old data types, and even then I'm not sure whether aliasing (and in the future also "laundering" or lack thereof) could still cause undefined behavior. Thus I recommend to avoid it.

DRY using the evil preprocessor

The definitions of operator+, operator- and the two operator* are almost identical. You can use a macro to save you from that repetition:

#define OP_FROM_SELF_ASSIGN_OP(op, lhstype, rhstype)         \
  inline Matrix4x4 operator op (lhstype lhs, rhstype rhs) \
  {                                                          \
      Matrix4x4 newMatrix = Matrix4x4(lhs);                  \
      newMatrix op ## = rhs;                                 \
      return newMatrix;                                      \
  }

Note: Untested, it may be necessary to have the token concatenation (##) in a helper macro.

Using it as such:

```
OP_FROM_SELF_ASSIGN_OP(+, const Matrix4x4&, const Matrix4x4&)
OP_FROM_SELF_ASSIGN_OP(

StackExchange Code Review Q#144381, answer score: 4