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.

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

Apply scale factor to ImageMeasure sizes #8545

Merged
merged 12 commits into from
Jun 23, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented May 4, 2023

Objective

In Bevy main, the unconstrained size of an ImageBundle or AtlasImageBundle UI node is based solely on the size of its texture and doesn't change with window scale factor or UiScale.

Solution

  • The size field of each ImageMeasure should be multiplied by the current combined scale factor.
  • Each ImageMeasure should be updated when the combined scale factor is changed.

Example:

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .insert_resource(UiScale { scale: 1.5 })
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(NodeBundle {
        style: Style {
            // The size of the "bevy_logo_dark.png" texture is 520x130 pixels
            width: Val::Px(520.),
            height: Val::Px(130.),
            ..Default::default()
        },
        background_color: Color::RED.into(),
        ..Default::default()
    });
    commands
        .spawn(ImageBundle {
            style: Style {
                position_type: PositionType::Absolute,
                ..Default::default()
            },
            image: UiImage::new(asset_server.load("bevy_logo_dark.png")),
            ..Default::default()
        });
}

The red node is given a size with the same dimensions as the texture. So we would expect the texture to fill the node exactly.

  • Result with Bevy main branch bb59509:
    image-size-broke

  • Result with this PR (and Bevy 0.10.1):
    image-size-fixed


Changelog

bevy_ui::widget::image

  • Update all ImageMeasures on changes to the window scale factor or UiScale.
  • Multiply ImageMeasure::size by the window scale factor and UiScale.

Migration Guide

* Update all UI Image node measure funcs on scale factor changes.
* Apply scale factor to measure funcs size value.
@ickshonpe ickshonpe changed the title Apply scale factor to ImageMeasure's size Apply scale factor to ImageMeasure sizes May 4, 2023
@mockersf
Copy link
Member

mockersf commented May 4, 2023

Could you add a comment on ImageMeasure that it should be in physical pixels? (like UiImageSize)

@mockersf mockersf added the A-UI Graphical user interfaces, styles, layouts, and widgets label May 4, 2023
@ickshonpe
Copy link
Contributor Author

Could you add a comment on ImageMeasure that it should be in physical pixels? (like UiImageSize)

Done,
Scale factor is so confusing, just realised the UiImageSize comment is wrong too It's the size of the texture not the size in physical or logical pixels, changed it as well. And it's a component, not a field.

* Added comment to `ImageMeasure` noting that its size value needs to be in physical  pixels.
@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label May 5, 2023
@alice-i-cecile alice-i-cecile added the A-Rendering Drawing game state to the screen label May 5, 2023
Multiply `ImageMeasure::size` by the combined scale factor for UI texture atlas images too.
Checking for changed scale factor is required, unless the `image_size.size` is set to the scaled image size, which doesn't seem desirable.
@ickshonpe
Copy link
Contributor Author

This fix needs to be included with 0.11 I think, otherwise updating any UI projects using images from 0.10 is going to be really painful.

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Jun 22, 2023
crates/bevy_ui/src/widget/image.rs Outdated Show resolved Hide resolved
// target size of the image
size: Vec2,
/// The size of the image's texture
pub size: Vec2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be editable by the user? Or should this be pub(crate)?

Copy link
Contributor Author

@ickshonpe ickshonpe Jun 22, 2023

Choose a reason for hiding this comment

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

Yes it's intended to be pub. That way someone can use ImageMeasure for their own custom widget if they want.

Otherwise, it doesn't matter that it's pub as measure funcs can't be accessed by users once they've been added to the layout tree.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 23, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 23, 2023
Merged via the queue into bevyengine:main with commit cdaae01 Jun 23, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants