Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Transform::{look_at_rh, look_at_lh} and deprecate look_at (continued) #514

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+274 −19
Open

Add Transform::{look_at_rh, look_at_lh} and deprecate look_at (continued) #514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e337856
Add Transform::{look_at_rh, look_at_lh} and deprecate look_at
aloucks Jun 8, 2020
a4ea451
Add more matrix4 and transform tests
aloucks Jun 27, 2020
09593fb
Add matrix3 look_at_[lh|rh] tests
aloucks Jun 27, 2020
dc084e1
Rename Matrix3::look_at_[rh|lh] to look_to_[rh|lh]
aloucks Jun 27, 2020
b9350a3
Remove duplicate code in the deprecated Matrix3::look_at function
aloucks Jun 27, 2020
faa69b6
Rename look_at -> look_to for Matrix2 and related
elrnv Aug 10, 2020
64e88bd
Rename look_at to look_to in Rotation trait
elrnv Aug 10, 2020
121c77b
Fix deprecated look_at usage + remove repeated code
elrnv Aug 10, 2020
f2284a5
Revert look_at test changes
elrnv Aug 10, 2020
d9d89de
Update docs of look_* functions
elrnv Aug 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 96 additions & 5 deletions src/matrix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,29 @@ impl<S> Matrix2<S> {
impl<S: BaseFloat> Matrix2<S> {
/// Create a transformation matrix that will cause `unit_x()` to point at
/// `dir`. `unit_y()` will be perpendicular to `dir`, and the closest to `up`.
#[deprecated = "Use Matrix2::look_to"]
pub fn look_at(dir: Vector2<S>, up: Vector2<S>) -> Matrix2<S> {
Matrix2::look_at_stable(dir, up.x * dir.y >= up.y * dir.x)
Matrix2::look_to(dir, up)
}

/// Crate a transformation that will cause `unit_x()` to point at
/// `dir`. This is similar to `look_at`, but does not take an `up` vector.
/// `dir`. This is similar to `look_to`, but does not take an `up` vector.
/// This will not cause `unit_y()` to flip when `dir` crosses over the `up` vector.
#[deprecated = "Use Matrix2::look_to_stable"]
pub fn look_at_stable(dir: Vector2<S>, flip: bool) -> Matrix2<S> {
Matrix2::look_to_stable(dir, flip)
}

/// Create a transformation matrix that will cause `unit_x()` to point at
/// `dir`. `unit_y()` will be perpendicular to `dir`, and the closest to `up`.
pub fn look_to(dir: Vector2<S>, up: Vector2<S>) -> Matrix2<S> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be look_to_{lh|rh}. I didn't update these because I didn't know what the current implementation was.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a handedness distinction for 2D rotations?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that there is because handedness defines the direction of rotation. However, I had the same (unresolved) question which is why I decided not to modify it in the other PR. My reasoning was that it's probably better to leave it alone until I (or someone else) has time to figure out the correct path forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. I thought handedness determines between coordinate systems. The direction of rotation should be indistinguishable here. I.e. the rotation is the same regardless of which way you turn. For 2D rotations, setting an upside-down up vector should cause the coordinate system to flip, which, I think does the equivalent of what the 3D handedness does. I.e. it flips the sign of the output matrix determinant. Maybe I'm missing something here? It seems like we can either do handedness here without the up vector or leave it as is.

Matrix2::look_to_stable(dir, up.x * dir.y >= up.y * dir.x)
}

/// Crate a transformation that will cause `unit_x()` to point at
/// `dir`. This is similar to `look_to`, but does not take an `up` vector.
/// This will not cause `unit_y()` to flip when `dir` crosses over the `up` vector.
pub fn look_to_stable(dir: Vector2<S>, flip: bool) -> Matrix2<S> {
let basis1 = dir.normalize();
let basis2 = if flip {
Vector2::new(basis1.y, -basis1.x)
Expand Down Expand Up @@ -189,14 +204,27 @@ impl<S: BaseFloat> Matrix3<S> {

/// Create a rotation matrix that will cause a vector to point at
/// `dir`, using `up` for orientation.
#[deprecated = "Use Matrix3::look_to_lh or Matrix3::look_to_rh for the right handed variation"]
pub fn look_at(dir: Vector3<S>, up: Vector3<S>) -> Matrix3<S> {
Matrix3::look_to_lh(dir, up)
}

/// Create a rotation matrix that will cause a vector to point at
/// `dir`, using `up` for orientation.
pub fn look_to_lh(dir: Vector3<S>, up: Vector3<S>) -> Matrix3<S> {
let dir = dir.normalize();
let side = up.cross(dir).normalize();
let up = dir.cross(side).normalize();

Matrix3::from_cols(side, up, dir).transpose()
}

/// Create a rotation matrix that will cause a vector to point at
/// `dir`, using `up` for orientation.
pub fn look_to_rh(dir: Vector3<S>, up: Vector3<S>) -> Matrix3<S> {
Matrix3::look_to_lh(-dir, up)
}

/// Create a rotation matrix from a rotation around the `x` axis (pitch).
pub fn from_angle_x<A: Into<Rad<S>>>(theta: A) -> Matrix3<S> {
// http://en.wikipedia.org/wiki/Rotation_matrix#Basic_rotations
Expand Down Expand Up @@ -333,7 +361,16 @@ impl<S: BaseFloat> Matrix4<S> {

/// Create a homogeneous transformation matrix that will cause a vector to point at
/// `dir`, using `up` for orientation.
#[deprecated = "Use Matrix4::look_to_rh or Matrix3::look_to_lh for the left handed variation"]
pub fn look_at_dir(eye: Point3<S>, dir: Vector3<S>, up: Vector3<S>) -> Matrix4<S> {
Self::look_to_rh(eye, dir, up)
}

/// Creates a homogeneous right-handed look-at transformation matrix.
///
/// This matrix will transform a vector to point in `dir` direction, using `up` for orientation
/// assuming a right-handed corrdinate system.
pub fn look_to_rh(eye: Point3<S>, dir: Vector3<S>, up: Vector3<S>) -> Matrix4<S> {
let f = dir.normalize();
let s = f.cross(up).normalize();
let u = s.cross(f);
Expand All @@ -347,10 +384,35 @@ impl<S: BaseFloat> Matrix4<S> {
)
}

/// Creates a homogeneous left-handed look-at transformation matrix.
///
/// This matrix will transform a vector to point in `dir` direction, using `up` for orientation
/// assuming a left-handed corrdinate system.
pub fn look_to_lh(eye: Point3<S>, dir: Vector3<S>, up: Vector3<S>) -> Matrix4<S> {
Matrix4::look_to_rh(eye, -dir, up)
}

/// Create a homogeneous transformation matrix that will cause a vector to point at
/// `center`, using `up` for orientation.
#[deprecated = "Use Matrix4::look_at_rh or look_at_lh for the left handed variation"]
pub fn look_at(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix4<S> {
Matrix4::look_at_dir(eye, center - eye, up)
Matrix4::look_to_rh(eye, center - eye, up)
}

/// Creates a homogeneous right-handed look-at transformation matrix.
///
/// This matrix will transform a vector to point at `center`, using `up` for orientation
/// assuming a right-handed corrdinate system.
pub fn look_at_rh(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix4<S> {
Matrix4::look_to_rh(eye, center - eye, up)
}

/// Creates a homogeneous left-handed look-at transformation matrix.
///
/// This matrix will transform a vector to point at `center`, using `up` for orientation
/// assuming a left-handed corrdinate system.
pub fn look_at_lh(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix4<S> {
Matrix4::look_to_lh(eye, center - eye, up)
}

/// Create a homogeneous transformation matrix from a rotation around the `x` axis (pitch).
Expand Down Expand Up @@ -1040,7 +1102,17 @@ impl<S: BaseFloat> approx::UlpsEq for Matrix4<S> {
impl<S: BaseFloat> Transform<Point2<S>> for Matrix3<S> {
fn look_at(eye: Point2<S>, center: Point2<S>, up: Vector2<S>) -> Matrix3<S> {
let dir = center - eye;
Matrix3::from(Matrix2::look_at(dir, up))
Matrix3::from(Matrix2::look_to(dir, up))
}

fn look_at_lh(eye: Point2<S>, center: Point2<S>, up: Vector2<S>) -> Matrix3<S> {
let dir = center - eye;
Matrix3::from(Matrix2::look_to(dir, up))
}

fn look_at_rh(eye: Point2<S>, center: Point2<S>, up: Vector2<S>) -> Matrix3<S> {
let dir = eye - center;
Matrix3::from(Matrix2::look_to(dir, up))
}

fn transform_vector(&self, vec: Vector2<S>) -> Vector2<S> {
Expand All @@ -1063,7 +1135,17 @@ impl<S: BaseFloat> Transform<Point2<S>> for Matrix3<S> {
impl<S: BaseFloat> Transform<Point3<S>> for Matrix3<S> {
fn look_at(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix3<S> {
let dir = center - eye;
Matrix3::look_at(dir, up)
Matrix3::look_to_lh(dir, up)
}

fn look_at_lh(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix3<S> {
let dir = center - eye;
Matrix3::look_to_lh(dir, up)
}

fn look_at_rh(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix3<S> {
let dir = center - eye;
Matrix3::look_to_rh(dir, up)
}

fn transform_vector(&self, vec: Vector3<S>) -> Vector3<S> {
Expand All @@ -1084,10 +1166,19 @@ impl<S: BaseFloat> Transform<Point3<S>> for Matrix3<S> {
}

impl<S: BaseFloat> Transform<Point3<S>> for Matrix4<S> {
#[allow(deprecated)]
fn look_at(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix4<S> {
Matrix4::look_at(eye, center, up)
}

fn look_at_lh(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix4<S> {
Matrix4::look_at_lh(eye, center, up)
}

fn look_at_rh(eye: Point3<S>, center: Point3<S>, up: Vector3<S>) -> Matrix4<S> {
Matrix4::look_at_rh(eye, center, up)
}

fn transform_vector(&self, vec: Vector3<S>) -> Vector3<S> {
(self * vec.extend(S::zero())).truncate()
}
Expand Down
4 changes: 2 additions & 2 deletions src/quaternion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,8 +484,8 @@ impl<S: BaseFloat> Rotation for Quaternion<S> {
type Space = Point3<S>;

#[inline]
fn look_at(dir: Vector3<S>, up: Vector3<S>) -> Quaternion<S> {
Copy link
Collaborator

@aloucks aloucks Aug 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be {lh|rh} variants. I wasn't sure about the consistency of the existing implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to consolidate the look functions in Transform and Rotation in a separate PR since it is more involved. This looks like it is at least has consistent naming. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my thinking is that it needs the handedness for it to be consistent and I don't think the implementations are consistent right now (i.e. they're not all LH or all RH and some of the implementations are on the raw structs instead of the traits).

What I think needs to happen is that someone needs to go through the remaining look_ function and determine if it's currently lh or rh and deprecate it in favor of an explicitly named version of that function. While checking these, the corresponding missing lh or rh version can also be added. I think you've done this for Matrix3, but the other ones are missing the handedness suffix. I skipped over the ones I wasn't sure of rather than just renaming them to look_to (without the suffix) because they'll just need to be renamed again.

I was also thinking that it doesn't really make sense to have look_at_ variants for structures that can only represent rotation. I think these should only have look_to_ functions. We should probably sort out these kinds of questions before considering the removal of Rotation.

Copy link
Contributor Author

@elrnv elrnv Aug 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably sort out these kinds of questions before considering the removal of Rotation.

Agreed, so shall we punt that to another PR?
For this PR I am happy with having the look_at vs look_to consistency throughout cgmath.

Matrix3::look_at(dir, up).into()
fn look_to(dir: Vector3<S>, up: Vector3<S>) -> Quaternion<S> {
Matrix3::look_to_lh(dir, up).into()
}

#[inline]
Expand Down
22 changes: 16 additions & 6 deletions src/rotation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,18 @@ where
type Space: EuclideanSpace;

/// Create a rotation to a given direction with an 'up' vector.
#[deprecated = "Use Rotation::look_to"]
fn look_at(
dir: <Self::Space as EuclideanSpace>::Diff,
up: <Self::Space as EuclideanSpace>::Diff,
) -> Self {
Self::look_to(dir, up)
}

/// Create a rotation to a given direction with an 'up' vector.
fn look_to(
dir: <Self::Space as EuclideanSpace>::Diff,
up: <Self::Space as EuclideanSpace>::Diff,
) -> Self;

/// Create a shortest rotation to transform vector 'a' into 'b'.
Expand Down Expand Up @@ -169,9 +178,9 @@ pub struct Basis2<S> {
}

impl<S: BaseFloat> Basis2<S> {
pub fn look_at_stable(dir: Vector2<S>, flip: bool) -> Basis2<S> {
pub fn look_to_stable(dir: Vector2<S>, flip: bool) -> Basis2<S> {
Basis2 {
mat: Matrix2::look_at_stable(dir, flip),
mat: Matrix2::look_to_stable(dir, flip),
}
}
}
Expand Down Expand Up @@ -208,9 +217,9 @@ impl<S: BaseFloat> Rotation for Basis2<S> {
type Space = Point2<S>;

#[inline]
fn look_at(dir: Vector2<S>, up: Vector2<S>) -> Basis2<S> {
fn look_to(dir: Vector2<S>, up: Vector2<S>) -> Basis2<S> {
Basis2 {
mat: Matrix2::look_at(dir, up),
mat: Matrix2::look_to(dir, up),
}
}

Expand Down Expand Up @@ -362,10 +371,11 @@ impl<'a, S: 'a + BaseFloat> iter::Product<&'a Basis3<S>> for Basis3<S> {
impl<S: BaseFloat> Rotation for Basis3<S> {
type Space = Point3<S>;

/// Construct a look-at view matrix assuming a left-handed coordinate system.
#[inline]
fn look_at(dir: Vector3<S>, up: Vector3<S>) -> Basis3<S> {
fn look_to(dir: Vector3<S>, up: Vector3<S>) -> Basis3<S> {
Basis3 {
mat: Matrix3::look_at(dir, up),
mat: Matrix3::look_to_lh(dir, up),
}
}

Expand Down
41 changes: 39 additions & 2 deletions src/transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,22 @@ use std::ops::Mul;
pub trait Transform<P: EuclideanSpace>: Sized + One {
/// Create a transformation that rotates a vector to look at `center` from
/// `eye`, using `up` for orientation.
fn look_at(eye: P, center: P, up: P::Diff) -> Self;
#[deprecated = "Use look_at_rh or look_at_lh"]
fn look_at(eye: P, center: P, up: P::Diff) -> Self {
Self::look_at_lh(eye, center, up)
}

/// Creates a right-handed look-at view transform.
///
/// This transform rotates a vector to look at `center` from `eye`, using `up` for orientation
/// assuming a right-handed coordinate system.
fn look_at_rh(eye: P, center: P, up: P::Diff) -> Self;

/// Creates a right-handed look-at view transform.
///
/// This transform rotates a vector to look at `center` from `eye`, using `up` for orientation
/// assuming a left-handed coordinate system.
fn look_at_lh(eye: P, center: P, up: P::Diff) -> Self;

/// Transform a vector using this transform.
fn transform_vector(&self, vec: P::Diff) -> P::Diff;
Expand Down Expand Up @@ -104,7 +119,29 @@ where
{
#[inline]
fn look_at(eye: P, center: P, up: P::Diff) -> Decomposed<P::Diff, R> {
let rot = R::look_at(center - eye, up);
let rot = R::look_to(center - eye, up);
let disp = rot.rotate_vector(P::origin() - eye);
Decomposed {
scale: P::Scalar::one(),
rot: rot,
disp: disp,
}
}

#[inline]
fn look_at_lh(eye: P, center: P, up: P::Diff) -> Decomposed<P::Diff, R> {
let rot = R::look_to(center - eye, up);
let disp = rot.rotate_vector(P::origin() - eye);
Decomposed {
scale: P::Scalar::one(),
rot: rot,
disp: disp,
}
}

#[inline]
fn look_at_rh(eye: P, center: P, up: P::Diff) -> Decomposed<P::Diff, R> {
let rot = R::look_to(eye - center, up);
let disp = rot.rotate_vector(P::origin() - eye);
Decomposed {
scale: P::Scalar::one(),
Expand Down
Loading