doyubkim / fluid-engine-dev

Fluid simulation engine for computer graphics applications
https://fluidenginedevelopment.org/
MIT License
1.88k stars 263 forks source link

makeTranslationMatrix should be transposed #323

Closed utilForever closed 2 years ago

utilForever commented 2 years ago

According to https://github.com/CubbyFlow/CubbyFlow/issues/125, Matrix is row-major so the translation part should be on ele[12], ele[13] ele[14]. I'll fix it ASAP. Thanks to @yangfengzzz!

doyubkim commented 2 years ago

Wait, are you sure? Matrix's ij convension is different than array -- ij and xy is in different order.

doyubkim commented 2 years ago

4 * i + j is correct. It points to ith row and jth column. So in a flat memory, column index moves faster.

utilForever commented 2 years ago

Agree. I think that 4 * i + j is correct, too. I'm confusing this logic:

template <typename T>
Matrix<T, 4, 4> Matrix<T, 4, 4>::makeTranslationMatrix(const Vector<T, 3>& t) {
    return Matrix(
        1, 0, 0, t.x,
        0, 1, 0, t.y,
        0, 0, 1, t.z,
        0, 0, 0, 1);
}
doyubkim commented 2 years ago

Hmm that's what a translation matrix should be when you perform matrix-vector multiplication in MV order. Some libraries use VM order, and then the matrix should be transposed.

utilForever commented 2 years ago

I totally understand. It's been a long time since I learned, so I think I was confused with the concept. It was my mistake. Thank you for your explanation and I'll close this issue.

doyubkim commented 2 years ago

No worries. Thanks for opening up the issue regardless.

doyubkim commented 2 years ago

@utilForever: to be fair, I do understand the confusion since I'm actually mixing row major matrix with column vector in Jet framework. Typically you would interpret a vector as a flat matrix when matrix system is row major. (See this post: https://fgiesen.wordpress.com/2012/02/12/row-major-vs-column-major-row-vectors-vs-column-vectors/)

Anyhow, that's how Jet framework defines matrix and vector at the moment. I was thinking of moving to column major once, but decided that it's not worth the effort.

yangfengzzz commented 2 years ago

Ok, I got it. With row major matrix and column vector, the code is correct. But I think it is better to use row major matrix and row vector.