use naga instead of spirv-reflect for shader reflection #2130

jakobhellermann · 2021-05-07T22:10:10Z

Replaces spirv-reflect with naga only for shader reflection. The glsl to spirv compilation ist still done using beyv-glsl-to-spirv.

For some reason the spirv-reflect dependency can't be removed though, because I then get a ton of linker errors (fixed in cart/glsl-to-spirv#13):

Errors

 ld.lld: error: undefined symbol: operator new[](unsigned long)
          >>> referenced by /home/travis/build/KhronosGroup/glslang/glslang/CInterface/glslang_c_interface.cpp
          >>>               glslang_c_interface.cpp.o:(DirStackFileIncluder::newIncludeResult(std::string const&, std::basic_ifstream<char, std::char_traits<char> >&, int) const) in archive /home/jakob/.cargo/registry/src/jackfan.us.kg-1ecc6299db9ec823/bevy-glsl-to-spirv-0.2.1/build/linux/libglslang.glsltospirv.a
          >>> referenced by /home/travis/build/KhronosGroup/glslang/glslang/MachineIndependent/ShaderLang.cpp
          >>>               ShaderLang.cpp.o:((anonymous namespace)::CompileDeferred(TCompiler*, char const* const*, int, int const*, char const* const*, char const*, EShOptimizationLevel, TBuiltInResource const*, int, EProfile, bool, bool, EShMessages, glslang::TIntermediate&, glslang::TShader::Includer&, std::string, glslang::TEnvironment*)) in archive /home/jakob/.cargo/registry/src/jackfan.us.kg-1ecc6299db9ec823/bevy-glsl-to-spirv-0.2.1/build/linux/libglslang.glsltospirv.a
          >>> referenced by /home/travis/build/KhronosGroup/glslang/glslang/MachineIndependent/ShaderLang.cpp
          >>>               ShaderLang.cpp.o:((anonymous namespace)::CompileDeferred(TCompiler*, char const* const*, int, int const*, char const* const*, char const*, EShOptimizationLevel, TBuiltInResource const*, int, EProfile, bool, bool, EShMessages, glslang::TIntermediate&, glslang::TShader::Includer&, std::string, glslang::TEnvironment*)) in archive /home/jakob/.cargo/registry/src/jackfan.us.kg-1ecc6299db9ec823/bevy-glsl-to-spirv-0.2.1/build/linux/libglslang.glsltospirv.a
          >>> referenced 17 more times
...

This could allow shader reflection to work on wasm, however I'm not sure if that is useful, as spirv isn't used in wasm contexts IIRC.
It should also fix a bug with spirv-reflect @mtsr was experiencing which lead to CubeArray images being incorrectly reflected as Cube.

fixes #2122

crates/bevy_render/src/shader/shader.rs

crates/bevy_render/src/shader/shader_reflect.rs

bjorn3 · 2021-05-08T08:42:52Z

For some reason the spirv-reflect dependency can't be removed though, because I then get a ton of linker errors:

I presume that beyv-glsl-to-spirv is missing some linker args then that spirv-reflect does declare. Probably -lstdc++ on Linux or -lc++ on macOS.

cart · 2021-05-14T18:06:52Z

Sounds like we have this tested on linux already. If we can verify the other glsl-to-spirv platforms (windows, macos) I think we're good to go!

I'd like to verify android too, but its currently broken for other reasons, which will make that job hard.

cart · 2021-05-14T18:10:20Z

To clarify: "If we can verify the other glsl-to-spirv platforms using the just-merged glsl-to-spirv version then we're good to go".

We will need to sort out the following issue before we can publish a new glsl-to-spirv version: cart/glsl-to-spirv#12. I'll ping the crates.io team to request a size increase for the new crate.

(which I consider a prerequisite to merging this pr)

NiklasEi · 2021-05-14T20:43:01Z

Sounds like we have this tested on linux already. If we can verify the other glsl-to-spirv platforms (windows, macos) I think we're good to go!

