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

Unify sprites and sprite sheets #5072

Closed
wants to merge 1 commit into from

Conversation

ManevilleF
Copy link
Contributor

@ManevilleF ManevilleF commented Jun 22, 2022

Related PR: #5070 (bevy_ui support for texture sheets)

Objective

Improve ergonomics for sprite sheets and remove:

  • SpriteSheetBundle
  • TextureAtlasSprite which is almost identic to Sprite

Dev side, this will improve maintenance and avoid duplicate queries and systems for sprite sheets

Solution

I replaced the SpriteBundle::texture field , a Handle<Image>, by a custom SpriteImage enum which can store either:

  • a Handle<Image>
  • a Handle<TextureAtlas> and texture sheet index

Tasks:

  • SpriteImage
  • Update extraction system
  • Docs
  • Update examples
  • test examples

Changelog

  • (BREAKING) SpriteBundle now takes a SpriteImage instead of a Handle<Image>
  • (BREAKING) Removed SpriteSheetBundle
  • (BREAKING) Removed TextureAtlasSprite component

Migration Guide

Sprites

let handle = asset_server.load("my_icon.png");
commands.spawn_bundle(SpriteBundle {
-   texture: handle,
+   texture: handle.into(),
   ..default()
});

or

let handle = asset_server.load("my_icon.png");
commands.spawn_bundle(SpriteBundle {
-   texture: handle,
+   texture: SpriteImage::Image(handle),
   ..default()
});

Sprite sheets

Before:

