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.

Sign up for GitHub

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

Jump to bottom

add storage_texture option to as_bind_group macro #9943

Merged
merged 1 commit into from
Jan 21, 2024
Merged

add storage_texture option to as_bind_group macro #9943

merged 1 commit into from
Jan 21, 2024

Conversation

HugoPeters1024
Copy link
Contributor

@HugoPeters1024 HugoPeters1024 commented Sep 27, 2023 •
edited
Loading

Objective

  • Add the ability to describe storage texture bindings when deriving AsBindGroup.
  • This is especially valuable for the compute story of bevy which deserves some extra love imo.

Solution

  • This add the ability to annotate struct fields with a #[storage_texture(0)] annotation.
  • Instead of adding specific option parsing for all the image formats and access modes, I simply accept a token stream and defer checking to see if the option is valid to the compiler. This still results in useful and friendly errors and is free to maintain and always compatible with wgpu changes.

Changelog

  • The #[storage_texture(..)] annotation is now accepted for fields of Handle<Image> in structs that derive AsBindGroup.
  • The game_of_life compute shader example has been updated to use AsBindGroup together with [storage_texture(..)] to obtain the BindGroupLayout.

Migration Guide

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use labels Sep 29, 2023
@JMS55 JMS55 added this to the 0.13 milestone Oct 1, 2023
IceSentry
IceSentry approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Sorry, took me a bit to get to this PR.

LGTM, good job!

examples/shader/compute_shader_game_of_life.rs Outdated
@@ -106,7 +109,7 @@ fn prepare_bind_group(
game_of_life_image: Res<GameOfLifeImage>,
render_device: Res<RenderDevice>,
) {
let view = gpu_images.get(&game_of_life_image.0).unwrap();
let view = gpu_images.get(&game_of_life_image.texture).unwrap();
let bind_group = render_device.create_bind_group(&BindGroupDescriptor {
Copy link
Contributor

@IceSentry IceSentry Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You could probably also use the AsBindGroup::as_bind_group() here

Copy link
Contributor Author

@HugoPeters1024 HugoPeters1024 Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That would require me getting the FallbackImage from somewhere, is that readily available? It's probably also not something we want here tbh.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can add fallback_image: Res<FallbackImage>, as a system parameter and then use it

examples/shader/compute_shader_game_of_life.rs
struct GameOfLifeImage(Handle<Image>);
#[derive(Resource, Clone, Deref, ExtractResource, AsBindGroup)]
struct GameOfLifeImage {
#[storage_texture(0, image_format = Rgba8Unorm, access = ReadWrite)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to use it here!

IceSentry
IceSentry suggested changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually, could you also add some docs here

bevy/crates/bevy_render/src/render_resource/bind_group.rs

Line 261 in 375af64

pub trait AsBindGroup {

You can just copy the block for textures and change the attributes to match your PR.

@Lazauya
Copy link

Lazauya commented Jan 5, 2024

Is the internal imports thing the only thing that's preventing this from being merged? Asking because I'm eager to use this 😅

@HugoPeters1024
Copy link
Contributor Author

Is the internal imports thing the only thing that's preventing this from being merged? Asking because I'm eager to use this 😅

I got a hunch that it's actually a flake, I'll try to rebase soon and see if that fixes it :)

@HugoPeters1024
add storage_texture option to as_bind_group macro
f43007c 
Changes:

- Add storage_texture option to as_bind_group macro
- Use it to generate the bind group layout for the compute shader example
@HugoPeters1024
Copy link
Contributor Author

@IceSentry are you in a position to get this merged? :)

@alice-i-cecile
Copy link
Member

We need a second approval then I can merge this :) See CONTRIBUTING.md for the mechanisms on how reviews work around here.

I would take a look myself, but this is beyond my rendering expertise.

atlv24
atlv24 approved these changes Jan 21, 2024
View reviewed changes
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

game of life perf went from 1350-1400 fps to 1400-1450 fps, shaved 0.02ms or so

@rparrett rparrett added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 21, 2024
@mockersf mockersf added this pull request to the merge queue Jan 21, 2024
Merged via the queue into bevyengine:main with commit 8afb3ce Jan 21, 2024
23 checks passed
slyedoc added a commit to slyedoc/bevy that referenced this pull request Feb 6, 2024
@slyedoc
added bevyengine#9943 changes for storage_texture
167a082
@kristoff3r kristoff3r mentioned this pull request Feb 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IceSentry IceSentry IceSentry approved these changes

@mockersf mockersf mockersf approved these changes

@atlv24 atlv24 atlv24 approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.
8 participants
@HugoPeters1024 @Lazauya @alice-i-cecile @IceSentry @mockersf @atlv24 @rparrett @JMS55