rustgd / cgmath

A linear algebra and mathematics library for computer graphics.
https://docs.rs/cgmath
Apache License 2.0
1.12k stars 155 forks source link

Fix Matrix2::look_at, add look_at_stable #491

Closed blargg closed 5 years ago

blargg commented 5 years ago

Changes

  1. Fixes Matrix2::look_at
  2. Adds tests for look_at
  3. Adds a new function, look_at_stable

Notes

I added a new function for 2d look at rotation. look_at is a bit weird in practice for 2d. For example, if you are making a basis matrix to orient a 2d character to look at a point, look_at will flip the character as they rotate past up or -up vectors. This is the best match for what look_at is supposed to do, I think.

look_at_stable will not flip based on orientation, you just pass in which way to flip. This is a bit easier to use to rotate 2d characters.

look_at_stable could have a better name. I think we can also consider removing the flip param, and just let the user flip the matrix with a transform later.

kvark commented 5 years ago

We don't need the normalization for "flip" flag computation

On Sep 3, 2019, at 16:00, blargg notifications@github.com wrote:

@blargg commented on this pull request.

In src/matrix.rs:

 pub fn look_at(dir: Vector2<S>, up: Vector2<S>) -> Matrix2<S> {
  • //TODO: verify look_at 2D
  • Matrix2::from_cols(up, dir).transpose()
  • let basis1 = dir.normalize();
  • let basis2 = if up.x basis1.y >= up.y basis1.x { Sounds good, it's way better to call into the same function for the common part. I think the only concern with that was that it would call noramlize on an already normalized vector, we would first normalize in look_at and then again when it calls look_at_stable.

Would you prefer if I Option 1: made a private function that is like look_at_stable, but assumes that the vector is normalized, that each of these call into, or Option 2: ignored the duplicate work, and call look_at_stable from look_at as you describe.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

blargg commented 5 years ago

Oh, thanks! I had missed the part of the initial comment. Added commit to the PR.

blargg commented 5 years ago

Sounds good, just updated.

kvark commented 5 years ago

bors r+

bors[bot] commented 5 years ago

Build succeeded