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

bgfx: fix shader compilation #5106

Merged
merged 1 commit into from
Sep 6, 2024
Merged

bgfx: fix shader compilation #5106

merged 1 commit into from
Sep 6, 2024

Conversation

bkmeneguello
Copy link
Contributor

Fixes #5105

  • Adds ".sc" as a supported extension for bgfx shaders;
  • Adds support for vs_, fs, and cs_ prefixes for vertex, fragment, and compute shaders;
  • Adds support for bin2c confguration (header generation)

@@ -47,7 +47,7 @@ package("bgfx")
local bimgdir = package:resourcefile("bimg")
local genie = is_host("windows") and "genie.exe" or "genie"
local args = {}
if is_plat("windows|native", "macosx", "linux") then
if is_plat("windows", "macosx", "linux") then
Copy link
Member

Choose a reason for hiding this comment

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

use package:is_plat

@waruqi waruqi requested a review from xq114 August 31, 2024 14:18
packages/b/bgfx/rules/shaders.lua Show resolved Hide resolved
packages/b/bgfx/rules/shaders.lua Show resolved Hide resolved
packages/b/bgfx/rules/shaders.lua Show resolved Hide resolved
if fileconfig and fileconfig.header then
table.insert(args, "--bin2c")
if fileconfig.header ~= true then
table.insert(args, fileconfig.header)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be more info about .header argument in the comments at the beginning of this file, e.g. What would happen if both -o <output> and --bin2c <header> are specified? Which of the <output> and the <header> will be generated? Is output_name = "shader.vert.h" really correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --bin2c option generates a header file containing the compiled shader to be included in the project instead of loaded in runtime. It the output_name is defined, the ".h" suffix is recommended, but it just generates a header file. The "

" string is optional and, if provided it's used as the variable containing the shader data, if not defined it uses the input file base name. Since not all rule configuration values follow strictly the shaderc inputs I didn't follow also and used a more explicit name. What is the best way to document this rule, if it's necessary to read the source anyway, is that worth documenting it better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course it's better documenting the rule on some website like xrepo.xmake.io, but it's not yet implemented, so as for now the only way to document it is to do so in the rule file. Seems that when there is --bin2c <header>, the variable name in the generated file will be <header>? If so, I'd suggest a different name for this config, e.g. export_var or array_name(align with https://bkaradzic.github.io/bgfx/tools.html#shader-compiler-shaderc ) or just array to be more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

array_name=true can become an advanced feature (deduce by extension .h is usually enough), so it's ok to document in the following way:

-- add_rules("@bgfx/shaders")
-- generate binary file:
-- add_files("shader.vert", {type = "vertex", output_dir = "shaders", output_name = "shader.vert.bin", profiles = {glsl = "330"}})
-- generate header file:
-- add_files("vs_shader.sc", {type = "vertex", output_dir = "shaders", output_name = "shader.vert.h", profiles = {glsl = "330"}})
-- generate header file exporting variable "vertex_src":
-- add_files("vs_shader.sc", {type = "vertex", output_dir = "shaders", output_name = "shader.vert.h", array_name = "vertex_src", profiles = {glsl = "330"}})
-- force to generate header file with default variable name:
-- add_files("vs_shader.sc", {type = "vertex", output_dir = "shaders", output_name = "shader.vert.inc", array_name = true, profiles = {glsl = "330"}})

@xq114 xq114 merged commit 149ce23 into xmake-io:dev Sep 6, 2024
65 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.

bgfx shaders rules not working
3 participants