Add thirdparty rendering deps as submodules#12
Add thirdparty rendering deps as submodules#12AR-DEV-1 wants to merge 3 commits intoRedot-Engine:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces Git submodules for bgfx, bimg, and bx graphics libraries, alongside comprehensive CMake build system configuration. It enables C++23 standard support, adds Clang libc++ compatibility, and establishes module scanning. The changes wire third-party graphics dependencies into the build pipeline with extensive toolchain support for shader compilation, texture encoding, and geometry processing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
engine/native/thirdparty/CMakeLists.txt (1)
8-9: Consider guarding the bgfx target property like bx and bimg.The
bgfxtarget property is set unconditionally, whilebxandbimgare guarded withif(TARGET ...). For consistency and to avoid a CMake error ifadd_subdirectory(bgfx.cmake)fails silently for some reason, consider adding the same guard.♻️ Optional: Add target existence check
# Prevent module scanning for bgfx & it's deps -set_target_properties(bgfx PROPERTIES CXX_SCAN_FOR_MODULES OFF) +if(TARGET bgfx) + set_target_properties(bgfx PROPERTIES CXX_SCAN_FOR_MODULES OFF) +endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/CMakeLists.txt` around lines 8 - 9, The bgfx target property is set unconditionally with set_target_properties(bgfx PROPERTIES CXX_SCAN_FOR_MODULES OFF), which can error if the bgfx target doesn't exist; mirror the existing guards used for bx and bimg by wrapping that call in an if(TARGET bgfx) ... endif block so the CXX_SCAN_FOR_MODULES property is only applied when the bgfx target is present.CMakeLists.txt (1)
11-24: Submodule auto-update logic looks good with minor suggestions.The implementation correctly checks for Git availability and
.gitdirectory presence before attempting updates. Two optional improvements:
- Consider adding
OUTPUT_QUIET ERROR_QUIETto reduce CMake output noise during normal configuration, or capture output for better error diagnostics.- The comparison
NOT GIT_SUBMOD_RESULT EQUAL "0"works but comparing against the integer0is more idiomatic forRESULT_VARIABLE.♻️ Optional: Quieter output with better error reporting
execute_process( COMMAND ${GIT_EXECUTABLE} submodule update --init --recursive WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} RESULT_VARIABLE GIT_SUBMOD_RESULT + OUTPUT_VARIABLE GIT_SUBMOD_OUTPUT + ERROR_VARIABLE GIT_SUBMOD_ERROR ) - if(NOT GIT_SUBMOD_RESULT EQUAL "0") - message(FATAL_ERROR "Failed to update submodules. Please run manually.") + if(NOT GIT_SUBMOD_RESULT EQUAL 0) + message(FATAL_ERROR "Failed to update submodules (exit code ${GIT_SUBMOD_RESULT}): ${GIT_SUBMOD_ERROR}") endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 11 - 24, Improve the submodule update step to suppress normal output and use an integer comparison for the result: when calling execute_process(...) that runs ${GIT_EXECUTABLE} submodule update --init --recursive, add OUTPUT_QUIET ERROR_QUIET (or capture OUTPUT_VARIABLE/ERROR_VARIABLE) to reduce noise, and change the conditional that checks GIT_SUBMOD_RESULT to compare against the integer 0 (e.g. use NOT GIT_SUBMOD_RESULT EQUAL 0 → GIT_SUBMOD_RESULT NOT EQUAL 0 or simply if(GIT_SUBMOD_RESULT)) so the RESULT_VARIABLE GIT_SUBMOD_RESULT is tested idiomatically; locate the changes around find_package(Git), execute_process(...) and the GIT_SUBMOD_RESULT check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/Compiler.cmake`:
- Around line 18-22: The bgfx/bx/bimg targets are created before the global
libc++ compile/link options are added, causing an ABI mismatch; fix by moving
the include(Compiler) invocation so add_compile_options(-stdlib=libc++) and
add_link_options(-stdlib=libc++) are applied before those targets are created
(e.g., call include(Compiler) before add_subdirectory(thirdparty) in
engine/native/CMakeLists.txt), or alternatively move the stdlib flag setup into
the top-level root CMakeLists.txt before any add_subdirectory() calls so bgfx,
bx, and bimg inherit the libc++ flags.
In `@engine/native/CMakeLists.txt`:
- Around line 1-4: The include order is wrong: compiler flags set by
include(Compiler) are applied after third-party targets are added, so bgfx and
other thirdparty targets may miss those flags; move include(Compiler) (and
include(Modules) if modules rely on its flags) before
add_subdirectory(thirdparty) so the Compiler CMake settings are in effect when
third-party targets are configured—specifically reorder the top-level
CMakeLists.txt so include(Compiler) runs prior to add_subdirectory(thirdparty).
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 11-24: Improve the submodule update step to suppress normal output
and use an integer comparison for the result: when calling execute_process(...)
that runs ${GIT_EXECUTABLE} submodule update --init --recursive, add
OUTPUT_QUIET ERROR_QUIET (or capture OUTPUT_VARIABLE/ERROR_VARIABLE) to reduce
noise, and change the conditional that checks GIT_SUBMOD_RESULT to compare
against the integer 0 (e.g. use NOT GIT_SUBMOD_RESULT EQUAL 0 →
GIT_SUBMOD_RESULT NOT EQUAL 0 or simply if(GIT_SUBMOD_RESULT)) so the
RESULT_VARIABLE GIT_SUBMOD_RESULT is tested idiomatically; locate the changes
around find_package(Git), execute_process(...) and the GIT_SUBMOD_RESULT check.
In `@engine/native/thirdparty/CMakeLists.txt`:
- Around line 8-9: The bgfx target property is set unconditionally with
set_target_properties(bgfx PROPERTIES CXX_SCAN_FOR_MODULES OFF), which can error
if the bgfx target doesn't exist; mirror the existing guards used for bx and
bimg by wrapping that call in an if(TARGET bgfx) ... endif block so the
CXX_SCAN_FOR_MODULES property is only applied when the bgfx target is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c270e018-f9b2-417e-a639-26622065ec85
📒 Files selected for processing (8)
.gitignore.gitmodulesCMakeLists.txtcmake/Compiler.cmakecmake/Modules.cmakeengine/native/CMakeLists.txtengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/bgfx.cmake
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
engine/native/thirdparty/cmake/bgfx/3rdparty/fcpp.cmake-28-29 (1)
28-29:⚠️ Potential issue | 🟡 MinorRemove the duplicated
cpp6.centry.
cpp6.cis listed twice inFCPP_SOURCESat lines 28-29. Some CMake generators surface duplicate source entries directly, and others may try to compile the same translation unit twice.Suggested fix
${FCPP_DIR}/cpp5.c ${FCPP_DIR}/cpp6.c - ${FCPP_DIR}/cpp6.c )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/3rdparty/fcpp.cmake` around lines 28 - 29, FCPP_SOURCES contains a duplicated entry for cpp6.c; remove the duplicate line so cpp6.c appears only once or deduplicate the list after population (e.g., call list(REMOVE_DUPLICATES FCPP_SOURCES)) to ensure the same translation unit isn't compiled twice; update the FCPP_SOURCES definition where cpp6.c is listed twice to reference cpp6.c only once or add the dedupe call immediately after that variable is set.engine/native/thirdparty/cmake/bgfx/3rdparty/glsl-optimizer.cmake-12-15 (1)
12-15:⚠️ Potential issue | 🟡 MinorQuote the
BGFX_DIRguard for consistency and clarity.While the unquoted form will still trigger the diagnostic correctly when the directory is missing, quoting path variables in CMake conditions is a best practice to ensure consistent argument parsing and improve code clarity.
Suggested fix
-if(NOT IS_DIRECTORY ${BGFX_DIR}) +if(NOT IS_DIRECTORY "${BGFX_DIR}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/3rdparty/glsl-optimizer.cmake` around lines 12 - 15, The BGFX_DIR guard uses an unquoted variable in the IF condition and message; update the guard to quote the variable everywhere it’s used (e.g., change IF(NOT IS_DIRECTORY ${BGFX_DIR}) to IF(NOT IS_DIRECTORY "${BGFX_DIR}") and include quotes around ${BGFX_DIR} in the message call) so argument parsing is consistent and the check/logging remain robust; adjust the condition and the message that references BGFX_DIR accordingly.engine/native/thirdparty/cmake/bgfx/3rdparty/glslang.cmake-46-51 (1)
46-51:⚠️ Potential issue | 🟡 MinorTypo in comment: "spriv-tools" → "spirv-tools".
Proposed fix
target_compile_definitions( glslang PRIVATE # - ENABLE_OPT=1 # spriv-tools + ENABLE_OPT=1 # spirv-tools ENABLE_HLSL=1 # )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/3rdparty/glslang.cmake` around lines 46 - 51, The comment next to the ENABLE_OPT compile definition contains a typo ("spriv-tools"); update the comment in the target_compile_definitions block for glslang so the comment reads "spirv-tools" instead, preserving the existing PRIVATE and definitions (ENABLE_OPT=1, ENABLE_HLSL=1) and formatting inside the target_compile_definitions call.engine/native/thirdparty/cmake/bimg/bimg.cmake-11-15 (1)
11-15:⚠️ Potential issue | 🟡 MinorMissing early return guard for
bimgtarget.This file lacks the standard
if(TARGET bimg) return() endif()guard present in other modules. Add the guard for consistency and to prevent duplicate target errors.Proposed fix
+if(TARGET bimg) + return() +endif() + # Ensure the directory exists if(NOT IS_DIRECTORY ${BIMG_DIR}) message(SEND_ERROR "Could not load bimg, directory does not exist. ${BIMG_DIR}") return() endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bimg/bimg.cmake` around lines 11 - 15, Add the standard early-return guard for the bimg target to prevent duplicate-target errors: at the top of this CMake snippet (before using BIMG_DIR) insert an if(TARGET bimg) return() endif() block so that if the bimg target already exists the script exits early; reference the TARGET bimg symbol and ensure the guard is placed prior to the existing IS_DIRECTORY check and message(SEND_ERROR ...) logic.engine/native/thirdparty/cmake/bgfx/3rdparty/glslang.cmake-11-15 (1)
11-15:⚠️ Potential issue | 🟡 MinorMissing early return guard for
glslangtarget.Add the standard target guard for consistency with other third-party modules.
Proposed fix
+if(TARGET glslang) + return() +endif() + # Ensure the directory exists if(NOT IS_DIRECTORY ${BGFX_DIR}) message(SEND_ERROR "Could not load bgfx, directory does not exist. ${BGFX_DIR}") return() endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/3rdparty/glslang.cmake` around lines 11 - 15, The glslang third-party CMake include is missing the standard early-return guard; add a guard at the top that checks if the glslang target already exists (e.g., if(TARGET glslang) return()) so the file exits immediately when the glslang target is present, matching the pattern used by other 3rdparty modules and preventing duplicate target definitions; place this check before any target or directory logic (reference the glslang target name "glslang" and the current directory-check block).engine/native/thirdparty/cmake/bgfx/3rdparty/spirv-cross.cmake-11-15 (1)
11-15:⚠️ Potential issue | 🟡 MinorMissing early return guard for
spirv-crosstarget.Unlike other third-party modules (e.g.,
webgpu.cmake,tint.cmake), this file lacks an early return guard. If included multiple times, CMake will fail with a duplicate target error.Proposed fix - add guard before directory check
+if(TARGET spirv-cross) + return() +endif() + # Ensure the directory exists if(NOT IS_DIRECTORY ${BGFX_DIR}) message(SEND_ERROR "Could not load bgfx, directory does not exist. ${BGFX_DIR}") return() endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/3rdparty/spirv-cross.cmake` around lines 11 - 15, Add an early-return guard to avoid defining the spirv-cross target twice: before the existing BGFX_DIR directory check, test for the spirv-cross target (e.g., using if(TARGET spirv-cross) or a module-specific guard variable) and return immediately if it already exists; keep the BGFX_DIR existence check and message(SEND_ERROR ...) as-is but only run them when the spirv-cross target is not present so repeated includes do not cause duplicate target errors.engine/native/thirdparty/cmake/bgfx/geometryc.cmake-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorIncorrect compile definition syntax - redundant
-Dprefix.
target_compile_definitionsautomatically prepends-D(or/Don MSVC). Using"-D_CRT_SECURE_NO_WARNINGS"results in the flag being doubled (e.g.,-D-D_CRT_SECURE_NO_WARNINGS), which may cause compiler warnings or errors.Proposed fix
-target_compile_definitions(geometryc PRIVATE "-D_CRT_SECURE_NO_WARNINGS") +target_compile_definitions(geometryc PRIVATE _CRT_SECURE_NO_WARNINGS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/geometryc.cmake` at line 24, The compile definition for target_compile_definitions on target geometryc is using a redundant "-D" prefix; change the call to pass the macro name without the "-D" so CMake can add the proper prefix (i.e., replace "-D_CRT_SECURE_NO_WARNINGS" with _CRT_SECURE_NO_WARNINGS in the target_compile_definitions invocation for geometryc).engine/native/thirdparty/cmake/bgfx/texturev.cmake-12-14 (1)
12-14:⚠️ Potential issue | 🟡 MinorGlob pattern too broad - may include non-source files.
The glob pattern
${BGFX_DIR}/tools/texturev/*captures all files without filtering by extension. This could include non-source files (e.g., README, config files) in the build, potentially causing compilation errors or warnings. Other tool modules (liketexturec.cmake) correctly filter to*.cppand*.h.Proposed fix
# Grab the texturev source files file(GLOB_RECURSE TEXTUREV_SOURCES # - ${BGFX_DIR}/tools/texturev/* + ${BGFX_DIR}/tools/texturev/*.cpp # + ${BGFX_DIR}/tools/texturev/*.h # )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/texturev.cmake` around lines 12 - 14, The glob for TEXTUREV_SOURCES is too broad and may pick up non-source files; restrict the pattern used for ${BGFX_DIR}/tools/texturev to only source/header files (e.g., *.cpp and *.h or whatever extensions used by other modules) so the build excludes docs/configs; update the TEXTUREV_SOURCES file(GLOB_RECURSE ...) invocation in texturev.cmake to mirror the filtered patterns used by texturec.cmake and reference ${BGFX_DIR}/tools/texturev and the TEXTUREV_SOURCES variable when making the change.
🧹 Nitpick comments (6)
engine/native/thirdparty/cmake/bgfx/util/ConfigureDebugging.cmake (1)
115-117: Remove the empty conditional block.The
if(NOT ... STREQUAL "")block at Lines 115-117 is empty and adds noise with no behavior.Proposed cleanup
- if(NOT "${ARG_${ARG}_${CONFIG_CAP}_${PROCESSOR_CAP}}" STREQUAL "") - - endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/util/ConfigureDebugging.cmake` around lines 115 - 117, Remove the empty conditional block that checks if(NOT "${ARG_${ARG}_${CONFIG_CAP}_${PROCESSOR_CAP}}" STREQUAL "") by deleting the entire if(...) ... endif() pair around ARG_${ARG}_${CONFIG_CAP}_${PROCESSOR_CAP} in ConfigureDebugging.cmake so no-noop branches remain; ensure surrounding logic and indentation are preserved and no other statements are accidentally removed.engine/native/thirdparty/cmake/bgfx/3rdparty/fcpp.cmake (1)
12-15: Quote theBGFX_DIRvariable in theIS_DIRECTORYcheck.Always quote variable references in CMake conditionals to follow best practices and avoid legacy re-evaluation pitfalls. Both quoted and unquoted forms behave identically here—if
BGFX_DIRis unset or invalid, the condition correctly evaluates to false and triggers the error message—but quoting is the safer, recommended pattern.Suggested fix
-if(NOT IS_DIRECTORY ${BGFX_DIR}) +if(NOT IS_DIRECTORY "${BGFX_DIR}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/3rdparty/fcpp.cmake` around lines 12 - 15, The IS_DIRECTORY conditional should quote the BGFX_DIR variable to avoid unquoted variable pitfalls; update the condition from using IS_DIRECTORY ${BGFX_DIR} to IS_DIRECTORY "${BGFX_DIR}" (leave the message(SEND_ERROR ...) and return() intact) so the check uses the quoted BGFX_DIR variable.cmake/Modules.cmake (2)
96-101:enable_modules()is inconsistent with other module-enabled targets.The
boost_ut_maintarget (lines 10-13) and library targets (lines 60-63) both setCXX_EXTENSIONS OFFalongsideCXX_SCAN_FOR_MODULES ON. This new helper function only setsCXX_SCAN_FOR_MODULES, which could lead to inconsistent behavior when used.Consider either adding
CXX_EXTENSIONS OFFhere for consistency, or settingCXX_EXTENSIONS OFFglobally (as mentioned in past review feedback) and removing it from individual targets.Option 1: Make enable_modules consistent
function(enable_modules target) set_target_properties( ${target} PROPERTIES CXX_SCAN_FOR_MODULES ON + CXX_EXTENSIONS OFF ) endfunction()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/Modules.cmake` around lines 96 - 101, The enable_modules function currently only sets CXX_SCAN_FOR_MODULES and is inconsistent with other targets (e.g., boost_ut_main and library targets) that also set CXX_EXTENSIONS OFF; update enable_modules to set both CXX_SCAN_FOR_MODULES ON and CXX_EXTENSIONS OFF so targets using enable_modules behave the same, or alternatively apply CXX_EXTENSIONS OFF globally and remove per-target settings—pick one approach and make the change in the enable_modules function (or the global CMake settings) to ensure consistent module-related properties.
85-86: Unit test targets missingCXX_EXTENSIONS OFF.Unlike the library targets and
boost_ut_main, unit test executables only getCXX_SCAN_FOR_MODULES ONbut notCXX_EXTENSIONS OFF. For consistency across all module-enabled targets, consider addingCXX_EXTENSIONS OFFhere as well.Suggested fix
- set_target_properties(${UNIT_TEST_TARGET} PROPERTIES CXX_SCAN_FOR_MODULES ON) + set_target_properties(${UNIT_TEST_TARGET} PROPERTIES + CXX_SCAN_FOR_MODULES ON + CXX_EXTENSIONS OFF + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/Modules.cmake` around lines 85 - 86, Unit test executable targets created with add_executable(${UNIT_TEST_TARGET} ${UNIT_TEST_FILE}) only set CXX_SCAN_FOR_MODULES ON; update the target properties for ${UNIT_TEST_TARGET} (via set_target_properties) to also set CXX_EXTENSIONS OFF so module behavior matches the library targets and boost_ut_main — i.e., modify the set_target_properties call referencing ${UNIT_TEST_TARGET} to include both CXX_SCAN_FOR_MODULES ON and CXX_EXTENSIONS OFF.CMakeLists.txt (1)
11-24: Submodule initialization logic is well-structured.The logic correctly checks for Git availability and repository presence before attempting submodule updates. This is a good defensive approach.
However, there's a minor issue on line 21:
RESULT_VARIABLEreturns an integer, but you're comparing against a string"0". While CMake is often lenient, the idiomatic form is to compare against the integer0.Suggested fix
- if(NOT GIT_SUBMOD_RESULT EQUAL "0") + if(NOT GIT_SUBMOD_RESULT EQUAL 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 11 - 24, The comparison currently treats RESULT_VARIABLE GIT_SUBMOD_RESULT as a string (if(NOT GIT_SUBMOD_RESULT EQUAL "0")); update the condition to compare it as an integer by using if(NOT GIT_SUBMOD_RESULT EQUAL 0) (or an equivalent integer comparison) so that the execute_process RESULT_VARIABLE check uses the proper integer comparison for GIT_SUBMOD_RESULT in the submodule update block.engine/native/thirdparty/cmake/bgfx/geometryv.cmake (1)
12-14: Glob pattern uses broad wildcard inconsistently with other tools.The
*wildcard inGLOB_RECURSEis overly broad compared to similar tool build scripts.geometryc.cmakeuses*.cpp, andbin2c.cmakeuses explicit*.cppand*.hpatterns. For consistency and to avoid unintended file inclusion, use explicit source file extensions.♻️ Suggested fix to match pattern of other tools
# Grab the geometryv source files file(GLOB_RECURSE GEOMETRYV_SOURCES # - ${BGFX_DIR}/tools/geometryv/* + ${BGFX_DIR}/tools/geometryv/*.cpp # + ${BGFX_DIR}/tools/geometryv/*.h # )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/cmake/bgfx/geometryv.cmake` around lines 12 - 14, The GLOB_RECURSE for GEOMETRYV_SOURCES is using a broad "*" which may include unintended files; change the GLOB_RECURSE call that defines GEOMETRYV_SOURCES to explicitly match source file extensions (e.g., use patterns like *.cpp and *.h or whatever extensions the other tool scripts use) under the ${BGFX_DIR}/tools/geometryv directory so it mirrors geometryc.cmake/bin2c.cmake patterns and avoids grabbing non-source files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/native/thirdparty/cmake/bgfx/3rdparty/tint.cmake`:
- Around line 57-68: The CMake target_compile_definitions for target "tint"
currently sets TINT_BUILD_WGSL_READER=0 while WGSL sources are included; change
the definition for TINT_BUILD_WGSL_READER to 1 so the WGSL reader is compiled
and can parse WGSL input. Keep the other TINT_BUILD_* definitions as-is (e.g.,
TINT_BUILD_WGSL_WRITER, TINT_BUILD_SPV_READER) and ensure the modification is
made in the target_compile_definitions block that defines tint.
In `@engine/native/thirdparty/cmake/bgfx/bgfx.cmake`:
- Around line 46-50: The target_compile_definitions call for the bgfx target is
setting BGFX_SHARED_LIB_BUILD as PUBLIC which wrongly propagates the build-only
macro to dependents; change the target_compile_definitions(bgfx PUBLIC
BGFX_SHARED_LIB_BUILD=1) to use PRIVATE instead so BGFX_SHARED_LIB_BUILD is
defined only when building the bgfx shared library (i.e., update the branch
handling BGFX_LIBRARY_TYPE == SHARED to call target_compile_definitions(bgfx
PRIVATE BGFX_SHARED_LIB_BUILD=1)).
- Around line 151-156: The iOS/tvOS branch currently passes all frameworks as
one quoted string to target_link_libraries (when CMAKE_SYSTEM_NAME MATCHES
iOS|tvOS) which CMake treats as a single item; update this block to mirror the
macOS pattern by calling find_library() (or add each framework as a separate
argument) for each framework (OpenGLES, Metal, UIKit, CoreGraphics, QuartzCore,
IOKit, CoreFoundation) and then pass the resulting variables individually to
target_link_libraries(bgfx PUBLIC ...) instead of a single quoted string so the
link line is generated correctly.
In `@engine/native/thirdparty/cmake/bgfx/examples.cmake`:
- Around line 39-95: The shader outputs are currently written under
${BGFX_DIR}/examples/... which modifies the submodule; change all OUTPUT
variables (DX11_OUTPUT, METAL_OUTPUT, ESSL_OUTPUT, GLSL_OUTPUT, SPIRV_OUTPUT,
WGSL_OUTPUT) to point into the build/binary tree (e.g. use
${CMAKE_BINARY_DIR}/bgfx/examples/runtime/shaders/... or
${CMAKE_CURRENT_BINARY_DIR}/shaders/...) instead of ${BGFX_DIR}, update the
corresponding set(...) calls used before invoking _bgfx_shaderc_parse, and
ensure any needed output directories are created (file(MAKE_DIRECTORY ...) for
those paths) so the parser writes artifacts into the binary tree rather than the
source bgfx submodule.
- Around line 107-110: The shader compilation custom_command creates
OUTPUT_FILES but they are not hooked to the example target so CMake will never
run the rule; attach the generated outputs to the example target
(example-${ARG_NAME}) by either calling target_sources(example-${ARG_NAME}
PRIVATE ${OUTPUT_FILES}) after the add_custom_command, or create a custom target
(e.g. add_custom_target(shaders-${ARG_NAME} DEPENDS ${OUTPUT_FILES})) and add a
target_link_libraries or add_dependencies(example-${ARG_NAME}
shaders-${ARG_NAME}) so that the example depends on the shader outputs and the
rules will be executed.
- Around line 162-165: The Android system libs are incorrectly linked with
INTERFACE to the static target example-${ARG_NAME}; change the scope to PRIVATE
so the static library itself links to android, EGL and GLESv2. In the ANDROID
branch adjust the target_link_libraries call for example-${ARG_NAME} from
INTERFACE to PRIVATE (while keeping the target_include_directories line for
${BGFX_DIR}/3rdparty/native_app_glue unchanged) so the native app glue headers
and Android system libraries are linked into the static example-common target
itself.
In `@engine/native/thirdparty/cmake/bgfx/util/ConfigureDebugging.cmake`:
- Around line 143-146: The code writes raw ${VALUE} into RESULT which can
produce invalid XML if VALUE contains &, < or >; update ConfigureDebugging.cmake
to escape XML entities before concatenation (e.g. create an escaped copy like
ESCAPED_VALUE via string(REPLACE) for "&" -> "&", "<" -> "<", ">" ->
">" and optionally '"'/"'"), then use ESCAPED_VALUE in the set(RESULT ...)
line instead of ${VALUE}; reference the variables VALUE, RESULT and OPTION in
the block that builds the <${OPTION}> element and add the escaping step
immediately before that concatenation.
In `@engine/native/thirdparty/cmake/bgfxToolUtils.cmake`:
- Around line 265-303: In _bgfx_geometryc_parse(), fix three wiring errors:
change the CCW block to test ARG_CCW (not ARG_QUALITY) and append "--cw" when
true; change the OBB block to append the "--obb" switch with the ARG_OBB value
(not "--mipskip"); and change the COMPRESS block to test ARG_COMPRESS (not
ARG_REF) and append the boolean switch "--compress" without a value (rather than
trying to pass "${ARG_COMPRESS}"); update the corresponding list(APPEND CLI ...)
calls for ARG_CCW, ARG_OBB, and ARG_COMPRESS accordingly.
- Around line 610-614: The else branch in bgfxToolUtils.cmake currently calls
message(error "shaderc: Unsupported platform") which CMake treats as a normal
status message and does not abort; update that call to use message(FATAL_ERROR
"shaderc: Unsupported platform") so configuration stops on unsupported
platforms, ensuring the unsupported-platform path in the shaderc selection logic
fails fast and avoids downstream undefined behavior.
- Around line 653-669: The add_custom_command call incorrectly prepends an extra
COMMAND token before the MKDIR_COMMANDS and COMMANDS variables causing "COMMAND
COMMAND ..." syntax; in the add_custom_command that builds using MKDIR_COMMANDS
and COMMANDS (see variables MKDIR_COMMANDS, COMMANDS and the add_custom_command
block), remove the literal COMMAND so that you pass the variables directly
(i.e., expand ${MKDIR_COMMANDS} ${COMMANDS} instead of COMMAND ${MKDIR_COMMANDS}
${COMMANDS}) so the already-included COMMAND entries are not duplicated.
In `@engine/native/thirdparty/cmake/bimg/bimg_encode.cmake`:
- Around line 69-87: The CMake probe reuses the same result variable and
overwrites per-file compile options, so only the first check runs and later
flags replace existing COMPILE_OPTIONS; for each check_cxx_compiler_flag call
use a unique result variable name (e.g. flag_supported_<sanitized_flag>) instead
of reusing flag_supported, keep using target_compile_options(bimg_encode PRIVATE
...) for global options, and in the per-file loop replace
set_source_files_properties(${file} PROPERTIES COMPILE_OPTIONS ${flag}) with
set_property(SOURCE ${file} APPEND PROPERTY COMPILE_OPTIONS ${flag}) so flags
are appended for C++ sources in BIMG_ENCODE_SOURCES rather than overwritten.
In `@engine/native/thirdparty/cmake/bx/bx.cmake`:
- Around line 117-123: The target_link_libraries calls for target "bx" use a mix
of keyword and plain signatures which triggers CMake CMP0023; change the two
plain calls that link CMAKE_THREAD_LIBS_INIT, dl and rt to use the keyword form
(e.g., PUBLIC) so they match the other lines that already use PUBLIC; update the
target_link_libraries(bx ${CMAKE_THREAD_LIBS_INIT} dl) and
target_link_libraries(bx rt) invocations to the keyword signature to avoid
configuration failure.
- Around line 98-100: The MSVC flags are being passed as a single argument
because the generator expression combines "/Zc:__cplusplus /Zc:preprocessor"
into one string; update the target_compile_options call so each MSVC flag is its
own generator-expression argument (i.e., call target_compile_options(bx PUBLIC
$<...:/Zc:__cplusplus> $<...:/Zc:preprocessor>) or separate
target_compile_options entries) so MSVC receives two distinct options; edit the
target_compile_options invocation that currently references the bx target to
split the flags accordingly.
In `@engine/native/thirdparty/cmake/Config.cmake.in`:
- Around line 24-28: The invocation of _bgfx_crosscompile_use_host_tool
incorrectly exposes the geometry viewer tool; change the call that currently
references geometryv to geometryc so that the geometry compiler tool
(bgfx::geometryc) is used by bgfx_compile_geometry(); update the
_bgfx_crosscompile_use_host_tool(geometryv) line to
_bgfx_crosscompile_use_host_tool(geometryc) to restore correct geometry
compilation in cross-compiled/installed packages.
- Around line 13-20: After the find_program() call, guard creation of the
imported target by verifying the tool was actually found: check the
${TOOL_NAME}_EXECUTABLE result (e.g. ensure it is not equal to
"${TOOL_NAME}-NOTFOUND" and optionally that the path exists with EXISTS
"${${TOOL_NAME}_EXECUTABLE}") before calling add_executable(bgfx::${TOOL_NAME}
IMPORTED) and set_target_properties(... IMPORTED_LOCATION
"${${TOOL_NAME}_EXECUTABLE}"); only create the bgfx::${TOOL_NAME} target when
the check passes so missing tools don't produce bogus IMPORTED targets.
In `@engine/native/thirdparty/cmake/version.cmake`:
- Around line 13-27: The execute_process calls that populate GIT_REV and
GIT_REV_COUNT are fragile: they pass a quoted format arg so Git sees literal
single quotes and they assume Git and the bgfx .git exist, leaving variables
empty if commands fail. Initialize safe defaults (e.g., "unknown" or "0") for
GIT_REV and GIT_REV_COUNT before running execute_process, call execute_process
with separate ARGS (no embedded single quotes) or use --pretty=format:%h (no
shell quotes) and check the RESULT_VARIABLE from execute_process; only overwrite
GIT_REV and GIT_REV_COUNT when RESULT is 0 and OUTPUT_VARIABLE is non-empty, and
ensure you reference GIT_EXECUTABLE and the bgfx working directory consistently
when performing these checks.
---
Minor comments:
In `@engine/native/thirdparty/cmake/bgfx/3rdparty/fcpp.cmake`:
- Around line 28-29: FCPP_SOURCES contains a duplicated entry for cpp6.c; remove
the duplicate line so cpp6.c appears only once or deduplicate the list after
population (e.g., call list(REMOVE_DUPLICATES FCPP_SOURCES)) to ensure the same
translation unit isn't compiled twice; update the FCPP_SOURCES definition where
cpp6.c is listed twice to reference cpp6.c only once or add the dedupe call
immediately after that variable is set.
In `@engine/native/thirdparty/cmake/bgfx/3rdparty/glsl-optimizer.cmake`:
- Around line 12-15: The BGFX_DIR guard uses an unquoted variable in the IF
condition and message; update the guard to quote the variable everywhere it’s
used (e.g., change IF(NOT IS_DIRECTORY ${BGFX_DIR}) to IF(NOT IS_DIRECTORY
"${BGFX_DIR}") and include quotes around ${BGFX_DIR} in the message call) so
argument parsing is consistent and the check/logging remain robust; adjust the
condition and the message that references BGFX_DIR accordingly.
In `@engine/native/thirdparty/cmake/bgfx/3rdparty/glslang.cmake`:
- Around line 46-51: The comment next to the ENABLE_OPT compile definition
contains a typo ("spriv-tools"); update the comment in the
target_compile_definitions block for glslang so the comment reads "spirv-tools"
instead, preserving the existing PRIVATE and definitions (ENABLE_OPT=1,
ENABLE_HLSL=1) and formatting inside the target_compile_definitions call.
- Around line 11-15: The glslang third-party CMake include is missing the
standard early-return guard; add a guard at the top that checks if the glslang
target already exists (e.g., if(TARGET glslang) return()) so the file exits
immediately when the glslang target is present, matching the pattern used by
other 3rdparty modules and preventing duplicate target definitions; place this
check before any target or directory logic (reference the glslang target name
"glslang" and the current directory-check block).
In `@engine/native/thirdparty/cmake/bgfx/3rdparty/spirv-cross.cmake`:
- Around line 11-15: Add an early-return guard to avoid defining the spirv-cross
target twice: before the existing BGFX_DIR directory check, test for the
spirv-cross target (e.g., using if(TARGET spirv-cross) or a module-specific
guard variable) and return immediately if it already exists; keep the BGFX_DIR
existence check and message(SEND_ERROR ...) as-is but only run them when the
spirv-cross target is not present so repeated includes do not cause duplicate
target errors.
In `@engine/native/thirdparty/cmake/bgfx/geometryc.cmake`:
- Line 24: The compile definition for target_compile_definitions on target
geometryc is using a redundant "-D" prefix; change the call to pass the macro
name without the "-D" so CMake can add the proper prefix (i.e., replace
"-D_CRT_SECURE_NO_WARNINGS" with _CRT_SECURE_NO_WARNINGS in the
target_compile_definitions invocation for geometryc).
In `@engine/native/thirdparty/cmake/bgfx/texturev.cmake`:
- Around line 12-14: The glob for TEXTUREV_SOURCES is too broad and may pick up
non-source files; restrict the pattern used for ${BGFX_DIR}/tools/texturev to
only source/header files (e.g., *.cpp and *.h or whatever extensions used by
other modules) so the build excludes docs/configs; update the TEXTUREV_SOURCES
file(GLOB_RECURSE ...) invocation in texturev.cmake to mirror the filtered
patterns used by texturec.cmake and reference ${BGFX_DIR}/tools/texturev and the
TEXTUREV_SOURCES variable when making the change.
In `@engine/native/thirdparty/cmake/bimg/bimg.cmake`:
- Around line 11-15: Add the standard early-return guard for the bimg target to
prevent duplicate-target errors: at the top of this CMake snippet (before using
BIMG_DIR) insert an if(TARGET bimg) return() endif() block so that if the bimg
target already exists the script exits early; reference the TARGET bimg symbol
and ensure the guard is placed prior to the existing IS_DIRECTORY check and
message(SEND_ERROR ...) logic.
---
Nitpick comments:
In `@cmake/Modules.cmake`:
- Around line 96-101: The enable_modules function currently only sets
CXX_SCAN_FOR_MODULES and is inconsistent with other targets (e.g., boost_ut_main
and library targets) that also set CXX_EXTENSIONS OFF; update enable_modules to
set both CXX_SCAN_FOR_MODULES ON and CXX_EXTENSIONS OFF so targets using
enable_modules behave the same, or alternatively apply CXX_EXTENSIONS OFF
globally and remove per-target settings—pick one approach and make the change in
the enable_modules function (or the global CMake settings) to ensure consistent
module-related properties.
- Around line 85-86: Unit test executable targets created with
add_executable(${UNIT_TEST_TARGET} ${UNIT_TEST_FILE}) only set
CXX_SCAN_FOR_MODULES ON; update the target properties for ${UNIT_TEST_TARGET}
(via set_target_properties) to also set CXX_EXTENSIONS OFF so module behavior
matches the library targets and boost_ut_main — i.e., modify the
set_target_properties call referencing ${UNIT_TEST_TARGET} to include both
CXX_SCAN_FOR_MODULES ON and CXX_EXTENSIONS OFF.
In `@CMakeLists.txt`:
- Around line 11-24: The comparison currently treats RESULT_VARIABLE
GIT_SUBMOD_RESULT as a string (if(NOT GIT_SUBMOD_RESULT EQUAL "0")); update the
condition to compare it as an integer by using if(NOT GIT_SUBMOD_RESULT EQUAL 0)
(or an equivalent integer comparison) so that the execute_process
RESULT_VARIABLE check uses the proper integer comparison for GIT_SUBMOD_RESULT
in the submodule update block.
In `@engine/native/thirdparty/cmake/bgfx/3rdparty/fcpp.cmake`:
- Around line 12-15: The IS_DIRECTORY conditional should quote the BGFX_DIR
variable to avoid unquoted variable pitfalls; update the condition from using
IS_DIRECTORY ${BGFX_DIR} to IS_DIRECTORY "${BGFX_DIR}" (leave the
message(SEND_ERROR ...) and return() intact) so the check uses the quoted
BGFX_DIR variable.
In `@engine/native/thirdparty/cmake/bgfx/geometryv.cmake`:
- Around line 12-14: The GLOB_RECURSE for GEOMETRYV_SOURCES is using a broad "*"
which may include unintended files; change the GLOB_RECURSE call that defines
GEOMETRYV_SOURCES to explicitly match source file extensions (e.g., use patterns
like *.cpp and *.h or whatever extensions the other tool scripts use) under the
${BGFX_DIR}/tools/geometryv directory so it mirrors geometryc.cmake/bin2c.cmake
patterns and avoids grabbing non-source files.
In `@engine/native/thirdparty/cmake/bgfx/util/ConfigureDebugging.cmake`:
- Around line 115-117: Remove the empty conditional block that checks if(NOT
"${ARG_${ARG}_${CONFIG_CAP}_${PROCESSOR_CAP}}" STREQUAL "") by deleting the
entire if(...) ... endif() pair around ARG_${ARG}_${CONFIG_CAP}_${PROCESSOR_CAP}
in ConfigureDebugging.cmake so no-noop branches remain; ensure surrounding logic
and indentation are preserved and no other statements are accidentally removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c61ca48-e292-4411-a835-b6b479f9ac38
⛔ Files ignored due to path filters (3)
engine/native/thirdparty/cmake/bgfx/generated/bounds.cpp.inis excluded by!**/generated/**engine/native/thirdparty/cmake/bgfx/generated/shader.cpp.inis excluded by!**/generated/**engine/native/thirdparty/cmake/bgfx/generated/vertexlayout.cpp.inis excluded by!**/generated/**
📒 Files selected for processing (48)
.gitmodulesCMakeLists.txtcmake/Modules.cmakeengine/native/CMakeLists.txtengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/bgfxengine/native/thirdparty/bimgengine/native/thirdparty/bxengine/native/thirdparty/cmake/Config.cmake.inengine/native/thirdparty/cmake/bgfx/3rdparty/dear-imgui.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/fcpp.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/glsl-optimizer.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/glslang.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/meshoptimizer.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/spirv-cross.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/spirv-opt.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/tint.cmakeengine/native/thirdparty/cmake/bgfx/3rdparty/webgpu.cmakeengine/native/thirdparty/cmake/bgfx/CMakeLists.txtengine/native/thirdparty/cmake/bgfx/bgfx.cmakeengine/native/thirdparty/cmake/bgfx/examples.cmakeengine/native/thirdparty/cmake/bgfx/geometryc.cmakeengine/native/thirdparty/cmake/bgfx/geometryv.cmakeengine/native/thirdparty/cmake/bgfx/shaderc.cmakeengine/native/thirdparty/cmake/bgfx/shared.cmakeengine/native/thirdparty/cmake/bgfx/texturev.cmakeengine/native/thirdparty/cmake/bgfx/util/ConfigureDebugging.cmakeengine/native/thirdparty/cmake/bgfxToolUtils.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/astc_encoder.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/edtaa3.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/etc1.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/etc2.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/iqa.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/libsquish.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/loadpng.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/miniz.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/nvtt.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/pvrtc.cmakeengine/native/thirdparty/cmake/bimg/3rdparty/tinyexr.cmakeengine/native/thirdparty/cmake/bimg/CMakeLists.txtengine/native/thirdparty/cmake/bimg/bimg.cmakeengine/native/thirdparty/cmake/bimg/bimg_decode.cmakeengine/native/thirdparty/cmake/bimg/bimg_encode.cmakeengine/native/thirdparty/cmake/bimg/texturec.cmakeengine/native/thirdparty/cmake/bx/CMakeLists.txtengine/native/thirdparty/cmake/bx/bin2c.cmakeengine/native/thirdparty/cmake/bx/bx.cmakeengine/native/thirdparty/cmake/version.cmake
✅ Files skipped from review due to trivial changes (9)
- engine/native/thirdparty/bimg
- engine/native/CMakeLists.txt
- engine/native/thirdparty/bgfx
- .gitmodules
- engine/native/thirdparty/cmake/bimg/3rdparty/astc_encoder.cmake
- engine/native/thirdparty/cmake/bimg/3rdparty/iqa.cmake
- engine/native/thirdparty/cmake/bimg/3rdparty/tinyexr.cmake
- engine/native/thirdparty/cmake/bgfx/3rdparty/meshoptimizer.cmake
- engine/native/thirdparty/cmake/bimg/3rdparty/nvtt.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/native/thirdparty/CMakeLists.txt
| target_compile_definitions( | ||
| tint | ||
| PRIVATE TINT_BUILD_GLSL_WRITER=0 | ||
| TINT_BUILD_HLSL_WRITER=0 | ||
| TINT_BUILD_MSL_WRITER=0 | ||
| TINT_BUILD_NULL_WRITER=0 | ||
| TINT_BUILD_SPV_READER=1 | ||
| TINT_BUILD_SPV_WRITER=0 | ||
| TINT_BUILD_WGSL_READER=0 | ||
| TINT_BUILD_WGSL_WRITER=1 | ||
| TINT_ENABLE_IR_VALIDATION=0 | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how shaderc links and uses tint to verify the configuration is appropriate
rg -n "tint" engine/native/thirdparty/cmake/bgfx/shaderc.cmake 2>/dev/null || echo "shaderc.cmake not in provided files"Repository: Redot-Engine/DraconicEngine
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Read the shaderc.cmake file to understand how tint is used
cat -n engine/native/thirdparty/cmake/bgfx/shaderc.cmake | head -50Repository: Redot-Engine/DraconicEngine
Length of output: 1580
🏁 Script executed:
#!/bin/bash
# Search for any references to shader format writers/readers that are disabled
rg -i "(GLSL_WRITER|HLSL_WRITER|MSL_WRITER|WGSL_READER|SPV_READER|SPV_WRITER|WGSL_WRITER)" engine/native/thirdparty/cmake/bgfx/ --type cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 661
🏁 Script executed:
#!/bin/bash
# Check if there are any other tint-related cmake files or configurations
fd -e cmake . engine/native/thirdparty/cmake/bgfx/ | xargs grep -l "tint" 2>/dev/nullRepository: Redot-Engine/DraconicEngine
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Find and examine shaderc source files for tint usage
fd -e cpp -e h . engine/native/thirdparty/cmake/bgfx/ -path "*/tools/shaderc/*" | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 239
🏁 Script executed:
#!/bin/bash
# Search for actual tint API calls in bgfx source
rg "tint::" engine/native/thirdparty/cmake/bgfx/ --type cpp --type h -A 2 | head -100Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check if shaderc source is available in the expected location
find engine/native/thirdparty -name "shaderc.cpp" -o -name "shaderc.h" 2>/dev/nullRepository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check the bgfx directory structure
ls -la engine/native/thirdparty/cmake/bgfx/ 2>/dev/null | head -30Repository: Redot-Engine/DraconicEngine
Length of output: 873
🏁 Script executed:
#!/bin/bash
# Look for shader format references in cmake files
rg "spv|wgsl|glsl|hlsl|msl" engine/native/thirdparty/cmake/bgfx/ -i --type cmake | head -30Repository: Redot-Engine/DraconicEngine
Length of output: 2934
🏁 Script executed:
#!/bin/bash
# Check if BGFX_DIR points to an external directory and check the tint.cmake file itself
cat -n engine/native/thirdparty/cmake/bgfx/3rdparty/tint.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 3165
Enable WGSL_READER to support WebGPU Shading Language input compilation.
The current configuration disables TINT_BUILD_WGSL_READER=0 while including WGSL language sources in the build (lines 35-36: ${TINT_DIR}/src/tint/lang/wgsl/*.cc). The enabled WGSL_WRITER alone enables only output conversion, not input parsing. If the shader pipeline requires reading WGSL source files and converting them to SPIR-V (via the enabled SPV_READER and disabled SPV_WRITER path, or directly via WGSL to SPIR-V), then WGSL_READER should be enabled. Disabling GLSL/HLSL/MSL writers is appropriate since spirv-cross handles those output formats.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/bgfx/3rdparty/tint.cmake` around lines 57 -
68, The CMake target_compile_definitions for target "tint" currently sets
TINT_BUILD_WGSL_READER=0 while WGSL sources are included; change the definition
for TINT_BUILD_WGSL_READER to 1 so the WGSL reader is compiled and can parse
WGSL input. Keep the other TINT_BUILD_* definitions as-is (e.g.,
TINT_BUILD_WGSL_WRITER, TINT_BUILD_SPV_READER) and ensure the modification is
made in the target_compile_definitions block that defines tint.
There was a problem hiding this comment.
This is from bgfx.cmake so we won't modify it IMO
There was a problem hiding this comment.
@AR-DEV-1, understood! Since tint.cmake (and other files under engine/native/thirdparty/cmake/bgfx/) are sourced from the upstream bgfx.cmake project, modifying them would create divergence from the upstream and complicate future updates. I'll keep this in mind for future reviews.
(ᵔ◡ᵔ) <br>
(\ /)
( . .) upstream is upstream!
c(")(")
✏️ Learnings added
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 12
File: engine/native/thirdparty/cmake/bgfx/3rdparty/tint.cmake:57-68
Timestamp: 2026-03-31T03:45:48.454Z
Learning: In Redot-Engine/DraconicEngine, the CMake files under `engine/native/thirdparty/cmake/bgfx/` (including `3rdparty/tint.cmake`, `bgfx.cmake`, `shaderc.cmake`, etc.) are sourced from the upstream bgfx.cmake project and should not be modified. Do not suggest changes to these files as the maintainers intentionally keep them unmodified to stay in sync with upstream.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: OldDev78
Repo: Redot-Engine/DraconicEngine PR: 1
File: cmake/Modules.cmake:42-45
Timestamp: 2026-03-18T23:05:32.169Z
Learning: In `cmake/Modules.cmake` (Redot-Engine/DraconicEngine), the relative path variable `${LIB_PATH}` is intentionally used (instead of the absolute `${LIB_DIR}`) in `file(GLOB ...)` calls and `string(MAKE_C_IDENTIFIER ...)` because the relative path is needed to derive a clean CMake target name via `string(MAKE_C_IDENTIFIER ${LIB_PATH} LIB_TARGET)`. Do not suggest replacing `${LIB_PATH}` with `${LIB_DIR}` in these contexts.
| if(BGFX_LIBRARY_TYPE STREQUAL STATIC) | ||
| add_library(bgfx STATIC ${BGFX_SOURCES}) | ||
| else() | ||
| add_library(bgfx SHARED ${BGFX_SOURCES}) | ||
| target_compile_definitions(bgfx PUBLIC BGFX_SHARED_LIB_BUILD=1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
head -60 engine/native/thirdparty/cmake/bgfx/bgfx.cmake | tail -20Repository: Redot-Engine/DraconicEngine
Length of output: 667
🏁 Script executed:
# Search for BGFX_SHARED_LIB_BUILD usage across the codebase
rg "BGFX_SHARED_LIB_BUILD" --type cmake --type cpp --type h -B 2 -A 2Repository: Redot-Engine/DraconicEngine
Length of output: 428
🏁 Script executed:
# Look for bgfx header files that might use export/import guards
fd -e h -e hpp ".*bgfx.*" engine/native/thirdparty/ | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# Search for BGFX_SHARED_LIB_BUILD in all files, especially headers
rg "BGFX_SHARED_LIB_BUILD" -B 3 -A 3 --max-count 20Repository: Redot-Engine/DraconicEngine
Length of output: 596
🏁 Script executed:
# Look for bgfx header files that might use export guards
find engine/native/thirdparty -name "*.h" -o -name "*.hpp" | xargs grep -l "BGFX_SHARED_LIB_BUILD" 2>/dev/null | head -5Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# Check if there are any __declspec or export/import patterns related to bgfx
rg "dllexport|dllimport|__declspec" engine/native/thirdparty/ -B 2 -A 2 | head -40Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# Find bgfx source/include directories
find engine/native/thirdparty -type d -name "*bgfx*" | head -10Repository: Redot-Engine/DraconicEngine
Length of output: 136
🏁 Script executed:
# Look for any main bgfx header file
find engine/native/thirdparty -name "bfx.h" -o -name "bgfx.h" 2>/dev/nullRepository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# Check the bgfx.cmake file more completely to understand source locations
cat engine/native/thirdparty/cmake/bgfx/bgfx.cmake | head -80Repository: Redot-Engine/DraconicEngine
Length of output: 2577
🏁 Script executed:
# Search for BGFX_SHARED_LIB_BUILD in bgfx headers
find engine/native/thirdparty/bgfx/include -type f \( -name "*.h" -o -name "*.hpp" \) | xargs grep -l "BGFX_SHARED_LIB_BUILD" 2>/dev/nullRepository: Redot-Engine/DraconicEngine
Length of output: 145
🏁 Script executed:
# Check for export/import macros or visibility declarations in bgfx headers
grep -r "dllexport\|dllimport\|BGFX_SHARED_LIB_BUILD" engine/native/thirdparty/bgfx/include/ 2>/dev/null | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# Search in bgfx/include for any export or visibility macros
find engine/native/thirdparty/bgfx/include -name "*.h" -exec grep -l "export\|import\|SHARED_LIB" {} \; 2>/dev/nullRepository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# List the actual contents of the bgfx directory
ls -la engine/native/thirdparty/bgfx/ 2>/dev/null || echo "Directory not found"Repository: Redot-Engine/DraconicEngine
Length of output: 179
🏁 Script executed:
# Check if bgfx is a git submodule or symlink
ls -lR engine/native/thirdparty/bgfx/ | head -30Repository: Redot-Engine/DraconicEngine
Length of output: 110
🏁 Script executed:
# Find all files that mention BGFX_SHARED_LIB_BUILD
rg "BGFX_SHARED_LIB_BUILD" -lRepository: Redot-Engine/DraconicEngine
Length of output: 117
BGFX_SHARED_LIB_BUILD should be PRIVATE instead of PUBLIC.
This macro is only needed while building the bgfx shared library itself. Setting it as PUBLIC propagates it to all downstream targets that link against bgfx, causing them to compile with a macro intended only for bgfx's internal build. This can cause issues if bgfx headers use this macro for export/import symbol annotations. Compare to other compile definitions in the same file (e.g., BGFX_CONFIG_RENDERER_OPENGL_MIN_VERSION), which are correctly set as PRIVATE.
Suggested fix
else()
add_library(bgfx SHARED ${BGFX_SOURCES})
- target_compile_definitions(bgfx PUBLIC BGFX_SHARED_LIB_BUILD=1)
+ target_compile_definitions(bgfx PRIVATE BGFX_SHARED_LIB_BUILD=1)
endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(BGFX_LIBRARY_TYPE STREQUAL STATIC) | |
| add_library(bgfx STATIC ${BGFX_SOURCES}) | |
| else() | |
| add_library(bgfx SHARED ${BGFX_SOURCES}) | |
| target_compile_definitions(bgfx PUBLIC BGFX_SHARED_LIB_BUILD=1) | |
| if(BGFX_LIBRARY_TYPE STREQUAL STATIC) | |
| add_library(bgfx STATIC ${BGFX_SOURCES}) | |
| else() | |
| add_library(bgfx SHARED ${BGFX_SOURCES}) | |
| target_compile_definitions(bgfx PRIVATE BGFX_SHARED_LIB_BUILD=1) | |
| endif() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/bgfx/bgfx.cmake` around lines 46 - 50, The
target_compile_definitions call for the bgfx target is setting
BGFX_SHARED_LIB_BUILD as PUBLIC which wrongly propagates the build-only macro to
dependents; change the target_compile_definitions(bgfx PUBLIC
BGFX_SHARED_LIB_BUILD=1) to use PRIVATE instead so BGFX_SHARED_LIB_BUILD is
defined only when building the bgfx shared library (i.e., update the branch
handling BGFX_LIBRARY_TYPE == SHARED to call target_compile_definitions(bgfx
PRIVATE BGFX_SHARED_LIB_BUILD=1)).
| if(${CMAKE_SYSTEM_NAME} MATCHES iOS|tvOS) | ||
| target_link_libraries( | ||
| bgfx | ||
| PUBLIC | ||
| "-framework OpenGLES -framework Metal -framework UIKit -framework CoreGraphics -framework QuartzCore -framework IOKit -framework CoreFoundation" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the specific CMake file
find . -path "*/engine/native/thirdparty/cmake/bgfx/bgfx.cmake" -type fRepository: Redot-Engine/DraconicEngine
Length of output: 119
🏁 Script executed:
# Get context around the flagged lines
cat -n engine/native/thirdparty/cmake/bgfx/bgfx.cmake | head -170 | tail -40Repository: Redot-Engine/DraconicEngine
Length of output: 1807
🏁 Script executed:
# Check if there are other framework linking patterns in the codebase
rg -n "target_link_libraries" engine/native/thirdparty/cmake/bgfx/bgfx.cmake | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 351
🏁 Script executed:
# Look for other iOS/tvOS framework linking patterns in CMake files
rg -n "framework.*Metal.*OpenGLES\|OpenGLES.*Metal" --type cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# Check the full context of the iOS/tvOS section
sed -n '145,170p' engine/native/thirdparty/cmake/bgfx/bgfx.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 962
🌐 Web query:
CMake target_link_libraries framework string handling Apple
💡 Result:
In CMake, linking Apple frameworks (macOS/iOS) with target_link_libraries requires special string handling to avoid linker errors like 'ld: library not found for -lFrameworkName'. Key methods: 1. Use find_library (recommended for portability): find_library(COCOA_LIB Cocoa) target_link_libraries(target PRIVATE
Citations:
- 1: https://cmake-developers.cmake.narkive.com/QwonvUsd/linking-apple-frameworks
- 2: https://stackoverflow.com/questions/17070101/why-i-cannot-link-the-mac-framework-file-with-cmake
- 3: https://cmake.org/cmake/help/latest/command/target_link_libraries.html
- 4: https://www.codestudy.net/blog/why-i-cannot-link-the-mac-framework-file-with-cmake/
- 5: https://scivision.dev/cmake-macos-framework-link
- 6: https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_FRAMEWORK.html
🌐 Web query:
CMake target_link_libraries separate arguments vs single string
💡 Result:
In CMake's target_link_libraries command, libraries and flags should be passed as separate arguments rather than as a single quoted string. The command parses each unquoted argument as an individual <item>, which can be a library target name, plain library name (e.g., "foo" becomes -lfoo), full path to a library file, link flag (starting with - but not -l or -framework), or generator expression. Separate arguments: target_link_libraries(myexe lib1 lib2 /path/to/lib3 -lfoo -framework Bar) Single string: target_link_libraries(myexe "lib1 lib2") # Treated as ONE item: literal string "lib1 lib2", linker searches for library named "lib1 lib2" (likely fails) Key reasons from official docs: - Plain library names and link flags are "treated as a command-line string fragment and will be used with no extra quoting or escaping" only when passed as separate arguments. CMake then transforms them appropriately (e.g., "foo" -> -lfoo). - A single quoted string containing spaces or semicolons is received as ONE item and passed literally to the linker without transformation or splitting. - For lists with semicolons (e.g., from variables), use generator expressions like "$<...>" to pass as a single item that expands to multiple items. - Generator expressions with spaces must be quoted to be treated as single arguments. This behavior is consistent across CMake versions (3.0+ to latest 4.3), as documented in the official target_link_libraries manual. Using separate arguments ensures proper linker flag generation and avoids shell quoting issues.
Citations:
- 1: https://cmake.org/cmake/help/latest/command/target_link_libraries.html
- 2: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html
- 3: https://www.typeerror.org/docs/cmake~3.15/command/target_link_libraries
🏁 Script executed:
# Check CMake documentation context and test understanding
python3 << 'EOF'
# CMake target_link_libraries documentation indicates:
# The command signature is: target_link_libraries(<target> [item1 [item2 [...]]]...)
# Each ITEM should be a separate argument
# When you pass "-framework A -framework B" as one string, CMake treats it as one item
# This is problematic because:
# 1. The linker receives the entire string as one argument
# 2. Different linkers may handle this differently
# 3. Proper CMake practice requires each library/framework as separate argument
print("CMake target_link_libraries expects each library as separate argument")
print("Passing multiple frameworks in one quoted string violates this pattern")
print("The macOS section in the same file (lines 157-170) correctly demonstrates")
print("the right approach: use find_library() + separate arguments")
EOFRepository: Redot-Engine/DraconicEngine
Length of output: 345
Split the frameworks into separate arguments or use find_library() for each, matching the macOS pattern.
Passing all frameworks as a single quoted string violates CMake conventions. CMake treats "-framework A -framework B ..." as one item rather than multiple separate arguments. The macOS branch (lines 157-170) correctly demonstrates the proper approach using find_library() for each framework. The iOS/tvOS branch should follow the same pattern to ensure proper link line generation and consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/bgfx/bgfx.cmake` around lines 151 - 156, The
iOS/tvOS branch currently passes all frameworks as one quoted string to
target_link_libraries (when CMAKE_SYSTEM_NAME MATCHES iOS|tvOS) which CMake
treats as a single item; update this block to mirror the macOS pattern by
calling find_library() (or add each framework as a separate argument) for each
framework (OpenGLES, Metal, UIKit, CoreGraphics, QuartzCore, IOKit,
CoreFoundation) and then pass the resulting variables individually to
target_link_libraries(bgfx PUBLIC ...) instead of a single quoted string so the
link line is generated correctly.
| set(DX11_OUTPUT ${BGFX_DIR}/examples/runtime/shaders/dx11/${FILENAME}.bin) | ||
| if(NOT "${TYPE}" STREQUAL "COMPUTE") | ||
| _bgfx_shaderc_parse( | ||
| DX11 ${COMMON} WINDOWS | ||
| PROFILE s_5_0 | ||
| O 3 | ||
| OUTPUT ${DX11_OUTPUT} | ||
| ) | ||
| else() | ||
| _bgfx_shaderc_parse( | ||
| DX11 ${COMMON} WINDOWS | ||
| PROFILE s_5_0 | ||
| O 1 | ||
| OUTPUT ${DX11_OUTPUT} | ||
| ) | ||
| endif() | ||
| list(APPEND OUTPUTS "DX11") | ||
| set(OUTPUTS_PRETTY "${OUTPUTS_PRETTY}DX11, ") | ||
| endif() | ||
|
|
||
| if(APPLE) | ||
| # metal | ||
| set(METAL_OUTPUT ${BGFX_DIR}/examples/runtime/shaders/metal/${FILENAME}.bin) | ||
| _bgfx_shaderc_parse(METAL ${COMMON} OSX PROFILE metal OUTPUT ${METAL_OUTPUT}) | ||
| list(APPEND OUTPUTS "METAL") | ||
| set(OUTPUTS_PRETTY "${OUTPUTS_PRETTY}Metal, ") | ||
| endif() | ||
|
|
||
| # essl | ||
| if(NOT "${TYPE}" STREQUAL "COMPUTE") | ||
| set(ESSL_OUTPUT ${BGFX_DIR}/examples/runtime/shaders/essl/${FILENAME}.bin) | ||
| _bgfx_shaderc_parse(ESSL ${COMMON} ANDROID PROFILE 100_es OUTPUT ${ESSL_OUTPUT}) | ||
| list(APPEND OUTPUTS "ESSL") | ||
| set(OUTPUTS_PRETTY "${OUTPUTS_PRETTY}ESSL, ") | ||
| endif() | ||
|
|
||
| # glsl | ||
| set(GLSL_OUTPUT ${BGFX_DIR}/examples/runtime/shaders/glsl/${FILENAME}.bin) | ||
| if(NOT "${TYPE}" STREQUAL "COMPUTE") | ||
| _bgfx_shaderc_parse(GLSL ${COMMON} LINUX PROFILE 140 OUTPUT ${GLSL_OUTPUT}) | ||
| else() | ||
| _bgfx_shaderc_parse(GLSL ${COMMON} LINUX PROFILE 430 OUTPUT ${GLSL_OUTPUT}) | ||
| endif() | ||
| list(APPEND OUTPUTS "GLSL") | ||
| set(OUTPUTS_PRETTY "${OUTPUTS_PRETTY}GLSL, ") | ||
|
|
||
| # spirv | ||
| if(NOT "${TYPE}" STREQUAL "COMPUTE") | ||
| set(SPIRV_OUTPUT ${BGFX_DIR}/examples/runtime/shaders/spirv/${FILENAME}.bin) | ||
| _bgfx_shaderc_parse(SPIRV ${COMMON} LINUX PROFILE spirv OUTPUT ${SPIRV_OUTPUT}) | ||
| list(APPEND OUTPUTS "SPIRV") | ||
| set(OUTPUTS_PRETTY "${OUTPUTS_PRETTY}SPIRV, ") | ||
| endif() | ||
|
|
||
| # wgsl | ||
| set(WGSL_OUTPUT ${BGFX_DIR}/examples/runtime/shaders/wgsl/${FILENAME}.bin) | ||
| _bgfx_shaderc_parse(WGSL ${COMMON} LINUX PROFILE wgsl OUTPUT ${WGSL_OUTPUT}) |
There was a problem hiding this comment.
Don't generate example shaders into ${BGFX_DIR}.
These outputs go into the bgfx submodule checkout under examples/runtime/shaders, which dirties the source tree and makes multiple build directories stomp each other's artifacts. Please emit them under the binary tree instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/bgfx/examples.cmake` around lines 39 - 95, The
shader outputs are currently written under ${BGFX_DIR}/examples/... which
modifies the submodule; change all OUTPUT variables (DX11_OUTPUT, METAL_OUTPUT,
ESSL_OUTPUT, GLSL_OUTPUT, SPIRV_OUTPUT, WGSL_OUTPUT) to point into the
build/binary tree (e.g. use
${CMAKE_BINARY_DIR}/bgfx/examples/runtime/shaders/... or
${CMAKE_CURRENT_BINARY_DIR}/shaders/...) instead of ${BGFX_DIR}, update the
corresponding set(...) calls used before invoking _bgfx_shaderc_parse, and
ensure any needed output directories are created (file(MAKE_DIRECTORY ...) for
those paths) so the parser writes artifacts into the binary tree rather than the
source bgfx submodule.
| add_custom_command( | ||
| MAIN_DEPENDENCY ${FILE} OUTPUT ${OUTPUT_FILES} ${COMMANDS} | ||
| COMMENT "Compiling shader ${PRINT_NAME} for ${OUTPUTS_PRETTY}" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l engine/native/thirdparty/cmake/bgfx/examples.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 125
🏁 Script executed:
sed -n '90,130p' engine/native/thirdparty/cmake/bgfx/examples.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 1424
🏁 Script executed:
cat -n engine/native/thirdparty/cmake/bgfx/examples.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 13209
Shader compilation rules are orphaned and won't execute.
add_custom_command(OUTPUT ...) defines how shader binaries should be compiled but doesn't attach them to any target. The example target at example-${ARG_NAME} never depends on ${OUTPUT_FILES}, so CMake won't schedule shader compilation. To fix this, either add the outputs to the target via target_sources() or create a custom target that the example depends on.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/bgfx/examples.cmake` around lines 107 - 110,
The shader compilation custom_command creates OUTPUT_FILES but they are not
hooked to the example target so CMake will never run the rule; attach the
generated outputs to the example target (example-${ARG_NAME}) by either calling
target_sources(example-${ARG_NAME} PRIVATE ${OUTPUT_FILES}) after the
add_custom_command, or create a custom target (e.g.
add_custom_target(shaders-${ARG_NAME} DEPENDS ${OUTPUT_FILES})) and add a
target_link_libraries or add_dependencies(example-${ARG_NAME}
shaders-${ARG_NAME}) so that the example depends on the shader outputs and the
rules will be executed.
| target_compile_features(bx PUBLIC cxx_std_14) | ||
| # (note: see bx\scripts\toolchain.lua for equivalent compiler flag) | ||
| target_compile_options(bx PUBLIC $<$<CXX_COMPILER_ID:MSVC>:/Zc:__cplusplus /Zc:preprocessor>) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "bx.cmake" -type fRepository: Redot-Engine/DraconicEngine
Length of output: 115
🌐 Web query:
CMake target_compile_options generator expression space-separated flags list semantics
💡 Result:
In CMake, target_compile_options adds items to the target's COMPILE_OPTIONS or INTERFACE_COMPILE_OPTIONS properties, which are semicolon-separated lists of compile options. Generator expressions (
Citations:
- 1: https://cmake.org/cmake/help/latest/command/target_compile_options.html
- 2: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html
- 3: https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_OPTIONS.html
- 4: https://stackoverflow.com/questions/65035750/cmake-generator-expressions-with-multiple-space-separated-entries
- 5: https://stackoverflow.com/questions/61433216/why-does-cmake-split-arguments-differently-for-add-compile-options-and-target-co
- 6: https://cmake.org/cmake/help/v3.25/guide/tutorial/Adding%20Generator%20Expressions.html
🏁 Script executed:
sed -n '95,105p' ./engine/native/thirdparty/cmake/bx/bx.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 494
🏁 Script executed:
sed -n '95,130p' ./engine/native/thirdparty/cmake/bx/bx.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 1129
Split the MSVC /Zc: flags into separate compile options.
The generator expression expands to a single string /Zc:__cplusplus /Zc:preprocessor, which CMake passes to MSVC as one argument containing an embedded space. MSVC will not recognize this as two separate flags and will fail to parse it correctly.
Suggested fix
target_compile_features(bx PUBLIC cxx_std_14)
# (note: see bx\scripts\toolchain.lua for equivalent compiler flag)
-target_compile_options(bx PUBLIC $<$<CXX_COMPILER_ID:MSVC>:/Zc:__cplusplus /Zc:preprocessor>)
+target_compile_options(
+ bx
+ PUBLIC
+ $<$<CXX_COMPILER_ID:MSVC>:/Zc:__cplusplus>
+ $<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor>
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_compile_features(bx PUBLIC cxx_std_14) | |
| # (note: see bx\scripts\toolchain.lua for equivalent compiler flag) | |
| target_compile_options(bx PUBLIC $<$<CXX_COMPILER_ID:MSVC>:/Zc:__cplusplus /Zc:preprocessor>) | |
| target_compile_features(bx PUBLIC cxx_std_14) | |
| # (note: see bx\scripts\toolchain.lua for equivalent compiler flag) | |
| target_compile_options( | |
| bx | |
| PUBLIC | |
| $<$<CXX_COMPILER_ID:MSVC>:/Zc:__cplusplus> | |
| $<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor> | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/bx/bx.cmake` around lines 98 - 100, The MSVC
flags are being passed as a single argument because the generator expression
combines "/Zc:__cplusplus /Zc:preprocessor" into one string; update the
target_compile_options call so each MSVC flag is its own generator-expression
argument (i.e., call target_compile_options(bx PUBLIC $<...:/Zc:__cplusplus>
$<...:/Zc:preprocessor>) or separate target_compile_options entries) so MSVC
receives two distinct options; edit the target_compile_options invocation that
currently references the bx target to split the flags accordingly.
| elseif(UNIX) | ||
| # Threads | ||
| find_package(Threads) | ||
| target_link_libraries(bx ${CMAKE_THREAD_LIBS_INIT} dl) | ||
|
|
||
| # Real time (for clock_gettime) | ||
| target_link_libraries(bx rt) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "bx.cmake" -type fRepository: Redot-Engine/DraconicEngine
Length of output: 115
🏁 Script executed:
wc -l ./engine/native/thirdparty/cmake/bx/bx.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 119
🏁 Script executed:
sed -n '100,125p' ./engine/native/thirdparty/cmake/bx/bx.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 769
🏁 Script executed:
cat -n ./engine/native/thirdparty/cmake/bx/bx.cmake | sed -n '100,130p'Repository: Redot-Engine/DraconicEngine
Length of output: 1107
🌐 Web query:
CMake target_link_libraries mixed signature error keyword form policy
💡 Result:
The "mixed signature error" for CMake's target_link_libraries command occurs when mixing the plain signature (e.g., target_link_libraries(target lib1 lib2)) with the keyword signature (e.g., target_link_libraries(target PRIVATE lib3)) for the same target. This triggers an error like: "The keyword signature for target_link_libraries has already been used with the target. All uses of target_link_libraries with a target must be either all-keyword or all-plain." This behavior is controlled by CMake Policy CMP0023, introduced in CMake 2.8.12. The policy enforces that plain and keyword signatures cannot be mixed to avoid subtle bugs in link interface propagation. - OLD behavior (default if policy unset in versions up to at least 3.23): Allows mixing but warns. - NEW behavior: Errors on mixing. In modern CMake (3.0+), unsetting the policy typically results in an error during configuration when mixing occurs, as the default handling prefers the stricter behavior. To fix: 1. Use only keyword form consistently: target_link_libraries(target PRIVATE/PUBLIC/INTERFACE items...) 2. Or use only plain form: target_link_libraries(target items...) Note: Plain form treats links as PUBLIC-like but lacks modern transitive usage requirements. Prefer the keyword form (modern signature) for better dependency transitivity and usage requirements. To explicitly control: cmake_policy(SET CMP0023 NEW) or OLD, or set cmake_minimum_required(VERSION 3.0) which implicitly sets policies. The policy remains active (not superseded) as of CMake 3.23+, per policies list.
Citations:
- 1: https://cmake.org/cmake/help/v3.6/policy/CMP0023.html
- 2: https://cmake.org/cmake/help/v3.7/policy/CMP0023.html
- 3: https://cmake.org/cmake/help/latest/command/target_link_libraries.html
- 4: https://cmake.org/cmake/help/v3.23/manual/cmake-policies.7.html
- 5: https://stackoverflow.com/questions/59522267/cmake-rejects-a-second-target-link-libraries-talking-about-keyword-vs-plain/59522268
- 6: https://cmake.org/cmake/help/v3.0/policy/CMP0023.html
- 7: https://stackoverflow.com/questions/59522267/cmake-rejects-a-second-target-link-libraries-talking-about-keyword-vs-plain
Use keyword form consistently for target_link_libraries(bx ...).
Lines 104, 112, and 116 already use the PUBLIC keyword signature, but lines 120 and 123 use the plain signature. CMake Policy CMP0023 (active in modern CMake 3.0+) enforces that all target_link_libraries calls for the same target must use either all-keyword or all-plain form. Mixing signatures will cause configuration failure with: "The keyword signature for target_link_libraries has already been used with the target."
Suggested fix
elseif(UNIX)
# Threads
find_package(Threads)
- target_link_libraries(bx ${CMAKE_THREAD_LIBS_INIT} dl)
+ target_link_libraries(bx PUBLIC ${CMAKE_THREAD_LIBS_INIT} dl)
# Real time (for clock_gettime)
- target_link_libraries(bx rt)
+ target_link_libraries(bx PUBLIC rt)
endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elseif(UNIX) | |
| # Threads | |
| find_package(Threads) | |
| target_link_libraries(bx ${CMAKE_THREAD_LIBS_INIT} dl) | |
| # Real time (for clock_gettime) | |
| target_link_libraries(bx rt) | |
| elseif(UNIX) | |
| # Threads | |
| find_package(Threads) | |
| target_link_libraries(bx PUBLIC ${CMAKE_THREAD_LIBS_INIT} dl) | |
| # Real time (for clock_gettime) | |
| target_link_libraries(bx PUBLIC rt) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/bx/bx.cmake` around lines 117 - 123, The
target_link_libraries calls for target "bx" use a mix of keyword and plain
signatures which triggers CMake CMP0023; change the two plain calls that link
CMAKE_THREAD_LIBS_INIT, dl and rt to use the keyword form (e.g., PUBLIC) so they
match the other lines that already use PUBLIC; update the
target_link_libraries(bx ${CMAKE_THREAD_LIBS_INIT} dl) and
target_link_libraries(bx rt) invocations to the keyword signature to avoid
configuration failure.
| if(NOT TARGET bgfx::${TOOL_NAME}) | ||
| find_program( | ||
| ${TOOL_NAME}_EXECUTABLE | ||
| NAMES bgfx-${TOOL_NAME} ${TOOL_NAME} | ||
| PATHS @BGFX_ADDITIONAL_TOOL_PATHS@ /usr/bin | ||
| ) | ||
| add_executable(bgfx::${TOOL_NAME} IMPORTED) | ||
| set_target_properties(bgfx::${TOOL_NAME} PROPERTIES IMPORTED_LOCATION "${${TOOL_NAME}_EXECUTABLE}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the Config.cmake.in file to see the exact code
fd -t f "Config.cmake.in" engine/native/thirdparty/cmake/Repository: Redot-Engine/DraconicEngine
Length of output: 117
🏁 Script executed:
# Read the file to see the context around lines 13-20
cat -n engine/native/thirdparty/cmake/Config.cmake.in | head -40Repository: Redot-Engine/DraconicEngine
Length of output: 1512
🏁 Script executed:
# Find bgfxToolUtils.cmake to see how these targets are used
fd -t f "bgfxToolUtils.cmake" engine/native/Repository: Redot-Engine/DraconicEngine
Length of output: 121
🏁 Script executed:
# Search for references to the tool targets (geometryc, etc.) in bgfxToolUtils.cmake
rg -n "bgfx::" engine/native/thirdparty/cmake/bgfxToolUtils.cmake -A 2 -B 2Repository: Redot-Engine/DraconicEngine
Length of output: 1498
Create host-tool targets only after find_program() succeeds.
When find_program() fails to locate a tool, it sets ${TOOL_NAME}_EXECUTABLE to *-NOTFOUND, but this macro still creates bgfx::${TOOL_NAME} and sets its IMPORTED_LOCATION to that non-existent path. This causes if(TARGET bgfx::...) guards in bgfxToolUtils.cmake to pass, deferring the failure to build time when the tool is actually invoked, making the real issue harder to diagnose. Add a check after find_program() to ensure the tool was found before creating the target.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/Config.cmake.in` around lines 13 - 20, After
the find_program() call, guard creation of the imported target by verifying the
tool was actually found: check the ${TOOL_NAME}_EXECUTABLE result (e.g. ensure
it is not equal to "${TOOL_NAME}-NOTFOUND" and optionally that the path exists
with EXISTS "${${TOOL_NAME}_EXECUTABLE}") before calling
add_executable(bgfx::${TOOL_NAME} IMPORTED) and set_target_properties(...
IMPORTED_LOCATION "${${TOOL_NAME}_EXECUTABLE}"); only create the
bgfx::${TOOL_NAME} target when the check passes so missing tools don't produce
bogus IMPORTED targets.
| _bgfx_crosscompile_use_host_tool(bin2c) | ||
| _bgfx_crosscompile_use_host_tool(texturec) | ||
| _bgfx_crosscompile_use_host_tool(shaderc) | ||
| _bgfx_crosscompile_use_host_tool(texturev) | ||
| _bgfx_crosscompile_use_host_tool(geometryv) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n engine/native/thirdparty/cmake/Config.cmake.in | sed -n '20,35p'Repository: Redot-Engine/DraconicEngine
Length of output: 591
🏁 Script executed:
rg "bgfx_compile_geometry" -A 10 -B 2Repository: Redot-Engine/DraconicEngine
Length of output: 1909
🏁 Script executed:
rg "geometryc|geometryv" --type cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 3380
Replace geometryv with geometryc on line 28.
bgfx_compile_geometry() requires bgfx::geometryc (the geometry compiler tool), but line 28 currently exposes geometryv (the geometry viewer). This breaks geometry compilation in cross-compiled or installed packages.
Suggested fix
_bgfx_crosscompile_use_host_tool(bin2c)
_bgfx_crosscompile_use_host_tool(texturec)
_bgfx_crosscompile_use_host_tool(shaderc)
_bgfx_crosscompile_use_host_tool(texturev)
-_bgfx_crosscompile_use_host_tool(geometryv)
+_bgfx_crosscompile_use_host_tool(geometryc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _bgfx_crosscompile_use_host_tool(bin2c) | |
| _bgfx_crosscompile_use_host_tool(texturec) | |
| _bgfx_crosscompile_use_host_tool(shaderc) | |
| _bgfx_crosscompile_use_host_tool(texturev) | |
| _bgfx_crosscompile_use_host_tool(geometryv) | |
| _bgfx_crosscompile_use_host_tool(bin2c) | |
| _bgfx_crosscompile_use_host_tool(texturec) | |
| _bgfx_crosscompile_use_host_tool(shaderc) | |
| _bgfx_crosscompile_use_host_tool(texturev) | |
| _bgfx_crosscompile_use_host_tool(geometryc) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/Config.cmake.in` around lines 24 - 28, The
invocation of _bgfx_crosscompile_use_host_tool incorrectly exposes the geometry
viewer tool; change the call that currently references geometryv to geometryc so
that the geometry compiler tool (bgfx::geometryc) is used by
bgfx_compile_geometry(); update the _bgfx_crosscompile_use_host_tool(geometryv)
line to _bgfx_crosscompile_use_host_tool(geometryc) to restore correct geometry
compilation in cross-compiled/installed packages.
| find_package(Git QUIET) | ||
|
|
||
| execute_process( | ||
| COMMAND "${GIT_EXECUTABLE}" -C bgfx log --pretty=format:'%h' -n 1 | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | ||
| OUTPUT_VARIABLE GIT_REV | ||
| ERROR_QUIET | ||
| ) | ||
|
|
||
| execute_process( | ||
| COMMAND "${GIT_EXECUTABLE}" -C bgfx rev-list --count HEAD | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | ||
| OUTPUT_VARIABLE GIT_REV_COUNT | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "version.cmake" -type fRepository: Redot-Engine/DraconicEngine
Length of output: 117
🏁 Script executed:
cat -n engine/native/thirdparty/cmake/version.cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 1695
🌐 Web query:
CMake execute_process single quotes format string behavior
💡 Result:
CMake's execute_process passes all COMMAND arguments VERBATIM to the child process via OS APIs (argv array on POSIX, command-line string on Windows), without invoking a shell. Single quotes in string literals have no special meaning or escaping behavior in CMakeLists.txt; they are treated as literal characters. To include a literal single quote (') in an argument string, write it directly: execute_process(COMMAND someprog arg'with'quote OUTPUT_VARIABLE result) This passes "arg'with'quote" as argv[1] to someprog on POSIX systems. When invoking a shell explicitly (e.g., COMMAND bash "-c" "cmd 'with' quote"), single quotes inside the "-c" argument string are interpreted by bash according to shell rules: they preserve their literal content without expansion. No special CMake escaping is needed for single quotes in double-quoted CMake strings destined for shell interpretation; CMake passes the ' literally. There is no "format string" feature in execute_process; arguments are literal strings, not formatted like printf. Any observed issues with quotes typically arise from shell command-line parsing (when shell is used) or Windows CreateProcess encoding, not CMake's handling of single quotes specifically. Examples from sources: - bash "-c" "echo -n hello | sed 's/hello/world/;'" works directly, with 's preserved literally for sed. - date +'%F %T' in bash -c string uses single quotes for literal % handling.
Citations:
- 1: https://cmake.org/cmake/help/latest/command/execute_process.html
- 2: https://stackoverflow.com/questions/35689501/cmakes-execute-process-and-arbitrary-shell-scripts
- 3: https://stackoverflow.com/questions/35072473/executing-bash-commands-from-a-cmake-file
Make the Git version probe resilient.
execute_process() passes arguments verbatim to the child process without shell interpretation, so --pretty=format:'%h' delivers the single quotes literally to Git, causing it to misinterpret the format string. Additionally, this block assumes both Git and bgfx's .git metadata are always present; when either is missing, GIT_REV and GIT_REV_COUNT end up empty, making the derived version variables invalid. Initialize defaults and only overwrite them when the Git commands succeed.
Suggested fix
find_package(Git QUIET)
-execute_process(
- COMMAND "${GIT_EXECUTABLE}" -C bgfx log --pretty=format:'%h' -n 1
- WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
- OUTPUT_VARIABLE GIT_REV
- ERROR_QUIET
-)
-
-execute_process(
- COMMAND "${GIT_EXECUTABLE}" -C bgfx rev-list --count HEAD
- WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
- OUTPUT_VARIABLE GIT_REV_COUNT
- OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET
-)
+set(GIT_REV "unknown")
+set(GIT_REV_COUNT 0)
+
+if(GIT_EXECUTABLE AND EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/bgfx/.git")
+ execute_process(
+ COMMAND "${GIT_EXECUTABLE}" -C bgfx log --pretty=format:%h -n 1
+ WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
+ OUTPUT_VARIABLE GIT_REV
+ OUTPUT_STRIP_TRAILING_WHITESPACE
+ RESULT_VARIABLE GIT_REV_RESULT
+ ERROR_QUIET
+ )
+
+ execute_process(
+ COMMAND "${GIT_EXECUTABLE}" -C bgfx rev-list --count HEAD
+ WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
+ OUTPUT_VARIABLE GIT_REV_COUNT
+ OUTPUT_STRIP_TRAILING_WHITESPACE
+ RESULT_VARIABLE GIT_REV_COUNT_RESULT
+ ERROR_QUIET
+ )
+
+ if(NOT GIT_REV_RESULT EQUAL 0)
+ set(GIT_REV "unknown")
+ endif()
+
+ if(NOT GIT_REV_COUNT_RESULT EQUAL 0)
+ set(GIT_REV_COUNT 0)
+ endif()
+endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/thirdparty/cmake/version.cmake` around lines 13 - 27, The
execute_process calls that populate GIT_REV and GIT_REV_COUNT are fragile: they
pass a quoted format arg so Git sees literal single quotes and they assume Git
and the bgfx .git exist, leaving variables empty if commands fail. Initialize
safe defaults (e.g., "unknown" or "0") for GIT_REV and GIT_REV_COUNT before
running execute_process, call execute_process with separate ARGS (no embedded
single quotes) or use --pretty=format:%h (no shell quotes) and check the
RESULT_VARIABLE from execute_process; only overwrite GIT_REV and GIT_REV_COUNT
when RESULT is 0 and OUTPUT_VARIABLE is non-empty, and ensure you reference
GIT_EXECUTABLE and the bgfx working directory consistently when performing these
checks.
3 participants
TL;DR
Note
Contributed by 2LazyDevs.
Summary by CodeRabbit