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

Proportional scaling for the sprite's texture. #17258

Merged

Conversation

silvestrpredko
Copy link
Contributor

Objective

Bevy sprite image mode lacks proportional scaling for the underlying texture. In many cases, it's required. For example, if it is desired to support a wide variety of screens with a single texture, it's okay to cut off some portion of the original texture.

Solution

I added scaling of the texture during the preparation step. To fill the sprite with the original texture, I scaled UV coordinates accordingly to the sprite size aspect ratio and texture size aspect ratio. To fit texture in a sprite the original quad is scaled and then the additional translation is applied to place the scaled quad properly.

Testing

For testing purposes could be used 2d/sprite_scale.rs. Also, I am thinking that it would be nice to have some tests for a crates/bevy_sprite/src/render/mod.rs:sprite_scale.


Showcase

image

@silvestrpredko silvestrpredko force-pushed the silvestr/fit-sprite-to-quad branch from 68bd63f to 0e7ca6f Compare January 9, 2025 13:47
This change introduces new texture scaling options for sprites that maintain aspect ratio:

- Add `TextureScale` enum with `FillCenter`, `FillStart`, `FillEnd`, `FitCenter`, `FitStart`, and `FitEnd` modes
- Extend `SpriteImageMode` with `ScaleMode` variant to support the new scaling options
@silvestrpredko silvestrpredko force-pushed the silvestr/fit-sprite-to-quad branch from 49186a3 to 296174f Compare January 9, 2025 19:00
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 10, 2025
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 14, 2025

I like the feature, and the algorithms seem to be correct. Some mild style / docs / naming feedback, but do push back if you disagree with any of them.

Much of this feels like it belongs in bevy_image, but we should move that later. Being able to share this style of logic across windows/UI/decals/sprites would be great.

silvestrpredko and others added 2 commits January 15, 2025 16:19
- Update SpriteImageMode::ScaleMode variant to Scale
- Fix unreachable error messages to be more specific
- Clean up sprite scale example code
- Update documentation comments

This change improves naming consistency and makes the sprite scaling API more intuitive while maintaining all existing functionality.
@alice-i-cecile alice-i-cecile requested review from BenjaminBrienen and removed request for tbillington January 15, 2025 18:36
Cargo.toml Outdated

[package.metadata.example.sprite_scale]
name = "Sprite Scale"
description = "Shows how a sprite could be scaled into a rectangle while keeping the aspect ratio"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description = "Shows how a sprite could be scaled into a rectangle while keeping the aspect ratio"
description = "Shows how a sprite can be scaled into a rectangle while keeping the aspect ratio"

@@ -120,6 +120,7 @@ Example | Description
[Sprite](../examples/2d/sprite.rs) | Renders a sprite
[Sprite Animation](../examples/2d/sprite_animation.rs) | Animates a sprite in response to an event
[Sprite Flipping](../examples/2d/sprite_flipping.rs) | Renders a sprite flipped along an axis
[Sprite Scale](../examples/2d/sprite_scale.rs) | Shows how a sprite could be scaled into a rectangle while keeping the aspect ratio
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[Sprite Scale](../examples/2d/sprite_scale.rs) | Shows how a sprite could be scaled into a rectangle while keeping the aspect ratio
[Sprite Scale](../examples/2d/sprite_scale.rs) | Shows how a sprite can be scaled into a rectangle while keeping the aspect ratio

