Material::specialize is provided an incomplete set of engine-sourced shader definitions

[ProfLander]

Bevy version

v0.10.0

Relevant system information

`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 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.

[robtfm]

undesirable, as is introducing a BaseShaderDefs newtype resource

just to clarify, i think putting the platform defs in a resource is a good approach. i think using it in a pipeline's FromWorld would be better than passing it to a specialize function though.

[ProfLander]

undesirable, as is introducing a BaseShaderDefs newtype resource

just to clarify, i think putting the platform defs in a resource is a good approach. i think using it in a pipeline's FromWorld would be better than passing it to a specialize function though.

I've opened #8202 to track this approach. It takes a more conservative route than the free-function / trait extension attempts, extending only the pipelines that actively consume the defs in question.

This touches on the footgun described earlier in the issue, since pipeline implementors are required to read the defs from the new BaseShaderDefs resource in order to make them available shader-side.

As it's not strongly trait-encoded as per #7903, it'll likely need to be documented somewhere around the pipeline traits in bevy_render. Beyond that it's probably not so bad, since PipelineCache will throw an error if any unrecognized defs are encountered.