search

AcademySoftwareFoundation / Imath

Imath is a C++ and python library of 2D and 3D vector, matrix, and math operations for computer graphics
https://imath.readthedocs.io
BSD 3-Clause "New" or "Revised" License
389 stars 117 forks source link

Nonsensical Matrix22::multDirMatrix #62

Open lgritz opened 4 years ago

lgritz commented 4 years ago

Matrix44 methods multDirMatrix and multVecMatrix operate on Vec3, doing the math as if it were Vec4 with the last component of 0 and 1, respectively, and then projecting back to 3D with a homogeneous divide. (For those who don't know, this is what allows 4x4 matrices to transform 3D points and vectors and capture effects such as translation and perspective projection.) Matrix33 has the same methods, operating on Vec2, performing 2D transformations with homogeneous 3x3 matrix.

Matrix22 also has multDirMatrix... but it operates on a Vec2 and has no homogeneous divide. So the nomenclature is incorrect, this is not a "direction" kind of multiply. It's just a regular mathematical 2x2 matrix transforming a 2D point (i.e., a synonym for the operator*(Vec2,Mat22)), not the equivalent operation of the Matrix33 and 44 methods having that name.

Does this bother anyone else? Should we maybe just remove this Matrix22 method?

meshula commented 4 years ago

Agree. I can't remember why we called these multDir and multVec in the first place (something about "Dir implies no translation" or something. I vaguely recall it felt clever at the time, but now I'm always forced to check the implementations to find out which variant has the math I want in it. Sigh

lgritz commented 4 years ago

We could add multPoint for clarity, and while leaving multVec for back compatibility, at least comment that multVec is a deprecated name because it's not clear what kind of vector it is.