What exactly do I need to do to test this?

mockersf · 2021-05-14T20:55:41Z

on macOS, after using this pr and changing bevy-glsl-to-spirv dependency to

bevy-glsl-to-spirv = { git = "https://github.com/cart/glsl-to-spirv", rev = "fe95427b7935d205885c7d2976c407f92e231205"}

    Updating git repository `https://github.com/cart/glsl-to-spirv`
    Updating git submodule `https://github.com/KhronosGroup/glslang.git`
error: failed to get `bevy-glsl-to-spirv` as a dependency of package `bevy_render v0.5.0 (bevy/crates/bevy_render)`

Caused by:
  failed to load source for dependency `bevy-glsl-to-spirv`

Caused by:
  Unable to update https://github.com/cart/glsl-to-spirv?rev=fe95427b7935d205885c7d2976c407f92e231205

Caused by:
  failed to update submodule `glsl-to-spirv-builder/glslang`

Caused by:
  object not found - no match for id (e22e71ed4b53713311dc24bcc54a44b95767cb92); class=Odb (9); code=NotFound (-3)

cart · 2021-05-14T20:55:51Z

Checkout this pr, checkout the glsl-to-spirv pr with the fix, edit bevy_render's cargo.toml to point to the local glsl-to-spirv checkout containing the PR changes, run a 3d example.

mtsr · 2021-05-15T09:21:22Z

I just:

Checked out out the above branch of glsl-to-spirv;
Changed bevy_render to use bevy-glsl-to-spirv = { path = "../../../glsl-to-spirv" }
Did a cargo update;
Tested the 2d, 3d and shader examples.

No problems.

jakobhellermann · 2021-05-16T18:29:14Z

@mockersf it seems like cargo can't handle bevy-glsl-to-spirv's submodule configuration. Can you try cloning the repo and adding the dependency using path = ...?

mockersf · 2021-05-16T20:41:25Z

works fine by pointing to a local clone 👍

Doesn't work yet as expected, because naga seems to ignore the `readonly` attribute: gfx-rs/naga#848

glsl-to-spirv seems to segfault sometimes when being called from multiple test threads

…andle for bindings

superdump · 2021-06-06T17:42:58Z

Just a note that this fixes #2122 for me on macOS. I have tested 3d_scene, shader_custom_material, array_texture, and shader_defs examples and all work fine. Can this be merged? What are we waiting on?

cart · 2021-06-07T20:35:24Z

At this point we're just blocked on a bevy-glsl-to-spirv release (see the comments about it above). I reached out to the crates.io team awhile back and they're cool with bumping the limit on the new crate, so I just need to publish an empty version of the crate, have them bump the limit on the crate, then publish the real version of the crate.

cart · 2021-06-07T21:11:55Z

Just published the empty crate and asked the crates.io team to bump the size limit.

mockersf · 2021-06-08T10:03:49Z

Unrelated to the switch to naga, but I just remembered that when updating bevy-glsl-to-spirv we should also change how it's selected (from #1819) to match the new platform supported

alice-i-cecile · 2021-09-22T22:27:03Z

Is this still relevant with the pipelined-rendering work?

cart · 2021-09-23T02:41:16Z

It might inform future reflection work, but the types have all changed a lot and so far we've been favoring a more explicit model. Auto-reflection on the level of the current bevy_render is too opinionated in a number of areas. I do think that we eventually might want something a bit more automatic, but I think we'll need to revisit the interfaces to ensure we aren't restricting things too much. I'll close this for now.

alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels May 7, 2021

jakobhellermann force-pushed the naga branch from e0bc05c to cf8d6f4 Compare May 7, 2021 22:31

NathanSWard reviewed May 7, 2021

View reviewed changes

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

NathanSWard reviewed May 7, 2021

View reviewed changes

crates/bevy_render/src/shader/shader_reflect.rs Show resolved Hide resolved

