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

Reflect shader stage for bind groups. #189

Merged
merged 7 commits into from
Aug 20, 2020

Conversation

StarArawn
Copy link
Contributor

@StarArawn StarArawn commented Aug 14, 2020

This PR changes the shader reflection to automatically pick the correct shader stage/s.

@StarArawn StarArawn changed the title Working on correctly reflecting shader stage for bind groups. Reflect shader stage for bind groups. Aug 14, 2020
@karroffel karroffel added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Aug 14, 2020
@cart
Copy link
Member

cart commented Aug 14, 2020

In general this looks like the right approach. Just left one comment.

@StarArawn StarArawn requested a review from cart August 15, 2020 03:22
@StarArawn StarArawn marked this pull request as ready for review August 15, 2020 03:23
@cart
Copy link
Member

cart commented Aug 15, 2020

Testing on my machine now. Then we should be good to go!

@StarArawn
Copy link
Contributor Author

@cart
After much debugging I have a hack to fix all of the examples. In most of the 3D examples the shader being used is forward.vert/frag which implements the camera uniform the "both" the vertex and fragment stage. The pass node specifies that the resulting uniform must have both a Vertex and a Fragment shader stage. Before when we didn't reflect this wasn't an issue, but now its a bit more of an issue. We need a better mechanism of picking the descriptor perhaps by ignoring shader stage... Perhaps our hash implementation shouldn't account for shader stage at all, but I don't think that's right. I also don't think we should be retrieving the binding descriptor based off of name so I'm a little stuck on how I can "unhack" this.

camera_bind_group_descriptor:
https://github.com/bevyengine/bevy/blob/master/crates/bevy_render/src/render_graph/nodes/pass_node.rs#L73

@StarArawn StarArawn mentioned this pull request Aug 18, 2020
@multun
Copy link
Contributor

multun commented Aug 19, 2020

Can you rebase this on master? Format checking was enabled, and I believe the CI doesn't yet check for it at the commit you're at

@StarArawn
Copy link
Contributor Author

@multun Seems like cargo fmt doesn't work in bevy on stable. I was able to format each file though using vscode/RSA. This is ready for review now.

@cart
Copy link
Member

cart commented Aug 20, 2020

Perhaps our hash implementation shouldn't account for shader stage at all, but I don't think that's right.

Yeah we need the hash to be a 1:1 map between "things wgpu cares about / would require a new wgpu descriptor to work"

I also don't think we should be retrieving the binding descriptor based off of name so I'm a little stuck on how I can "unhack" this.

i think retrieving binding descriptor by index (without checking name) in a cross-shader context is probably even more shakey. what if the shader sets binding 0 to something that isnt the camera? names / conventions give us a set of "rules" we can rely on.

I'm a little stuck on how I can "unhack" this.

Yeah this is a little weird. I think I'm fine with merging this as-is, but we should create a tracking issue to resolve the underlying problem. This probably won't be the last time it comes up.

@cart cart merged commit 1ebb7e4 into bevyengine:master Aug 20, 2020
@cart
Copy link
Member

cart commented Aug 20, 2020

Follow up via #264

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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants