Fixed bgfx_compile_shaders incorrectly generating output path, and generat quality of life update for all tool util functions #255

tabdroid-1 · 2024-11-15T02:36:23Z

changes in 'bgfx_compile_geometry', 'bgfx_compile_texture', 'bgfx_compile_binary_to_header' and 'bgfx_compile_shaders':

If path doesnt exist to output path, it will be generated on build system generation. (fix for bgfx_compile_shaders, adition to rest of functions)

All these functions create log file in 'bgfx_tool_logs' directory in root of build directory.
ex:

binary_to_header_compile_output_..-assets-bin-compiled-test.h.log
-------------------------------   ---------------------------------
                  tool used            relative path to asset output
                  
shader_compile_output_..-assets-shaders-essl-vs.sc.bin.h.log
---------------------    ---------------------------------
          tool used         relative path to asset output

also i added mention in readme for 'bgfx_compile_geometry' which was missing for some reason

i have only tested this on linux. i have no access windows or others atm

i also had to call add_custom_target for each functions else it didnt run

add_custom_command(
	OUTPUT ${OUTPUTS} #
	COMMAND ${COMMANDS} #
	MAIN_DEPENDENCY ${SHADER_FILE_ABSOLUTE} #
        COMMENT "Compiling ${TARGET_NAME}" #
	DEPENDS ${ARGS_VARYING_DEF} #
)

add_custom_target(
        ${FILE_NAME_APPROPRIATE_OUTPUT_PATH} ALL #
        DEPENDS ${OUTPUTS} #
)

…om bgfx_compile_shaders

…utput path

…h if it doesnt exist

…oesnt exist and fixed bgfx_compile_binary_to_header not creatign output path if it doesnt exist

…doesnt exist and written mention of it in readme

github-actions · 2024-11-15T02:37:12Z

cmake/bgfxToolUtils.cmake

+        get_filename_component(OUTPUT_DIR "${ARG_OUTPUT_FILE}" DIRECTORY)
+        file(MAKE_DIRECTORY ${OUTPUT_DIR}) # create output path if it does not exist


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

get_filename_component(OUTPUT_DIR "${ARG_OUTPUT_FILE}" DIRECTORY)

file(MAKE_DIRECTORY ${OUTPUT_DIR}) # create output path if it does not exist

get_filename_component(OUTPUT_DIR "${ARG_OUTPUT_FILE}" DIRECTORY)

file(MAKE_DIRECTORY ${OUTPUT_DIR}) # create output path if it does not exist

github-actions · 2024-11-15T02:37:13Z

cmake/bgfxToolUtils.cmake

+        file(RELATIVE_PATH INPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_INPUT_FILE})
+        file(RELATIVE_PATH OUTPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_OUTPUT_FILE})


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

file(RELATIVE_PATH INPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_INPUT_FILE})

file(RELATIVE_PATH OUTPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_OUTPUT_FILE})

file(RELATIVE_PATH INPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_INPUT_FILE})

file(RELATIVE_PATH OUTPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_OUTPUT_FILE})

github-actions · 2024-11-15T02:37:13Z

cmake/bgfxToolUtils.cmake

+        string(REPLACE " " "_"  FILE_NAME_APPROPRIATE_OUTPUT_PATH ${OUTPUT_FILE_RELATIVE})
+        string(REPLACE "/" "-"  FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH})
+        string(REPLACE "\\" "-"  FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH}) # probably need this for windows


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

string(REPLACE " " "_" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${OUTPUT_FILE_RELATIVE})

string(REPLACE "/" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH})

string(REPLACE "\\" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH}) # probably need this for windows

string(REPLACE " " "_" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${OUTPUT_FILE_RELATIVE})

string(REPLACE "/" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH})

string(REPLACE "\\" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH}

)# probably need this for windows

github-actions · 2024-11-15T02:37:13Z

cmake/bgfxToolUtils.cmake

+			COMMAND bgfx::bin2c ${CLI} > bgfx_tool_logs/binary_to_header_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log #
+            COMMENT "Compiling ${INPUT_FILE_RELATIVE} as header" #


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

COMMAND bgfx::bin2c ${CLI} > bgfx_tool_logs/binary_to_header_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log #

COMMENT "Compiling ${INPUT_FILE_RELATIVE} as header" #

COMMAND bgfx::bin2c ${CLI} >

bgfx_tool_logs/binary_to_header_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log #

COMMENT "Compiling ${INPUT_FILE_RELATIVE} as header" #

github-actions · 2024-11-15T02:37:13Z

cmake/bgfxToolUtils.cmake

+        add_custom_target(
+            ${FILE_NAME_APPROPRIATE_OUTPUT_PATH} ALL #
+             DEPENDS ${ARG_OUTPUT_FILE} #
+        )


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

add_custom_target(

${FILE_NAME_APPROPRIATE_OUTPUT_PATH} ALL #

DEPENDS ${ARG_OUTPUT_FILE} #

)

add_custom_target(

${FILE_NAME_APPROPRIATE_OUTPUT_PATH} ALL #

DEPENDS ${ARG_OUTPUT_FILE} #

)

github-actions · 2024-11-15T02:37:14Z

cmake/bgfxToolUtils.cmake

+                file(RELATIVE_PATH OUTPUT_RELATIVE ${CMAKE_BINARY_DIR} ${OUTPUT})
+
+                string(REPLACE " " "_"  FILE_NAME_APPROPRIATE_OUTPUT_PATH ${OUTPUT_RELATIVE})
+                string(REPLACE "/" "-"  FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH})
+                string(REPLACE "\\" "-"  FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH}) # probably need this for windows
+
+				list(APPEND COMMANDS COMMAND bgfx::shaderc ${CLI} > bgfx_tool_logs/shader_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log)


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

file(RELATIVE_PATH OUTPUT_RELATIVE ${CMAKE_BINARY_DIR} ${OUTPUT})

string(REPLACE " " "_" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${OUTPUT_RELATIVE})

string(REPLACE "/" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH})

string(REPLACE "\\" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH}) # probably need this for windows

list(APPEND COMMANDS COMMAND bgfx::shaderc ${CLI} > bgfx_tool_logs/shader_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log)

file(RELATIVE_PATH OUTPUT_RELATIVE ${CMAKE_BINARY_DIR} ${OUTPUT})

string(REPLACE " " "_" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${OUTPUT_RELATIVE})

string(REPLACE "/" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH})

string(REPLACE "\\" "-" FILE_NAME_APPROPRIATE_OUTPUT_PATH ${FILE_NAME_APPROPRIATE_OUTPUT_PATH}

)# probably need this for windows

list(

APPEND

COMMANDS

COMMAND

bgfx::shaderc

${CLI}

>

bgfx_tool_logs/shader_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log

)

github-actions · 2024-11-15T02:37:15Z

cmake/bgfxToolUtils.cmake

 			endforeach()

+            file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/bgfx_tool_logs)


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/bgfx_tool_logs)

file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/bgfx_tool_logs)

github-actions · 2024-11-15T02:37:15Z

cmake/bgfxToolUtils.cmake

+            # This is to prevent error if compiling as binary and header in same output path 
+            set(TARGET_NAME "${SHADER_FILE_RELATIVE}")
+            if(ARGS_AS_HEADERS)  
+                set(TARGET_NAME "${SHADER_FILE_RELATIVE} as header")
+            endif()


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

# This is to prevent error if compiling as binary and header in same output path

set(TARGET_NAME "${SHADER_FILE_RELATIVE}")

if(ARGS_AS_HEADERS)

set(TARGET_NAME "${SHADER_FILE_RELATIVE} as header")

endif()

# This is to prevent error if compiling as binary and header in same output path

set(TARGET_NAME "${SHADER_FILE_RELATIVE}")

if(ARGS_AS_HEADERS)

set(TARGET_NAME "${SHADER_FILE_RELATIVE} as header")

endif()

github-actions · 2024-11-15T02:37:15Z

cmake/bgfxToolUtils.cmake

+				OUTPUT ${OUTPUTS} #
+				COMMAND ${COMMANDS} #
+				MAIN_DEPENDENCY ${SHADER_FILE_ABSOLUTE} #
+                COMMENT "Compiling ${TARGET_NAME}" #


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

COMMENT "Compiling ${TARGET_NAME}" #

COMMENT "Compiling ${TARGET_NAME}" #

github-actions · 2024-11-15T02:37:15Z

cmake/bgfxToolUtils.cmake

+            add_custom_target(
+                ${FILE_NAME_APPROPRIATE_OUTPUT_PATH} ALL #
+                DEPENDS ${OUTPUTS} #
+            )


[cmake-format] _{reported by reviewdog 🐶}

Suggested change

add_custom_target(

${FILE_NAME_APPROPRIATE_OUTPUT_PATH} ALL #

DEPENDS ${OUTPUTS} #

)

add_custom_target(

${FILE_NAME_APPROPRIATE_OUTPUT_PATH} ALL #

DEPENDS ${OUTPUTS} #

)

bwrsandman · 2024-11-17T17:00:30Z

Apply this patch to fix the failing format test
https://github.com/bkaradzic/bgfx.cmake/actions/runs/11849223278/artifacts/2191073664

bwrsandman

Testing the change locally, it immediately fails to compile shaders with:

[build] [1/24] Compiling ../../assets/shaders/imgui/vs_imgui_image.sc as header
[build] FAILED: include/generated/shaders/imgui/glsl/vs_imgui_image.sc.bin.h include/generated/shaders/imgui/essl/vs_imgui_image.sc.bin.h include/generated/shaders/imgui/spirv/vs_imgui_image.sc.bin.h code/openblack/cmake-build-presets/ninja-multi-vcpkg/include/generated/shaders/imgui/glsl/vs_imgui_image.sc.bin.h code/openblack/cmake-build-presets/ninja-multi-vcpkg/include/generated/shaders/imgui/essl/vs_imgui_image.sc.bin.h code/openblack/cmake-build-presets/ninja-multi-vcpkg/include/generated/shaders/imgui/spirv/vs_imgui_image.sc.bin.h 
[build] cd code/openblack/cmake-build-presets/ninja-multi-vcpkg/src && code/openblack/cmake-build-presets/ninja-multi-vcpkg/vcpkg_installed/x64-linux/tools/bgfx/shaderc -f code/openblack/assets/shaders/imgui/vs_imgui_image.sc -i code/openblack/cmake-build-presets/ninja-multi-vcpkg/vcpkg_installed/x64-linux/include/bgfx -o code/openblack/cmake-build-presets/ninja-multi-vcpkg/include/generated/shaders/imgui/glsl/vs_imgui_image.sc.bin.h --bin2c vs_imgui_image_glsl --type vertex --varyingdef code/openblack/assets/shaders/imgui/varying.def.sc --profile 120 -O 0 --Werror > bgfx_tool_logs/shader_compile_output_include-generated-shaders-imgui-glsl-vs_imgui_image.sc.bin.h.log && code/openblack/cmake-build-presets/ninja-multi-vcpkg/vcpkg_installed/x64-linux/tools/bgfx/shaderc -f code/openblack/assets/shaders/imgui/vs_imgui_image.sc -i code/openblack/cmake-build-presets/ninja-multi-vcpkg/vcpkg_installed/x64-linux/include/bgfx -o code/openblack/cmake-build-presets/ninja-multi-vcpkg/include/generated/shaders/imgui/essl/vs_imgui_image.sc.bin.h --bin2c vs_imgui_image_essl --type vertex --varyingdef code/openblack/assets/shaders/imgui/varying.def.sc --profile 300_es -O 0 --Werror > bgfx_tool_logs/shader_compile_output_include-generated-shaders-imgui-essl-vs_imgui_image.sc.bin.h.log && code/openblack/cmake-build-presets/ninja-multi-vcpkg/vcpkg_installed/x64-linux/tools/bgfx/shaderc -f code/openblack/assets/shaders/imgui/vs_imgui_image.sc -i code/openblack/cmake-build-presets/ninja-multi-vcpkg/vcpkg_installed/x64-linux/include/bgfx -o code/openblack/cmake-build-presets/ninja-multi-vcpkg/include/generated/shaders/imgui/spirv/vs_imgui_image.sc.bin.h --bin2c vs_imgui_image_spv --type vertex --varyingdef code/openblack/assets/shaders/imgui/varying.def.sc --profile spirv -O 0 --Werror > bgfx_tool_logs/shader_compile_output_include-generated-shaders-imgui-spirv-vs_imgui_image.sc.bin.h.log
[build] /bin/sh: line 1: bgfx_tool_logs/shader_compile_output_include-generated-shaders-imgui-glsl-vs_imgui_image.sc.bin.h.log: No such file or directory

Looking at my directory structure code/openblack/cmake-build-presets/ninja-multi-vcpkg/bgfx_tool_logs was created but I think you're not taking into account ${CMAKE_BINARY_DIR} at some step.

bwrsandman · 2024-11-17T17:08:39Z

cmake/bgfxToolUtils.cmake

 		add_custom_command(
 			OUTPUT ${ARG_OUTPUT_FILE} #
-			COMMAND bgfx::bin2c ${CLI} #
+			COMMAND bgfx::bin2c ${CLI} > bgfx_tool_logs/binary_to_header_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log #


will this work on windows?

bwrsandman · 2024-11-17T17:10:07Z

I don't really see the advantage of logging build output to a file.
The logical place to put this is in stdout like with c++ compiling, if you you're developing you want to see logs of your build tools in your build logs, not fish around in files...

tabdroid-1 · 2024-11-17T17:12:50Z

ok i will remove logging in to file and figure out the error. logging probably is going to be hinder rather than helpful.

tabdroid-1 · 2024-11-17T19:06:21Z

could you give me your local test cmakelists.txt to test if its working as you expect it to?

also file structure.

tabdroid-1 added 5 commits November 15, 2024 00:46

Fix incorrect output directory generation and log output generated fr…

e49699b

…om bgfx_compile_shaders

Fix error when compiling same shader as binary and header with same o…

a56d4df

…utput path

bgfx_compile_binary_to_header creates log file and creates output pat…

83c498a

…h if it doesnt exist

bgfx_compile_texture creates log file and creates output path if it d…

cc1ea95

…oesnt exist and fixed bgfx_compile_binary_to_header not creatign output path if it doesnt exist

bgfx_compile_geometry creates log file and creates output path if it …

8f820c4

…doesnt exist and written mention of it in readme

tabdroid-1 requested a review from bwrsandman as a code owner November 15, 2024 02:36

github-actions bot reviewed Nov 15, 2024

View reviewed changes

bwrsandman requested changes Nov 17, 2024

View reviewed changes

bwrsandman reviewed Nov 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bgfx_compile_shaders incorrectly generating output path, and generat quality of life update for all tool util functions #255

Fixed bgfx_compile_shaders incorrectly generating output path, and generat quality of life update for all tool util functions #255

tabdroid-1 commented Nov 15, 2024 •

edited

Loading

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

github-actions bot Nov 15, 2024

bwrsandman commented Nov 17, 2024

bwrsandman left a comment

bwrsandman Nov 17, 2024

bwrsandman commented Nov 17, 2024

tabdroid-1 commented Nov 17, 2024 •

edited

Loading

tabdroid-1 commented Nov 17, 2024 •

edited

Loading

		get_filename_component(OUTPUT_DIR "${ARG_OUTPUT_FILE}" DIRECTORY)
		file(MAKE_DIRECTORY ${OUTPUT_DIR}) # create output path if it does not exist

		file(RELATIVE_PATH INPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_INPUT_FILE})
		file(RELATIVE_PATH OUTPUT_FILE_RELATIVE ${CMAKE_BINARY_DIR} ${ARG_OUTPUT_FILE})

		COMMAND bgfx::bin2c ${CLI} > bgfx_tool_logs/binary_to_header_compile_output_${FILE_NAME_APPROPRIATE_OUTPUT_PATH}.log #
		COMMENT "Compiling ${INPUT_FILE_RELATIVE} as header" #

		endforeach()

		file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/bgfx_tool_logs)

	COMMENT "Compiling ${TARGET_NAME}" #
	COMMENT "Compiling ${TARGET_NAME}" #

Fixed bgfx_compile_shaders incorrectly generating output path, and generat quality of life update for all tool util functions #255

Are you sure you want to change the base?

Fixed bgfx_compile_shaders incorrectly generating output path, and generat quality of life update for all tool util functions #255

Conversation

tabdroid-1 commented Nov 15, 2024 • edited Loading

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

bwrsandman commented Nov 17, 2024

bwrsandman left a comment

Choose a reason for hiding this comment

bwrsandman Nov 17, 2024

Choose a reason for hiding this comment

bwrsandman commented Nov 17, 2024

tabdroid-1 commented Nov 17, 2024 • edited Loading

tabdroid-1 commented Nov 17, 2024 • edited Loading

tabdroid-1 commented Nov 15, 2024 •

edited

Loading

tabdroid-1 commented Nov 17, 2024 •

edited

Loading

tabdroid-1 commented Nov 17, 2024 •

edited

Loading