-
-
Notifications
You must be signed in to change notification settings - Fork 4k
spec_v2
: migrate bloom
#19966
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
base: main
Are you sure you want to change the base?
spec_v2
: migrate bloom
#19966
Conversation
shader: fragment_shader.clone(), | ||
entry_point: Some("upsample".into()), | ||
targets: vec![Some(ColorTargetState { | ||
format: TextureFormat::Rgba8Unorm, // placeholder |
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.
Since most of this is just placeholder I feel like it would make more sense to have no targets in the base descriptor and then add it in the specializer. That or we need a better way to have placeholders, I don't like the idea of using a comment for that.
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.
Sounds good, I guess it probably doesn't make sense to update individual fields in ColorTargetState
independent of the whole struct anyways.
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.
I think anywhere else that needs placeholders should be defaultable now too
similar utils for layout etc should be added later as needed
@@ -153,3 +159,9 @@ pub struct ComputePipelineDescriptor { | |||
/// If this is false, reading from workgroup variables before writing to them will result in garbage values. | |||
pub zero_initialize_workgroup_memory: bool, | |||
} | |||
|
|||
fn filling_insert_at<T: Clone>(vec: &mut Vec<T>, index: usize, filler: T, value: T) { |
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.
I'm not sure I understand why this is necessary. Why not just push the target directly in the specializer?
|
||
fn specialize(&self, key: Self::Key) -> RenderPipelineDescriptor { | ||
let layout = vec![self.bind_group_layout.clone()]; | ||
// TODO: should this error? |
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.
I believe this should be an error yes. Not having a fragment state at this point is not expected and indicates something is very wrong.
Objective
Testing
bloom_3d
example worksFor reviewers:
Please bikeshed! This is where we start to make patterns for using
spec_v2
. Some initial thoughts:upsampling_pipeline
. What should we do there?Specializer<RenderPipeline>
for eachBloomXPipeline
, but this ended up making fields likebind_group_layout
inaccessible to resource prep systems. Instead, I've made separate specializer structs and put the caches as fields on their respectivePipeline
s. Does that seem like a good pattern?GetBaseDescriptor
entirely and the Resource derive forSpecializedCache
? It seems like it might be an anti-pattern.