64 / 64.github.io

Delta blog
https://64.github.io
MIT License
25 stars 5 forks source link

tonemap.cpp: Error in mul function #2

Closed AnotherCommander closed 2 years ago

AnotherCommander commented 2 years ago

Hi

The tone mapping article is great and very informative and helpful. However, I think there is an error there in the mul function code used for ACES. The error is replicated also in this repository in tonemap.cpp. In the article it reads:

vec3 mul(const std::array<vec3, 3>& m, const vec3& v)
{
    float x = m[0][0] * v[0] + m[0][1] * v[1] + m[0][2] * v[2];
    float y = m[1][0] * v[1] + m[1][1] * v[1] + m[1][2] * v[2];
    float z = m[2][0] * v[1] + m[2][1] * v[1] + m[2][2] * v[2];
    return vec3(x, y, z);
}

but I think the correct way to multiply the matrix with the vector would be:

vec3 mul(const std::array<vec3, 3>& m, const vec3& v)
{
    float x = m[0][0] * v[0] + m[0][1] * v[1] + m[0][2] * v[2];
    float y = m[1][0] * v[0] + m[1][1] * v[1] + m[1][2] * v[2];
    float z = m[2][0] * v[0] + m[2][1] * v[1] + m[2][2] * v[2];
    return vec3(x, y, z);
}

This had me headdesking for a good while because I couldn't get my scene to render as expected, until I noticed the two v[1]s near the start of the float y and float z lines.

64 commented 2 years ago

Oof, good spot. Sorry about that. I'll fix this later today.

64 commented 2 years ago

Fixed in 936048cc07bf8740fc4b64257114d1732a10514f, thanks.