Skip to content

Yet another Migrate bevy_sprite to required components - #15562

Closed
KirmesBude wants to merge 18 commits into
bevyengine:mainfrom
KirmesBude:feature/sprite-required-components
Closed

KirmesBude wants to merge 18 commits into
bevyengine:mainfrom
KirmesBude:feature/sprite-required-components

Conversation

@KirmesBude

@KirmesBude KirmesBude commented Oct 1, 2024 •

Copy link
Copy Markdown
Contributor

Objective

Migrate bevy_sprite Sprite to required components.
Alternative to #15489
It does not use any of the proposals from here so feel free to disregard this PR.

Solution

  • Sprite renamed to SpriteProperties
  • Introduce new wrapper component Sprite (contains Handle), that requires SpriteProperties, Transform, Visibility and SyncToRenderWorld
  • Sprite specific usage of Handle replaced with Sprite

Previously:

commands.spawn(SpriteBundle {
    texture: textures.add(Image::default()),
    sprite: Sprite {
        color: Color::srgb(7.5, 0.0, 7.5),
       ..default()
    },
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});

Now:

commands.spawn((
    Sprite(textures.add(Image::default())),
    SpriteProperties {
        color: Color::srgb(7.5, 0.0, 7.5),
       ..default()
    },
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
});

Maybe SpriteProperties should just be part of Sprite, not sure. I am open to changing that. Having a dedicated wrapper component for an asset is nice though, e.g. for change detection.
I did not touch TextureAtlas, but I can see the motivation for that in the context of editor usability/suggestion - I just do not think required components are the right tool for that.

Testing

I ran all tests that I modified and everything seemed to work, but I did not compare to main.
bevy_sprite tests were also run and passed.


Migration Guide

Asset handles for sprite images must now be wrapped in the Sprite component. Raw handles as components no longer render sprites. The component previously known as Sprite is now called SpriteProperties.

As a result SpriteBundle has been deprecated. Instead, use the Sprite components directly.

commands.spawn(SpriteBundle {
    texture: textures.add(Image::default()),
    sprite: Sprite {
        color: Color::srgb(7.5, 0.0, 7.5),
       ..default()
    },
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});

Now:

commands.spawn((
    Sprite(textures.add(Image::default())),
    SpriteProperties {
        color: Color::srgb(7.5, 0.0, 7.5),
       ..default()
    },
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
});

If you have any queries for sprites using Handle make sure to change them to Sprite.

Comment thread examples/asset/asset_decompression.rs Outdated
@@ -111,7 +111,7 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
compressed: asset_server.load("data/compressed_image.png.gz"),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to handle this generically now.

@KirmesBude KirmesBude Oct 1, 2024 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added Handle<A>: Into<C> trait bounds. That means C needs to implement From<Handle<A>, which is true for Sprite.
May not be the best solution.

@KirmesBude KirmesBude changed the title Feature/sprite required components Yet another Migrate bevy_sprite to required components Oct 1, 2024
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen S-Waiting-on-SME This is currently waiting for an SME to resolve something controversial C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Oct 1, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 1, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-SME This is currently waiting for an SME to resolve something controversial labels Oct 8, 2024

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have a mild preference for this design. It's more flexible and the migration is easier. Either one is fine by me though.

@wilk10 wilk10 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on this.

I very much prefer this implementation to the alternative PR.

I know that there's no promise of stability but at least reducing churn for users when upgrading is an important factor to consider in this case, in my opinion.

I particularly like that it keeps Sprite and TextureAtlas separate, like it was discussed at length, approved and merged with #5103 , whereas the blessed PR kinda reverts that change.

I'll try to argue my case on Discord, I guess.

Edit: Discussion on Discord

@cart

cart commented Oct 8, 2024

Copy link
Copy Markdown
Member

Responded with thoughts here: #15489 (comment)
(please discuss there)

@alice-i-cecile

Copy link
Copy Markdown
Member

Merging #15489 instead, per the discussion linked just above :)

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 M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants