`Material::specialize` is provided an incomplete set of engine-sourced shader definitions
- Dominant language
- Rust
- Stars
- 48.2k
- Forks
- 4.8k
- Avg merge
- 3d 22h
- Merged PRs (30d)
- 161
Description
## Bevy version
`v0.10.0`
## Relevant system information
```ignore
`AdapterInfo { name: "AMD Radeon RX 6900 XT (RADV NAVI21)", vendor: 4098, device: 29631, device_type: DiscreteGpu, driver: "radv", driver_info: "Mesa 22.3.6", backend: Vulkan }`
```
## What you did
* Implemented a custom `Material` type
* Inside `Material::specialize`, inspected the contents of `VertexState::shader_defs` and `FragmentState::shader_defs` to determine which definitions would be active inside the respective shaders.
## What went wrong
* `NO_ARRAY_TEXTURES_SUPPORT`, `SIXTEEN_BYTE_ALIGNMENT`, and `AVAILABLE_STORAGE_BUFFER_BINDINGS` activate in shaders, but do not appear in either `shader_defs` vector.
* This prevents the `Material` in question from reasoning about the state of certain mesh view bindings with respect to its shaders, such as point lights, cluster information, and globals.
## Additional information
The definitions in question are injected into the `shader_defs` vector that gets passed to `ShaderProcessor::process` in `ShaderCache::get`.
This occurs after `Material::specialize`, which is part of the `Specialized*Pipeline::specialize` call hierarchy - the entirety of which is also subject to the issue in question - and originates in various render world systems related to concrete implementations of those traits.
This does not match the lifecycle of the data that drives these definitions. `NO_ARRAY_TEXTURES_SUPPORT` and `SIXTEEN_BYTE_ALIGNMENT` are conditional over the `webgl` feature flag, and `AVAILABLE_STORAGE_BUFFER_BINDINGS` is driven by `wgpu::Limits::max_storage_buffers_per_shader_stage`.
Neither of these will change over the course of the program, so there is no intrinsic reason to reconstruct them whenever `ShaderCache::get` is called. In addition to creating a consistency problem, this is spending CPU time where memory could do the job.
Furthermore, the data used to construct said definitions is freely available from the language and ECS respectively. This encourages affected users to duplicate bevy-internal code, violating the DRY principle and creating brittle points of failure should the definitions or logic behind them change in a future engine version.
This can be fixed by introducing some form of top-level definition storage to the specialization trait / call hierarchy, and providing access so members of the tree can construct their definition sets from it.
### Related
**Incidental:** #7771 `ShaderCache`'s late-injection behaviour was previously preventing the use of SPIR-V by `Material` implementors, as `ShaderProcessor` was checking the `shader_defs` vector for zero-length (as it does not support processing binary) and returning an error.
The `ShaderProcessor` issue was initially addressed by opening #7772, but discussion with @superdump on Discord (documented [here](https://gist.github.com/Shfty/646b691f8934f601a0bb721d37d10ec5) for issue tracker transparency) concluded that the definitions in question should be made available to `Material`, so they could be manually cleared by the user. To this end, #7903 was opened as an alternate solution with the intent of providing a more idiomatic fix.
Recently #7772 was merged (closing #7771), presumably as an intermediary measure to remove the blocker for SPIR-V users. No comment was made with respect to the state of #7903, so it can be assumed that the intent is still to fix the issue in a robust way, as originally discussed.
#7903 was reviewed by @superdump re. potential footgun caused by moving said defs into a free function and requiring `Specialized*Pipeline` implementors to call it, which was addressed by moving them to a member inside `PipelineCache` - of which `ShaderCache` is a member - and extending the `*::specialize` trait hierarchy with a new `shader_defs` input, mutated in lieu of constructing an empty vec by existing implementors.
Further discussion with @robtfm in #7903 indicates adding a base `shader_defs` member to `PipelineCache` for this purpose is undesirable, as is introducing a `BaseShaderDefs` newtype resource to be consumed in applicable render systems, and extending the `*::specialize` hierarchy with a new parameter. That would leave static storage (i.e. `Lazy`), or some yet-to-be determined mechanism that satisfies the various requirements.
This issue has been opened to enumerate the problem in full context, act as an anchor for the previously free-floating #7903, and open discussion on a desirable approach to fixing it.
Contributor guide
Assessment
This issue has not been assessed yet.