Skip to content

Sprite material v2 #10845

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

Cupnfish
Copy link
Contributor

@Cupnfish Cupnfish commented Dec 2, 2023

Objective

The objective of this PR is to add Material functionality to the existing Sprite component, allowing users to customize the rendering pipeline of sprites by defining their own SpriteMaterial.

Solution

Two versions of the implementation are provided:

1. In this version, a new SpriteWithMaterial component is introduced. It includes a material field to specify the sprite's material tint. The SpriteWithMaterial struct is similar to the existing Sprite struct, with the addition of the material field. However, migrating from the original Sprite to SpriteWithMaterial may be inconvenient.

2. In this version, the original Sprite component is reused, and a SpriteMaterialMarke component is introduced to assist in filtering query results. The changes include adding the material field to the SpriteSheetWithMaterialBundle struct. Migrating from the original Sprite to the SpriteMaterialMarke version requires fewer code changes.

Changelog

  • Added two new bundles: SpriteWithMaterialBundle and SpriteSheetWithMaterialBundle, which include additional components for sprite materials.
#[derive(Bundle, Clone)]
pub struct SpriteWithMaterialBundle<M: SpriteMaterial> {
    pub sprite: Sprite,
    pub transform: Transform,
    pub global_transform: GlobalTransform,
    pub texture: Handle<Image>,
    pub material: Handle<M>,
    pub visibility: Visibility,
    pub inherited_visibility: InheritedVisibility,
    pub view_visibility: ViewVisibility,
    pub marke: SpriteMaterialMarke,
}

#[derive(Bundle, Clone)]
pub struct SpriteSheetWithMaterialBundle<M: SpriteMaterial> {
    pub sprite: TextureAtlasSprite,
    pub texture_atlas: Handle<TextureAtlas>,
    pub material: Handle<M>,
    pub transform: Transform,
    pub global_transform: GlobalTransform,
    pub visibility: Visibility,
    pub inherited_visibility: InheritedVisibility,
    pub view_visibility: ViewVisibility,
    pub marke: SpriteMaterialMarke,
}
  • Modified the SpritePipeline to include filtering conditions for the new components (SpriteMaterialMarke) during extraction.
sprite_query: Extract<
    Query<
        (
            Entity,
            &ViewVisibility,
            &Sprite,
            &GlobalTransform,
            &Handle<Image>,
        ),
        Without<SpriteMaterialMarke>, // Added this condition
    >,
>,
atlas_query: Extract<
    Query<
        (
            Entity,
            &ViewVisibility,
            &TextureAtlasSprite,
            &GlobalTransform,
            &Handle<TextureAtlas>,
        ),
        Without<SpriteMaterialMarke>, // Added this condition
    >,
>,
  • Introduced a new SpriteMaterial trait for defining sprite materials, inspired by the design of UiMaterial.
pub trait SpriteMaterial: AsBindGroup + Asset + Clone + Sized {
    fn vertex_shader() -> ShaderRef {
        ShaderRef::Default
    }

    fn fragment_shader() -> ShaderRef {
        ShaderRef::Default
    }

    #[allow(unused_variables)]
    #[inline]
    fn specialize(descriptor: &mut RenderPipelineDescriptor, key: SpriteMaterialKey<Self>) {}
}
  • Added SpriteMaterialPlugin and SpriteMaterialPipeline to support sprite materials in the pipeline.
pub struct SpriteMaterialPlugin<M: SpriteMaterial>(PhantomData<M>);

#[derive(Resource)]
pub struct SpriteMaterialPipeline<M: SpriteMaterial> {
    pub view_layout: BindGroupLayout,
    pub texture_layout: BindGroupLayout,
    pub material_layout: BindGroupLayout,
    pub vertex_shader: Option<Handle<Shader>>,
    pub fragment_shader: Option<Handle<Shader>>,
    pub dummy_white_gpu_image: GpuImage,
    marker: PhantomData<M>,
}

Please note that there are two versions, v1 and v2, with slight differences in implementation details. The main distinction is that v1 uses a custom sprite component, while v2 utilizes the existing one. The changes in the instantiation of sprite vertex information in v1 account for the replacement of the color field with the material field. Additionally, both versions have significant differences in the queue_sprites section compared to the original Sprite pipeline.

