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

Added Transform multiplication and Quaternion::mul_direction #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added Transform multiplication and Quaternion::mul_direction #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions src/quaternion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,13 @@ macro_rules! quaternion_complete_mod {
pub fn into_vec3(self) -> Vec3<T> {
self.into()
}

/// Shortcut for `self * Vec4::from_point(rhs)`.
Copy link
Owner

Choose a reason for hiding this comment

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

The comment should mention Vec4::from_direction instead :)

pub fn mul_direction<V: Into<Vec3<T>> + From<Vec4<T>>>(self, rhs: V) -> V
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would make sense to add mul_point() as well, since it's also a convenience for forcing a W value :)

where T: Real + MulAdd<T,T,Output=T>
{
(self * Vec4::from_direction(rhs.into())).into()
}
}

#[cfg(feature = "mint")]
Expand Down
31 changes: 23 additions & 8 deletions src/transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ macro_rules! transform_complete_mod {
// look_at
// rotate_around

use std::ops::Add;
use std::ops::{Add, Mul};
use $crate::num_traits::{Zero, One, real::Real};
use $crate::ops::*;
use crate::vec::$mod::*;
Expand All @@ -26,7 +26,7 @@ macro_rules! transform_complete_mod {
/// let (p, rz, s) = (Vec3::unit_x(), 3.0_f32, 5.0_f32);
/// let a = Mat4::scaling_3d(s).rotated_z(rz).translated_3d(p);
/// let b = Mat4::from(Transform {
/// position: p,
/// position: p,
/// orientation: Quaternion::rotation_z(rz),
/// scale: Vec3::broadcast(s),
/// });
Expand Down Expand Up @@ -68,9 +68,24 @@ macro_rules! transform_complete_mod {
}
}

/// LERP on a `Transform` is defined as LERP-ing between the positions and scales,
impl<T> Mul for Transform<T,T,T>
where T: Real + MulAdd<T,T,Output=T>
{
type Output = Self;
fn mul(self, rhs: Self) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

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

I agree with the idea to allow Transform to be multiplied by another, just like matrices!
I would have some remarks though :

  1. I'd like to be super confident that this implementation is correct and works in all cases; maybe it actually does, but I'm not sure. Would be nice to compare to a proven impl such as in GLM or Unreal (with their FTransform struct), but carefully noting the order in which they perform operations (e.g do they transform LHS by RHS, or the reverse);
  2. Following point 1, I'd like some tests in a doc comment, like most operations; doesn't have to be very involved, but it's good to have such sanity checks.
  3. This makes me think that it would be great if we added methods transformed_by() and transform_other() to Transform; then, multiplication would be expressed as a.transformed_by(b);. I'm surprised I never added such an important feature in the first place ¯_(ツ)_/¯.
    These methods should take Transform by ref rather than by value, and return a value. There could be "in-place" variants as well, like transform_by() which would operate on &mut self.

So the plan for 3. would be :

  • pub fn transform_other(&self, other: &Self) -> Self { /* your impl, i.e self * other, i.e transforming the other by self */ }
  • pub fn transformed_by(&self, other: &Self) -> Self { other.transform_other(self) }
  • pub fn transform_by(&mut self, other: &Self) { *self = other.transform_other(self); }
  • pub fn mul(self, rhs: Self) -> Self { self.transform_other(rhs) }

Copy link
Contributor Author

@zesterer zesterer Oct 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to be super confident that this implementation is correct

This is the implementation used by nalgebra, albeit with scaling applied to the translation component (obviously, rotation is invariant under scaling).

In hindsight, I don't think that my application of scaling is correct under rotation when the scale is non-scalar: I'll correct that.

Edit: Actually, I don't think that I can even determine the correct code without the order of application being made explicit. What order are the translation/orientation/scaling operations supposed to be made in?

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I don't think that I can even determine the correct code without the order of application being made explicit. What order are the translation/orientation/scaling operations supposed to be made in?

I think it's safe to look at Unreal's void FTransform::Multiply(FTransform* OutTransform, const FTransform* A, const FTransform* B).
I'd really have liked to see Unity's implementation as well, but it's part of the native source code, which isn't publicly available.

Their implementation is very similar to your initial impl, except they compute OutTransform->Translation = B->Rotation*(B->Scale3D*A->Translation) + B->Translation, so they do q*(s*t) rather than (q*t)*s.
I'm not sure why they have a MultiplyUsingMatrixWithScale kind of fallback though, that looks wrong to me.

We should note that when they write A * B, it means "B applied over A, so A transformed by B".

But, when we write A * B, we will want to mean "A applied over B, so B transformed by A".
That would be consistent with the multiplication order currently adopted for matrices.

let shift = self.orientation.mul_direction(rhs.position) * self.scale;

Self {
position: self.position + shift,
orientation: self.orientation * rhs.orientation,
scale: self.scale * rhs.scale,
}
}
}

/// LERP on a `Transform` is defined as LERP-ing between the positions and scales,
/// and performing SLERP between the orientations.
impl<P,O,S,Factor> Lerp<Factor> for Transform<P,O,S>
impl<P,O,S,Factor> Lerp<Factor> for Transform<P,O,S>
where Factor: Copy + Into<O>,
P: Lerp<Factor,Output=P>,
S: Lerp<Factor,Output=S>,
Expand All @@ -93,9 +108,9 @@ macro_rules! transform_complete_mod {
}
}

/// LERP on a `Transform` is defined as LERP-ing between the positions and scales,
/// LERP on a `Transform` is defined as LERP-ing between the positions and scales,
/// and performing SLERP between the orientations.
impl<'a,P,O,S,Factor> Lerp<Factor> for &'a Transform<P,O,S>
impl<'a,P,O,S,Factor> Lerp<Factor> for &'a Transform<P,O,S>
where Factor: Copy + Into<O>,
&'a P: Lerp<Factor,Output=P>,
&'a S: Lerp<Factor,Output=S>,
Expand All @@ -117,8 +132,8 @@ macro_rules! transform_complete_mod {
}
}
}
}
}
}
}

#[cfg(all(nightly, feature="repr_simd"))]
pub mod repr_simd {
Expand Down