fix: incorrect sprite size for aabb when sprite has rect and no custom_size

[MarcoMeijer]

Objective

Fixes #12736

Solution

Use sprite rect to calculate sprite size for aabb when custom_size is None

[mgi388]

Out of interest is there any way to write or update a unit test to prove the existing bug and that this change fixes it?

[MarcoMeijer]

Sure I will add a unit test to it

[mgi388]

Here's some testing of this PR which I think shows that it fixes the issue as expected. I think it's worth someone eye-balling the results to check that they match their expectations, but they seem to AFAICT.

In the original issue #12736 the reporter mentions anchor being "outside of the range -0.5..0.5", but per the results below the bug appears to exist with standard anchors (also, FWIW, does an anchor outside that coordinate system have undefined behavior or are they also valid?)

Examples below run using cargo run --example sprite where I replaced the built-in sprite.rs example with the code for each test:

Note

Image is 256x256

With Bevy 5c3ae32 (more or less main due to #12935)

Only setting an anchor

Code

//! Modified sprite.ts.

use bevy::{color::palettes::tailwind::AMBER_400, prelude::*};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, (setup, show_bounding_boxes))
        .add_systems(Update, (show_transform_gizmo, update_camera))
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(SpriteBundle {
        texture: asset_server.load("branding/bevy_bird_dark.png"),
        sprite: Sprite {
            anchor: bevy::sprite::Anchor::TopLeft,
            ..default()
        },
        ..default()
    });
}

fn show_bounding_boxes(mut config: ResMut<GizmoConfigStore>) {
    config.config_mut::<AabbGizmoConfigGroup>().1.draw_all = true;
}

fn show_transform_gizmo(mut gizmos: Gizmos, query: Query<&Transform, With<Sprite>>) {
    for transform in query.iter() {
        gizmos.sphere(transform.translation, Quat::IDENTITY, 3.5, AMBER_400);
    }
}

fn update_camera(
    keyboard: Res<ButtonInput<KeyCode>>,
    time: Res<Time>,
    mut listeners: Query<&mut Transform, With<Camera>>,
) {
    let mut transform = listeners.single_mut();

    let speed = 200.;

    if keyboard.pressed(KeyCode::ArrowRight) {
        transform.translation.x += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowLeft) {
        transform.translation.x -= speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowUp) {
        transform.translation.y += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowDown) {
        transform.translation.y -= speed * time.delta_seconds();
    }
}

✅ Result

Only setting a rect

Code

//! Modified sprite.ts.

use bevy::{color::palettes::tailwind::AMBER_400, prelude::*};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, (setup, show_bounding_boxes))
        .add_systems(Update, (show_transform_gizmo, update_camera))
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(SpriteBundle {
        texture: asset_server.load("branding/bevy_bird_dark.png"),
        sprite: Sprite {
            rect: Some(Rect::new(0.0, 0.0, 150., 150.)),
            ..default()
        },
        ..default()
    });
}

fn show_bounding_boxes(mut config: ResMut<GizmoConfigStore>) {
    config.config_mut::<AabbGizmoConfigGroup>().1.draw_all = true;
}

fn show_transform_gizmo(mut gizmos: Gizmos, query: Query<&Transform, With<Sprite>>) {
    for transform in query.iter() {
        gizmos.sphere(transform.translation, Quat::IDENTITY, 3.5, AMBER_400);
    }
}

fn update_camera(
    keyboard: Res<ButtonInput<KeyCode>>,
    time: Res<Time>,
    mut listeners: Query<&mut Transform, With<Camera>>,
) {
    let mut transform = listeners.single_mut();

    let speed = 200.;

    if keyboard.pressed(KeyCode::ArrowRight) {
        transform.translation.x += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowLeft) {
        transform.translation.x -= speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowUp) {
        transform.translation.y += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowDown) {
        transform.translation.y -= speed * time.delta_seconds();
    }
}

❌ Result

Anchor and rect

Code

//! Modified sprite.ts.

use bevy::{color::palettes::tailwind::AMBER_400, prelude::*};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, (setup, show_bounding_boxes))
        .add_systems(Update, (show_transform_gizmo, update_camera))
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(SpriteBundle {
        texture: asset_server.load("branding/bevy_bird_dark.png"),
        sprite: Sprite {
            anchor: bevy::sprite::Anchor::TopLeft,
            rect: Some(Rect::new(0.0, 0.0, 150., 150.)),
            ..default()
        },
        ..default()
    });
}