NathanSWard reviewed May 7, 2021

View reviewed changes

crates/bevy_render/src/shader/shader_reflect.rs Outdated Show resolved Hide resolved

jakobhellermann force-pushed the naga branch from 32d32cd to 9ee5d77 Compare May 8, 2021 07:59

jakobhellermann mentioned this pull request May 8, 2021

link c++ stdlib cart/glsl-to-spirv#13

Merged

mockersf mentioned this pull request May 8, 2021

Example shader/array_texture crash with wgpu error #2122

Closed

bevyengine deleted a comment May 16, 2021

jakobhellermann added 10 commits May 22, 2021 12:07

use naga instead of spirv-reflect for shader reflection

e5d4589

address review comments

86aef34

add test and pretty assertions

c55e9c3

better error message for unsupported sampler

3b6822d

sort bindings inside bind group

30455f9

sort vertex attributes

d40e50f

set readonly flag for buffer, add test case

b348a7b

Doesn't work yet as expected, because naga seems to ignore the `readonly` attribute: gfx-rs/naga#848

merge shader reflection tests into one

75732ed

glsl-to-spirv seems to segfault sometimes when being called from multiple test threads

only consider global variables of storage class uniform, storage or h…

18a4308

…andle for bindings

replace match with matches!

1e283d6

jakobhellermann force-pushed the naga branch from 438fc2e to 1e283d6 Compare May 22, 2021 10:07

don't panic on resource without name

315353e

NathanSWard mentioned this pull request Jun 26, 2021

Replace glsl-to-spirv and spirv-reflect with Naga #209

Closed

matthew-salerno mentioned this pull request Jun 28, 2021

Breakout example crashes with SIGSEGV #2384

Closed

cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021

mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021

cart closed this Sep 23, 2021

jakobhellermann deleted the naga branch September 23, 2021 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use naga instead of spirv-reflect for shader reflection #2130

use naga instead of spirv-reflect for shader reflection #2130

jakobhellermann commented May 7, 2021 •

edited

Loading

bjorn3 commented May 8, 2021

cart commented May 14, 2021

cart commented May 14, 2021

NiklasEi commented May 14, 2021

mockersf commented May 14, 2021 •

edited

Loading

cart commented May 14, 2021 •

edited

Loading

mtsr commented May 15, 2021 •

edited

Loading

jakobhellermann commented May 16, 2021

mockersf commented May 16, 2021

superdump commented Jun 6, 2021 •

edited

Loading

cart commented Jun 7, 2021

cart commented Jun 7, 2021

mockersf commented Jun 8, 2021 •

edited

Loading

alice-i-cecile commented Sep 22, 2021

cart commented Sep 23, 2021

use naga instead of spirv-reflect for shader reflection #2130

use naga instead of spirv-reflect for shader reflection #2130

Conversation

jakobhellermann commented May 7, 2021 • edited Loading

bjorn3 commented May 8, 2021

cart commented May 14, 2021

cart commented May 14, 2021

NiklasEi commented May 14, 2021

mockersf commented May 14, 2021 • edited Loading

cart commented May 14, 2021 • edited Loading

mtsr commented May 15, 2021 • edited Loading

jakobhellermann commented May 16, 2021

mockersf commented May 16, 2021

superdump commented Jun 6, 2021 • edited Loading

cart commented Jun 7, 2021

cart commented Jun 7, 2021

mockersf commented Jun 8, 2021 • edited Loading

alice-i-cecile commented Sep 22, 2021

cart commented Sep 23, 2021

jakobhellermann commented May 7, 2021 •

edited

Loading

mockersf commented May 14, 2021 •

edited

Loading

cart commented May 14, 2021 •

edited

Loading

mtsr commented May 15, 2021 •

edited

Loading

superdump commented Jun 6, 2021 •

edited

Loading

mockersf commented Jun 8, 2021 •

edited

Loading