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

Improve shader profile extension to be more compatible with bgfx #247

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

Andrewfeng123
Copy link

Currently, the profiles spirv and metal are abbreviated to spv and mtl, which means the compiled shaders are in the directories spv and mtl. This is different from the convention used in bgfx (see bgfx/examples/runtime), which leaves them as spirv and metal. Conforming to the bgfx has an added benefit: it is now possible to directly use the loadShader and loadProgram functions defined in bgfx_utils (in bgfx/examples/common/) just like the example programs. This was not possible before since the extensions spirv and metal were hard-coded into bgfx_utils.

Example:

In the CMakeLists.txt at the root of the project:

bgfx_compile_shaders(
	TYPE VERTEX
	SHADERS ${CMAKE_SOURCE_DIR}/shaders/vs.sc
	VARYING_DEF ${CMAKE_SOURCE_DIR}/shaders/varying.def.sc
    INCLUDE_DIRS "${CMAKE_SOURCE_DIR}/bgfx.cmake/bgfx/examples/" "${CMAKE_SOURCE_DIR}/bgfx.cmake/bgfx/src/"
	OUTPUT_DIR ${CMAKE_BINARY_DIR}/shaders
)

bgfx_compile_shaders(
	TYPE FRAGMENT
	SHADERS ${CMAKE_SOURCE_DIR}/shaders/fs.sc
	VARYING_DEF ${CMAKE_SOURCE_DIR}/shaders/varying.def.sc
    INCLUDE_DIRS "${CMAKE_SOURCE_DIR}/bgfx.cmake/bgfx/examples/" "${CMAKE_SOURCE_DIR}/bgfx.cmake/bgfx/src/"
	OUTPUT_DIR ${CMAKE_BINARY_DIR}/shaders
)

add_executable(main src/main.cpp 
    shaders/vs.sc 
    shaders/fs.sc
)

In main.cpp:

// ...
#include "bgfx_utils.h"
// ...
bgfx::ProgramHandle m_program = loadProgram("vs.sc", "fs.sc");

…s/runtime/shaders; fix incorrect parameter names; enforce consistent syntax when appending to CLI.
@bwrsandman
Copy link
Collaborator

Dx9 has been dropped. Can you take this opportunity to remove it?

@bwrsandman
Copy link
Collaborator

bwrsandman commented Sep 19, 2024

Unfortunately the changes break embedded shaders with BGFX_EMBEDDED_SHADER when using bgfx_compile_shaders with AS_HEADERS:

error: ‘vs_line_spv’ was not declared in this scope; did you mean ‘vs_line_spirv’?
[build]    97 |     BGFX_EMBEDDED_SHADER(vs_line), BGFX_EMBEDDED_SHADER(vs_line_instanced),        

image

The headers generate a variable with this naming:

static const uint8_t vs_line_spirv[924] =

but the BGFX_EMBEDDED_SHADER macro expects vs_line_spv

@Andrewfeng123
Copy link
Author

@bwrsandman Okay I pushed a new commit, please let me know if this works. This is a bit of a hack, but I don't see another way of doing so without changing bgfx.

Copy link
Collaborator

@bwrsandman bwrsandman left a comment

Choose a reason for hiding this comment

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

Tested on linux (only spirv). Aside from having to change my own code to look in spirv/ (it was spv/ before), it works as before.

I haven't tested other platforms such as metal, as I see it's not in _bgfx_get_profile_path_ext PROFILE.

@bwrsandman bwrsandman merged commit 1139067 into bkaradzic:master Nov 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants