Skip to content
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

[Merged by Bors] - Add push contant config to layout #7681

Closed
wants to merge 8 commits into from

Conversation

Neo-Zhixing
Copy link
Contributor

@Neo-Zhixing Neo-Zhixing commented Feb 15, 2023

Objective

Allow for creating pipelines that use push constants. To be able to use push constants. Fixes #4825

As of right now, trying to call RenderPass::set_push_constants will trigger the following error:

thread 'main' panicked at 'wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 59, Vulkan)>`
    In a set_push_constant command
    provided push constant is for stage(s) VERTEX | FRAGMENT | VERTEX_FRAGMENT, however the pipeline layout has no push constant range for the stage(s) VERTEX | FRAGMENT | VERTEX_FRAGMENT

Solution

Add a field push_constant_ranges to RenderPipelineDescriptor and ComputePipelineDescriptor.

This PR supersedes #4908 which now contains merge conflicts due to significant changes to bevy_render.

Meanwhile, this PR also made the layout field of RenderPipelineDescriptor and ComputePipelineDescriptor non-optional. If the user do not need to specify the bind group layouts, they can simply supply an empty vector here. No need for it to be optional.


Changelog

  • Add a field push_constant_ranges to RenderPipelineDescriptor and ComputePipelineDescriptor
  • Made the layout field of RenderPipelineDescriptor and ComputePipelineDescriptor non-optional.

Migration Guide

  • Add push_constant_ranges: Vec::new() to every RenderPipelineDescriptor and ComputePipelineDescriptor
  • Unwrap the optional values on the layout field of RenderPipelineDescriptor and ComputePipelineDescriptor. If the descriptor has no layout, supply an empty vector.

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible labels Feb 15, 2023
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM small nit, but not a blocker

Note for reviewers, most of the changes are just adding a new field to existing structs. The actual change is otherwise fairly small.

pub layout: Option<Vec<BindGroupLayout>>,
pub layout: Vec<BindGroupLayout>,
/// The push constant ranges for this pipeline.
pub push_constant_ranges: Vec<PushConstantRange>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's no Default impl, I would add a comment saying that the expected default value is an empty vec.

@IceSentry
Copy link
Contributor

Actually, wouldn't it be possible to keep the Option for layout and use an Option for push constants too? It would reduce the noise a bit and make it a bit more obvious what to use as a default value.

@Neo-Zhixing
Copy link
Contributor Author

Neo-Zhixing commented Feb 15, 2023

That's certainly doable, but I would argue that wrapping values in Some(...) is equally noisy. And Vec::new() is a perfectly reasonable value to be used as a default here. The default value for Vec is an empty vector, just like None is the default for Option.

The only field that doesn't already implement Default here is VertexState. Perhaps in the future we can implement From<VertexState> for RenderPipelineDescriptor so you would write

RenderPipelineDescriptor {
    layout: vec![layout1, layout2],
    ...VertexState {
        // Fields for vertex state
    }.into()
}

@IceSentry
Copy link
Contributor

To be clear, by noisy, I meant the current diff is noisy, not the actual usage. I'm fine with no Option personally especially considering that you need to specify a layout anyway most of the time.

Yeah, it would be nice to have a default Impl but for now I'm happy with this.

Copy link
Contributor

@kurtkuehnert kurtkuehnert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I like the move from Option to Vec. It avoids quite a bit of noise when creating pipelines.

We should probably add a default value to our pipeline descriptors.

Copy link
Contributor

@bonsairobo bonsairobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % the PreHashMap change.

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/render_resource/pipeline_cache.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/render_resource/pipeline_cache.rs Outdated Show resolved Hide resolved
@IceSentry
Copy link
Contributor

@Neo-Zhixing the CI failure is related to an unused import, I assume because of the PreHashMap change.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 16, 2023
@james7132 james7132 requested a review from superdump February 16, 2023 19:56
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

bors bot pushed a commit that referenced this pull request Feb 17, 2023
# Objective

Allow for creating pipelines that use push constants. To be able to use push constants. Fixes #4825

As of right now, trying to call `RenderPass::set_push_constants` will trigger the following error:

```
thread 'main' panicked at 'wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 59, Vulkan)>`
    In a set_push_constant command
    provided push constant is for stage(s) VERTEX | FRAGMENT | VERTEX_FRAGMENT, however the pipeline layout has no push constant range for the stage(s) VERTEX | FRAGMENT | VERTEX_FRAGMENT
```
## Solution

Add a field push_constant_ranges to` RenderPipelineDescriptor` and `ComputePipelineDescriptor`.

This PR supersedes #4908 which now contains merge conflicts due to significant changes to `bevy_render`.

Meanwhile, this PR also made the `layout` field of `RenderPipelineDescriptor` and `ComputePipelineDescriptor` non-optional. If the user do not need to specify the bind group layouts, they can simply supply an empty vector here. No need for it to be optional.

---

## Changelog
- Add a field push_constant_ranges to RenderPipelineDescriptor and ComputePipelineDescriptor
- Made the `layout` field of RenderPipelineDescriptor and ComputePipelineDescriptor non-optional.


## Migration Guide

- Add push_constant_ranges: Vec::new() to every `RenderPipelineDescriptor` and `ComputePipelineDescriptor`
- Unwrap the optional values on the `layout` field of `RenderPipelineDescriptor` and `ComputePipelineDescriptor`. If the descriptor has no layout, supply an empty vector.


Co-authored-by: Zhixing Zhang <[email protected]>
@bors bors bot changed the title Add push contant config to layout [Merged by Bors] - Add push contant config to layout Feb 17, 2023
@bors bors bot closed this Feb 17, 2023
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 18, 2023
# Objective

Allow for creating pipelines that use push constants. To be able to use push constants. Fixes bevyengine#4825

As of right now, trying to call `RenderPass::set_push_constants` will trigger the following error:

```
thread 'main' panicked at 'wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 59, Vulkan)>`
    In a set_push_constant command
    provided push constant is for stage(s) VERTEX | FRAGMENT | VERTEX_FRAGMENT, however the pipeline layout has no push constant range for the stage(s) VERTEX | FRAGMENT | VERTEX_FRAGMENT
```
## Solution

Add a field push_constant_ranges to` RenderPipelineDescriptor` and `ComputePipelineDescriptor`.

This PR supersedes bevyengine#4908 which now contains merge conflicts due to significant changes to `bevy_render`.

Meanwhile, this PR also made the `layout` field of `RenderPipelineDescriptor` and `ComputePipelineDescriptor` non-optional. If the user do not need to specify the bind group layouts, they can simply supply an empty vector here. No need for it to be optional.

---

## Changelog
- Add a field push_constant_ranges to RenderPipelineDescriptor and ComputePipelineDescriptor
- Made the `layout` field of RenderPipelineDescriptor and ComputePipelineDescriptor non-optional.


## Migration Guide

- Add push_constant_ranges: Vec::new() to every `RenderPipelineDescriptor` and `ComputePipelineDescriptor`
- Unwrap the optional values on the `layout` field of `RenderPipelineDescriptor` and `ComputePipelineDescriptor`. If the descriptor has no layout, supply an empty vector.


Co-authored-by: Zhixing Zhang <[email protected]>
@mockersf mockersf mentioned this pull request Feb 23, 2023
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-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Push Constants
6 participants