Spirv shader bypass by molikto · Pull Request #3996 · bevyengine/bevy

molikto · 2022-02-20T12:36:30Z

Objective

Expose wgpu spirv-shader-passthrough as a bevy feature.

Solution

add a feature to bevy_render and bevy itself.

alternative: #3997
More alternative:

having a different file extension passthrough.spv
some API to runtime specify if shader is loaded passthrough or not

(But I didn't implement these, because the passthrough is hopefully only temporary, and I don't see why people want some passthrough but some doesn't.)

# Conflicts: # crates/bevy_render/src/options.rs # Conflicts: # crates/bevy_render/src/options.rs

bjorn3 · 2022-02-20T12:43:08Z

Cargo.toml

  "bevy_internal/bevy_ui",
 ]

+spirv_shader_passthrough = ["bevy_internal/spirv_shader_passthrough" ]


Please add this to the feature docs and note that it is incompatible with wasm.

And any other system which doesn't support spirv, i.e dx12 and metal, afaik.

I think it is only compatible with Vulkan. Maybe easier to say that.

@molikto please add documentation of the feature to docs/cargo_features.md and make note that it is only supported for Vulkan, if that is indeed correct.

@Shfty documentation is still missing here.

crates/bevy_render/src/settings.rs

molikto · 2022-03-02T06:14:49Z

I am more in favor of #3997 so until we decided which PR to take I am not addressing comments to this PR :-)

superdump

I made a couple of suggestions that allow enabling the feature but not failing if the backend does not support it, rather falling back to non-passthrough behaviour. Code that wants/needs passthrough can then used RenderDevice::features() to check for support at runtime and load the appropriate shader.

crates/bevy_render/src/renderer/render_device.rs

crates/bevy_render/src/settings.rs

Co-authored-by: Robert Swain <robert.swain@gmail.com>

superdump

This looks good to me but it needs updating and the build is failing. I prefer this PR to the other one as this one allows people to enable the feature if they need it, and if they need it, it is only available based on whether it is actually supported or not as indicated by the render device wgpu::Features.

superdump

LGTM just missing some documentation.

superdump · 2022-03-08T06:25:04Z

Cargo.toml

  "bevy_internal/bevy_ui",
 ]

+spirv_shader_passthrough = ["bevy_internal/spirv_shader_passthrough" ]


@molikto please add documentation of the feature to docs/cargo_features.md and make note that it is only supported for Vulkan, if that is indeed correct.

cart · 2022-03-09T20:54:32Z

Is there context on why both #3996 and #3997 were closed? This seems like a desirable feature.

superdump · 2022-03-10T00:47:06Z

I’m also confused by this being closed. I was wondering if I would see another PR open in its place…

molikto · 2022-03-10T02:49:05Z

personally I don't like this approach anymore, it adds a feature flag to bevy and I think it's just confusing for people that don't care about this feature at all. I think maybe a proper solution is when we have asset .meta files.

ProfLander · 2023-03-19T09:00:24Z

I've come across a need for this while developing Bevy Rust-GPU; naga has some bugs and unimplemented SPIR-V features that prevent the use of certain valid rust-gpu code (most notably, code emitted by the new SPIR-T optimizations rust-gpu enabled by default in 0.6).

I've verified that the affected shaders function as intended after manually patching this PR into my own bevy branch.

To wit, I'm interested in adopting this PR @cart @superdump; it still compiles with up-to-date bevy, so should be mergeable in its current state if there are no additional changes that might be desirable in hindsight.

superdump

Still missing documentation of the feature. @Shfty if you want to make a new PR to address that, please credit molikto for their work. Ideally just make a branch from theirs and make your changes on top. :)

superdump · 2023-03-19T09:17:35Z

Cargo.toml

  "bevy_internal/bevy_ui",
 ]

+spirv_shader_passthrough = ["bevy_internal/spirv_shader_passthrough" ]


@Shfty documentation is still missing here.

ProfLander · 2023-03-19T20:07:38Z

Great 🙂 will do.

Unfortunately it looks like molikto deleted their branch, so I'll have to reimplement and add credit in the commit messages / upcoming PR. Will do some research on the current state of SPIR-V support for various APIs, sort the docs, and open a new pull.

