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

Push Constants #4825

Closed
MDeiml opened this issue May 22, 2022 · 7 comments
Closed

Push Constants #4825

MDeiml opened this issue May 22, 2022 · 7 comments
Labels
A-Rendering Drawing game state to the screen C-Enhancement A new feature

Comments

@MDeiml
Copy link
Contributor

MDeiml commented May 22, 2022

What problem does this solve or what need does it fill?

It would be nice to support using push constants. Even though the initial wgsl spec will not have push constants (gpuweb/gpuweb#612) they are supported in naga (https://github.com/gfx-rs/naga/blob/master/tests/in/push-constants.wgsl) as well as wgpu.

But it seems that there was a decision not to include them. Specifically you cannot create pipelines with layouts that contain push constants. This is because the concept of a RenderPipelineLayout in wgpu was not translated to the bevy API and instead became just &[BindGroupLayout]. To create a RenderPipelineLayout in wgpu you specify bind group layouts and push constant ranges, so this seems like a weird design to me.

I'm talking about this code, which always sets the push constant ranges to be empty:

render_device.create_pipeline_layout(&PipelineLayoutDescriptor {
bind_group_layouts: &bind_group_layouts,
..default()
})

What solution would you like?

Change RenderPipelineDescriptor or it's layout field so it's possible to specify push constant ranges.

What alternative(s) have you considered?

Create a pipeline without using pipeline cache or don't use push constants.

Additional Context

In order to use push constants in shaders we would also need #4824


I'd be happy to implement this, but it would require minor breaking changes to RenderPipelineDescriptor and it seems to me that maybe there already was a decision to not support them, so I wanted to ask first.

@MDeiml MDeiml added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels May 22, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels May 22, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 22, 2022

Previously discussed in #1733 and #1369.

For internal adoption, my only major concern is "web compatibility". I don't want to maintain two render paths yet (one for web and one for "native"). Imo we should have a real "motivating" use case before merging. If it can't be internal (for now), then I think it needs to be "allowing users to adopt push constants". And if thats true, we should probably have an example.

The rework directly exposes the ability to enable push constants via gpu (because it uses wgpu directly), so I do think we can close this out.

I don't have the expertise to evaluate the matter further, but I figured I can surface the most relevant bits of the conversation.

@MDeiml
Copy link
Contributor Author

MDeiml commented May 22, 2022

Thanks for the context. Assuming the mentioned rework is already done, this is still relevant. Push constants are still not exposed in RenderPipelineDescriptor.

Even if push constants will not be used with the "built-in" shaders, it would still make sense to make them available to people who want to write their own shaders / render pipelines.

EDIT: I was wrong, TrackedRenderPass already supports push constants. Does that mean they are supposed to be supported?

@cart
Copy link
Member

cart commented May 24, 2022

I would like to support push constants. Things have changed a lot since #1733 and #1369. I think the only missing piece at this point is updating the RenderPipelineDescriptor to include the push constant layout and adapting LayoutCache to accept push constant layouts (and take them into account for layout identity ... they must be a part of the key used to identify the layout).

@james7132
Copy link
Member

@cart would you be opposed to having platform specific optimizations using push constants? The push constant size limitations are basically just big enough to fit two Affine3s and a few extra bits, which coincidentally is exactly the right size for a MeshUniform. It'd preclude users from using them for mesh rendering, but it'd likely be a sizable speed up opportunity on platforms that support it.

@cart
Copy link
Member

cart commented May 25, 2022

If this can be done without adding extra complexity to user-facing shader apis, I think I'm open to it. Hard to say before looking at the impl + perf tradeoffs first, but its an area I think we should explore.

@MDeiml
Copy link
Contributor Author

MDeiml commented Jun 3, 2022

I would like to support push constants. Things have changed a lot since #1733 and #1369. I think the only missing piece at this point is updating the RenderPipelineDescriptor to include the push constant layout and adapting LayoutCache to accept push constant layouts (and take them into account for layout identity ... they must be a part of the key used to identify the layout).

That and #4824. I got push constants working in my fork. It's not a lot of changes, but RenderPipelineDescriptor does have to change I think.

@alice-i-cecile
Copy link
Member

@MDeiml if you're comfortable, a link to that branch or a PR would be great. It doesn't need to be perfect to start, but having something tangible will help focus the discussion.

@bors bors bot closed this as completed in 16feb9a Feb 17, 2023
myreprise1 pushed a commit to myreprise1/bevy that referenced this issue 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 <me@neoto.xin>
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-Enhancement A new feature
Projects
None yet
4 participants