let image = asset_server.load("sheet.png");
let atlas_handle = texture_atlases.add(TextureAtlas::from_grid(image, Vec2::splate(100.0), 2, 2);
commands.spawn_bundle(SpriteSheetBundle {
   texture_atlas: atlas_handle,
   sprite: TextureAtlasSprite::new(0),
   ..default()
});

after:

let image = asset_server.load("sheet.png");
let atlas_handle = texture_atlases.add(TextureAtlas::from_grid(image, Vec2::splate(100.0), 2, 2);
commands.spawn_bundle(SpriteBundle {
   texture: SpriteImage::TextureAtlas {
      handle: atlas_handle,
      index: 0,
   },
   ..default()
});

@ManevilleF ManevilleF changed the title Sprite textures is now an enum Unify sprites and sprite sheets Jun 22, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Jun 22, 2022
use bevy_reflect::Reflect;
use bevy_render::texture::{Image, DEFAULT_IMAGE_HANDLE};

/// The sprite texture
Copy link
Member

Choose a reason for hiding this comment

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

These docs are not useful to users who do not know what a sprite is.


/// The sprite texture
#[derive(Component, Clone, Debug, Reflect)]
pub enum SpriteImage {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps just Sprite? I'm curious about others opinion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well Sprite is already a component

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jun 22, 2022

We should run some of the 2D stress tests before and after this PR. Ideally we also put together a texture-atlas stress test in some form too.

We are introducing a branch, so I would expect to see some performance regression, but it may be very small in practice.

On the plus side this would make swapping an entity between a single sprite and a sprite sheet much faster because there's no components being added and removed. I can't imagine that happens much though in practice?

@mockersf
Copy link
Member

I'm not sure I like having to add those .into() when just using an image

@ManevilleF
Copy link
Contributor Author

I'm not sure I like having to add those .into() when just using an image

I get that, and the enum approach adds some abstraction that prevents querying directly for Handle<Image> like before.

Note that you already have to call .into() for bevy_ui for example, since its using a UiImage which is a container for the handle.

Either way I think its a choice between:

  • Direct handle manipulation
  • type wrappers

But I do like the enum wrapper as it removes redundancy, but maybe there is a better way

@ManevilleF ManevilleF mentioned this pull request Jun 26, 2022
@ManevilleF ManevilleF changed the title Unify sprites and sprite sheets Unify sprites and sprite sheets (enum approach) Jun 26, 2022
@ManevilleF ManevilleF changed the title Unify sprites and sprite sheets (enum approach) Unify sprites and sprite sheets Jun 26, 2022
@ManevilleF ManevilleF marked this pull request as ready for review June 27, 2022 07:32
@alice-i-cecile
Copy link
Member

Closing in favor of #5103.

@ManevilleF ManevilleF deleted the feat/sprite_rework branch July 4, 2022 13:51
@s-puig s-puig mentioned this pull request Oct 12, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2024
# Objective

> Old MR: #5072 
> ~~Associated UI MR: #5070~~
> Adresses #1618

Unify sprite management

## Solution

- Remove the `Handle<Image>` field in `TextureAtlas` which is the main
cause for all the boilerplate
- Remove the redundant `TextureAtlasSprite` component
- Renamed `TextureAtlas` asset to `TextureAtlasLayout`
([suggestion](#5103 (comment)))
- Add a `TextureAtlas` component, containing the atlas layout handle and
the section index

The difference between this solution and #5072 is that instead of the
`enum` approach is that we can more easily manipulate texture sheets
without any breaking changes for classic `SpriteBundle`s (@mockersf
[comment](#5072 (comment)))

Also, this approach is more *data oriented* extracting the
`Handle<Image>` and avoiding complex texture atlas manipulations to
retrieve the texture in both applicative and engine code.
With this method, the only difference between a `SpriteBundle` and a
`SpriteSheetBundle` is an **additional** component storing the atlas
handle and the index.

~~This solution can be applied to `bevy_ui` as well (see #5070).~~

EDIT: I also applied this solution to Bevy UI

## Changelog

- (**BREAKING**) Removed `TextureAtlasSprite`
- (**BREAKING**) Renamed `TextureAtlas` to `TextureAtlasLayout`
- (**BREAKING**) `SpriteSheetBundle`:
  - Uses a  `Sprite` instead of a `TextureAtlasSprite` component
- Has a `texture` field containing a `Handle<Image>` like the
`SpriteBundle`
- Has a new `TextureAtlas` component instead of a
`Handle<TextureAtlasLayout>`
- (**BREAKING**) `DynamicTextureAtlasBuilder::add_texture` takes an
additional `&Handle<Image>` parameter
- (**BREAKING**) `TextureAtlasLayout::from_grid` no longer takes a
`Handle<Image>` parameter
- (**BREAKING**) `TextureAtlasBuilder::finish` now returns a
`Result<(TextureAtlasLayout, Handle<Image>), _>`
- `bevy_text`:
  - `GlyphAtlasInfo` stores the texture `Handle<Image>`
  - `FontAtlas` stores the texture `Handle<Image>`
- `bevy_ui`:
- (**BREAKING**) Removed `UiAtlasImage` , the atlas bundle is now
identical to the `ImageBundle` with an additional `TextureAtlas`

## Migration Guide

* Sprites

```diff
fn my_system(
  mut images: ResMut<Assets<Image>>, 
-  mut atlases: ResMut<Assets<TextureAtlas>>, 
+  mut atlases: ResMut<Assets<TextureAtlasLayout>>, 
  asset_server: Res<AssetServer>
) {
    let texture_handle: asset_server.load("my_texture.png");
-   let layout = TextureAtlas::from_grid(texture_handle, Vec2::new(25.0, 25.0), 5, 5, None, None);
+   let layout = TextureAtlasLayout::from_grid(Vec2::new(25.0, 25.0), 5, 5, None, None);
    let layout_handle = atlases.add(layout);
    commands.spawn(SpriteSheetBundle {
-      sprite: TextureAtlasSprite::new(0),
-      texture_atlas: atlas_handle,
+      atlas: TextureAtlas {
+         layout: layout_handle,
+         index: 0
+      },
+      texture: texture_handle,
       ..Default::default()
     });
}
```
* UI


```diff
fn my_system(
  mut images: ResMut<Assets<Image>>, 
-  mut atlases: ResMut<Assets<TextureAtlas>>, 
+  mut atlases: ResMut<Assets<TextureAtlasLayout>>, 
  asset_server: Res<AssetServer>
) {
    let texture_handle: asset_server.load("my_texture.png");
-   let layout = TextureAtlas::from_grid(texture_handle, Vec2::new(25.0, 25.0), 5, 5, None, None);
+   let layout = TextureAtlasLayout::from_grid(Vec2::new(25.0, 25.0), 5, 5, None, None);
    let layout_handle = atlases.add(layout);
    commands.spawn(AtlasImageBundle {
-      texture_atlas_image: UiTextureAtlasImage {
-           index: 0,
-           flip_x: false,
-           flip_y: false,
-       },
-      texture_atlas: atlas_handle,
+      atlas: TextureAtlas {
+         layout: layout_handle,
+         index: 0
+      },
+      image: UiImage {
+           texture: texture_handle,
+           flip_x: false,
+           flip_y: false,
+       },
       ..Default::default()
     });
}
```

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: François <mockersf@gmail.com>
Co-authored-by: IceSentry <IceSentry@users.noreply.github.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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants