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

[aloucks]

The corresponding functions have been added to Matrix4 and Decomposed and are now consistent.

Matrix3, Matrix2, and the Rotation trait are only partially updated. The look_at methods on these should probably be renamed to look_to_[lh|rh] as they take a direction vector, not a focus point. Does handedness even make sense for Matrix2?

The older functions have all been left in-tact and marked deprecated. I don't think the old functionality should be removed in the near or medium term.

[aloucks]

The Matrix4 and Decomposed look_at_[rh|lh] and look_to_[lh|rh] functions now produce the same results as glam and directx math. Feedback and verification that the Matrix3 functions work as expected would be helpful.

[kvark]

Thank you for this work! Having a couple of suggestions here

[elrnv]

Since this is still open, and we have a chance to clean up the API with this PR a bit. I want to squeeze in a few suggestions:

• It would be helpful to have the distinction between _lh and _rh calls in the docs somewhere,
• At a first glance it seems that look_at on Rotation can be deprecated and all of look_{at|to}_{lh|rh} functions can be moved to Transform. Then Transform could be implemented on Basis# types to fill in that functionality. I think this could clear up some of the confusion between Rotation and Transform at the cost of some functionality duplication (e.g. between rotate_{point|vector} and transform_{point|vector}).
• The one liners could probably be marked #[inline].
• look_at on Matrix2 should perhaps be renamed to look_to for consistency.

[kvark]

These are interesting ideas, and they seem reasonable at the first glance. @aloucks could you check and see if this resonates with your vision?

[aloucks]

Rotation::look_at accepts a direction vector and should definitely be renamed to look_to_{lh|rh}. The difference between the Rotation and Transform variants of these functions is that the Rotation ones do not take in the eye position. I don't know if it makes sense to implement Transform for the Basis# and Quaternion structs.

I agree that Matrix2::look_at should also be renamed to look_to_{lh|rh}. I don't know what the current implementation is and there is some convolution with Rotation and Matrix3 so I opted to leave it alone for the time being.

It's also confusing where some structs have the look_* functions implemented on the struct itself and others solely rely on the trait implementations. This should be made consistent as well.

The Rotation and Transform traits are a bit wound up together along with the look_ functions. There needs to be further refactoring but I didn't want to bite off more than I could chew and opted to make the minimal change as the first step. I agree with @elrnv in general about cleaning up the API, but this PR was meant to be the first step, not the final result :)

[elrnv]

That's fair, thanks for the detailed response :)

I think I get the scope of this PR. Then I'd be happy with just getting the naming of the look_ stuff to be consistent with this PR.

[kvark]

@aloucks @elrnv are we blocked on any decision making here, or just can't find time to finish this?

[elrnv]

If it's the latter, I don't mind filling in the last renaming changes, or otherwise submitting another PR for that.

[aloucks]

If it's the latter, I don't mind filling in the last renaming changes, or otherwise submitting another PR for that.

Excellent! It would be great if you want to address the other changes in a separate PR.

[aloucks]

@kvark It looks like the build failure is due to nightly simd.

[kvark]

Looks like a legitimate test issue on CI:

 ---- operators::test_iter_product stdout ----
thread 'operators::test_iter_product' panicked at 'assertion failed: `(left == right)`
  left: `Quaternion { v: Vector3 [-0.5862987, 0.44298634, 0.5764234], s: 0.3574254 }`,
 right: `Quaternion { v: Vector3 [-0.3574254, -0.5764234, 0.4429863], s: -0.58629864 }`', tests/quaternion.rs:108:9

I wonder if it's caused by #500 but left unnoticed?

[aloucks]

@kvark Yeah, It was caused by #500, but it's still related to the simd implementation (which I didn't realize needed to be updated due to it being nightly only). I don't recall the failed build showing up. Do you remember if it visible before the merge?

This kind of begs another question, should the simd impls be removed or otherwise disabled? It was always nightly only and doesn't compile at all on newer versions of rust.

[kvark]

Honestly, I'd just remove the nightly-only stuff. We have other math crates that can do SIMD on stable, so hardly anyone would want to prefer cgmath for them.

The Travis CI has been wonky and not always triggering, hence we missed the breakage. We need to move to GHA ASAP.

[kvark]

since we are about to release a breaking version, let's just kill all the deprecated stuff, perhaps?

[aloucks]

I deprecated them for two reasons. 1) As a consumer of a library I always appreciate when items are deprecated for one or two releases before being outright removed, and 2) If a bug is introduced, the the old versions are still there for reference and fallback.

[aloucks]

Honestly, I'd just remove the nightly-only stuff. We have other math crates that can do SIMD on stable, so hardly anyone would want to prefer cgmath for them.

@kvark Agreed. I'll try to get to it this week but no guarantee unfortunately. If someone else has time to remove the nightly simd stuff sooner, that would be great.