Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Jul 5, 2025

Objective

  • minimally migrate bloom to spec_v2, without as many opinionated changes as the previous closed pr

Testing

  • bloom_3d example works

For reviewers:

Please bikeshed! This is where we start to make patterns for using spec_v2. Some initial thoughts:

  • I'm not sure if I'm happy with the error handling or placeholders in upsampling_pipeline. What should we do there?
    • @atlv24 and @tychedelia have mentioned removing custom base descriptors, and providing a single default descriptor. That would solve the issue, but I'm concerned about the consequences for composing specializers. It's still worth considering though!
  • In an earlier version of this PR I implemented Specializer<RenderPipeline> for each BloomXPipeline, but this ended up making fields like bind_group_layout inaccessible to resource prep systems. Instead, I've made separate specializer structs and put the caches as fields on their respective Pipelines. Does that seem like a good pattern?
    • should we remove GetBaseDescriptor entirely and the Resource derive for SpecializedCache? It seems like it might be an anti-pattern.

@ecoskey ecoskey mentioned this pull request Jul 5, 2025
43 tasks
shader: fragment_shader.clone(),
entry_point: Some("upsample".into()),
targets: vec![Some(ColorTargetState {
format: TextureFormat::Rgba8Unorm, // placeholder
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

ecoskey added 2 commits July 5, 2025 14:17
similar utils for layout etc should be added later as needed
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 6, 2025
@@ -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) {
Copy link
Contributor

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?
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants