Matrix column switching

Question

I'm currently working in the math library for a game framework that I'm writing and am working to improve some functionality that was affected during a refactor.

Formerly, my Matrix4.SwapColumns(int, int) function looked like this:

/// Documentation
public void SwapCols(int i, int j)
{
    for(int k = 0; k < 4; k++)
    {
        temp = this[k][i];
        this[k][i] = this[k][j];
        this[k][j] = temp;*/
    }
}

This worked because Matrix4[x][y] was a two-dimensional float array, which could be accessed and changed in one step. Now, since I'm using a new version of the operator as follows:

public Vector4 this[int a]
{
    get
    {
        if (a > 3 || a < 0) return Vector4.Zero;
        return (a == 0 ? row1 : (a == 1 ? row2 : (a == 2 ? row3 : row4)));
    }
    set
    {
        if (a == 0) row1 = value;
        else if (a == 1) row2 = value;
        else if (a == 2) row3 = value;
        else if (a == 3) row4 = value;
    }
}

The original method is no longer possible since the values that are part of the Vector4 are immutable, due to a C# language feature relating to properties (which I usually love having, since it makes sure you don't think you're reassigning the variable in one call to the properties' actual method implementations).

Here's the code review I need. Essentially, the new SwapColumns method looks like this:

/// Documentation
public void SwapCols(int i, int j)
{
    // New arguement checking goes here
    int[] indicies = new int[] { 0, 1, 2, 3 };
    indicies[i] = j;
    indicies[j] = i;

    Vector4 tempRow = Vector4.Zero;

    int[] positions = new int[4];
    for (int row = 0; row < 4; row++)
    {
        tempRow = this[row];
        for (int col = 0; col < 4; col++)
        {
            if (indicies[col] != col)
            {
                tempRow[col] = this[row][indicies[col]];
            }
        }
        this[row] = tempRow;
    }
}

Without changing the fact that my Matrix4[] returns a Vector4, since that API needs to stay the same, how can I improve this SwapCols method?

Answer 1

Vector4

An invalid input (out of range index) should never silently return an arbitrary result. If you just return Vector4.Zero you will hide bugs in your code. Computations will be wrong and you will need to debug step by step your code to understand where. Especially for complex expressions this is tedious and time consuming. Throw an exception for wrong inputs!

---

Also you should use a proper name for parameters, a is almost meaningless.

if (index > 3 || index < 0)
    throw new ArgumentOutOfRangeException("index", index, "Must be in range [0..4)");

---

• Logic to return element value is hard to read:

return (a == 0 ? row1 : (a == 1 ? row2 : (a == 2 ? row3 : row4)));

Let's see few alternatives:

switch (index) {
    case 0: return row1;
    case 1: return row2;
    case 2: return row3;
    case 3: return row4;
    default:
        throw new ArgumentOutOfRange(...);    
}

Or:

if (index == 0)
    return row1;
else if (index == 1)
    return row2;
else if (index == 2)
    return row3;
else if (index == 3)
    return row4;
else
    throw new ArgumentOutOfRange(...);

Why did you name row1 variable you access with index 0? It's an implementation detail but it may cause subtle bugs. Keep names self-descriptive and coherent (BTW why vector elements are called rowX, can't it be a column vector?)

However bigger problem is logic itself. There is no reason to have four separate variables. Array access is pretty fast and highly optimized (moreover using that if logic you may highly impact performance if JIT compiler won't generate CMOV assembly instruction instead of multiple CMP and JZ (because they're not branch-predictor friendly). Your final code should then be:

private Vector4 _columns = ...

public Vector4 this[int columnIndex]
{
    get { return _columns[columnIndex]; }
    set
    {
        if (value == null)
            throw new ArgumentNullException();

        _vectors[columnIndex] = value; 
    }
}

Matrix4

With Vector4 simplified code you will also simplify SwapColumns(), essentially it may be your original unchanged code. What's the C# feature related to properties you want to have? If Vector4 is an immutable type then you may go with a struct (and note it may be mutable internally).

In general if you're coding for performance in a multi-threaded environment (like a game) I think you should seriously consider to keep your Matrix4 an immutable struct. When you really need to swap columns you create a new object. If it's not an operation you perform thousands times per second it will let you (and eventually JIT compiler) to perform optimizations and avoid locks.

Also take a look to SIMD extensions in .NET Framework. SIMD instructions may greatly improve performance.