-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
improve shader import model #5703
Conversation
I would be happy to help develop and maintain the |
thanks, i appreciate it - worth noting though that even if it's part of bevy it can still be imported and used as a standalone crate if you find it useful for you outside of bevy. i am mostly concerned with bevy since that's where i want to use it, so if @cart wants to own it that's totally fine with me. |
I actually have a use for it outside of Bevy, but I think that integrating it with Bevy would be most useful to the most people. |
added overrides, so functions can be replaced dynamically, and added an example todo still: provide a mechanism for general overrides (so you can override the default lighting functions - this only allows for overriding on a per-material basis currently). |
and added global/default overrides, with |
Gave this a very quick once over (this will take a lot of time to review properly), but I now have a decent understanding of the approach taken. Some quick thoughts:
|
thanks for looking it over. i'm very happy for it to be under bevy org or part of bevy core as you prefer, and happy it would benefit from bevy's review process as well.
the
this could work easily if they are still qualified at point of use, but i guess that's not what you're intending. in the current impl there's no lexing done outside of naga, it's all regex-based - we prefix all the members of a module with a sentinel string and the (base32 encoded) module name to make a "namespace", and then search/replace if we want to import individual names and use them without qualification it may need more care to make sure we only modify exactly what we should. it's probably fine to just use a regex with initial whitespace but i haven't fully thought it through. at worst,
marking functions as overridable is straight-forward. personally i would prefer not to have artificial restrictions on the injection points i can choose, but i'm more familiar with the pbr internals than many users and maybe the support burden would be an issue. but perhaps just documenting "safe" entry points for user overrides would be enough?
one thing to bear in mind: the "API" that we would be fixing through overridable function signatures is not quite as restrictive as it seems initially - we can also take the vertex/fragment input and store it to a global variable, so that overriding functions have access to whatever data they need. at the extreme we could also do this with other derived data and reduce the overridable function signatures down to practically nothing. this might be inefficient (i'm not sure how good the driver dead-code analysis is wrt calculations that get stored to globals but never read) but the purge module could fix it if it's a problem. definitely interested to hear other people's thoughts here though.
note it is only manually defined for vertex output (there's a shared struct for frag input) ... it is grim though. the current restriction seems to be that structs can be used in entry-point IO, but only one level deep. i'm not entirely sure where the restriction comes from. ideally in future wgsl (or naga) will allow us to have entry-point IO where only the leaves of a heirarchy need to be located. |
i've moved @bulitin(position) into the MeshVertexOutput structs so we can use them directly as the vertex stage output and fragment stage input, without needing to embed them within another struct layer. this seems good to me ... i'm not sure if there's a reason why we are not doing this already. edit: but of course it still can't be so easily extended by authors of custom vertex shaders ... oops |
further to the above, the struct depth limitation for entry point IO comes from the WGSL spec. i've tried briefly to extend the naga internals to accept nested structs but have hit a point where my lack of spirv knowledge is blocking me and i am not really prepared to learn it well enough to debug. i think it would be possible though since spirv seems to use globals for stage IO. but i think it's not so bad as it is. depending on if you use custom vertex shaders for
|
The custom_gltf_vertex_attribute and texture_binding_array examples are also broken (unresolved imports). |
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.
This is looking great! A huge step forward in the usability and modularity of our shader code. Now that we have these capabilities, I think our next steps should be to consolidate imports and improve the UX of writing shaders / consuming built-in Bevy functionality. I think we have too many different modules right now. Some thoughts:
- Combine things like mesh_bindings, mesh_functions, mesh_vertex_output, etc into
mesh
. Now that we have granular imports we shouldn't need the granular modules in most cases. - Consider creating preludes to make it super simple to do common things.
# Objective #5703 caused the normal prepass to fail as the prepass uses `pbr_functions::apply_normal_mapping`, which uses `mesh_view_bindings::view` to determine mip bias, which conflicts with `prepass_bindings::view`. ## Solution pass the mip bias to the `apply_normal_mapping` function explicitly.
# Objective Followup bugfix for #5703. Without this we get the following error when CAS (Contrast Adaptive Sharpening) is enabled: ``` 2023-06-29T01:31:23.829331Z ERROR bevy_render::render_resource::pipeline_cache: failed to process shader: error: unknown type: 'FullscreenVertexOutput' ┌─ crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/robust_contrast_adaptive_sharpening.wgsl:63:17 │ 63 │ fn fragment(in: FullscreenVertexOutput) -> @location(0) vec4<f32> { │ ^^^^^^^^^^^^^^^^^^^^^^ unknown type │ = unknown type: 'FullscreenVertexOutput' ``` @robtfm I wouldn't expect this to fail. I was under the impression the `#import bevy_core_pipeline::fullscreen_vertex_shader` would pull "everything" from that file into this one?
Since 10f5c92, parallax mapping was broken. When bevyengine#5703 was merged, the change from `in.uv` to `uv` in the pbr shader was reverted. So the shader would use the wrong coordinate to sample the various textures. We revert to using the correct uv.
# Objective Since 10f5c92, shadows were broken for models with morph target. When #5703 was merged, the morph target code in `render/mesh.wgsl` was correctly updated to use the new import syntax. However, similar code exists in `prepass/prepass.wgsl`, but it was never update. (the reason code is duplicated is that the `Vertex` struct is different for both files). ## Solution Update the code, so that shadows render correctly with morph targets.
Unused since #5703
Objective
operate on naga IR directly to improve handling of shader modules.
the ultimate objective is to make it possible to
but ... since this is already big, adds some value and (i think) is at feature parity with the existing code, i wanted to push this now.
Solution
i made a crate called naga_oil (https://github.com/robtfm/naga_oil - unpublished for now, could be part of bevy) which manages modules by
then integrated this into bevy, replacing some of the existing shader processing stuff. also reworked examples to reflect this.
Migration Guide
shaders that don't use
#import
directives should work without changes.the most notable user-facing difference is that imported functions/variables/etc need to be qualified at point of use, and there's no "leakage" of visible stuff into your shader scope from the imports of your imports, so if you used things imported by your imports, you now need to import them directly and qualify them.
the current strategy of including/'spreading'
mesh_vertex_output
directly into a struct doesn't work any more, so these need to be modified as per the examples (e.g. color_material.wgsl, or many others). mesh data is assumed to be in bindgroup 2 by default, if mesh data is bound into bindgroup 1 instead then the shader defMESH_BINDGROUP_1
needs to be added to the pipeline shader_defs.