Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Replace the has been deprecated glsl-to-spirv with shaderc-rs crate #44

Closed
wants to merge 1 commit into from
Closed

Conversation

jinleili
Copy link

@jinleili jinleili commented Jul 25, 2019

Since glsl-to-spirv has been deprecated, wgpu-rs/issues/36 is related to glsl spec, and glslc is still iterating fast, use shaderc-rs to follow up on future updates from glslc .

Extra information

compile shaderc-rs need to install Pathon 3, and maybe need to remove local computer’s CMakeCache.txt file to let cmake use latest Pathon version.

If the following error message appears when compiling on macOS:

CMake Error at .../CMakeTestCXXCompiler.cmake:53 (message):
  The C++ compiler
  ...
  is not able to compile a simple test program.

Make sure have XCode installed as well as command line tools: xcode-select --install

@rukai
Copy link
Contributor

rukai commented Jul 25, 2019

I support this change, but we need to explain how to build shaderc in the examples readme. You can link to or copy the relevant text from the shaderc readme https://github.com/google/shaderc-rs

@rukai
Copy link
Contributor

rukai commented Jul 25, 2019

oh... and I think we should consider using https://github.com/Ralith/vk-shader-macros instead of shaderc-rs.
Its just a nice wrapper over the top of shaderc-rs, but it helps ensure that we keep our spirv as [u32] instead of [u8] which is UB due to bad alignment.

Currently gfx hal 0.2 takes spirv as &[u8] which is wrong and will be fixed in 0.3
We will need to workaround the gfx bad api, by doing something like this: https://github.com/rukai/PF_Sandbox/blob/84eec5f6855790289991f7eb3d75a2e46653a9f1/pf_sandbox/src/wgpu/mod.rs#L192

@jinleili
Copy link
Author

@rukai When build wgpu-rs examples, shaderc will be compiled automaticly.

@rukai
Copy link
Contributor

rukai commented Jul 25, 2019

shaderc-rs handles compilation but your environment has to be specifically setup for it to work, depending on your OS you may need to setup a C++ compiler + ninja or maybe just install the shaderc library via your package manager, if such a package is available.

@jinleili
Copy link
Author

jinleili commented Jul 25, 2019

shaderc compile result CompilationArtifact can output [u32] spirv stream :).
If use vk-shader-macros, perhaps it's a bit more convenient to use, but since it's just a wrapped shaderc, can't avoid the shaderc compilation process.

@kvark
Copy link
Member

kvark commented Jul 25, 2019

shaderc-rs handles compilation but your environment has to be specifically setup for it to work, depending on your OS you may need to setup a C++ compiler + ninja or maybe just install the shaderc library via your package manager, if such a package is available.

That's a red flag for me. We don't want to require any more steps than there is. We'd even like to reduce the preparation by, say, eliminating the "--features " parameter.

Could you explain to me one thing. So glsl-to-spirv is deprecated, ok. Is it not doing it's job? In my book, as long as it's working for our needs, we should keep it (given that it doesn't require that much of an environment setup). Later, we'd replace it by something more Rust-ic.

@rukai
Copy link
Contributor

rukai commented Jul 25, 2019

Yeah it is bad. I see shaderc-rs as the lesser evil though. I think in this case grenlight was prompted to make this pr because glsl-to-spirv was miscompiling his spirv?

This has been discussed many times before:
https://github.com/Lokathor/learn-gfx-hal/issues/4
gfx-rs/gfx#2546
gfx-rs/gfx#2731

The other option is resuming maintence of glsl-to-spirv under the gfx org. I can help set that up, but I am not interested in long-term maintenence, I have enough projects already. Additionally only trusted people could supply prs to update the windows exe.

@jinleili
Copy link
Author

We don't want to require any more steps than there is. We'd even like to reduce the preparation by, say, eliminating the "--features " parameter.

I agree with your opinion. Although wgpu users may already have C++ and Pathon environments, but it is possible to generate additional configurations, that is not friendly enough to some users

@aloucks
Copy link
Contributor

aloucks commented Jul 30, 2019

Additionally only trusted people could supply prs to update the windows exe.

You could remove the binary from glsl-to-spirv and require the Vulkan SDK to be installed with glslc or glslangValidator on the path.

@seivan
Copy link
Collaborator

seivan commented Aug 11, 2019

Not sure if it matters but as of today there is a path of making glslang, which glsl-to-spirv uses to work on mobile devices, like iOS. I know that MoltenVK has managed to do that. Abandoning that means effort has to be put in for making sure that shaderc and it’s rust wrapper does so as well.

I’m not against the move but there’s an uncertainty that shaderc can compiled for and run on iOS while glslang has been proven to do - at least the MoltenVK guys managed to do so with their build scripts at least.

Obviously I’m ignoring any issues with the macro itself and only discussing the internal libs here so if there’s an issue with the macro that’s a different topic.

My plan is eventually make sure the examples run on iOS as well because I see them as testcases for wgpu working as expected :-).

Edit: Both libs aren’t written in Rust which means that arguments against Shaderc and setting up build environments are the same for glslang.

Edit2: Gah writing on mobile, shaderc doesn’t need to set up a build environment(?) the Rust wrapper seems to manage to set things up, not sure if it’s building from scratch or I happen to have every dependency or if it’s using precompiled libs.

@kvark
Copy link
Member

kvark commented Aug 12, 2019

I feel like we could skip the whole glsl-to-spirv -> shaderc transition and instead focus on generating SPIRV in Rust based off GLSL, which becomes more and more of an option.

Is there an urgency here? I mean, wgpu-rs doesn't even have GLSL anywhere in its API. So as long as our examples (that use glsl-to-spirv) still work, we are good.

@jinleili
Copy link
Author

glsl-to-spirv or shaderc they can all do their work right now, and compared to other enhancements (the use of external texture data :) ), this may not be so urgent, after all, on mobile devices, users can precompile shaders into SPIRV files for use.

@kvark
Copy link
Member

kvark commented Jun 8, 2020

Closing this as stale. The mid-term plan for the shaders here is using WGSL language and Naga for translating them into SPIR-V.

@kvark kvark closed this Jun 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants