Added Transform multiplication and Quaternion::mul_direction #78
Conversation
| @@ -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)`. | |||
There was a problem hiding this comment.
The comment should mention Vec4::from_direction instead :)
| where T: Real + MulAdd<T,T,Output=T> | ||
| { | ||
| type Output = Self; | ||
| fn mul(self, rhs: Self) -> Self { |
There was a problem hiding this comment.
I agree with the idea to allow Transform to be multiplied by another, just like matrices!
I would have some remarks though :
- 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);
- 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.
- This makes me think that it would be great if we added methods
transformed_by()andtransform_other()toTransform; then, multiplication would be expressed asa.transformed_by(b);. I'm surprised I never added such an important feature in the first place ¯_(ツ)_/¯.
These methods should takeTransformby ref rather than by value, and return a value. There could be "in-place" variants as well, liketransform_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) }
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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)`. | |||
| pub fn mul_direction<V: Into<Vec3<T>> + From<Vec4<T>>>(self, rhs: V) -> V | |||
There was a problem hiding this comment.
I think it would make sense to add mul_point() as well, since it's also a convenience for forcing a W value :)
|
I've been trying to find the time to get round to this, but I'm not currently able to schedule any. I might be able to get this done during the winter break. |
|
No pressure, free time is hard to find, especially during this period. I must confess I haven't taken any time to dig further, also knowing that it turns out not to be simple, requires tests, and strangely, (given how useful it is) has not been requested by anybody before. So no problem, progress will come when it wants to! |
This adds two new features:
A
Mulimpl forTransformthat transforms one in terms of other.A
mul_pointmethod onQuaternion, bringing symmetry withMat4.