@Shfty

**Note:** This is an adoption of @Shfty 's adoption (#8131) of #3996! All I've done is updated the branch and run the docs CI. > **Note:** This is an adoption of #3996, originally authored by @molikto > > # Objective > Allow use of `wgpu::Features::SPIRV_SHADER_PASSTHROUGH` and the corresponding `wgpu::Device::create_shader_module_spirv` for SPIR-V shader assets. > > This enables use-cases where naga is not sufficient to load a given (valid) SPIR-V module, i.e. cases where naga lacks support for a given SPIR-V feature employed by a third-party codegen backend like `rust-gpu`. > > ## Solution > * Reimplemented the changes from [Spirv shader bypass #3996](#3996), on account of the original branch having been deleted. > * Documented the new `spirv_shader_passthrough` feature flag with the appropriate platform support context from [wgpu's documentation](https://docs.rs/wgpu/latest/wgpu/struct.Features.html#associatedconstant.SPIRV_SHADER_PASSTHROUGH). > > ## Changelog > * Adds a `spirv_shader_passthrough` feature flag to the following crates: > > * `bevy` > * `bevy_internal` > * `bevy_render` > * Extends `RenderDevice::create_shader_module` with a conditional call to `wgpu::Device::create_shader_module_spirv` if `spirv_shader_passthrough` is enabled and `wgpu::Features::SPIRV_SHADER_PASSTHROUGH` is present for the current platform. > * Documents the relevant `wgpu` platform support in `docs/cargo_features.md` --------- Co-authored-by: Josh Palmer <1253239+Shfty@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

enable shader bypass

5c8762e

# Conflicts: # crates/bevy_render/src/options.rs # Conflicts: # crates/bevy_render/src/options.rs

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 20, 2022

bjorn3 reviewed Feb 20, 2022

View reviewed changes

molikto mentioned this pull request Feb 20, 2022

Shader passthrough alternative #3997

Closed

james7132 added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Feb 21, 2022

superdump reviewed Mar 2, 2022

View reviewed changes

crates/bevy_render/src/settings.rs Outdated Show resolved Hide resolved

superdump requested changes Mar 2, 2022

View reviewed changes

crates/bevy_render/src/renderer/render_device.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/settings.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/settings.rs Outdated Show resolved Hide resolved

molikto and others added 3 commits March 2, 2022 14:20

Update crates/bevy_render/src/renderer/render_device.rs

33de9f4

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_render/src/settings.rs

6893f31

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_render/src/settings.rs

fa43db9

Co-authored-by: Robert Swain <robert.swain@gmail.com>

molikto requested a review from superdump March 2, 2022 06:21

superdump requested changes Mar 6, 2022

View reviewed changes

molikto added 3 commits March 8, 2022 12:16

fix: build

181b997

format

5c0b934

format

67c98e0

molikto requested a review from superdump March 8, 2022 04:57

superdump requested changes Mar 8, 2022

View reviewed changes

molikto closed this Mar 9, 2022

molikto deleted the spirv-shader-passthrough branch May 17, 2022 04:46

superdump reviewed Mar 19, 2023

View reviewed changes

This was referenced Mar 19, 2023

Naga fails to translate certain correct SPIR-V Bevy-Rust-GPU/bevy-rust-gpu#17

Open

Implement SPIR-V passthrough #8131

Closed

bas-ie mentioned this pull request Sep 21, 2024

Spirv passthrough main (adopted, part deux) #15352

Merged

Uh oh!

Conversation

molikto commented Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

bjorn3 Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

DJMcNab Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

superdump Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

superdump Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

superdump Mar 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

molikto commented Mar 2, 2022

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

superdump Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

cart commented Mar 9, 2022

Uh oh!

superdump commented Mar 10, 2022

Uh oh!

molikto commented Mar 10, 2022

Uh oh!

ProfLander commented Mar 19, 2023

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

superdump Mar 19, 2023

Choose a reason for hiding this comment

Uh oh!

ProfLander commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

molikto commented Feb 20, 2022 •

edited

Loading

ProfLander commented Mar 19, 2023 •

edited

Loading