Vector2 overloaded operators are unsafe

[aerosayan]

Hi,

This is a really great project and I'm very impressed by how much you're able to achieve. Best of luck in future.

I cloned your repo and went through the code, the free function based code architecture is really amazing.

However I observed that the mathematical operator overloads in the Vector2 class (and possibly the other vector variants) are unsafe because they modify the LHS term.

Ex : From file vector2.inl

/// Subtracts the vector @a b from @a a and returns the result.
inline Vector2 operator-(Vector2 a, const Vector2& b)
{
    a -= b;
    return a;
}

We can clearly see that the LHS term is getting modified. This seems to be a very unsafe side effect as this essentially modifies the LHS term in a binary operation.

I haven't checked, but I wonder how the variables in an equation such as Vector2 z = a+b*a-b*a+c; will be modified. One of these partial equations , say a+b will modify the value of a and it will produce bad results in the other partial equations a-b and a+c. The final result will most probably be wrong.

I ran grep -Rn Vector2 on the root of your project and observed that Vector2 isn't really being used much in the whole code base. Most uses were found to be in the file unit_tests.cpp.

static void test_vector2()
{
    {
        const Vector2 a = vector2(1.2f,  4.2f);
        const Vector2 b = vector2(2.7f, -1.9f);
        const Vector2 c = a - b;
        ENSURE(fequal(c.x, -1.5f, 0.0001f));
        ENSURE(fequal(c.y,  6.1f, 0.0001f));
    }
.
.
.

Of course the asserts ensure that the values inside the Vector2 c are correct, but we don't really expect the Vector2 a to be changed, because we might want to use them in the future.

A better alternative might be to define a local Vector2 variable inside the overloaded operator functions and after doing the operations, return it using move semantics--to make it have "good" performance. However we also have the overhead of generating the local Vector2 variable and there is a possibility that the code won't be inlined when we have a big equation. Expression templates provide a good solution in situations like this, but that might be an overkill.

I also see that the code is returning some local variables without using move semantics and might lead to performance loss.

/// Returns the linearly interpolated vector between @a a and @a b at time @a t in [0, 1].
inline Vector2 lerp(const Vector2& a, const Vector2& b, f32 t)
{
    Vector2 v;
    v.x = lerp(a.x, b.x, t);
    v.y = lerp(a.y, b.y, t);
    return v;
}

The overloaded operators can also be removed. But it would be great if a solution was found that made the overloaded operators perform better and was also safe.

Best of luck.

[dbartolini]

Hi and thank you for your kind words,

Regarding your analysis:

1. I fail to see how Vector2::operator-(a, b) is unsafe, given that LHS is passed by value?

2. I know move semantics have their fair use cases (even when used as an optimization technique), but, anyone looking for performance gains in its vector math should be really looking at properly supporting SIMD instead. The math library in Crown is intended for simple generic expressions and RVO is sufficient at removing copy overhead in all practical cases I encountered.

[aerosayan]

My bad. I didn't see that you're passing by value there. Well I kinda feel silly now. lol.