fn show_bounding_boxes(mut config: ResMut<GizmoConfigStore>) {
    config.config_mut::<AabbGizmoConfigGroup>().1.draw_all = true;
}

fn show_transform_gizmo(mut gizmos: Gizmos, query: Query<&Transform, With<Sprite>>) {
    for transform in query.iter() {
        gizmos.sphere(transform.translation, Quat::IDENTITY, 3.5, AMBER_400);
    }
}

fn update_camera(
    keyboard: Res<ButtonInput<KeyCode>>,
    time: Res<Time>,
    mut listeners: Query<&mut Transform, With<Camera>>,
) {
    let mut transform = listeners.single_mut();

    let speed = 200.;

    if keyboard.pressed(KeyCode::ArrowRight) {
        transform.translation.x += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowLeft) {
        transform.translation.x -= speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowUp) {
        transform.translation.y += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowDown) {
        transform.translation.y -= speed * time.delta_seconds();
    }
}

❌ Result

With the changes from this PR

Only setting an anchor

✅ Result

Only setting a rect

✅ Result

Anchor and rect

✅ Result

Anchor outside of the range -0.5..0.5 and rect (to prove fix for original issue)

Code

//! Modified sprite.ts.

use bevy::{color::palettes::tailwind::AMBER_400, prelude::*, sprite::Anchor};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, (setup, show_bounding_boxes))
        .add_systems(Update, (show_transform_gizmo, update_camera))
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(SpriteBundle {
        texture: asset_server.load("branding/bevy_bird_dark.png"),
        sprite: Sprite {
            anchor: Anchor::Custom(Vec2::new(1., 1.)),
            rect: Some(Rect::new(0.0, 0.0, 150., 150.)),
            ..default()
        },
        ..default()
    });
}

fn show_bounding_boxes(mut config: ResMut<GizmoConfigStore>) {
    config.config_mut::<AabbGizmoConfigGroup>().1.draw_all = true;
}

fn show_transform_gizmo(mut gizmos: Gizmos, query: Query<&Transform, With<Sprite>>) {
    for transform in query.iter() {
        gizmos.sphere(transform.translation, Quat::IDENTITY, 3.5, AMBER_400);
    }
}

fn update_camera(
    keyboard: Res<ButtonInput<KeyCode>>,
    time: Res<Time>,
    mut listeners: Query<&mut Transform, With<Camera>>,
) {
    let mut transform = listeners.single_mut();

    let speed = 200.;

    if keyboard.pressed(KeyCode::ArrowRight) {
        transform.translation.x += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowLeft) {
        transform.translation.x -= speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowUp) {
        transform.translation.y += speed * time.delta_seconds();
    }
    if keyboard.pressed(KeyCode::ArrowDown) {
        transform.translation.y -= speed * time.delta_seconds();
    }
}

✅ Result

[MarcoMeijer]

@mgi388 I mentioned the range -0.5..0.5 because when it is outside that range there will be an area of the sprite that is outside of the aabb box. See this picture from running your last example on bevy 0.13.2.

Which is causing the bug I was having of the sprite not being rendered.

When the origin is in the range -0.5..0.5 the aabb box is just bigger then it should be, so it is being rendered when it doesn't have to. Thats probably why it didn't get noticed before since most people have the origin within the sprite.

[mgi388]

@MarcoMeijer makes sense. I probably should have added the "outside the range" test against main as well. I just ran it, and it looks the same as 0.13.2 as you've just shared, so I won't bother updating my test results.

Anyway, given my test results above, do all the results when the changes in your PR is applied look expected and correct to you?

[MarcoMeijer]

@mgi388 yep they look good to me

[mgi388]

Code looks good to me, I've tested it too, so giving an approval.

[mgi388]

@alice-i-cecile would you be able to help direct this to the right reviewers? I picked this out and reviewed it myself because I was debugging anchor in my own thing, and I'd like to help the OP get it reviewed/merged if it's good to go.

[alice-i-cecile]

@bugsweeper @lennart would the two of you be able to review this based on your work in #12769? <3 I'll take a look too.

[alice-i-cecile]

Good little fix, and a nice regression test to match.

[bugsweeper]

I think this problem weakly corelates to my work because my changes fixed problem based on sprite subdivision. But I confirm that bounds of sprite slices similary calculated using texture size, sprite.rect.size or custom_size, but in reverse order (because of using reassignment) for same priorities