-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Allow SPIR-V shaders to process when shader defs are present #7772
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
Any chance of that review, @superdump? If this relatively small change ends up merged, Bevy Rust-GPU will be a step closer to providing a WGSL-comparable workflow for rust-gpu shader crates in mainline bevy. |
if shader_defs_unique.is_empty() { | ||
return Ok(ProcessedShader::SpirV(source.clone())); | ||
} | ||
return Err(ProcessShaderError::ShaderFormatDoesNotSupportShaderDefs); | ||
return Ok(ProcessedShader::SpirV(source.clone())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it makes sense to log a message saying something like "loaded a SpirV shader, it will ignore shader defs"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have thought that would be implicit, or something that would belong documentation-side if anything; the processor is either interested in SPIR-V or not, so my initial feeling is that it would end up filling the log with obvious statements in a project that makes extensive use of the format.
However, @superdump suggested an alternate approach on Discord that seems less intrusive; keeping the hard error, but giving Material
implementors the opportunity to clear out all built-in shader defs at specialization time.
Currently NO_*_SUPPORT
and SIXTEEN_BYTE_ALIGNMENT
get injected after the fact and create the hard error situation this PR aims to remedy, so moving them earlier in the pipeline would avoid breaking the processor's existing assumptions while simultaneously allowing Material
the control it needs for the use-case.
I was planning to open another pull to that effect anyway, so will take a swing at it and see where things go from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New PR: #7903
This is super important for a (edit: Spirv utilizing) subset of bevy users and we need a solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be rebased before it can be merged
7188740
to
a6b6731
Compare
Objective
Solution
Fixes #7771