Improvments on the shader compiler utility

[object71]

The custom command didn't recognize bgfx::shaderc and the generator expression seems bettter. I also added an include option since it might be important to be able to define those.

[object71]

Hey @bwrsandman These are some small improvements on the shader compilation helper. Mainly it wasn't working for me because it couldn't resolve bgfx::shaderc so I replaced it with $<TARGET_FILE:shaderc>. I also got a few more ideas though - since the internal implementation adds custom commands that are not linked to any targets it would be benefitial to set the output files into a variable so that some target can depend on those files. Otherwise some platforms don't execute the custom command to generate the shaders. And I found that I was missing the feature for setting an include directory.

Are these changes okay? None of them are breaking changes.

[bwrsandman]

I'm curious to know more about your setup. If we can fix your target and avoid using target_file, I would prefer it.

One of the reasons I prefer keeping the target is that it tells cmake to build the target implicitly.

[object71]

You can check out https://github.com/Paper-Cranes-Ltd/big2-stack but I just got an error that it doesn't recognize bgfx::shaderc

I work on windows. Target file generator also tells it to build the target.

[bwrsandman]

Looks like you're using fetchcontent. I'll have to test this. My first guess is maybe the namespace doesn't exist. The target should be put in the right namespaces if the library is installed and you use the generated targets there.

[object71]

Is the $ generator going to break installed targets?

[object71]

@bwrsandman did you manage to test it out?

[bwrsandman]

Sorry, I completely forgot.

I've done some testing now and here's a better solution that works with fetchcontent and installed targets. In bgfxToolUtils.cmake, for each executable (bin2c, texturec, geometryc and shaderc) add this

if (NOT TARGET shaderc AND TARGET bgfx::shaderc)
    add_executable(bgfx::shaderc ALIAS shaderc)
endif()

I'm not sure if it's better to put in the function namespace or in the top scope.

[mode777]

I have the same problem where bgfx::shaderc was not found when including bgfx.cmake as a subdirectory in cmake. I added this to my top-level CMakeLists.txt so now, at least it will run the command

if (NOT TARGET bgfx::shaderc AND TARGET shaderc)
    add_executable(bgfx::shaderc ALIAS shaderc)
endif()

[Carbone13]

On Ninja + MSVC, it doesn't work, $ solution works, but the one proposed by @bwrsandman don't My CMakeLists look like :

project(repro)
add_subdirectory(bgfx.cmake)

bgfx_compile_shader_to_header(
  TYPE VERTEX
  SHADERS vs.sc
  VARYING_DEF varying.def.sc
  OUTPUT_DIR ${CMAKE_BINARY_DIR}/include/generated/shaders
)

add_executable(repro main.cpp vs.sc)

[object71]

It doesn't work for me either even though I added it. Target file is the only solution that works correctly since it also implicitly handles the depenedency correctly.

@bwrsandman Since you metioned it why don't we want to build the target implicitly - we depend on it to be built so that the shaders get compiled.

[bwrsandman]

The problem with $<TARGET_FILE:shaderc> is that is breaks installed targets:

[cmake] CMake Error at cmake-build-presets/ninja-multi-vcpkg/vcpkg_installed/x64-linux/share/bgfx/bgfxToolUtils.cmake:606 (add_custom_command):
[cmake]   Error evaluating generator expression:
[cmake] 
[cmake]     $<TARGET_FILE:shaderc>
[cmake] 
[cmake]   No target "shaderc"
[cmake] Call Stack (most recent call first):
[cmake]   src/CMakeLists.txt:46 (bgfx_compile_shader_to_header)

Fetchcontent downloads and adds the project to your cmake project to build. However, if you run cmake --install on bgfx and use find_package(bgfx) in your project, there is no target and shaderc is under the bgfx:: namespace.

Using $<TARGET_FILE:bgfx::shaderc> works on installed bgfx but at that point you run into the same problems in your fetchcontent project and should just be using bgfx::shaders.

[bwrsandman]

@Carbone13 @object71 Are you both using msvc and ninja?

Can you share the error message? When I tried the code it worked so it might just need some tuning.

[object71]

With your last suggestions it works now for me. I squashed the commits to a single one.