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

Sprite slice scaling and tiling #5213

Closed
wants to merge 3 commits into from

Conversation

ManevilleF
Copy link
Contributor

@ManevilleF ManevilleF commented Jul 5, 2022

Objective

Implement sprite tiling and 9 slice scaling for bevy_sprite.
Allowing slice scaling and texture tiling.

Basic scaling vs 9 slice scaling:

Traditional_scaling_vs_9-slice_scaling

Slicing example:

Screenshot 2022-07-05 at 15 05 49

Tiling example:

Screenshot 2022-07-06 at 14 35 34

Solution

  • Sprite now has a draw_mode field storing a SpriteDrawMode enum with three variants:
    • Simple
    • Tiled to have sprites tile horizontally and/or vertically
    • Sliced allowing 9 slicing the texture and optionally tile some sections with a Textureslicer.
  • I updated the bevy_sprite extraction stage to extract potentially multiple textures instead of one, depending on the draw_mode
  • I added two examples showcasing the slicing and tiling feature.

TODO list

  • slicing
  • scale factor limit
  • Slice Tiling:
    • Basic stretch mode
    • Center tiling
    • Sides tiling
  • Sprite tiling without slices

Blocking

To discuss

I want to move the slicer to be an Asset and avoid to compute the slices every frame, like for TextureAtlas but the slicing depends on the Sprite::custom_size and therefore needs to be updated every frame or on Changed<Sprite> detection

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Jul 5, 2022
@ManevilleF
Copy link
Contributor Author

ManevilleF commented Jul 5, 2022

Now I'm not sure if I can put the slicer in an Asset, I can compute the Rect values from a given image size and store it but the slices must be computed every frame since the sprite can be resized. Or maybe I can compute some change detection..

@mockersf how did you do it in https://github.com/vleue/bevy_ninepatch ? I looked at the code after you gave me the link but I don't understand the logic

@ManevilleF ManevilleF marked this pull request as ready for review July 6, 2022 12:55
@ManevilleF
Copy link
Contributor Author

Slicing and tiling work, I think this is ready to be reviewed @alice-i-cecile

@ManevilleF ManevilleF changed the title Sprite Slice scaling Sprite slice scaling and tiling Jul 6, 2022
Cargo.toml Outdated Show resolved Hide resolved
/// Struct defining a [`Sprite`](crate::Sprite) border with padding values
#[repr(C)]
#[derive(Default, Clone, Copy, Debug, Reflect)]
pub struct BorderRect {
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates the Rect type, which is re-exported from taffy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should bevy_sprite include taffy as a dependency?

Copy link
Member

@alice-i-cecile alice-i-cecile Jul 6, 2022

Choose a reason for hiding this comment

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

I'm not sure. I dislike that as it's not a light dependency.

I think we should keep this as it is for now, but probably open an issue once this is merged to think about how we can reduce tech debt here. It's going to be very silly once we try to implement this for bevy_ui.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it I could use a Rect directly, with the min/max values. Adding a constructor from padding values could do the trick but it sacrifices ergonomics a bit

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Very useful feature, and the code quality is good! There are a couple conceptual / architecture questions for you, plus some nits.

crates/bevy_sprite/src/texture_slice.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/texture_slice.rs Show resolved Hide resolved
crates/bevy_sprite/src/texture_slice.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/texture_slice.rs Show resolved Hide resolved
@ManevilleF
Copy link
Contributor Author

@alice-i-cecile I changed the logic to enable sprite tiling even without slicing, using an enum in Sprite::draw_mode and removing TextureSlicer from being a component.

I think this is better.

But now that I changed a lot of stuff in the extraction stage of the sprite render pipeline, benchmarking is definetely required

@ManevilleF
Copy link
Contributor Author

many_sprites stress test:

On this MR:

2022-07-07T09:27:50.191875Z  INFO many_sprites: Sprites: 102400
2022-07-07T09:27:50.191926Z  INFO bevy diagnostic: frame_time                      :    0.058695s (avg 0.060466s)
2022-07-07T09:27:50.191937Z  INFO bevy diagnostic: fps                             :   17.037371  (avg 16.569809)
2022-07-07T09:27:50.191941Z  INFO bevy diagnostic: frame_count                     : 403.000000

On Main:

2022-07-07T09:29:16.821865Z  INFO many_sprites: Sprites: 102400
2022-07-07T09:29:16.821931Z  INFO bevy diagnostic: frame_time                      :    0.052531s (avg 0.050298s)
2022-07-07T09:29:16.821944Z  INFO bevy diagnostic: fps                             :   19.036325  (avg 19.903592)
2022-07-07T09:29:16.821949Z  INFO bevy diagnostic: frame_count                     : 415.000000

So there is some performance regression for regular sprites. I'm going to investigate this

@ManevilleF
Copy link
Contributor Author

ManevilleF commented Jul 7, 2022

The big issue is accessing the Image from the handle in the extraction stage. I removed this step for SpriteDrawMode::Simple sprites, meaning no performance regressions.

Now this is still required for Sliced and Tiled draw modes. A more optimized solution would be to retrieve the image size lazily on the Added<Sprite> event or to ask the user to give the image dimensions.
I'm trying to think about a more ergonomic way to handle this, since the image dimensions are accessed again in the queue stage of the pipeline.

};

let slices = match &sprite.draw_mode {
SpriteDrawMode::Sliced(slicer) => slicer.compute_slices(
Copy link
Contributor

Choose a reason for hiding this comment

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

'compute' suggests something that should be done in prepare as it is work that takes time to do and extract is meant to be as fast as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note though that if it doesn't take any significant amount of time for a lot of such entities (i.e. doesn't add more than say 0.1ms compared to as many entities that just use simple sprites) but it would take more time in a separate prepare stage system due to having to iterate over them all again, then it can be OK to do it here. So basically profile and present results.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having now seen the slicer code, I suggest moving that to the prepare stage.

Copy link
Contributor Author

@ManevilleF ManevilleF Jul 7, 2022

Choose a reason for hiding this comment

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

I'll move every computation in prepare once #5247 is merged

draw_size: sprite.custom_size.unwrap_or(image_size),
offset: Vec2::ZERO,
};
slice.tiled(*stretch_value, (*tile_x, *tile_y))
Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks like something that should be done in prepare.

};
for slice in slices {
let mut transform: GlobalTransform = *transform;
transform.translation = transform.mul_vec3(slice.offset.extend(0.0));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another example of something that should be done in prepare.

crates/bevy_sprite/src/sprite.rs Show resolved Hide resolved
crates/bevy_sprite/src/texture_slice.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/texture_slice.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/texture_slice.rs Outdated Show resolved Hide resolved
@ManevilleF ManevilleF force-pushed the feat/sprite_slice branch 2 times, most recently from 01107a8 to 07c0ab7 Compare July 14, 2022 09:14
@Weibye Weibye added the S-Blocked This cannot move forward until something else changes label Aug 19, 2022
register example

Refactoring

Fixed issue on top/bot rects, added max scaling for corners

Tiling

Fixed example artifacts

Update Cargo.toml

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Example page update

Sprite draw mode

removed extra type registration

Fixed performance regression

Rect slicing

Code cleanup

Applied review suggestions

Rebased on main
@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jun 19, 2023
@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Jun 19, 2023
@JMS55 JMS55 removed this from the 0.12 milestone Sep 28, 2023
@ManevilleF ManevilleF mentioned this pull request Nov 16, 2023
1 task
@ManevilleF
Copy link
Contributor Author

Closing in favor of #10588

@ManevilleF ManevilleF closed this Nov 16, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jan 15, 2024
> Replaces #5213

# Objective

Implement sprite tiling and [9 slice
scaling](https://en.wikipedia.org/wiki/9-slice_scaling) for
`bevy_sprite`.
Allowing slice scaling and texture tiling.

Basic scaling vs 9 slice scaling:


![Traditional_scaling_vs_9-slice_scaling](https://user-images.githubusercontent.com/26703856/177335801-27f6fa27-c569-4ce6-b0e6-4f54e8f4e80a.svg)

Slicing example:

<img width="481" alt="Screenshot 2022-07-05 at 15 05 49"
src="https://user-images.githubusercontent.com/26703856/177336112-9e961af0-c0af-4197-aec9-430c1170a79d.png">

Tiling example:

<img width="1329" alt="Screenshot 2023-11-16 at 13 53 32"
src="https://github.com/bevyengine/bevy/assets/26703856/14db39b7-d9e0-4bc3-ba0e-b1f2db39ae8f">

# Solution

- `SpriteBundlue` now has a `scale_mode` component storing a
`SpriteScaleMode` enum with three variants:
  - `Stretched` (default) 
  - `Tiled` to have sprites tile horizontally and/or vertically
- `Sliced` allowing 9 slicing the texture and optionally tile some
sections with a `Textureslicer`.
- `bevy_sprite` has two extra systems to compute a
`ComputedTextureSlices` if necessary,:
- One system react to changes on `Sprite`, `Handle<Image>` or
`SpriteScaleMode`
- The other listens to `AssetEvent<Image>` to compute slices on sprites
when the texture is ready or changed
- I updated the `bevy_sprite` extraction stage to extract potentially
multiple textures instead of one, depending on the presence of
`ComputedTextureSlices`
- I added two examples showcasing the slicing and tiling feature.

The addition of `ComputedTextureSlices` as a cache is to avoid querying
the image data, to retrieve its dimensions, every frame in a extract or
prepare stage. Also it reacts to changes so we can have stuff like this
(tiling example):


https://github.com/bevyengine/bevy/assets/26703856/a349a9f3-33c3-471f-8ef4-a0e5dfce3b01

# Related 

- [ ] Once #5103 or #10099 is merged I can enable tiling and slicing for
texture sheets as ui

# To discuss

There is an other option, to consider slice/tiling as part of the asset,
using the new asset preprocessing but I have no clue on how to do it.

Also, instead of retrieving the Image dimensions, we could use the same
system as the sprite sheet and have the user give the image dimensions
directly (grid). But I think it's less user friendly

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com>
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-Feature A new feature, making something new possible S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Blocked This cannot move forward until something else changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants