glm::lookAt() superfluous up normalization

[Roaoul]

Hi,

In gtc/matrix_transform.inl, the lookAt function code includes a superfluous normalization of the up vector. This error was possibly copied from the GLU man page for gluLookAt (man page error acknowledged by Jon Leech). The current GLM (0.9.4.5) source code reads:

    detail::tvec3<T> f = normalize(center - eye);
    detail::tvec3<T> u = normalize(up);      // Accomplishes nothing
    detail::tvec3<T> s = normalize(cross(f, u));
    u = cross(s, f);

The code should read:

    detail::tvec3<T> f = normalize(center - eye);
    detail::tvec3<T> s = normalize(cross(f, up)); // changed u to up in cross.
    u = cross(s, f);

This is the GLU source code (correct), according to Jon Leech:

normalize(forward);
/* Side = forward x up */
cross(forward, up, side);
normalize(side);
/* Recompute up as: up = side x forward */
cross(side, forward, up);
... construct view matrix

This fix should not alter the correctness of the glm::lookAt result. It only removes a pointless calculation.

[Groovounet]

Hi,

I noticed your bug report in the mailing list but I didn't realized it was for GLM.

Your proposed change is correct and it will be applied in GLM 0.9.5.

Thanks for contributing,
Christophe

[Roaoul]

My pleasure! Thanks for a great product!!
Paul

On Thu, Sep 12, 2013 at 6:01 PM, Christophe Riccio <notifications@github.com

wrote:

Hi,

I noticed your bug report in the mailing list but I didn't realized it was
for GLM.

Your proposed change is correct and it will be applied in GLM 0.9.5.

Thanks for contributing,
Christophe

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/114#issuecomment-24366866
.

[Roaoul]

Oops. I removed the allocation of u.

The code should read:

detail::tvec3<T> f = normalize(center - eye);
detail::tvec3<T> s = normalize(cross(f, up)); // changed u to up in cross.
detail::tvec3<T> u = cross(s, f);

[Groovounet]

This bug is now fixed in GLM 0.9.5 branch.

Thanks for contributing,
Christophe

[Roaoul]

Thanks!!

On Tue, Sep 17, 2013 at 3:15 PM, Christophe Riccio <notifications@github.com

wrote:

This bug is now fixed in GLM 0.9.5 branch.

Thanks for contributing,
Christophe

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/114#issuecomment-24627061
.