[Merged by Bors] - fix mul_vec3 tranformation order: should be scale -> rotate -> translate

[rsk700]

Objective

Lets say we need to rotate stretched object for this purpose we can created stretched Child and add as child to Parent, later we can rotate Parent, Child in this situation should rotate keeping it form, it is not the case with SpriteBundle currently. If you try to do it with SpriteBundle it will deform.

Solution

My pull request fixes order of transformations to scale -> rotate -> translate, with this fix SpriteBundle behaves as expected in described rotation, without deformation. Here is quote from "Essential Mathematics for Games":

Generally, the desired order we wish to use for these transforms is to scale first, then rotate, then translate. Scaling first gives us the scaling along the axes we expect. We can then rotate around the origin of the frame, and then translate it into place.

I'm must say when I was using MaterialMesh2dBundle it behaves correctly in both cases with bevy main and with my fix, don't know why, was not able to figure it out why there is difference.

here is code I was using for testing:

use bevy::{
    prelude::*,
    render::render_resource::{Extent3d, TextureDimension, TextureFormat},
    sprite::{MaterialMesh2dBundle, Mesh2dHandle},
};

fn main() {
    let mut app = App::new();
    app.insert_resource(ClearColor(Color::rgb(0.1, 0.2, 0.3)))
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup);
    app.run();
}

fn setup(
    mut commands: Commands,
    mut images: ResMut<Assets<Image>>,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    let mut c = OrthographicCameraBundle::new_2d();
    c.orthographic_projection.scale = 1.0 / 10.0;
    commands.spawn_bundle(c);
    // note: mesh somehow works for both variants
    // let quad: Mesh2dHandle = meshes.add(Mesh::from(shape::Quad::default())).into();
    // let child = commands
    //     .spawn_bundle(MaterialMesh2dBundle {
    //         mesh: quad.clone(),
    //         transform: Transform::from_translation(Vec3::new(0.0, 0.0, -1.0))
    //             .with_scale(Vec3::new(10.0, 1.0, 1.0)),
    //         material: materials.add(ColorMaterial::from(Color::BLACK)),
    //         ..Default::default()
    //     })
    //     .id();
    // commands
    //     .spawn_bundle(MaterialMesh2dBundle {
    //         mesh: quad,
    //         transform: Transform::from_rotation(Quat::from_rotation_z(0.78))
    //             .with_translation(Vec3::new(0.0, 0.0, 10.0)),
    //         material: materials.add(ColorMaterial::from(Color::WHITE)),
    //         ..Default::default()
    //     })
    //     .push_children(&[child]);

    let white = images.add(get_image(Color::rgb(1.0, 1.0, 1.0)));
    let black = images.add(get_image(Color::rgb(0.0, 0.0, 0.0)));
    let child = commands
        .spawn_bundle(SpriteBundle {
            texture: black,
            transform: Transform::from_translation(Vec3::new(0.0, 0.0, -1.0))
                .with_scale(Vec3::new(10.0, 1.0, 1.0)),
            ..Default::default()
        })
        .id();
    commands
        .spawn_bundle(SpriteBundle {
            texture: white,
            transform: Transform::from_rotation(Quat::from_rotation_z(0.78))
                .with_translation(Vec3::new(0.0, 0.0, 10.0)),
            ..Default::default()
        })
        .push_children(&[child]);
}

fn get_image(color: Color) -> Image {
    let mut bytes = Vec::with_capacity((1 * 1 * 4 * 4) as usize);
    let color = color.as_rgba_f32();
    bytes.extend(color[0].to_le_bytes());
    bytes.extend(color[1].to_le_bytes());
    bytes.extend(color[2].to_le_bytes());
    bytes.extend(1.0_f32.to_le_bytes());
    Image::new(
        Extent3d {
            width: 1,
            height: 1,
            depth_or_array_layers: 1,
        },
        TextureDimension::D2,
        bytes,
        TextureFormat::Rgba32Float,
    )
}

here is screenshot with bevy main and my fix:

[alice-i-cecile]

This is well-justified, and I agree with this convention. Thanks for tracking this down, and finding references to back up the choice.

[DJMcNab]

I couldn't see a link to the previous discussion in #1755 in the PR description.

[CptPotato]

I agree with @rsk700 that scale => rotate => translate is the most intuitive and I would personally prefer it that way.

Though there are some inconsistencies and questions that are still not answered in #1755: mainly SRT vs RST and how to propagate scaling.
As I understand it, rotate => scale => translate is the current approach in bevy and it would require some changes to GlobalTransform to have consistent scale => rotate => translate instead.

It's out of the scope of this PR but I think it's still relevant to consider. In my opinion it would make sense to double down on one strategy and stick with that.

[InnocentusLime]

I have recently bumped into this issue as an unknowing novice...

I am using bevy_rapier2d and occasionally enable the debugging render of colliders to see how well they approximate the sprites.

To display the colliders, bevy_rapier2d scales the Transform according to the size of colliders (For example a sphere with 0.65 radius will scale the entity by 0.65 in all directions). Given that and the fact that having Transform's scale and collider's scale match up means less headaches, I started scaling all entities to match the colliders (planning to use custom_size of sprites for edge cases when the sprite was going to be bigger than the collider).

And it turned out that this approach, sadly, doesn't work with current bevy.

[alice-i-cecile]

Adding S-Ready-For-Final-Review as this is largely a matter of taste that needs a solid decision. More reviews (or even debate) are unlikely to add anything.

[ickk]

Worth noting that without this PR:

• transform.local_x() et al. don't actually give you basis vectors of the transform, but of Transform::from_rotation(transform.rotation), which is a bit misleading/confusing & had me pretty stumped for a while when I needed them.

• Using transform.compute_matrix() to get the 3d affine transformation matrix, and then applying it to a vector will perform a transform that is inconsistent with the current behavior of mul_vec3 (but is consistent with the mul_vec3 contained in this PR).

[superdump]

Unity apparently does SRT. Godot empirically does translation last but I don't know if it is RST or SRT. Unreal Engine 4.27.2 empirically does translation last but I don't know if RST or SRT.

[cart]

I did an "engine op order" investigation here: #1755 (comment)

[superdump]

I did an "engine op order" investigation here: #1755 (comment)

Ah good. :) However, that comment says they basically all do SRT and that bevy does too. But this PR seems to be about making bevy do SRT. Do you have a comment on this PR?

[cart]

With the exception of the mul_vec3 impl, Bevy is already SRT. We do hierarchy maintenance without doing matrix conversions (we operate directly on Transform types, where order doesn't matter ... it calls mul_vec internally to offset the translation, but that case isn't affected by this ordering from what I can tell). During rendering, we do a final conversion from GlobalTransform (still S/R/T) to a Mat4, using glam's from_scale_rotation_transform. This generates an SRT matrix. This is why this issue isn't reproducible for Mesh2d (and why it wouldn't be reproducible in 3d either).

Sprites, on the other hand. Use GlobalTransform::mul_vec3 to transform the four quad vertices when generating the "batched vertex buffer". Here, the mismatched RST approach in that implementation shows itself.

The solution here is exactly whats done in this PR: fix the mul_vec3 implementation to line up with the approach we use elsewhere (SRT).

I think this made it this far in an inconsistent state because most people aren't manually transforming Vec3s with transforms. The "batched sprite rendering" is new as of 0.6, which "surfaced" this for the first time.

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-benches
• check-doc
• check-markdown-links
• check-missing-examples-in-docs
• check-unused-dependencies
• ci
• markdownlint
• run-examples

[superdump]

Thanks for the explanation.