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

Use BindGroupLayoutEntryBuilder in texture_binding_array example #13169

Merged

Conversation

mgi388
Copy link
Contributor

@mgi388 mgi388 commented May 2, 2024

Objective

  • I've been using the texture_binding_array example as a base to use multiple textures in meshes in my program
  • I only realised once I was deep in render code that these helpers existed to create layouts
  • I wish I knew the existed earlier because the alternative (filling in every struct field) is so much more verbose

Solution

  • Use BindGroupLayoutEntries::with_indices to teach users that the helper exists
  • Also fix typo which should be texture_2d.

Alternatives considered

  • Just leave it as is to teach users about every single struct field
  • However, leaving as is leaves users writing roughly 29 lines versus roughly 2 lines for 2 entries and I'd prefer the 2 line approach

Testing

Ran the example locally and compared before and after.

Before:

image

After:

image

Also fix typo which should be `texture_2d`.
Copy link
Contributor

github-actions bot commented May 2, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@mgi388
Copy link
Contributor Author

mgi388 commented May 2, 2024

@robtfm and @IceSentry hope I can ping you for a review. I see that you both worked on a) the example and b) the helpers.

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.

I'm not sure if the comments with the full wgsl bind group code is necessary but it doesn't really hurt either so that's fine I guess.

@mgi388
Copy link
Contributor Author

mgi388 commented May 2, 2024

I'm not sure if the comments with the full wgsl bind group code is necessary but it doesn't really hurt either so that's fine I guess.

I'm impartial, I just left it in there because it was already there.

FWIW, as someone relatively unfamiliar with how these entries map to the WGSL bind group code, I will say I found it to be a nice clue to how they are actually mapped. E.g. the 1 index in Rust code means I should look for a @binding(1) in the WGSL code.

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples labels May 2, 2024
@IceSentry
Copy link
Contributor

FWIW, as someone relatively unfamiliar with how these entries map to the WGSL bind group code, I will say I found it to be a nice clue to how they are actually mapped. E.g. the 1 index in Rust code means I should look for a @binding(1) in the WGSL code.

In that case it's worth leaving there. I tried to make that api to be relatively clear to what it maps to in wgsl, but I'm very biased since I'm the one that made it so it's understandable that it isn't as obvious to other people. Anyway, I also just want more people to use it because it makes writing that kind of code much easier.

@robtfm robtfm 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 May 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 2, 2024
Merged via the queue into bevyengine:main with commit 78bf48b May 2, 2024
31 checks passed
@mgi388 mgi388 deleted the helper-in-texture-binding-array branch May 2, 2024 21:46
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-Examples An addition or correction to our examples 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants