cgmath icon indicating copy to clipboard operation
cgmath copied to clipboard

Matrix3::look_at is implemented wrong (?)

Open FrozenKiwi opened this issue 7 years ago • 6 comments

This is my first time with cgmath/rust/etc, so apologies if this is due to misunderstanding something fundamental about cgmath.

The matrix created by Matrix3::look_at will not map the fwd basis vector (0,0,1) back to it's input. If the function created the transform correctly, then the axis defined as forward (X or Z) when multiplied by the matrix should result in the input fwd vector.

Or in code;

                             let fwd = Vector3::new(1.0, 0.0, 1.0).normalize(); // Any input
                             let up = Vector3::new(0.0, 1.0, 0.0);
                             let side  = up.cross(fwd).normalize();

                             let bad = Matrix3::look_at(fwd, up);

                             let test = Vector3::new(0.0, 0.0, 1.0);
                             let bad_res = bad * test; // Results in (-0.7,0,0.7)

                             let good = Matrix3::from_cols(side, up, fwd);
                             let good_res = good * test; // Results in (0.7,0,0.7). same vec as input

Basically, the transpose makes no sense.

Would you like me to make a pull request? I ask because there seems to be a bit of confusion around look_at

https://github.com/brendanzab/cgmath/pull/412

(Side note) Please make look_at consistent! At a bare minimum with itself, but ideally with common math libraries. Personally, I would only read documentation on a math library as a very last resort, as I assume familiarity with existing implementations should be sufficient.

FrozenKiwi avatar Feb 11 '18 16:02 FrozenKiwi

The bad matrix looks correct to me: http://oi63.tinypic.com/k1dsv5.jpg

It transforms a world coordinate (0.0,0.0,1.0) in red above to the local coordinate (-0.7,0.0,0.7) in the space defined by the blue axes (represented by the look_at matrix)

Is the issue that OpenGL cameras are supposed to have -Z be "in front" of the camera so that it can be mapped to positive Z (positive depth) in NDC coordinates?

spearman avatar Feb 11 '18 21:02 spearman

Thanks for the reply, I suspected the problem might be a different definition of what a look-at is supposed to do...

I find this explanation rather confusing. I kinda understand what your saying: that if the matrix is used for the basis, then the vector (in its original basis, relative to the look-at) is still pointing the same way. Problem is, I kinda don't know how this relates to what is meant by "look at".

Easiest and most obvious use for a look-at is on a camera. Given cam pos, find vec to target (v = ptarg-pcam). Use v as input to look_at, and the result should 'look' at the target. The Z axis (the axis the camera looks down, just ignore negation) should point directly at the target. pcam + t * v == ptarg (for some t). In other words, ori * unit_z should always equal input fwd. However, in the example above, the result is perpendicular.

Lots of other uses for look-ats, eg heads, guns, etc. All involve fwd axis (usually Z) pointing directly at the target.

I'm genuinely curious - what is the use for the look-at in its current state? I've never (in quite some time) found myself looking for an inverted look-at specifically, possibly just never solved this problem? Strange anywho...

FrozenKiwi avatar Feb 12 '18 04:02 FrozenKiwi

If the definition of look_at is disputed, the method could be removed and added as an example, leaving its implementation to consuming libs.

richard-uk1 avatar Apr 22 '18 11:04 richard-uk1

It looks like Matrix3::look_at and Decomposed::look_at are using a LH coordinate system where as Matrix4::look_at is using a RH system. I think it would make sense to have unambiguous methods like look_at_rh and look_at_lh.

aloucks avatar Jun 02 '18 22:06 aloucks

Ah - that could possibly be it? However, it doesn't address the root issue that the original example code as shown fails to do what it claims, which is "look at" the target.

I disagree heartily with the above suggestion though. In this suggestion, when I want a look at, I have to know which matrix I'm using, then figure out which order this version is applied in, then google to figure out which coordinate system I should be using. Absolutely none of which I care about, or in any way improves my results.

It's hard enough remembering in between packages which are LH and RH. To expect a user to remember within a package is an unacceptable mental load.

From a usability perspective, these options are not solving the problem. They are pushing the responsibility for knowing how the library works onto the client. Which defeats the point of having a library in the first place - a library provides a solid foundation so we (as application developers) don't have to think about them.

Make things consistent, simple, and don't make your users memorize everything. And for goodness sake - the results of a function call should match the name of the function!

FrozenKiwi avatar Jun 06 '18 15:06 FrozenKiwi

It's probably worth it to document which handedness Transform::look_at is meant to use (it might be a bug that the Matrix3 and Matrix4 aren't consistent here in their implementations of Transform::look_at?).

I believe there's still value to having separate handedness versions of look_at (and the projection matrix functions). For example, if an app happens to not be in the "default" (i.e. OpenGL) left-handed coordinate system (e.g. the asset pipeline outputs in a right-handed system), knowing explicitly which methods to use is extremely useful.

@FrozenKiwi There fortunately isn't much to memorize—just take note of the handedness of one's assets and use that for look_at_lh/look_at_rh and perspective_lh/perspective_rh.

tangmi avatar Mar 05 '19 09:03 tangmi