Comment on lines 953 to 984
ScalingMode::FitStart => {
if texture_ratio > quad_ratio {
// The quad is scaled to match the image ratio, and the quad translation is adjusted
// to start of the quad within the original quad size.
let scale = Vec2::new(1.0, quad_ratio / texture_ratio);
let new_quad = *quad_size * scale;
let offset = *quad_size - new_quad;
*quad_translation = Vec2::new(0.0, -offset.y);
*quad_size = new_quad;
} else {
let scale = Vec2::new(texture_ratio / quad_ratio, 1.0);
let new_quad = *quad_size * scale;
let offset = *quad_size - new_quad;
*quad_translation = Vec2::new(offset.x, 0.0);
*quad_size = new_quad;
}
}
ScalingMode::FitEnd => {
if texture_ratio > quad_ratio {
let scale = Vec2::new(1.0, quad_ratio / texture_ratio);
let new_quad = *quad_size * scale;
let offset = *quad_size - new_quad;
*quad_translation = Vec2::new(0.0, offset.y);
*quad_size = new_quad;
} else {
let scale = Vec2::new(texture_ratio / quad_ratio, 1.0);
let new_quad = *quad_size * scale;
let offset = *quad_size - new_quad;
*quad_translation = Vec2::new(-offset.x, 0.0);
*quad_size = new_quad;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are identical except for 1 minus sign, and there are similar duplication in the other Start/End cases. On one hand it would be nice if the repetition could be factored out so e.g. FitEnd is just FitStart and set the minus. On the other hand this code is pretty simple and easy to read.

It might also become simpler if you match on (scaling_mode, quad_ratio > texture_ratio) since every case has that if inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concern about code duplication, and I agree that maintaining clarity is important. However, considering the minimal amount of code duplication we currently have, I believe it's reasonable to keep things as they are. The existing structure seems to be functioning well without introducing unnecessary complexity.

/// Fill rect with a centered texture.
#[default]
FillCenter,
/// Scale the texture to fill the rect with a start of the texture,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Scale the texture to fill the rect with a start of the texture,
/// Scale the texture to fill the rect with the start of the texture,

This isn't really very clear about what the "start" of the texture is.

/// Scale the texture to fill the rect with a start of the texture,
/// maintaining the aspect ratio.
FillStart,
/// Scale the texture to fill the rect with a end of the texture,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Scale the texture to fill the rect with a end of the texture,
/// Scale the texture to fill the rect with the end of the texture,

FillEnd,
/// Scaling the texture will maintain the original aspect ratio
/// and ensure that the original texture fits entirely inside the rect.
/// At least one axis (X or Y) will fit exactly. The result is centered inside the rect.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// At least one axis (X or Y) will fit exactly. The result is centered inside the rect.
/// At least one axis (x or y) will fit exactly. The result is centered inside the rect.

FitCenter,
/// Scaling the texture will maintain the original aspect ratio
/// and ensure that the original texture fits entirely inside rect.
/// At least one axis (X or Y) will fit exactly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// At least one axis (X or Y) will fit exactly.
/// At least one axis (x or y) will fit exactly.

FitStart,
/// Scaling the texture will maintain the original aspect ratio
/// and ensure that the original texture fits entirely inside rect.
/// At least one axis (X or Y) will fit exactly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// At least one axis (X or Y) will fit exactly.
/// At least one axis (x or y) will fit exactly.


let square = asset_server.load("textures/slice_square_2.png");
let banner = asset_server.load("branding/banner.png");

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be some example cases with a sprite from a texture atlas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be some example cases with a sprite from a texture atlas.

It's important to note that the sprite sheet won't function correctly with scaling. Scaling completely overrides both the scaling and offset for the sprite, disregarding the rect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated the documentation to properly notify a user that it couldn't be used for a sprite sheet. For now, I don't know how to easily add additional scaling of the sprite sheet to the desired rect

@@ -813,7 +813,10 @@ fn compute_texture_slices(
[[0., 0., 1., 1.], [0., 0., 1., 1.], [1., 1., rx, ry]]
}
SpriteImageMode::Auto => {
unreachable!("Slices should not be computed for ImageScaleMode::Stretch")
unreachable!("Slices should not be computed for SpriteImageMode::Stretch")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unreachable!("Slices should not be computed for SpriteImageMode::Stretch")
unreachable!("Slices can not be computed for SpriteImageMode::Auto")

unreachable!("Slices should not be computed for SpriteImageMode::Stretch")
}
SpriteImageMode::Scale(_) => {
unreachable!("Slices should not be computed for SpriteImageMode::Scale")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unreachable!("Slices should not be computed for SpriteImageMode::Scale")
unreachable!("Slices can not be computed for SpriteImageMode::Scale")

@@ -430,6 +431,7 @@ pub fn extract_sprites(
image_handle_id: sprite.image.id(),
anchor: sprite.anchor.as_vec(),
original_entity: Some(original_entity),
scaling_mode: sprite.image_mode.scale(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer it if the sprite geometry was calculated during extraction rather than in prepare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the geometry logic happens in prepare for now. I am going with less resistance and fewer changes in the codebase to achieve the necessary functionality.

pub enum ScalingMode {
/// Scale the texture uniformly (maintain the texture's aspect ratio)
/// so that both dimensions (width and height) of the texture will be equal
/// to or larger than the corresponding dimension of the rect.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you mean the rect field of the sprite? Needs to be clearer I think.

Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

This seems like a good start but looks like it still needs some work. It took me a while to understand the API and the docs aren't very clear. It probably doesn't make much that much impact but I'd like to see some sprite benchmarks as well.

Maybe there needs to be some sort of scale with overflow option as well, but I'm not really sure and that could be added in a followup.

/// Can be used in [`SpriteImageMode::Scale`].
#[derive(Debug, Clone, Copy, PartialEq, Default, Reflect)]
#[reflect(Debug)]
pub enum ScalingMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe this type might be better split up into different two enums, one Fill/Fit and the other Start/Center/End.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think maybe this type might be better split up into different two enums, one Fill/Fit and the other Start/Center/End.

I don't think that will be convenient for the end user. There are only six variants and no more are planned to add I guess. That led to the single field in Sprite that corresponds to scaling, adding one more enum adds more complexity.

Then SpriteImageMode::Scale needs to be a struct enum variant with two fields, one for Fill/Fit and the second one for direction Start/Center/End. In my humble opinion, it will not bring convenience for the current API.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 18, 2025
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 18, 2025
@silvestrpredko
Copy link
Contributor Author

I've been considering how to effectively apply scaling to a specific rectangle within a texture. My approach involves summing the scaling factors and then appropriately adjusting the offsets to accommodate the various scaling options. This way, I can ensure that the scaling is applied uniformly and maintains the correct alignment within the texture.

Please take a look. @alice-i-cecile @ickshonpe @BenjaminBrienen

Screen.Recording.2025-01-20.at.21.54.54.mov

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jan 21, 2025
@silvestrpredko
Copy link
Contributor Author

@ickshonpe just a gentle ping to review the latest changes.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 23, 2025
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jan 24, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 24, 2025
Merged via the queue into bevyengine:main with commit deb135c Jan 24, 2025
33 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 C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants