Bug in the Mul impl for Transform and GlobalTransform

[TethysSvensson]

Bevy version

I have tested both the current master as of this writing (9976ecb) and v0.7.0.

What you did

I think there is a bug in the Mul implementation of Transform and GlobalTransform. I would expect these two expressions to be equivalent (up to rounding errors):

    let t1 = Transform::from_scale(Vec3::new(2.0, 1.0, 1.0));
    let quat = Quat::from_rotation_z(TAU / 4.0);
    let t2 = Transform::from_rotation(quat)
        * Transform::from_scale(Vec3::new(1.0, 2.0, 1.0))
        * Transform::from_rotation(quat.inverse());
    // t1 and t2 are not the same

I would expect this, because computing the same transform using matrix multiplications yields the expected result:

    let t3 = Transform::from_matrix(
        Transform::from_rotation(quat).compute_matrix()
            * Transform::from_scale(Vec3::new(1.0, 2.0, 1.0)).compute_matrix()
            * Transform::from_rotation(quat.inverse()).compute_matrix(),
    );
    // t1 and t3 are the same up to rounding errors

What went wrong

I was expecting the two transforms to be equivalent up to rounding point errors. However t2 ends up scaling the y-axis instead of the x-axis.

Additional information

I believe the bug is in mul_transform where the scales from the two transforms are multiplied together without taking the rotation into account.

I think there is a similar bug in rotate and rotate_around as well.

[nicopap]

if this gets fixed, this should be updated as well #4891

[oddfacade]

duplicates #4588. do we want this fixed? i can make a PR, but practically speaking it's just going to look like this unless we want to change how transforms are represented internally (or disallow nonuniform scale). i don't know much about that discussion but here are some links: #4379, #1755.

[HackerFoo]

It's impossible to fix this in general without changing the representation, because TRS can't represent shear, which will happen when using non-uniform scale and arbitrary rotations.

[TethysSvensson]

@HackerFoo In that case I think bevy should either adopt a more general representation or remove support for support non-uniform scales altogether.

Personally, I don't think there is any reason to keep non-uniform scale as it is currently implemented, as it is a huge footgun.

It has caused me and my coworkers to waste multiple hours on a bug, because we never considered that bevys mul implementation would not match matrix multiplication.

[HackerFoo]

I worked around this bug in my app for a while, but ultimately wrote #4379 as a compromise, because changing Transform to something that supports non-uniform scaling is more difficult. Then I wrote a custom transform component that I use in my app where non-uniform scaling is needed, and use that to set the GlobalTransform.

Just using an Affine3A for all transforms doesn't work, because it becomes difficult to handle e.g. rotation and scale animations separately, or even just rotation between two transforms.

[TethysSvensson]

The current implementation is not even associative as illustrated by this example:

let t1 = Transform::from_scale(Vec3::new(2.0, 1.0, 1.0));
let t2 = Transform::from_rotation(Quat::from_rotation_z(TAU / 4.0));
let t3 = Transform::from_translation(Vec3::new(1.0, 0.0, 0.0));

let left = (t1 * t2) * t3;
let right = t1 * (t2 * t3);

assert_eq!(left.translation.y.round(), 2.0);
assert_eq!(right.translation.y.round(), 1.0);

[oddfacade]

@HackerFoo Good point re: shear. Seems like nonuniform scaling is completely unusable with TRS representation, and should probably be removed if that's the representation we're sticking with. Unless I'm overlooking a use case for scaling that is always aligned to the world axes regardless of object rotation?

it becomes difficult to handle e.g. rotation and scale animations separately, or even just rotation between two transforms.

Do you have a specific problem/usage in mind? I think I remember reading some discussion about this in one of the other related issues but I can't find it.

[TethysSvensson]

I think #4379 as it looks right now solves this issue for GlobalTransform, but not for Transform.

Would it be a big problem if the Mul implementation was replaced by a function with a suitable warning about non-uniform scale behaving in a non-intuitive way?

[TethysSvensson]

@cart Do you have an opinion on this? The Mul impl for Transform is still non-associative even after #4379 has landed.

[HackerFoo]

I have proposed removing it. Transforms should only be accumulated using GlobalTransforms, and if you want to do something else that doesn't have the properties of multiplication, it shouldn't be called multiplication. It might be worth adding an Isometry type like Rapier's, which only has translation (as a Vec3) and rotation (as a Quat), which supports all the expected operations, and is useful for physics.

[TethysSvensson]

It is possible to keep the scale as well, as long as it is not a non-uniform scale.

[oddfacade]

To some extent this is a question of resolving conflicting requirements. The way I see it most solutions are going to fall broadly into one of three categories.

1. If a Transform is intended to be a convenient way for the user to specify a local coordinate space relative to the object's parent, then it makes sense to keep nonuniform scale and remove multiplication.
2. If we want it to have an actual algebraic structure that can be used to transform between coordinate spaces then it should probably be limited to uniform scaling (i.e. it represents a similarity transform) or no scaling (i.e. it's an isometry).
3. If we want (2) without compromising on the non-uniform scaling thing, then it should be an affine transform instead.

[HackerFoo]

Other possible representations that support multiplication are:

• Replace the scale vector with a 3x3 matrix (I use this). The rotation is redundant, but allows rotation independent of scaling.
• Add another rotation before scaling (TRSR). The 3x3 matrix from Affine3A can be converted to RSR using singular value decomposition.

This is why #4379 only modifies GlobalTransform, because there are multiple valid choices for individual transforms, but they all can (and must)l be converted to a matrix. I would prefer to generalize transform propagation to work with types provided by the user. So you can use the existing Transform, or a different representation if it isn't suitable.