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

Implement SPIR-V passthrough #8131

Closed
wants to merge 6 commits into from

Conversation

Shfty
Copy link
Contributor

@Shfty Shfty commented Mar 19, 2023

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, 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.

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

@Shfty Shfty changed the title Implement SPIR-V passthrough (Adoption of #3996) Implement SPIR-V passthrough Mar 19, 2023
@Shfty
Copy link
Contributor Author

Shfty commented Mar 19, 2023

Also, I've removed spirv_shader_passthrough from the default feature in the bevy crate:

Passthrough is inherently unsafe in the Rust sense, so making it an explicit choice seems like the wiser call.

Plus, enabling it by default implicitly creates cases whereby supported platforms use passthrough, and unsupported ones use naga, as per the wgpu::Features check added during the original pull.

This is desirable behaviour from a compatibility standpoint, but has the potential to catch users off-guard if their shader behaves differently across passthrough / non-passthrough (i.e. by crashing their app with a panic when naga can't translate a SPIR-V module).

So, it seems better as an opt-in - that positions the existing naga setup as the compatibility baseline, with the option to drop down to unsafe passthrough land made available for projects that require it.

@Shfty Shfty force-pushed the spirv-passthrough-main branch 2 times, most recently from 86b0686 to 7b60649 Compare March 20, 2023 03:00
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 20, 2023
@Shfty Shfty force-pushed the spirv-passthrough-main branch from 7b60649 to b70c34a Compare March 21, 2023 21:11
@Shfty Shfty force-pushed the spirv-passthrough-main branch from 24b138a to 7bdd1fd Compare March 24, 2023 21:15
@tychedelia tychedelia added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Aug 16, 2024
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

I can't test because I'm on macOS, but this makes sense to me and seems very useful for certain users. It's gated behind a feature so should be safe either way.

Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Yep, looks reasonable.

@NthTensor NthTensor added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 24, 2024
Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@alice-i-cecile
Copy link
Member

Once this is passing CI I'll merge it in. Sorry for the very long wait!

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Sep 2, 2024
@bas-ie
Copy link
Contributor

bas-ie commented Sep 21, 2024

Couldn't resist adopting an adoption! #15352

@alice-i-cecile
Copy link
Member

Closing as adopted.

auto-merge was automatically disabled September 22, 2024 14:46

Pull request was closed

github-merge-queue bot pushed a commit that referenced this pull request Sep 22, 2024
**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>
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-Usability A targeted quality-of-life change that makes Bevy easier to use S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants