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

elrnv · 2020-08-11T00:51:32Z

This is a continuation of #508 with the following additions:

_lh and _rh suffixes are explained in each affected function.
look_at is renamed to look_to on Matrix2 and 2D rotations for consistency.
Other uses of look_at were changed to look_to as necessary in the Rotation trait.
Remove internal usage of deprecated functions.

These changes make look_at and look_to functions consistent in meaning throughout the crate (with exception of deprecated functions).

aloucks · 2020-08-11T04:07:25Z

src/matrix.rs

+
+    /// 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> {


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

Is there a handedness distinction for 2D rotations?

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.

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.

aloucks · 2020-08-11T04:08:18Z

src/quaternion.rs

@@ -474,7 +474,7 @@ impl<S: BaseFloat> From<Quaternion<S>> for Basis3<S> {

 impl<S: BaseFloat> Rotation<Point3<S>> for Quaternion<S> {
    #[inline]
-    fn look_at(dir: Vector3<S>, up: Vector3<S>) -> Quaternion<S> {


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

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?

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.

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.

kvark · 2020-11-23T16:23:54Z

Could somebody sum up the status of this PR? Is there a consensus, and what's left to do?

elrnv · 2020-11-23T18:54:59Z

IIRC this PR aims to get consistent naming for _to and _at variants of the look_ functions on top of #508.

It does this to a certain extent without committing to major changes to the Rotation and Transform APIs. In particular, this PR changes look_at to look_to in Rotation without the _lh or _rh suffixes for now in hopes of properly resolving the overlap between the Rotation and Transform APIs in another PR since I don't have the time at the moment to dwell on this. The remaining decision I think is whether to roll back the changes to the Rotation trait or keep it as is since it has better consistency for the time being, and it's not clear that the next PR will come any time soon.

The other point to resolve is whether Matrix2 needs _lh and _rh variants for the look_to function. In my opinion it does not, but I am more than happy to either revert that change or do something else there.

#508 leaves the look_to and look_at function variants in an inconsistent state to (I think) avoid breaking the APIs of things like Rotation and Matrix2 temporarily before a more appropriate solution is found. Given the cadence of this crate, I think the changes in this PR are appropriate to get some consistency albeit not necessarily with respect to _lh and _rh variants of the look_ functions.

Corresponding functions have been added to Matrix4 and Decomposed and are now consistent. Matrix3, Matrix2, and the Rotation trait are only partially updated.

This makes the Matrix3/4 and Decomposed `look_at_*` functions consistent with looking at a center/focus point and `look_to_*` functions consistent with looking in a direction.

Rename functions look_at* to look_to* on Matrix2 and related rotation transforms.

Remove a repated instance of Matrix4 look_at transform.

Previously look_at functions were erroneously renamed to look_to_{lh|rh} as a response to deprecation warnings. Instead what should've happened is that `allow(deprecated)` annotations are fixed to suppress tests that verify that deprecated functions are working. This commit reintroduced the tests for deprecated functions and suppresses the deprecation warnings.

This change clarifies the meaning of the _lh and _rh suffixes.

thatcomputerguy0101 · 2024-07-13T17:37:52Z

I believe the current implementation of look_at/to in Matrix2 is suited for 2d spritework, where in order for a sprite to maintain an "upwards" orientation while looking in the opposite direction, it must mirror itself. It is a related, but different operation than in the Matrix3 and Matrix4 implementations. Even with that in mind, I would still favor renaming it to look_to instead of look_at.

aloucks reviewed Aug 11, 2020

View reviewed changes

aloucks mentioned this pull request Nov 22, 2020

Release 0.18? #499

Closed

aloucks and others added 10 commits November 23, 2020 14:07

Add Transform::{look_at_rh, look_at_lh} and deprecate look_at

e337856

Corresponding functions have been added to Matrix4 and Decomposed and are now consistent. Matrix3, Matrix2, and the Rotation trait are only partially updated.

Add more matrix4 and transform tests

a4ea451

Add matrix3 look_at_[lh|rh] tests

09593fb

Rename Matrix3::look_at_[rh|lh] to look_to_[rh|lh]

dc084e1

This makes the Matrix3/4 and Decomposed `look_at_*` functions consistent with looking at a center/focus point and `look_to_*` functions consistent with looking in a direction.

Remove duplicate code in the deprecated Matrix3::look_at function

b9350a3

Rename look_at -> look_to for Matrix2 and related

faa69b6

Rename functions look_at* to look_to* on Matrix2 and related rotation transforms.

Rename look_at to look_to in Rotation trait

64e88bd

Fix deprecated look_at usage + remove repeated code

121c77b

Remove a repated instance of Matrix4 look_at transform.

Update docs of look_* functions

d9d89de

This change clarifies the meaning of the _lh and _rh suffixes.

elrnv force-pushed the look_at branch from 46713c9 to d9d89de Compare November 23, 2020 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

elrnv commented Aug 11, 2020 •

edited

Loading

aloucks Aug 11, 2020

elrnv Aug 12, 2020

aloucks Aug 12, 2020

elrnv Aug 13, 2020

aloucks Aug 11, 2020 •

edited

Loading

elrnv Aug 12, 2020

aloucks Aug 12, 2020

elrnv Aug 13, 2020 •

edited

Loading

kvark commented Nov 23, 2020

elrnv commented Nov 23, 2020

thatcomputerguy0101 commented Jul 13, 2024

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

Are you sure you want to change the base?

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

Conversation

elrnv commented Aug 11, 2020 • edited Loading

aloucks Aug 11, 2020

Choose a reason for hiding this comment

elrnv Aug 12, 2020

Choose a reason for hiding this comment

aloucks Aug 12, 2020

Choose a reason for hiding this comment

elrnv Aug 13, 2020

Choose a reason for hiding this comment

aloucks Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

elrnv Aug 12, 2020

Choose a reason for hiding this comment

aloucks Aug 12, 2020

Choose a reason for hiding this comment

elrnv Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

kvark commented Nov 23, 2020

elrnv commented Nov 23, 2020

thatcomputerguy0101 commented Jul 13, 2024

elrnv commented Aug 11, 2020 •

edited

Loading

aloucks Aug 11, 2020 •

edited

Loading

elrnv Aug 13, 2020 •

edited

Loading