delegate layout reflection to RenderResourceContext #691
Conversation
memoryruins
added
O-Web
|
Moving reflect_pipeline_layout to RenderResourceContext seems reasonable, but I'm less a fan of requiring a name and the set/binding in DynamicBindings. Manually declaring DynamicBindings was meant to be a temporary measure anyway. I'd like to reflect them from the values on I know thats a big ask, but DynamicBindings are already a usability wart and I don't really want to make that worse. |
mrk-its
force-pushed
the
delegate_pipeline_layout_reflection
branch
from
b39511a to
4cf9624
Compare
|
I don't like these declarations as well. Maybe 4cf9624 is step in good direction? |
|
Yeah I think thats a better intermediate step while we sort out making it completely implicit. Your change both makes declarations consistent across backends and cuts down on boilerplate slightly. I think we should read dynamic bindings from RenderResourceBindings, then set the PipelineSpecialization according to those values. If thats easy, lets just do that instead (rather than break a public api twice). If its hard, im cool with tabling it. |
|
If dynamic_index is Some, then its a dynamic binding.
|
Ok, I'll try to go this way |
mrk-its
force-pushed
the
delegate_pipeline_layout_reflection
branch
5 times, most recently
from
ad89d22 to
8c801c0
Compare
|
It was easier than expected. No more manual dynamic bindings declarations! |
crates/bevy_pbr/src/entity.rs
Outdated
| @@ -26,21 +26,7 @@ impl Default for PbrComponents { | |||
| Self { | |||
| render_pipelines: RenderPipelines::from_pipelines(vec
There was a problem hiding this comment.
We can replace the specialized() constructor with new() now.
| @@ -18,7 +18,6 @@ use std::borrow::Cow; | |||
| pub struct PipelineSpecialization { | |||
| pub shader_specialization: ShaderSpecialization, | |||
| pub primitive_topology: PrimitiveTopology, | |||
| pub dynamic_bindings: Vec<DynamicBinding>, | |||
There was a problem hiding this comment.
Moving this out of PipelineSpecialization feels a bit wrong. I totally get that by directly consuming RenderResourceBindings, we can cut down on abstraction (and the extra Vec allocation), but having everything that "specializes" a pipeline in one place seems like a good design to keep. If in the future we want to do something like "hash pipeline specialization to identify a pipeline", we would need to undo the change you made here to ensure the dynamic bindings are accounted for.
mrk-its
force-pushed
the
delegate_pipeline_layout_reflection
branch
4 times, most recently
from
7bb2c8a to
4a7df1f
Compare
|
I pushed a proposed change that accomplishes the following:
|
|
Thanks, taking a look (I'll also check if this still works with bevy_webgl2). |
|
It looks much better now. I've also checked it works with bevy_webgl. Ship it! |
|
Awesome its a new lint that they just rolled out. You've gotta do |
Also: * auto-reflect DynamicBindings * use RenderPipeline::new, update dynamic_bindings linting.
mrk-its
force-pushed
the
delegate_pipeline_layout_reflection
branch
from
761948c to
a4b5f7f
Compare
It is probably last but one PR required by current version of webgl2 backend.
The main thing here is to delegate pipeline layout reflection to
RenderResourceContext.reflect_pipeline_layoutmethod. This way webgl2 backend can easly replace layout reflection with custom implementation using webgl2 API itself - here is current implementation: https://github.com/mrk-its/bevy/blob/0ce1e2ab7885f9ee922713c10bfbb358d4ceaf79/crates/bevy_webgl2/src/renderer/webgl2_render_resource_context.rs#L122The other, but related, thing is introducing of
namefield in DynamicBinding structure. It allows for defining dynamic bindings by name, in addition to (binding_group_index, binding_index) pair. This is required because version "300 es" of GLSL doesn't support declaring theselayout(set = x, binding = y)parameters for uniform blocks and webgl2 backend uses matching by name.(default implementation simply ignores this
namefield and still matches uniforms by (binding_group, binding) pair). Thisnamefield is also good as human readable description of particular DynamicBinding definition (I removed comments there because they are not necessary now).