vsg-dev / vsgFramework

Framework for building VulkanScenGraph related projects together
MIT License
10 stars 7 forks source link

Add building of SPIRV-headers and -tools to have GLSL shader support in libvsg #7

Closed rhabacker closed 2 years ago

rhabacker commented 2 years ago

With this merge request applied, you can see the issue mentioned at https://github.com/vsg-dev/VulkanSceneGraph/issues/512.

robertosfield commented 2 years ago

I used to think SPIR-Tools was part of glslang, at some point this must have changed, or perhaps it was just optional dependency. Looking at glslang CMakeLists.txt files there are references to SPIRV-Tools so perhaps SPIRV-Headers/Tools should be built first.

Ideally there would a cleaner way to build GLSL to SPIRV rather than jumping through the awkwardness of glslang, I'm open to suggestions.

robertosfield commented 2 years ago

I merged this PR as a libvsg-issue-512 branch, and on build can now recreate the VSG compile error. I presume we'll need to add some extra compile checks to workaround it.

[ 45%] Performing build step for 'VulkanSceneGraph' Consolidate compiler generated dependencies of target vsg [ 0%] Building CXX object src/vsg/CMakeFiles/vsg.dir/utils/ShaderCompiler.cpp.o In file included from /home/robert/Dev/vsgFramework/src/VulkanSceneGraph/src/vsg/utils/ShaderCompiler.cpp:25: /home/robert/Dev/vsgFramework/src/VulkanSceneGraph/src/vsg/utils/ResourceLimits.cpp:155:10: error: cannot convert ‘’ to ‘int’ in initialization 155 | }}; | ^

rhabacker commented 2 years ago

I used to think SPIR-Tools was part of glslang, at some point this must have changed, or perhaps it was just optional dependency. Looking at glslang CMakeLists.txt files there are references to SPIRV-Tools so perhaps SPIRV-Headers/Tools should be built first

Yes, this is achieved by merging this pr.

Ideally there would a cleaner way to build GLSL to SPIRV rather than jumping through the awkwardness of glslang, I'm open to suggestions.

With https://github.com/KhronosGroup/SPIRV-Tools/commit/9c0ae6bb8ea49ef0c154e65278b973ef226adb9b SPIRV-Tools got cmake find_package, which supports using

find_package(SPIRV-Tools)
find_package(SPIRV-Tools-opt)

directly in the top level CMakeLists.txt of VulkanSceneGraph instead of using vsg_glslang.cmake for that packages. There may be still some add_library(.. ALIAS) settings required to match recently used targets.

Using find_package(glslang) to completely delete vsg_glslang.cmake might also be possible, but probably requires further customization.

robertosfield commented 2 years ago

Urghgh. What a splurg of crap glslang history dumps on us. So many hacks in the VSG ShaderCompiler code already. I've just been reviewing the changes to TBuiltInResouce and see additions we'll need to add.

Perhaps we just need to drop support for older glslang version. This has it's own consequences though. This is a bigger topic than is appropriate for this discussion, vsg-users would be a better place. Could start up such a thread, and provide a link to these discussions. I would normally do this myself but I'm really struggling to use the keyboard.

Big questions are what minimum version of glslang would give us a better base to work from - would this allow us to remove the vsg_glslang.cmake? What version of glslang do users already have on their systems? Installed from repos?

Thanks.

rhabacker commented 2 years ago

Perhaps we just need to drop support for older glslang version.

There is a less invasive approach by adding find_package support and using vsg_glslang.cmake as a fallback for older packages.

robertosfield commented 2 years ago

On Tue, 6 Sept 2022 at 12:25, Ralf Habacker @.***> wrote:

Perhaps we just need to drop support for older glslang version.

There is a less invasive approach by adding find_package support and using vsg_glslang.cmake as a fallback for older packages.

Adding more build combinations in the build and source files just increases the maintenance burden, we've already got different hacks for different folks in the codebase because of glslang, we can't keep adding more.

We need to go in the opposite direction and simplify.

Message ID: @.***>

rhabacker commented 2 years ago

We need to go in the opposite direction and simplify.

I understand - to be ready for 1.0?

Here is a branch with the required changes for VulkanSceneGraph that can be used for testing or should I open a pr for VulkanSceneGraph directly now?

robertosfield commented 2 years ago

On Tue, 6 Sept 2022 at 13:14, Ralf Habacker @.***> wrote:

We need to go in the opposite direction and simplify.

I understand - to be ready for 1.0?

Here is a branch with the required changes https://github.com/rhabacker/VulkanSceneGraph/tree/find-glslang-support for VulkanSceneGraph that can be used for testing or should I open a pr for VulkanSceneGraph directly now?

Definitely something to settle for 1.0.

We'll need to establish min versions for glslang, SPIRV-Tools and SPIRV-Tools-opt. The later two would also need add to the vsg configs find_dependency list.

The other big challenge will be figuring out how best to the changes in the DefaultTBuiltInResource. There is some clunky cmake build tests to establish what features are available, this will either need to be extended or if possible use a glslang header version to help with, or find a glslang function to get this setting for us.

Message ID: @.***>

rhabacker commented 2 years ago

Big questions are what minimum version of glslang would give us a better base to work from

I can give answers from my current experience with openSUSE:

Vanilla glslang 11.11.0 will not be usable in shared build mode (see https://build.opensuse.org/request/show/1001407)

For openSUSE, a corresponding fix has recently been made to the glslang package (see https://build.opensuse.org/package/revisions/X11:Wayland/glslang) but it cannot be applied to the glslang project.

A corresponding support requires changes to the cmake build system of glslang (all cmake targets installed in shared build mode, which use static libraries, would have to be an alias to the cmake target glslang, since this already contains the exported symbols of the static libraries).

Since a corresponding patch for glslang is still missing, the only thing left to do at the moment is to add SPIRV-tools* to vsgFramework.

robertosfield commented 2 years ago

Hi Ralf,

I don't know what to do with SUSE package link.

I am spread thin enough that I need to concentrate on the core VSG and try to steer a path that works across platforms but need members of the community on those platforms to provide a distillation of what is necessary to build OK.

It would be worth trying out your changes to the find_package glsl/spirv-toolsspirv-tools-opt as a PR against vsg-dev/VulkanSceneGraph so I can merge it as a branch so that others can test it out. I have a broken arm and am struggling to work so I need others to take initiative.

Cheers, Robert.

Message ID: @.***>

rhabacker commented 2 years ago

I don't know what to do with SUSE package link.

Simply clicking on the the "Files changed" link on revision 96 and 97 shows what is required to fix the mentioned issue in a distribution package - I added that as reference.

I have a broken arm and can only work with difficulty,

I'm sorry for you

so I need the initiative of others.

no problem - please merge this pr first, which is required for a reproducable build environment

It would be worth trying out your changes to the find_package glsl/spirv-toolsspirv-tools-opt as a PR against vsg-dev/VulkanSceneGraph so I can merge it as a branch so that others can test it out.

I will create the one for VulkanScenegraph afterwards

robertosfield commented 2 years ago

FYI, I've just merged by vsgFramework will soon switch to the BuildFlexibility branch that uses FetchContent, so that branch will need to changed as well.