@Cupnfish Cupnfish mentioned this pull request Dec 2, 2023
@Cupnfish
Copy link
Contributor Author

Cupnfish commented Dec 2, 2023

#1738

Copy link
Contributor

github-actions bot commented Dec 2, 2023

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@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 targeted quality-of-life change that makes Bevy easier to use labels Dec 4, 2023
@cloud303-cholden
Copy link

@Cupnfish Would you consider adding a binding for globals? Might be handy for some people. I got it working in a local version of your fork and can submit a PR if you want.

@Cupnfish
Copy link
Contributor Author

@cloud303-cholden Sure, I'm also considering some improvements for this PR and have found a few minor issues. It hasn't been merged into the latest branch yet. You can submit a PR on my branch, and I'll fix those bugs as well.

@cloud303-cholden
Copy link

@Cupnfish Here it is: link.

Copy link
Contributor

github-actions bot commented Jan 1, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

1 similar comment
Copy link
Contributor

github-actions bot commented Jan 1, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@zenoxs
Copy link

zenoxs commented Feb 1, 2024

Would love to see this feature implemented in the next version, any progress on the PR ? 🙏

@Cupnfish
Copy link
Contributor Author

Cupnfish commented Feb 1, 2024

Would love to see this feature implemented in the next version, any progress on the PR ? 🙏

I'm not entirely sure if the current functionality covers most use cases. However, I do have plans to add an example similar to examples/shader/shader_instancing.rs, but specifically designed for sprites. I'll also aim to maximize the reuse of existing render pipelines by leveraging what's already been implemented for sprites. Once these tasks are complete, I believe this pull request will have accomplished its main objectives.

@aekobear
Copy link

This question probably comes from a place of ignorance, but is there a reason you need the SpriteMaterialMarke component in addition the component for the material itself?

It would be convenient if a material could be added to a sprite simply by adding a single material component to the existing sprite bundle, potentially obviating the need for a new bundle in the first place

@Cupnfish
Copy link
Contributor Author

Cupnfish commented Mar 18, 2024

This question probably comes from a place of ignorance, but is there a reason you need the SpriteMaterialMarke component in addition the component for the material itself?

It would be convenient if a material could be added to a sprite simply by adding a single material component to the existing sprite bundle, potentially obviating the need for a new bundle in the first place

@aekobear
In fact, you can directly insert the material and marker on the existing sprite bundle, which is equally convenient. There's no need for you to build a separate bundle.
Why would you need such a marker? The main purpose is to avoid interfering with the existing sprite rendering pipeline.

Copy link

@PraxTube PraxTube left a comment

Choose a reason for hiding this comment

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

This looks pretty promising. Are there any reasons for why this hasn't been merged in? This seems like a fairly decent solution (and also not a super complicated one), but I have heard that it's supposedly not super easy to add sprite shader support. I suppose there are some limitations to this?

It's also using Bevy 0.12, so there may be some issues with updating this to 0.15.

in.i_model_transpose_col2,
)) * vec4<f32>(vertex_position, 1.0);
out.uv = vec2<f32>(vertex_position.xy) * in.i_uv_offset_scale.zw + in.i_uv_offset_scale.xy;
out.color = in.i_color;
Copy link

Choose a reason for hiding this comment

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

I don't quite get this what in.i_color is here. From my tests it seems that SpriteVertexOutput.color is just a vec4 that has either all 0 or 1 based on the alpha of that pixel.

Comment on lines +16 to +19
var color = textureSample(sprite_texture, sprite_sampler, in.uv);
// let g = input.color.r * color.r + input.color.g * color.g + input.color.b * color.b;
let g = in.color.r * color.r + in.color.g * color.g + in.color.b * color.b;
return vec4<f32>(g, g, g, color.a);
Copy link

Choose a reason for hiding this comment

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

Don't quite understand this, in.color.r seems to just always be either 0 or 1? Why not use

var color = textureSample(sprite_texture, sprite_sampler, in.uv);
let g = 0.21 * color.r + 0.71 * color.g + 0.07 * color.b;
return vec4<f32>(g, g, g, color.a);

to calculate the grayscale?

Comment on lines +61 to +71
commands.spawn((
SpriteSheetWithMaterialBundle {
texture_atlas: texture_atlas_handle,
sprite: TextureAtlasSprite::new(animation_indices.first),
transform: Transform::from_scale(Vec3::splat(6.0)),
material: sprite_materials.add(GrayScale {}),
..default()
},
animation_indices,
AnimationTimer(Timer::from_seconds(0.1, TimerMode::Repeating)),
));
Copy link

Choose a reason for hiding this comment

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

I also don't think that we need another Bundle for this, instead

.insert(SpriteMaterialMarke)
.insert(sprite_materials.add(GrayScale {}));

works with the normal SpriteSheetBundle. It's also possible to just make a SpriteMaterialBundle with the material and the Marke so that you only need to insert the bundle (would also make it impossible to forget to add the Marke).

Copy link

Choose a reason for hiding this comment

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

This is even easier with Bevy 0.15's required components rework. SpriteMaterialMarke can be set as a required component so that it'll be added automatically when a sprite material is added

Copy link

@PraxTube PraxTube Jan 1, 2025

Choose a reason for hiding this comment

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

Yeah I was thinking about that too, but do we know what Component a sprite material is? In the example the struct GrayScale is our sprite material, but the way we are turning it into a sprite materials is through SpriteMaterialPlugin::<GrayScale>, is that enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's great to see someone interested in this. I think I will update this PR soon to resolve the conflicts, and I might need everyone's help to review it at that time

Copy link

Choose a reason for hiding this comment

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

You can count me in. I am honestly quite surprised that seemingly few people are interested in this being merged in. Especially because the solution seems quite alright from both API side as well as from the backend (at least from my perspective).

@IceSentry
Copy link
Contributor

Sorry for not commenting sooner, I've been very busy during december.

I'm not sure how I feel about the direction of this PR. One of our long term goal for sprites in bevy is to use the Mesh2d apis and sprites simply being a thin abstraction on top.

Generally, if someone wants to draw a sprite with a custom material I would suggest using a 2d quad with a mesh2d material this makes things a lot easier to maintain for us since it doesn't require adding more code to bevy. The only issue with that approach is when people want to use a texture atlas but I think we should simply provide a way for users to easily configure the mesh2d material vertex shader to support a texture atlas.

To be clear, I'm not saying there's no need for this PR but I need to think about this more.

@Cupnfish
Copy link
Contributor Author

Cupnfish commented Jan 5, 2025

@IceSentry I agree with you that implementing it on existing sprites is a low-return task. Any progress on the new mesh2d-based sprite?

@IceSentry
Copy link
Contributor

Not really, the main thing that we need to figure out is to speed up rendering transparent meshes. Once it becomes fast enough then we could consider switching to it. The opaque pass is currently faster with mesh2d than sprite and it doesn't even take advantage of a ton of recent performance improvements to the renderer so there's a lot of improvements possible there.

I don't think anyone is actively working on this right now, but right now the main blocker is performance. If we switch sprites to mesh2d it has to be very close or faster performance wise.

@Cupnfish
Copy link
Contributor Author

I would like to share some issues and thoughts I encountered while handling conflicts. Firstly, in the current implementation, many parts directly copied the existing sprite rendering pipeline code. This is because, while writing this part of the code, my understanding of some details was not yet deep enough. My main goal was to ensure that implementing the new sprite material would not negatively impact the existing sprite implementation, especially in terms of performance. Therefore, I chose to use SpriteMaterialMark to filter Query to avoid interference.

However, during the process of resolving conflicts, I gradually realized that this might not be the optimal solution. After referencing the implementation of mesh, I removed a significant amount of redundant code and made new attempts based on that. Nonetheless, there is still some distance from the ideal implementation I envisioned.

Based on the current migration experience, I believe we do need a sprite based on mesh implementation. This approach has significant advantages in terms of scalability and maintainability, and of course, we must ensure that the performance of the new sprite does not degrade. Therefore, it may take more time to explore a more reasonable implementation solution. If anyone has any ideas or suggestions, feel free to discuss them here.

@squk
Copy link

squk commented Mar 1, 2025

This is great work! I'm curious if there's any updates on the Mesh2d sprite?

@janhohenheim
Copy link
Member

Triage: has merge conflicts, author would like some design help

@janhohenheim janhohenheim added S-Needs-Design This issue requires design work to think about how it would best be accomplished S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Help The author needs help finishing this PR. and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels May 17, 2025
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Help The author needs help finishing this PR. S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

9 participants