Skip to content

Yet another Migrate bevy_sprite to required components #15562

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

Closed

Conversation

KirmesBude
Copy link
Contributor

@KirmesBude KirmesBude commented Oct 1, 2024

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.

@@ -111,7 +111,7 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
compressed: asset_server.load("data/compressed_image.png.gz"),
Copy link
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.

Copy link
Contributor Author

@KirmesBude KirmesBude Oct 1, 2024

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-Needs-SME Decision or review from an SME is required C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-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-Needs-SME Decision or review from an SME is required labels Oct 8, 2024
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.

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

Copy link
Contributor

@wilk10 wilk10 left a comment

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
Copy link
Member

cart commented Oct 8, 2024

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

@alice-i-cecile
Copy link
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-Needs-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
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants