Conversation
atlv24
added
A-Rendering
atlv24
added
C-Code-Quality
atlv24
force-pushed
the
ad/shader-cache
branch
3 times, most recently
from
b54cdf3 to
7f07213
Compare
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Reviewer note: this is needed because in a future commit I make ShaderData generic over ShaderModule, but the Default derive does not pick up on the fact that it doesnt need ShaderData: Default to make this struct Default, since its in a HashMap (which is Default regardless of K and V)
crates/bevy_shader/src/shader.rs
Outdated
|
|
||
| /// A shader, as defined by its [`ShaderSource`](wgpu::ShaderSource) and [`ShaderStage`](naga::ShaderStage) | ||
| /// This is an "unprocessed" shader. It can contain preprocessor directives. | ||
| /// A an "unprocessed" shader. It can contain preprocessor directives. |
There was a problem hiding this comment.
Reviewer note: this comment was really outdated, ShaderSource and ShaderStage are not involved at this point of a Shader's lifecycle, its just text at this point.
| fn from(value: NonZero<u32>) -> Self { | ||
| Self(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Reviewer note: this is necessary because define_atomic_id lives in bevy_render
There was a problem hiding this comment.
Maybe it should be elsewhere? bevy_util or something?
There was a problem hiding this comment.
ive considered it, there was some opposition back when i made bevy_image, so i ended up figuring out a way to avoid pulling the stuff that needed it out. There is growing evidence to needing this generally though, so perhaps a good followup, allowing more image stuff to be pulled into bevy_image
| Wgsl(String), | ||
| /// Naga module. | ||
| Naga(naga::Module), | ||
| } |
There was a problem hiding this comment.
Reviewer note: this is introduced to avoid taking on a dependency on wgpu just for the ShaderSource struct, but also is a looooot simpler than that struct.
alice-i-cecile
added
S-Needs-Testing
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
On board with this plan, and the changes seem fine. I don't have time to do testing right now though, so I would like more testing before we merge.
| shader_cache: Arc::new(Mutex::new(ShaderCache::new( | ||
| device.features(), | ||
| render_adapter.get_downlevel_capabilities().flags, | ||
| load_module, |
There was a problem hiding this comment.
Here it's not clear that the load_module function is coming from the global scope (unless you open the file, go to reference with the language server), maybe rename the top level function to load_module_global or load_cache_module just to make it more clear?
There was a problem hiding this comment.
true! i think its fine though since this would only be confusing upon review, not during actual code maintenance though? since we'll have our editor open and it'll be navigable most of the time we care about this going forward
There was a problem hiding this comment.
i think the only thing that would make it obvious to me is wrapping it in a closure, but i think that looks ugly so i dont wanna do it lmao
atlv24
added
C-Bug
# Objective - split off bevy_shader from bevy_render to allow for defining shader libraries and materials for scenes without depending on a renderer ## Solution - a bunch of small refactor commits, then bevy_shader! (review commit by commit) ## Testing - please help with this, especially with spirv passthrough and decoupled_naga testing: i only ran 3d_scene at every step to make sure i didnt break anything. EDIT: this has been tested now, decoupled_naga works, and I also fixed a bug that was on main when wesl and decoupled_naga were both present it didnt compile, now it does # Future Work - Split ShaderCacheError out of PipelineCacheError (or just rename) (punted because breaking change) - Consider renaming PipelineCacheId to ShaderCacheId ? (punted because breaking change) - bevy_material - bevy_bsdf (bindingless pbr shader library) - move view.wgsl into bevy_camera after bevyengine#20313 lands ?
4 participants
Objective
Solution
Testing
EDIT: this has been tested now, decoupled_naga works, and I also fixed a bug that was on main when wesl and decoupled_naga were both present it didnt compile, now it does
Future Work