2
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I don't have time this second to address all of your points, but here's some reasoning behind a few: Currently I only support row-major matrices, similar to Monogame/XNA, OpenTK, and other frameworks. This means that my [] operator must be defined to return the rows (but no argument about storing them in an array, that leads to solving a few of my problems I think). I'd also prefer to not make my Matrix4 and Vector4 classes immutable, when I said that I was referring to the fact that since the row vectors are returned from the [] property, they can't be modifed since the property is a method. \$\endgroup\$ – Pip Feb 10 '16 at 15:26
  • \$\begingroup\$ Yes, to make them immutable is absolutely optional. However if they are classes and you have setters in vector then they also matrix is mutable. Eve not if you gave just row vectors I would make it clear...at least in parameter name. It's for free for future readers \$\endgroup\$ – Adriano Repetti Feb 10 '16 at 15:32
  • \$\begingroup\$ So I eventually decided to go the route of having [] return elements 1-16 of the matrix, and having [,] return [row, col]. Elements are stored in fields M11-M44, consistent with other frameworks, with the side effect that they can be used as column vectors as well. Thank you! I've taken a lot of your advice to apply. \$\endgroup\$ – Pip Feb 12 '16 at 20:43

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.