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

[Merged by Bors] - Remove unnecessary struct in Material AsBindGroup example #6701

Closed
wants to merge 2 commits into from

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Nov 20, 2022

Objective

  • Reduce confusion around uniform bindings in materials. I've seen multiple people on discord get confused by it because it uses a struct that is named the same in the rust code and the wgsl code, but doesn't contain the same data. Also, the only reason this works is mostly by chance because the memory happens to align correctly.

Solution

  • Remove the confusing parts of the doc

Notes

It's not super clear in the diff why this causes confusion, but essentially, the rust code defines a CustomMaterial struct with a color and a texture, but in the wgsl code the struct with the same name only contains the color. People are confused by it because the struct in wgsl doesn't need to be there.

You can have complex structs on each side and the macro will even combine it for you if you reuse a binding index, but as it is now, this example seems to confuse more than help people.

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation labels Nov 20, 2022
@Andrewp2
Copy link

I can't tell if this has been fixed, but I know you used to not be able to have just a vec as a uniform in WGSL, you would have to wrap it in a struct

https://matrix.to/#/!XFRnMvAfptAHthwBCx:matrix.org/$fP5iH7XEkp39V7EeRTS3rbeHa7Oj8NOl65sGGJgyjRY?via=matrix.org&via=mozilla.org&via=kyju.org

@Andrewp2
Copy link

Looks like it was fixed in gfx-rs/wgpu#2584 , therefore PR LGTM!

@james7132 james7132 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 Nov 24, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 28, 2022
# Objective

- Reduce confusion around uniform bindings in materials. I've seen multiple people on discord get confused by it because it uses a struct that is named the same in the rust code and the wgsl code, but doesn't contain the same data. Also, the only reason this works is mostly by chance because the memory happens to align correctly.

## Solution

- Remove the confusing parts of the doc

## Notes

It's not super clear in the diff why this causes confusion, but essentially, the rust code defines a `CustomMaterial` struct with a color and a texture, but in the wgsl code the struct with the same name only contains the color. People are confused by it because the struct in wgsl doesn't need to be there.

You _can_ have complex structs on each side and the macro will even combine it for you if you reuse a binding index, but as it is now, this example seems to confuse more than help people.
@bors bors bot changed the title Remove unnecessary struct in Material AsBindGroup example [Merged by Bors] - Remove unnecessary struct in Material AsBindGroup example Nov 28, 2022
@bors bors bot closed this Nov 28, 2022
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
…#6701)

# Objective

- Reduce confusion around uniform bindings in materials. I've seen multiple people on discord get confused by it because it uses a struct that is named the same in the rust code and the wgsl code, but doesn't contain the same data. Also, the only reason this works is mostly by chance because the memory happens to align correctly.

## Solution

- Remove the confusing parts of the doc

## Notes

It's not super clear in the diff why this causes confusion, but essentially, the rust code defines a `CustomMaterial` struct with a color and a texture, but in the wgsl code the struct with the same name only contains the color. People are confused by it because the struct in wgsl doesn't need to be there.

You _can_ have complex structs on each side and the macro will even combine it for you if you reuse a binding index, but as it is now, this example seems to confuse more than help people.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…#6701)

# Objective

- Reduce confusion around uniform bindings in materials. I've seen multiple people on discord get confused by it because it uses a struct that is named the same in the rust code and the wgsl code, but doesn't contain the same data. Also, the only reason this works is mostly by chance because the memory happens to align correctly.

## Solution

- Remove the confusing parts of the doc

## Notes

It's not super clear in the diff why this causes confusion, but essentially, the rust code defines a `CustomMaterial` struct with a color and a texture, but in the wgsl code the struct with the same name only contains the color. People are confused by it because the struct in wgsl doesn't need to be there.

You _can_ have complex structs on each side and the macro will even combine it for you if you reuse a binding index, but as it is now, this example seems to confuse more than help people.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…#6701)

# Objective

- Reduce confusion around uniform bindings in materials. I've seen multiple people on discord get confused by it because it uses a struct that is named the same in the rust code and the wgsl code, but doesn't contain the same data. Also, the only reason this works is mostly by chance because the memory happens to align correctly.

## Solution

- Remove the confusing parts of the doc

## Notes

It's not super clear in the diff why this causes confusion, but essentially, the rust code defines a `CustomMaterial` struct with a color and a texture, but in the wgsl code the struct with the same name only contains the color. People are confused by it because the struct in wgsl doesn't need to be there.

You _can_ have complex structs on each side and the macro will even combine it for you if you reuse a binding index, but as it is now, this example seems to confuse more than help people.
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-Docs An addition or correction to our documentation 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.

6 participants