Add rendering stuff, cross-platform support, camera controller & SDL input logic#16
Open
AR-DEV-1 wants to merge 31 commits into
Open
Add rendering stuff, cross-platform support, camera controller & SDL input logic#16AR-DEV-1 wants to merge 31 commits into
AR-DEV-1 wants to merge 31 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a complete rendering engine foundation with SDL3 windowing, BGFX GPU rendering, C++20 modules, and integrated subsystems for memory management, I/O, platform abstraction, input handling, scene graphs, and a multi-pass render pipeline. All systems are built using modern C++ module syntax and tightly integrated via CMake module libraries. ChangesRendering Engine Foundation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
AR-DEV-1
force-pushed
the
rendering-hardware-abstraction
branch
from
1928886 to
2e90dc6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
engine/native/core/filesystem/filesystem.cpp (2)
21-23: Error message references "shader" in generic function.Same issue as above - the message should be generic.
Proposed fix
if (size <= 0) { - std::println("Error: Shader file is empty: {}", path); + std::println("Error: File is empty or size unavailable: {}", path); return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/filesystem/filesystem.cpp` around lines 21 - 23, The error message in the empty-file check uses a shader-specific message; update the std::println call in filesystem.cpp (the block checking "if (size <= 0)" that references variables size and path) to use a generic message such as "Error: File is empty: {}" (or similar) so it no longer mentions "Shader"; ensure the change is applied to the std::println invocation that currently reads "Error: Shader file is empty: {}" and preserves the path interpolation.
14-18: Generic function has shader-specific error messages.
load_binaryis a general-purpose filesystem function, but the error messages reference "shader file". This will be confusing when the function is used for non-shader assets.Proposed fix
if (!file.is_open()) { - std::println("Error: Could not find shader file at: {}", path); + std::println("Error: Could not open file at: {}", path); // Return an empty vector return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/filesystem/filesystem.cpp` around lines 14 - 18, The error message inside load_binary currently says "shader file" which is misleading for a general-purpose function; change the std::println call in the load_binary function (the branch checking file.is_open()) to a generic message such as "Error: Could not open file at: {}" or "Error: Failed to open file: {}" and keep returning the empty vector; ensure you still include the path variable in the formatted output so callers see which file failed.engine/native/rendering/rhi/rhi_bgfx.cpp (1)
115-121: Vertex layout is duplicated fromvertex.cppm.This layout definition duplicates
PosColorVertex::init()invertex.cppm. If the vertex format changes, both locations must be updated, risking inconsistency.Consider using the centralized definition from
PosColorVertexor removing the unused code fromvertex.cppm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 115 - 121, The vertex layout in create_vertex_buffer duplicates PosColorVertex::init() from vertex.cppm; update create_vertex_buffer to reuse the centralized definition instead of redefining it: replace the inline bgfx::VertexLayout construction in create_vertex_buffer with a call to the shared initializer (e.g., PosColorVertex::init(layout) or use PosColorVertex::layout if a static layout exists), and remove the redundant layout code (or alternatively consolidate by moving the single authoritative layout to PosColorVertex and updating both call sites to reference it).CMakeLists.txt (2)
49-50: X11 pkg-config results are discovered but not used.
pkg_check_modulespopulates${X11_LIBS_LIBRARIES},${X11_LIBS_INCLUDE_DIRS}, and${X11_LIBS_LDFLAGS}, but lines 64-70 hardcode the library names instead. This loses pkg-config benefits (correct paths, linker flags, transitive dependencies).Proposed fix using pkg-config results
target_link_libraries(draconic PRIVATE rhi core bgfx bx bimg -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive c++ c++abi - X11 - Xext - Xcursor - Xrandr - Xrender - Xi - Xfixes + ${X11_LIBS_LIBRARIES} dl pthread m ) +target_include_directories(draconic PRIVATE ${X11_LIBS_INCLUDE_DIRS})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 49 - 50, The pkg-config results from pkg_check_modules(X11_LIBS ...) are discovered but not used; replace the hardcoded X11 library names with the pkg-config variables: add the include dirs from ${X11_LIBS_INCLUDE_DIRS} (via target_include_directories or include_directories), add ${X11_LIBS_LDFLAGS} to the target link/compile flags if needed (or use target_link_libraries with ${X11_LIBS_LIBRARIES}), and remove the hardcoded library list so transitive deps and correct paths from X11_LIBS are honored.
52-74: Linux-specific build logic lacks platform guards.The
--whole-archivelinker flags, explicitc++/c++abilinking, and X11 dependencies are Linux/Clang-specific. This will break builds on other platforms (macOS, Windows). Since this is WIP and Linux-focused, consider adding a guard or TODO.Example platform guard
+if(CMAKE_SYSTEM_NAME STREQUAL "Linux") add_executable(draconic engine/native/main/main.cpp) enable_modules(draconic) target_link_libraries(draconic PRIVATE rhi core ... ) +else() + message(WARNING "draconic executable is currently only supported on Linux") +endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 52 - 74, The Linux-specific linker and dependency lines in the add_executable("draconic" / engine/native/main/main.cpp) and target_link_libraries(draconic ...) block (the -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive, explicit c++ / c++abi, and X11-related libs) need to be guarded so they only apply on Linux; update the CMake logic to wrap these platform-specific link flags/dependencies with a conditional that detects Linux (e.g., UNIX and not APPLE) and keep the generic target_link_libraries entries for cross-platform deps outside that guard, leaving a TODO comment if this is WIP.engine/native/rendering/rhi/rhi.cppm (1)
7-10: Consider defining an explicit invalid handle constant.The implementation (
rhi_bgfx.cpp:150) usesUINT16_MAXas a sentinel for "no index buffer", but this isn't documented in the interface. Defining an explicit constant would make the API clearer:constexpr BufferHandle InvalidBuffer = UINT16_MAX;This is optional for WIP code but would improve API clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi.cppm` around lines 7 - 10, Define and document an explicit invalid-handle constant for BufferHandle (and optionally for PipelineHandle/ShaderHandle/ViewID) so callers and implementations don't rely on magic UINT16_MAX; add a constexpr like InvalidBuffer (using BufferHandle) and update uses (e.g., the place that checks for UINT16_MAX as "no index buffer" in rhi_bgfx.cpp where BufferHandle is handled) to compare against that named constant instead of UINT16_MAX, and update any comments/signatures to mention the sentinel.engine/native/main/main.cpp (1)
36-46: Only Linux/X11 is supported; consider adding a note for Wayland/other platforms.The native handle extraction only handles X11 via the
#if defined(__linux__)block. SDL3 also supports Wayland on Linux, and the engine will likely need macOS/Windows support. As this is WIP, consider adding a TODO or fallback diagnostic.Example Wayland placeholder
`#if` defined(__linux__) SDL_PropertiesID props = SDL_GetWindowProperties(window); if (driver && std::string_view(driver) == "x11") { ndt = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_X11_DISPLAY_POINTER, nullptr); nwh = (void*)(uintptr_t)SDL_GetNumberProperty(props, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, 0); } + else if (driver && std::string_view(driver) == "wayland") + { + // TODO: Add Wayland support + // ndt = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_WAYLAND_DISPLAY_POINTER, nullptr); + // nwh = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_WAYLAND_SURFACE_POINTER, nullptr); + std::println("Wayland support not yet implemented"); + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/main/main.cpp` around lines 36 - 46, The current Linux-only native handle extraction uses SDL_GetWindowProperties and X11-specific properties (SDL_GetPointerProperty with SDL_PROP_WINDOW_X11_DISPLAY_POINTER and SDL_PROP_WINDOW_X11_WINDOW_NUMBER) when driver == "x11"; add a short TODO and a fallback diagnostic path for Wayland/other platforms: detect when driver is not "x11" (or when SDL reports Wayland) and log or assert a clear message indicating unsupported backend, and leave a TODO in the SDL_GetWindowProperties/ndt/nwh handling to implement Wayland/macOS/Windows-specific property extraction later; reference SDL_GetWindowProperties, SDL_GetPointerProperty, SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, driver, ndt, and nwh so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 39-41: The C flag variable CMAKE_C_FLAGS incorrectly receives the
C++-only flag "-stdlib=libc++", causing C compiler warnings/errors; remove
"-stdlib=libc++" from CMAKE_C_FLAGS and ensure "-stdlib=libc++" (and "-lc++abi"
linker flags) are only appended to CMAKE_CXX_FLAGS and CMAKE_EXE_LINKER_FLAGS
(or conditionally added when the compiler is a C++ compiler) so that
CMAKE_CXX_FLAGS gets the C++ specific flag while CMAKE_C_FLAGS remains clean.
In `@engine/native/core/filesystem/filesystem.cpp`:
- Around line 29-33: When file.read(reinterpret_cast<char*>(buffer.data()),
size) fails the function currently returns an empty vector silently; change it
to emit a diagnostic before returning (e.g., log or throw) that includes the
filename/size and the system error (use errno/std::strerror(errno) or
std::error_code) so failures are visible; update the failure path that
references file.read and buffer to call the project's logger (or std::cerr) with
a clear message or throw a std::runtime_error containing the error details
instead of returning {}.
In `@engine/native/main/main.cpp`:
- Around line 125-127: The main function in main.cpp lacks an explicit success
return; add an explicit "return 0;" as the final statement in the main()
function (after the comment "// Fun fact: AR literally went mad & tis is the
result") so successful execution returns 0 consistently with the existing error
returns of -1 and improves clarity.
In `@engine/native/rendering/CMakeLists.txt`:
- Around line 1-3: The build currently adds the rhi target but never compiles
the shader source files; add add_custom_command rules that invoke shaderc on
engine/native/rendering/shaders/vs.sc, fs.sc and varying.def.sc to produce
vs_triangle.bin and fs_triangle.bin (and any varying/binary outputs) and place
them in the folder where the runtime loader expects them, then create a custom
target (e.g., rhi_shaders) that depends on those outputs and make the rhi target
depend on that custom target (use add_dependencies(rhi, rhi_shaders)) so the
shader binaries are generated before rhi is built; reference the shaderc
executable defined in thirdparty CMake (use the same variable/name) and ensure
the outputs are listed in the add_custom_command so CMake tracks them.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 50-53: The init() implementation in rhi_bgfx.cpp calls
bgfx::init(init) and returns early on failure without informing callers; change
the RHI init contract to return a bool (or otherwise signal failure) and
propagate that to main.cpp so callers can abort or handle the error: update the
rhi_bgfx.cpp init() to return false when bgfx::init(init) fails (instead of only
printing), update the RHI interface/signature used by main.cpp to accept the
bool return and check it, and ensure main.cpp stops initialization or reports
the error if init() returns false; alternatively, if changing the signature is
impossible, introduce a clearly named initialized flag (e.g.,
RHI_Bgfx::initialized) that init() sets to true/false and make callers
(main.cpp) check that flag before using the RHI.
- Around line 142-145: The submit() function dereferences handles without
validation which can cause out-of-bounds access when p.pipeline or
p.vertex_buffer are invalid; add defensive bounds checks before using
g_pipelines and g_buffers (check p.pipeline < g_pipelines.size() and
p.vertex_buffer < g_buffers.size()), and if a check fails, log or assert with
context (e.g., include the invalid index and ViewID) and early-return or use a
safe fallback pipeline/buffer so the function cannot crash; update submit(),
referencing the symbols submit, Pipeline, Buffer, g_pipelines, g_buffers,
p.pipeline and p.vertex_buffer.
In `@engine/native/rendering/rhi/vertex.cppm`:
- Around line 16-24: PosColorVertex::init() and its static layout are unused and
duplicate the bgfx::VertexLayout built inline in create_vertex_buffer(); either
delete both init() and the static layout from vertex.cppm to remove dead code,
or refactor create_vertex_buffer() to reuse PosColorVertex::layout by (a)
exposing a single initializer (e.g., keep PosColorVertex::init() or a function
that returns/ensures the static layout is initialized) and (b) replacing the
inline bgfx::VertexLayout in create_vertex_buffer() with a reference to
PosColorVertex::layout so the vertex format is defined in one place (ensure the
layout is initialized before use).
In `@engine/native/thirdparty/CMakeLists.txt`:
- Line 17: Move the set(CMAKE_POSITION_INDEPENDENT_CODE ON) line to before the
add_subdirectory() calls so that bx, bimg, and bgfx are built with
position-independent code; specifically, open the thirdparty CMakeLists (where
add_subdirectory(bx), add_subdirectory(bimg), add_subdirectory(bgfx) are
invoked) and place the CMAKE_POSITION_INDEPENDENT_CODE ON setting above those
add_subdirectory(...) lines to ensure static libraries used by the SHARED rhi
target are compiled PIC.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 49-50: The pkg-config results from pkg_check_modules(X11_LIBS ...)
are discovered but not used; replace the hardcoded X11 library names with the
pkg-config variables: add the include dirs from ${X11_LIBS_INCLUDE_DIRS} (via
target_include_directories or include_directories), add ${X11_LIBS_LDFLAGS} to
the target link/compile flags if needed (or use target_link_libraries with
${X11_LIBS_LIBRARIES}), and remove the hardcoded library list so transitive deps
and correct paths from X11_LIBS are honored.
- Around line 52-74: The Linux-specific linker and dependency lines in the
add_executable("draconic" / engine/native/main/main.cpp) and
target_link_libraries(draconic ...) block (the -Wl,--whole-archive
SDL3::SDL3-static -Wl,--no-whole-archive, explicit c++ / c++abi, and X11-related
libs) need to be guarded so they only apply on Linux; update the CMake logic to
wrap these platform-specific link flags/dependencies with a conditional that
detects Linux (e.g., UNIX and not APPLE) and keep the generic
target_link_libraries entries for cross-platform deps outside that guard,
leaving a TODO comment if this is WIP.
In `@engine/native/core/filesystem/filesystem.cpp`:
- Around line 21-23: The error message in the empty-file check uses a
shader-specific message; update the std::println call in filesystem.cpp (the
block checking "if (size <= 0)" that references variables size and path) to use
a generic message such as "Error: File is empty: {}" (or similar) so it no
longer mentions "Shader"; ensure the change is applied to the std::println
invocation that currently reads "Error: Shader file is empty: {}" and preserves
the path interpolation.
- Around line 14-18: The error message inside load_binary currently says "shader
file" which is misleading for a general-purpose function; change the
std::println call in the load_binary function (the branch checking
file.is_open()) to a generic message such as "Error: Could not open file at: {}"
or "Error: Failed to open file: {}" and keep returning the empty vector; ensure
you still include the path variable in the formatted output so callers see which
file failed.
In `@engine/native/main/main.cpp`:
- Around line 36-46: The current Linux-only native handle extraction uses
SDL_GetWindowProperties and X11-specific properties (SDL_GetPointerProperty with
SDL_PROP_WINDOW_X11_DISPLAY_POINTER and SDL_PROP_WINDOW_X11_WINDOW_NUMBER) when
driver == "x11"; add a short TODO and a fallback diagnostic path for
Wayland/other platforms: detect when driver is not "x11" (or when SDL reports
Wayland) and log or assert a clear message indicating unsupported backend, and
leave a TODO in the SDL_GetWindowProperties/ndt/nwh handling to implement
Wayland/macOS/Windows-specific property extraction later; reference
SDL_GetWindowProperties, SDL_GetPointerProperty,
SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, driver,
ndt, and nwh so the change is easy to locate.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 115-121: The vertex layout in create_vertex_buffer duplicates
PosColorVertex::init() from vertex.cppm; update create_vertex_buffer to reuse
the centralized definition instead of redefining it: replace the inline
bgfx::VertexLayout construction in create_vertex_buffer with a call to the
shared initializer (e.g., PosColorVertex::init(layout) or use
PosColorVertex::layout if a static layout exists), and remove the redundant
layout code (or alternatively consolidate by moving the single authoritative
layout to PosColorVertex and updating both call sites to reference it).
In `@engine/native/rendering/rhi/rhi.cppm`:
- Around line 7-10: Define and document an explicit invalid-handle constant for
BufferHandle (and optionally for PipelineHandle/ShaderHandle/ViewID) so callers
and implementations don't rely on magic UINT16_MAX; add a constexpr like
InvalidBuffer (using BufferHandle) and update uses (e.g., the place that checks
for UINT16_MAX as "no index buffer" in rhi_bgfx.cpp where BufferHandle is
handled) to compare against that named constant instead of UINT16_MAX, and
update any comments/signatures to mention the sentinel.
🪄 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: c45da71e-d688-4600-ad17-e177e862e7ff
📒 Files selected for processing (19)
.gitmodulesCMakeLists.txtCMakePresets.jsoncmake/Compiler.cmakeengine/native/CMakeLists.txtengine/native/core/CMakeLists.txtengine/native/core/filesystem/filesystem.cppengine/native/core/filesystem/filesystem.cppmengine/native/main/main.cppengine/native/rendering/CMakeLists.txtengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/vertex.cppmengine/native/rendering/shaders/fs.scengine/native/rendering/shaders/varying.def.scengine/native/rendering/shaders/vs.scengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/bxengine/native/thirdparty/sdl
💤 Files with no reviewable changes (2)
- CMakePresets.json
- cmake/Compiler.cmake
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
CMakeLists.txt (1)
39-41:⚠️ Potential issue | 🟠 MajorGuard the libc++ flags by toolchain and remove them from
CMAKE_C_FLAGS.Line 40 still pushes the C++-only
-stdlib=libc++switch into C compilations, and Lines 39-41 are unconditional even though this setup only makes sense for a Clang/libc++ toolchain. Either fail fast on non-Clang toolchains or add these switches only under a Clang C++ branch.Suggested fix
+if (NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang") + message(FATAL_ERROR "DraconicEngine currently requires Clang/libc++") +endif() + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++") -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -stdlib=libc++") set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -stdlib=libc++ -lc++abi")Is `-stdlib=libc++` valid for C compilation, and which compilers/toolchains support that flag?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 39 - 41, The libc++ flags are being applied unconditionally (and mistakenly to C code via CMAKE_C_FLAGS); guard them by detecting the C++ compiler and only apply to Clang-like toolchains: wrap the set(...) calls in an if that checks CMAKE_CXX_COMPILER_ID for "Clang" or "AppleClang" (e.g. if(CMAKE_CXX_COMPILER_ID MATCHES "Clang|AppleClang")), remove the line setting CMAKE_C_FLAGS, and either add an else branch that emits a clear FATAL_ERROR when a non-Clang C++ compiler is detected or simply skip applying the flags for other toolchains so only set CMAKE_CXX_FLAGS and CMAKE_EXE_LINKER_FLAGS inside that Clang-specific block.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
61-61: Use CMake's native whole-archive handling here.Since this project requires CMake 4.0 (which supports the feature), replace the GNU-style linker flags with
$<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static>. This is the portable, standard CMake way to express whole-archive semantics and will automatically map to the correct linker syntax across different toolchains (GNU/Clang--whole-archive, MSVC/WHOLEARCHIVE, etc.).Suggested refactor
- -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive + "$<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 61, Replace the GNU-specific linker flags used to force whole-archive linking with CMake's native generator expression: locate the target link libraries entry that currently uses "-Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive" and swap it to use the CMake LINK_LIBRARY generator expression $<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static> so CMake emits the correct whole-archive directive for the active toolchain (reference SDL3::SDL3-static in the target_link_libraries or add_link_options usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 28-31: Replace the host-dependent -march=native option in the
Clang branch with the repository's explicit ISA baseline used in
cmake/Compiler.cmake (AVX2/FMA); update the add_compile_options call inside the
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") block to use the explicit flags
representing AVX2/FMA (e.g., -mavx2 and -mfma or the equivalent -march=haswell)
so the build is consistent across hosts and matches the project's declared
baseline.
- Around line 76-84: The runtime fails to find shader files because CMake copies
them into a "shaders" subfolder but the loader in engine/native/main/main.cpp
calls load_binary("vs_triangle.bin") and load_binary("fs_triangle.bin") with no
directory; either update those calls to load_binary("shaders/vs_triangle.bin")
and load_binary("shaders/fs_triangle.bin") in main.cpp, or change the CMake
target command (the add_custom_command that uses
$<TARGET_FILE_DIR:draconic>/shaders) to copy directly into
$<TARGET_FILE_DIR:draconic> (remove the "/shaders" suffix) so the copied files
match the existing load paths.
- Around line 49-50: The pkg-config call currently ignores discovered flags
because pkg_check_modules is not creating an imported target; change the
pkg_check_modules(X11_LIBS REQUIRED x11 xext xcursor xrandr xrender xi xfixes)
invocation to create an imported target (use the IMPORTED_TARGET option) so it
registers PkgConfig::X11_LIBS, then update the target_link_libraries invocation
that currently lists hardcoded library names (e.g., X11, Xext, Xcursor, Xrandr,
Xrender, Xi, Xfixes) to link against the PkgConfig::X11_LIBS imported target
instead so compiler/linker search paths and flags discovered by pkg-config are
used.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 39-41: The libc++ flags are being applied unconditionally (and
mistakenly to C code via CMAKE_C_FLAGS); guard them by detecting the C++
compiler and only apply to Clang-like toolchains: wrap the set(...) calls in an
if that checks CMAKE_CXX_COMPILER_ID for "Clang" or "AppleClang" (e.g.
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang|AppleClang")), remove the line setting
CMAKE_C_FLAGS, and either add an else branch that emits a clear FATAL_ERROR when
a non-Clang C++ compiler is detected or simply skip applying the flags for other
toolchains so only set CMAKE_CXX_FLAGS and CMAKE_EXE_LINKER_FLAGS inside that
Clang-specific block.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Line 61: Replace the GNU-specific linker flags used to force whole-archive
linking with CMake's native generator expression: locate the target link
libraries entry that currently uses "-Wl,--whole-archive SDL3::SDL3-static
-Wl,--no-whole-archive" and swap it to use the CMake LINK_LIBRARY generator
expression $<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static> so CMake emits the
correct whole-archive directive for the active toolchain (reference
SDL3::SDL3-static in the target_link_libraries or add_link_options usage).
🪄 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: 17a47ca7-3f0c-44f0-b314-5fa3cd83dd70
⛔ Files ignored due to path filters (2)
engine/native/rendering/shaders/fs_triangle.binis excluded by!**/*.binengine/native/rendering/shaders/vs_triangle.binis excluded by!**/*.bin
📒 Files selected for processing (1)
CMakeLists.txt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
engine/native/core/filesystem/filesystem.cpp (1)
9-35: Error messages are shader-specific but function is generic.The error messages at lines 15 and 22 reference "shader file" but
load_binary()is a general-purpose function. Consider using generic messages like "Could not open file" and "File is empty" to match the function's broader purpose.Proposed fix
if (!file.is_open()) { - std::println("Error: Could not find shader file at: {}", path); + std::println("Error: Could not open file at: {}", path); // Return an empty vector return {}; } std::streamsize size = file.tellg(); if (size <= 0) { - std::println("Error: Shader file is empty: {}", path); + std::println("Error: File is empty or unreadable: {}", path); return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/filesystem/filesystem.cpp` around lines 9 - 35, The error messages in load_binary are shader-specific; change them to generic file messages: replace "Could not find shader file" and "Shader file is empty" with general phrases like "Could not open file" and "File is empty" (refer to the load_binary function and its local ifstream variable 'file' and the error paths that call std::println), and ensure the final read-failure message stays generic ("Failed to read file contents") so the function remains reusable for any binary file.engine/native/thirdparty/CMakeLists.txt (1)
16-31: SDL configuration looks appropriate for X11-only static linking.The settings correctly disable shared builds, Wayland, and shared audio backends while enabling X11. One minor note: the file is missing a newline at end of file.
🤖 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 16 - 31, The file is missing a trailing newline at EOF; open the CMakeLists.txt containing the SDL configuration (the block with set(SDL_MAIN_HANDLED ON ... set(SDL_X11 ON ...) and add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/sdl sdl-build)) and add a single newline character at the end of the file so the file ends with a newline.engine/native/rendering/rhi/rhi_bgfx.cpp (1)
106-115: Note:createProgramwithdestroyShaders=trueinvalidates input handles.The third parameter
truetobgfx::createProgram()causes the vertex and fragment shader handles to be destroyed after the program is created. This meansdesc.vsanddesc.fsbecome invalid after this call and cannot be reused for other pipelines.This is likely intentional for simple use cases, but if shader reuse across pipelines is ever needed, this behavior would need to change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 106 - 115, create_pipeline currently calls bgfx::createProgram(vs, fs, true) which destroys the shader handles and invalidates desc.vs/desc.fs; change this to not auto-destroy so shaders can be reused: in create_pipeline (and related code using PipelineDesc and desc.vs/desc.fs) call bgfx::createProgram(vs, fs, false) and ensure shader lifetime is managed explicitly (e.g., destroy shaders only when they are truly no longer needed or add a shader ownership/cleanup path) so desc.vs and desc.fs remain valid for reuse across pipelines.
🤖 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/main/main.cpp`:
- Around line 58-59: draco::rhi::init(ndt, nwh, 1280, 720) now returns a bool
indicating success; update the call site to check that return value and handle
failure (e.g., log an error and abort/return non-zero) to avoid subsequent RHI
calls crashing. Specifically, in main where draco::rhi::init(ndt, nwh, 1280,
720) is invoked, capture the bool result, call your logging/error path (or
std::cerr/process exit) on false, and ensure you do not proceed to further
draco::rhi::* calls when init fails.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 156-161: The code accesses g_buffers[p.index_buffer] when
p.index_buffer != UINT16_MAX without verifying the index is within g_buffers
bounds; add a bounds check before indexing: ensure p.index_buffer <
g_buffers.size() (or the appropriate buffer count) before reading
g_buffers[p.index_buffer] and only then assign Buffer& ib and call
bgfx::setIndexBuffer(ib.ibh); keep the existing UINT16_MAX sentinel check and
handle the out-of-range case (skip setting the index buffer and optionally log
or assert) in the same block where p.index_buffer is tested.
---
Nitpick comments:
In `@engine/native/core/filesystem/filesystem.cpp`:
- Around line 9-35: The error messages in load_binary are shader-specific;
change them to generic file messages: replace "Could not find shader file" and
"Shader file is empty" with general phrases like "Could not open file" and "File
is empty" (refer to the load_binary function and its local ifstream variable
'file' and the error paths that call std::println), and ensure the final
read-failure message stays generic ("Failed to read file contents") so the
function remains reusable for any binary file.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 106-115: create_pipeline currently calls bgfx::createProgram(vs,
fs, true) which destroys the shader handles and invalidates desc.vs/desc.fs;
change this to not auto-destroy so shaders can be reused: in create_pipeline
(and related code using PipelineDesc and desc.vs/desc.fs) call
bgfx::createProgram(vs, fs, false) and ensure shader lifetime is managed
explicitly (e.g., destroy shaders only when they are truly no longer needed or
add a shader ownership/cleanup path) so desc.vs and desc.fs remain valid for
reuse across pipelines.
In `@engine/native/thirdparty/CMakeLists.txt`:
- Around line 16-31: The file is missing a trailing newline at EOF; open the
CMakeLists.txt containing the SDL configuration (the block with
set(SDL_MAIN_HANDLED ON ... set(SDL_X11 ON ...) and
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/sdl sdl-build)) and add a single
newline character at the end of the file so the file ends with a newline.
🪄 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: d4605d44-cf86-44ef-bfaf-24cf3828e471
📒 Files selected for processing (7)
CMakeLists.txtengine/native/core/filesystem/filesystem.cppengine/native/main/main.cppengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/vertex.cppmengine/native/thirdparty/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- engine/native/rendering/rhi/rhi.cppm
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/native/rendering/rhi/vertex.cppm
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
AR-DEV-1
force-pushed
the
rendering-hardware-abstraction
branch
from
08e0f83 to
6458806
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
engine/native/rendering/rhi/rhi_bgfx.cpp (1)
117-132: Consider making the vertex layout configurable.The vertex layout is hardcoded to
Position (3 floats) + Color0 (4 Uint8). While this matches the currentPosColorVertexformat invertex.cppm, this tight coupling may limit flexibility when additional vertex formats are needed (e.g., vertices with normals, UVs, or tangents).For now this is acceptable for the initial implementation, but consider parameterizing the layout or using a vertex format descriptor in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 117 - 132, The create_vertex_buffer function hardcodes a bgfx::VertexLayout (Position + Color0) which couples vertex format to this implementation; modify create_vertex_buffer (or add an overload) to accept a vertex layout descriptor (e.g., a bgfx::VertexLayout or a lightweight vertex format enum/struct) as an argument instead of constructing layout internally, use that passed layout when calling bgfx::createVertexBuffer, and update callers that build PosColorVertex to pass the appropriate layout; ensure g_buffers still stores the returned vbh and behavior unchanged.engine/native/main/main.cpp (1)
36-46: Platform support is limited to Linux/X11.The native handle extraction only supports Linux with X11. Other platforms (Windows, macOS, Wayland) will result in null handles and early exit. This is acceptable for initial development but should be expanded.
Do you want me to open an issue to track adding Windows and macOS platform support for native window handles?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/main/main.cpp` around lines 36 - 46, Current code only extracts native handles for Linux/X11 using SDL_GetWindowProperties and X11-specific properties (SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER), causing null handles on other platforms; update the extraction to handle other drivers by branching on the SDL video driver (the existing driver variable) and adding platform-specific handle retrieval for "windows", "cocoa"/"macos", and "wayland". Use SDL's platform-agnostic helper (SDL_GetWindowWMInfo / SDL_version + SDL_VERSIONNUM checks) or the appropriate SDL functions for each backend to populate ndt and nwh, and keep the existing X11 branch that uses SDL_GetPointerProperty/SDL_GetNumberProperty for SDL_PROP_WINDOW_X11_*, so handles are non-null on Windows (HWND), macOS (NSWindow/NSView), and Wayland as available.
🤖 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/main/main.cpp`:
- Around line 80-85: The shader-load failure path returns early without cleaning
up RHI and SDL resources; update the block that checks vs_data.empty() ||
fs_data.empty() (where it currently prints "Failed to load shaders" and returns
-1) to first call draco::rhi::shutdown(), destroy the created SDL window (e.g.
SDL_DestroyWindow(window)) and perform SDL_Quit() (and any other local cleanup
like freeing context if used) before returning the error code so RHI and SDL are
properly torn down.
- Around line 116-121: The RenderPacket is default-initialized so index_buffer
ends up 0 and can be misinterpreted in submit(); explicitly mark "no index
buffer" by setting packet.index_buffer = UINT16_MAX after creating the packet
(before calling draco::rhi::submit) so submit()'s check (p.index_buffer !=
UINT16_MAX) correctly skips index handling; update the code that constructs the
packet (the RenderPacket packet{} block) to assign packet.index_buffer =
UINT16_MAX.
---
Nitpick comments:
In `@engine/native/main/main.cpp`:
- Around line 36-46: Current code only extracts native handles for Linux/X11
using SDL_GetWindowProperties and X11-specific properties
(SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER),
causing null handles on other platforms; update the extraction to handle other
drivers by branching on the SDL video driver (the existing driver variable) and
adding platform-specific handle retrieval for "windows", "cocoa"/"macos", and
"wayland". Use SDL's platform-agnostic helper (SDL_GetWindowWMInfo / SDL_version
+ SDL_VERSIONNUM checks) or the appropriate SDL functions for each backend to
populate ndt and nwh, and keep the existing X11 branch that uses
SDL_GetPointerProperty/SDL_GetNumberProperty for SDL_PROP_WINDOW_X11_*, so
handles are non-null on Windows (HWND), macOS (NSWindow/NSView), and Wayland as
available.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 117-132: The create_vertex_buffer function hardcodes a
bgfx::VertexLayout (Position + Color0) which couples vertex format to this
implementation; modify create_vertex_buffer (or add an overload) to accept a
vertex layout descriptor (e.g., a bgfx::VertexLayout or a lightweight vertex
format enum/struct) as an argument instead of constructing layout internally,
use that passed layout when calling bgfx::createVertexBuffer, and update callers
that build PosColorVertex to pass the appropriate layout; ensure g_buffers still
stores the returned vbh and behavior unchanged.
🪄 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: 6ebcef73-8f7e-4fdc-9546-ca7a65447dfd
⛔ Files ignored due to path filters (2)
engine/native/rendering/shaders/fs_triangle.binis excluded by!**/*.binengine/native/rendering/shaders/vs_triangle.binis excluded by!**/*.bin
📒 Files selected for processing (19)
.gitmodulesCMakeLists.txtCMakePresets.jsoncmake/Compiler.cmakeengine/native/CMakeLists.txtengine/native/core/CMakeLists.txtengine/native/core/filesystem/filesystem.cppengine/native/core/filesystem/filesystem.cppmengine/native/main/main.cppengine/native/rendering/CMakeLists.txtengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/vertex.cppmengine/native/rendering/shaders/fs.scengine/native/rendering/shaders/varying.def.scengine/native/rendering/shaders/vs.scengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/bxengine/native/thirdparty/sdl
💤 Files with no reviewable changes (2)
- CMakePresets.json
- cmake/Compiler.cmake
✅ Files skipped from review due to trivial changes (8)
- engine/native/thirdparty/sdl
- .gitmodules
- engine/native/rendering/shaders/fs.sc
- engine/native/rendering/CMakeLists.txt
- engine/native/rendering/rhi/vertex.cppm
- engine/native/rendering/shaders/varying.def.sc
- engine/native/rendering/rhi/rhi.cppm
- engine/native/thirdparty/bx
🚧 Files skipped from review as they are similar to previous changes (6)
- engine/native/CMakeLists.txt
- engine/native/core/CMakeLists.txt
- engine/native/core/filesystem/filesystem.cppm
- engine/native/rendering/shaders/vs.sc
- engine/native/thirdparty/CMakeLists.txt
- CMakeLists.txt
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/main/main.cpp`:
- Around line 25-27: After successfully calling SDL_Init, the error paths return
without tearing down SDL resources; update the early returns so that when window
creation fails (the check on variable "window" after SDL_CreateWindow) you call
SDL_Quit() before returning, and for the native-handle failure branch(s) (the
code that checks the created window's native handle between the SDL_CreateWindow
and main loop) call SDL_DestroyWindow(window) then SDL_Quit() before returning;
use the SDL_DestroyWindow and SDL_Quit functions to ensure symmetric teardown
while retaining the existing SDL_GetError logging.
- Around line 4-5: main.cpp currently includes bgfx/bx headers and uses
BGFX_STATE_* flags; remove those includes and any direct use of BGFX symbols and
instead use the RHI public API: stop including <bgfx/bgfx.h> and <bx/math.h> in
main.cpp, initialize RenderPacket::model via an RHI-provided default matrix or
initialization helper (add or call something like RHI::defaultModelMatrix() or
RHI::InitRenderPacketModel(RenderPacket&)), and replace raw BGFX_STATE_* uses
with RHI-exposed pipeline state constants or an enum (e.g.
RHI::PipelineState::Opaque / RHI::STATE_DEFAULT) so main.cpp only references RHI
types; update code that sets pipeline state to use the new RHI constants and
ensure rhi_bgfx.cpp continues using bx::mtxIdentity internally to implement the
default model behavior.
- Around line 40-44: The current native-handle extraction only handles the "x11"
driver and leaves ndt/nwh null for other Linux drivers causing cryptic failures;
update the block that checks driver / std::string_view(driver) to either (a) add
explicit handling for other drivers you intend to support (e.g., check for
"wayland" and call the appropriate SDL_GetPointerProperty/SDL_GetNumberProperty
keys) or (b) add a clear fail-fast path that logs or returns an explicit
"unsupported video driver" error when driver is not "x11" (ensure ndt and nwh
are not used after that); modify the code around variables driver, ndt, nwh and
the SDL_GetPointerProperty/SDL_GetNumberProperty calls to implement the chosen
behavior so downstream null-handle assertions cannot occur silently.
🪄 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: b14593cd-076d-4b40-adcb-ab607ac86df5
📒 Files selected for processing (1)
engine/native/main/main.cpp
…objects, vertex layout abstraction, scissor rects, resource destruction queue & global uniforms (view/projection)
AR-DEV-1
changed the title
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
engine/native/rendering/CMakeLists.txt (1)
1-53:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftShader compilation rules still missing.
The three module library targets are wired correctly, but there is still no
add_custom_command/shadercinvocation that produces thevs_triangle.bin/fs_triangle.binartifacts referenced at runtime. The rootCMakeLists.txtcopy step atCMakeLists.txtlines 77–83 stages whatever is inengine/native/rendering/shaders/, so unless.binoutputs land there first the runtime loader will keep failing.Repeating the prior recommendation: add
add_custom_commandrules that invokeshaderconvs.sc,fs.sc, andvarying.def.sc, group them into arhi_shaderstarget, andadd_dependencies(rhi rhi_shaders).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/CMakeLists.txt` around lines 1 - 53, The CMakeLists defines module targets (add_modules_library(rhi), rendergraph, renderer) but never generates the shader .bin outputs referenced at runtime; add add_custom_command rules that invoke shaderc to compile engine/native/rendering/shaders/vs.sc, fs.sc and varying.def.sc into vs_triangle.bin and fs_triangle.bin (and any varying binary) and collect those commands into a custom target (e.g., rhi_shaders); then call add_dependencies(rhi rhi_shaders) so the rhi target depends on the compiled shader artifacts and they will be available for the existing copy step.
🟠 Major comments (19)
engine/native/core/io/image_loader.cpp-35-38 (1)
35-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard decoded dimensions before size multiplication.
Line 36 multiplies
width * height * 4without validating bounds. Malformed or extreme inputs can overflowsizeand lead to invalid copy ranges.Add dimension and overflow checks
+ if (width <= 0 || height <= 0) { + stbi_image_free(data); + return result; + } + + const size_t w = static_cast<size_t>(width); + const size_t h = static_cast<size_t>(height); + if (w > (std::numeric_limits<size_t>::max() / 4) / h) { + stbi_image_free(data); + return result; + } - size_t size = static_cast<size_t>(width) * height * 4; + size_t size = w * h * 4;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/io/image_loader.cpp` around lines 35 - 38, Validate and guard width/height and the multiplication before computing size and calling result.pixels.assign; specifically, cast width/height to size_t (e.g., size_t w = static_cast<size_t>(width), h = static_cast<size_t>(height)), check for zero dimensions, ensure h <= SIZE_MAX/4 and then ensure w <= SIZE_MAX/(h*4) to avoid overflow, then compute size = w * h * 4 and also check the input buffer (data) contains at least size bytes before calling result.pixels.assign(data, data + size); if any check fails, return or handle the error instead of proceeding.engine/native/core/io/image_loader.cpp-21-24 (1)
21-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse non-throwing filesystem checks for graceful failure paths.
Line 21 calls the throwing
std::filesystem::exists(path)overload. Permission and system errors can throw and skip your intended error return path.Safer existence check
+ std::error_code ec; - if (!std::filesystem::exists(path)) { + if (!std::filesystem::exists(path, ec) || ec) { std::println("Error: Image path does not exist: {}", path.string()); return result; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/io/image_loader.cpp` around lines 21 - 24, The current check uses the throwing overload std::filesystem::exists(path) which can throw on permission or system errors; change this to the non-throwing variant by calling std::filesystem::exists(path, std::error_code&) (or query std::filesystem::status(path, ec)) and handle the error_code: if exists returns false or ec is set, log the path and ec.message and return result, ensuring the graceful failure path is preserved; update the check around the existing path and result variables in image_loader.cpp accordingly.engine/native/core/io/image_loader.cppm-14-15 (1)
14-15:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftChange ImageData struct to use wider types and update dependent APIs to prevent silent truncation.
The
ImageDatastruct usesuint16_tfor width/height (lines 14-15), which truncates decoded image dimensions larger than 65535 pixels. This is confirmed by explicit casts in the implementation (image_loader.cpp lines 39-40) wherestb_imagereturnsintvalues cast touint16_t.The issue propagates downstream:
create_texture()andcreate_framebuffer()APIs also expectuint16_tdimensions, so the fix requires updating the struct definition, both function signatures, and their implementations.Proposed changes
struct ImageData { std::vector<uint8_t> pixels; - uint16_t width = 0; - uint16_t height = 0; + uint32_t width = 0; + uint32_t height = 0; uint8_t channels = 0; bool is_valid = false; };Also update function signatures in rhi.cppm:
- TextureHandle create_texture(const void* data, uint16_t width, uint16_t height, uint32_t flags = 0); + TextureHandle create_texture(const void* data, uint32_t width, uint32_t height, uint32_t flags = 0); - FramebufferHandle create_framebuffer(uint16_t width, uint16_t height, TextureFormat format); + FramebufferHandle create_framebuffer(uint32_t width, uint32_t height, TextureFormat format);And update implementations in rhi_bgfx.cpp accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/io/image_loader.cppm` around lines 14 - 15, Change ImageData.width and ImageData.height from uint16_t to a wider unsigned type (uint32_t) to avoid truncation, then update all dependent APIs and implementations that expect uint16_t—specifically update function signatures and usages of create_texture(...) and create_framebuffer(...) in rhi.cppm and their implementations in rhi_bgfx.cpp to accept uint32_t dimensions, and remove the unsafe int->uint16_t casts in image_loader.cpp(m) so stb_image int outputs are stored in the new uint32_t fields and passed through unchanged to the RHI.engine/native/platform/CMakeLists.txt-11-13 (1)
11-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
WAYLAND_LIBS REQUIREDwill break Linux builds without Wayland dev packages.The PR notes that Wayland support is added but "still not working", yet
pkg_check_modules(... REQUIRED ...)aborts CMake configuration whenwayland-client,wayland-egl,wayland-cursor, oreglare not installed. Many minimal/server/X11-only environments won't have these. Either dropREQUIREDand feature-gate Wayland, or split the call so X11 stays mandatory and Wayland is optional.♻️ Suggested approach
- pkg_check_modules(X11_LIBS REQUIRED IMPORTED_TARGET x11 xext xcursor xrandr xrender xi xfixes) - pkg_check_modules(WAYLAND_LIBS REQUIRED IMPORTED_TARGET wayland-client wayland-egl wayland-cursor egl) - set(PLATFORM_DEPS PkgConfig::X11_LIBS PkgConfig::WAYLAND_LIBS) + pkg_check_modules(X11_LIBS REQUIRED IMPORTED_TARGET x11 xext xcursor xrandr xrender xi xfixes) + pkg_check_modules(WAYLAND_LIBS IMPORTED_TARGET wayland-client wayland-egl wayland-cursor egl) + set(PLATFORM_DEPS PkgConfig::X11_LIBS) + if(WAYLAND_LIBS_FOUND) + list(APPEND PLATFORM_DEPS PkgConfig::WAYLAND_LIBS) + target_compile_definitions(platform PRIVATE DRACO_HAS_WAYLAND=1) + endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/platform/CMakeLists.txt` around lines 11 - 13, The pkg_check_modules call makes WAYLAND_LIBS required and will abort configuration on systems without Wayland dev packages; change the CMake so X11 remains mandatory (keep pkg_check_modules(X11_LIBS REQUIRED IMPORTED_TARGET ...)) but invoke pkg_check_modules for Wayland without REQUIRED (pkg_check_modules(WAYLAND_LIBS IMPORTED_TARGET ...)) and conditionally append PkgConfig::WAYLAND_LIBS to PLATFORM_DEPS only when found, e.g., set a USE_WAYLAND or WAYLAND_FOUND flag based on the non-REQUIRED check and then add PkgConfig::WAYLAND_LIBS to PLATFORM_DEPS if that flag is true; this preserves X11 as required and makes Wayland optional/feature-gated.engine/native/platform/CMakeLists.txt-16-29 (1)
16-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLinking SHARED
platformwithSDL3::SDL3-staticwhiledraconicalso linksSDL3::SDL3-staticdirectly creates duplicate SDL3 global state.
platform(SHARED) privately embeds SDL3::SDL3-static, and the main executabledraconicalso links SDL3::SDL3-static directly. This results in two independent copies of SDL3 in the same process with separate global state (event queues, video subsystem, etc.), violating SDL3's assumption of single global state and causing potential crashes or silent bugs.Fix by either making
platforma static/object library or switching to linkSDL3::SDL3(shared) instead of the static variant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/platform/CMakeLists.txt` around lines 16 - 29, The platform shared library is currently linking the static SDL3 target (see target_link_libraries(platform PRIVATE SDL3::SDL3-static)) which, combined with the draconic executable also linking SDL3::SDL3-static, creates duplicate SDL global state; to fix it either change the library type from SHARED to STATIC/OBJECT (update add_library(platform SHARED -> STATIC or OBJECT) and keep linking SDL3::SDL3-static) or leave platform as SHARED but switch its SDL link to the shared target (replace SDL3::SDL3-static with SDL3::SDL3 in target_link_libraries(platform ...)); update CMake accordingly so that only one copy of SDL3 is present at runtime.engine/native/rendering/rendergraph/rendergraph.cpp-25-25 (1)
25-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the per-pass
printlnfrom frame execution.This runs in the render loop for every pass, so even small scenes will flood stdout and turn frame time into blocking I/O.
💡 Suggested fix
- std::println("[RenderGraph] Executing pass: {}", pass.name); // Debug + // Keep pass execution logging behind an explicit debug flag if needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rendergraph/rendergraph.cpp` at line 25, Remove the per-pass std::println call that prints "[RenderGraph] Executing pass: {}" with pass.name; this call runs every frame and blocks I/O. Edit the code that iterates/executess passes (the location containing std::println and pass.name) to either delete the println entirely or replace it with a non-blocking debug/log-level gated statement (e.g., only log when a debug flag or verbose logging is enabled) so normal frame execution does not perform stdout printing.engine/native/main/main.cpp-143-147 (1)
143-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't resize the renderer every frame.
This turns resize into unconditional per-frame work. If the backend recreates swapchain/backbuffer state inside
rhi::resize(), you'll pay that cost even while the window size is stable.💡 Suggested fix
+ static int prev_w = 1280; + static int prev_h = 720; int w, h; SDL_GetWindowSize(window, &w, &h); - draco::rendering::rhi::resize((uint16_t)w, (uint16_t)h); - draco::rendering::renderer::resize((uint16_t)w, (uint16_t)h); + if (w != prev_w || h != prev_h) { + draco::rendering::rhi::resize(static_cast<uint16_t>(w), static_cast<uint16_t>(h)); + draco::rendering::renderer::resize(static_cast<uint16_t>(w), static_cast<uint16_t>(h)); + prev_w = w; + prev_h = h; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/main/main.cpp` around lines 143 - 147, Currently SDL_GetWindowSize(window, &w, &h) is followed by unconditional calls to draco::rendering::rhi::resize and draco::rendering::renderer::resize every frame; change this to only call those resize functions when the window size actually changes by tracking the previous width/height (e.g., a static or member prev_w/prev_h) and comparing to the new w/h obtained from SDL_GetWindowSize(window, &w, &h), and call draco::rendering::rhi::resize((uint16_t)w, (uint16_t)h) and draco::rendering::renderer::resize((uint16_t)w, (uint16_t)h) only when w != prev_w or h != prev_h, then update prev_w/prev_h.engine/native/rendering/renderer/renderer.cpp-60-62 (1)
60-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard zero-height windows before building the projection.
A minimized window can report
screen_height == 0. That makesaspectinf/NaN and feeds invalid values intoperspective().💡 Suggested fix
- float aspect = float(g_ctx.screen_width) / float(g_ctx.screen_height); + const auto safe_height = std::max<uint16_t>(g_ctx.screen_height, 1); + float aspect = float(g_ctx.screen_width) / float(safe_height);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/renderer/renderer.cpp` around lines 60 - 62, The code computes aspect = g_ctx.screen_width / g_ctx.screen_height and calls draco::rendering::rhi::perspective(proj_mtx, g_ctx.main_camera.fov, aspect, ...), which will produce inf/NaN when g_ctx.screen_height is zero (minimized window); fix by guarding g_ctx.screen_height == 0 before computing aspect and calling perspective: if height is zero either skip/early-return rendering or use a safe fallback aspect (e.g., 1.0f) and/or clamp the denominator, ensuring the call to draco::rendering::rhi::perspective always receives a finite aspect; update the code around the aspect calculation and the draco::rendering::rhi::perspective call (references: g_ctx.screen_height, g_ctx.screen_width, proj_mtx, g_ctx.main_camera.fov, draco::rendering::rhi::perspective).engine/native/main/main.cpp-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
<print>with a supported output mechanism or document a minimum compiler version.The
<print>header is used across 6 compilation units (main.cpp, renderer.cpp, rendergraph.cpp, rhi_bgfx.cpp, filesystem.cpp, image_loader.cpp), butstd::printlnis not fully supported in libc++ bundled with Clang 14 or earlier. Clang 14 reports'print' file not foundas an error.While
CMAKE_CXX_STANDARD 23is enforced, there is no documented minimum compiler version. Either add an explicit compiler version requirement (e.g., Clang 16+) in CMakeLists.txt, or switch these error/debug messages to the project's existing logging infrastructure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/main/main.cpp` at line 1, Replace the unsupported <print> usage across main.cpp, renderer.cpp, rendergraph.cpp, rhi_bgfx.cpp, filesystem.cpp, and image_loader.cpp by either switching to the project's existing logging/output API (use the project's logger functions) or to a widely-supported alternative such as <iostream>/std::cout/std::cerr; alternatively, if you intend to keep std::println, add a documented minimum compiler requirement to CMakeLists.txt (e.g., require Clang >=16 or set a policy using target_compile_features / CMAKE_CXX_COMPILER_VERSION) so builds fail with a clear message rather than the '<print> file not found' error.engine/native/thirdparty/CMakeLists.txt-26-29 (1)
26-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce these SDL cache options for consistency.
These four toggles are the only SDL settings here not written as cache entries with
CACHE BOOL "" FORCE. On reconfigure or preset-driven builds, stale cache values can override plainset()calls, silently re-enabling shared backend loading despite these settings.Suggested fix
-set(SDL_WAYLAND_SHARED OFF) -set(SDL_X11_SHARED OFF) -set(SDL_ALSA_SHARED OFF) -set(SDL_PULSEAUDIO_SHARED OFF) +set(SDL_WAYLAND_SHARED OFF CACHE BOOL "" FORCE) +set(SDL_X11_SHARED OFF CACHE BOOL "" FORCE) +set(SDL_ALSA_SHARED OFF CACHE BOOL "" FORCE) +set(SDL_PULSEAUDIO_SHARED OFF CACHE BOOL "" FORCE)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/thirdparty/CMakeLists.txt` around lines 26 - 29, The four SDL variables SDL_WAYLAND_SHARED, SDL_X11_SHARED, SDL_ALSA_SHARED, and SDL_PULSEAUDIO_SHARED should be set as cache entries to override stale CMake cache values; replace the plain set(...) usage with CACHE BOOL "" FORCE for each variable so they become persistent cache entries (e.g., change the set calls for SDL_WAYLAND_SHARED, SDL_X11_SHARED, SDL_ALSA_SHARED, SDL_PULSEAUDIO_SHARED to use CACHE BOOL "" FORCE).CMakeLists.txt-67-72 (1)
67-72:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWrap Linux-only linker flags in platform guards.
Lines 67-72 contain
-Wl,--whole-archive(GNU ld/lld syntax),c++,c++abi,dl,pthread, andm—all Linux/Unix-specific. On Windows (MSVC),link.exerejects the-Wlflag and these libraries don't exist. macOS rejects--whole-archive(uses-force_loadinstead).This conflicts with the cross-platform architecture already present in
engine/native/platform/CMakeLists.txt(which usesif(WIN32)/elseif(APPLE)/else()guards).Wrap these dependencies using the same pattern:
🛠️ Suggested fix
target_link_libraries(draconic PRIVATE core io memory image_loader platform input camera # Camera controller rhi renderer rendergraph bgfx bx bimg - -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive - c++ - c++abi - dl - pthread - m ) + +if(UNIX AND NOT APPLE) + target_link_libraries(draconic PRIVATE + -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive + c++ c++abi dl pthread m) +elseif(APPLE) + target_link_libraries(draconic PRIVATE + -Wl,-force_load,$<TARGET_FILE:SDL3::SDL3-static> + c++ c++abi dl pthread m) +else() + target_link_libraries(draconic PRIVATE SDL3::SDL3-static) +endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 67 - 72, Wrap the Linux/Unix-only linker flags and libraries currently listed (the -Wl,--whole-archive / -Wl,--no-whole-archive around SDL3::SDL3-static and the libraries c++ c++abi dl pthread m) in platform-specific CMake guards like the existing pattern used elsewhere (if(WIN32) / elseif(APPLE) / else()). For WIN32: do not add the -Wl or Unix libraries (use target_link_libraries with SDL3::SDL3 or Visual Studio-friendly libs if needed); for APPLE: replace --whole-archive semantics with the macOS equivalent (e.g., -force_load usage for the SDL3 static target) or link normally; for the else() (Linux) branch: keep the -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive and link c++ c++abi dl pthread m. Update the CMakeLists.txt block containing those flags so each platform only receives appropriate linker flags.engine/native/rendering/rhi/rhi.cppm-63-68 (1)
63-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd default values to uninitialized descriptor members.
ViewDesc::{x,y,w,h,clear_flags,clear_color}andPipelineDesc::statelack defaults. They're read directly inapply_view()(line 448) andcreate_pipeline()(line 181) without validation, allowing garbage values to pass into bgfx. Initialize them safely:Safe defaults
struct ViewDesc { FramebufferHandle fb = InvalidFramebuffer; - uint16_t x, y, w, h; - uint32_t clear_flags; - uint32_t clear_color; + uint16_t x = 0, y = 0, w = 0, h = 0; + uint32_t clear_flags = 0; + uint32_t clear_color = 0; }; @@ struct PipelineDesc { ShaderHandle vs; ShaderHandle fs; - PipelineState state; + PipelineState state = PipelineState::Default;Also applies to: 142-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi.cppm` around lines 63 - 68, ViewDesc members x,y,w,h,clear_flags,clear_color and PipelineDesc::state are uninitialized and can pass garbage into bgfx when used in apply_view() and create_pipeline(); update the ViewDesc definition to give x=0,y=0,w=0,h=0,clear_flags=0,clear_color=0 by default and ensure PipelineDesc::state is value-initialized (e.g. default-construct or = {}), so apply_view() and create_pipeline() always see safe defaults instead of indeterminate memory.engine/native/rendering/rhi/rhi.cppm-244-245 (1)
244-245:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTemplate function
destroy_resourceis exported without a definition.In C++20 modules, exported template functions must have their definitions in the module interface unit (.cppm) for external modules to instantiate them. The template declaration at lines 244-245 lacks a corresponding definition, making the function unusable for any external importer attempting to call it with a concrete type. Add the template definition to the exported namespace in this file, or replace it with explicit exported specializations for the required handle types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi.cppm` around lines 244 - 245, The exported template declaration void destroy_resource(T handle) is missing its definition, so add an exported template definition for destroy_resource<T> in this module interface (rhi.cppm) within the same exported namespace (the one that currently declares destroy_resource) so external importers can instantiate it; alternatively, if only a few concrete handle types are used, provide exported explicit specializations/overloads for those handle types (e.g., destroy_resource(DeviceHandle), destroy_resource(BufferHandle), etc.) in the module interface instead of the generic template.engine/native/rendering/rhi/rhi_bgfx.cpp-99-100 (1)
99-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hard-code Vulkan—enable cross-platform renderer fallback.
Line 100 forces
init.type = bgfx::RendererType::Vulkan, which blocks fallback to D3D/Metal/OpenGL on platforms where Vulkan is unavailable. bgfx supports automatic platform-appropriate selection viainit.type = bgfx::RendererType::Countor fallback viainit.fallback = true. Make the renderer type configurable or use auto-selection to allow the RHI to work across all platforms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 99 - 100, The code currently forces bgfx::Init init{}; init.type = bgfx::RendererType::Vulkan which prevents bgfx from choosing a platform-appropriate renderer; replace the hard-coded assignment by allowing auto-selection or a configurable override: locate the bgfx::Init init{} and remove the fixed init.type = bgfx::RendererType::Vulkan, set init.type = bgfx::RendererType::Count and enable fallback with init.fallback = true OR read a configurable renderer type (e.g., a parameter or config value named rendererType) and only set init.type when that config is present, leaving Count as the default; ensure any new config uses the same bgfx::RendererType enum for compatibility.engine/native/rendering/rhi/rhi_bgfx.cpp-273-283 (1)
273-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImplement texture sampling flags from the API parameter instead of hardcoding clamp sampling.
The API declares a
flagsparameter (defaulting to 0) but the implementation ignores it entirely, always creating clamp-sampled textures. This prevents callers from controlling texture wrapping and filtering behavior, silently discarding any custom flags passed in.Use the API parameter
- TextureHandle create_texture(const void* data, uint16_t w, uint16_t h, uint32_t) + TextureHandle create_texture(const void* data, uint16_t w, uint16_t h, uint32_t flags) { RHI_ASSERT(data != nullptr, "Texture data is null"); RHI_ASSERT(w > 0 && h > 0, "Invalid texture dimensions"); auto tex = bgfx::createTexture2D( w, h, false, 1, bgfx::TextureFormat::RGBA8, - BGFX_SAMPLER_U_CLAMP | BGFX_SAMPLER_V_CLAMP, + flags == 0 ? (BGFX_SAMPLER_U_CLAMP | BGFX_SAMPLER_V_CLAMP) : flags, bgfx::copy(data, w * h * 4) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 273 - 283, The create_texture function ignores its API uint32_t flags parameter and always uses BGFX_SAMPLER_U_CLAMP | BGFX_SAMPLER_V_CLAMP; update create_texture to consume the flags argument (or combine it with any necessary defaults) when building the bgfx sampler flags passed to bgfx::createTexture2D instead of the hardcoded clamp values, mapping any API-specific flag bits to the corresponding BGFX_SAMPLER_* constants so callers can control wrapping/filtering behavior.engine/native/rendering/rhi/rhi_bgfx.cpp-136-145 (1)
136-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
shutdown()misses dynamic vertex buffers.The buffer teardown destroys
vbhandibh, butcreate_dynamic_vertex_buffer()storesdvbh. Any live dynamic buffer survives shutdown and leaks GPU resources on teardown/reinit. Add cleanup fordvbh:Cleanup addition
for (auto& slot : g_buffers.internal().raw()) { if (!slot.alive) continue; + if (bgfx::isValid(slot.value.dvbh)) + bgfx::destroy(slot.value.dvbh); + if (bgfx::isValid(slot.value.vbh)) bgfx::destroy(slot.value.vbh); if (bgfx::isValid(slot.value.ibh)) bgfx::destroy(slot.value.ibh);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 136 - 145, The shutdown() buffer teardown loop currently destroys slot.value.vbh and slot.value.ibh but omits dynamic vertex buffers created by create_dynamic_vertex_buffer(); update the loop that iterates over g_buffers.internal().raw() in shutdown() to also check bgfx::isValid(slot.value.dvbh) and call bgfx::destroy(slot.value.dvbh) for any alive slot so dynamic buffers are properly released (reference symbols: shutdown(), create_dynamic_vertex_buffer(), g_buffers, slot.value.dvbh).engine/native/rendering/rhi/rhi_bgfx.cpp-174-183 (1)
174-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShader ownership mismatch with
createProgram(..., true)allows unsafe double-deletion.The third parameter to
bgfx::createProgram()is a_destroyShadersflag that, when true, transfers shader ownership to bgfx—the program will destroy the shader handles when the program itself is destroyed. However,g_shaderstreats shaders as independently managed resources, anddestroy_shader()callsdestroy_later()assuming the application retains ownership. Ifdestroy_shader()is called on a shader that was used to create a program, the shader will be destroyed twice: once by bgfx when the program is destroyed, and again when the deferred destruction queue processes it.Suggested fix
- bgfx::ProgramHandle prog = bgfx::createProgram(resolve(desc.vs), resolve(desc.fs), true); + bgfx::ProgramHandle prog = bgfx::createProgram(resolve(desc.vs), resolve(desc.fs), false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 174 - 183, create_pipeline currently calls bgfx::createProgram(..., true) which transfers shader ownership to bgfx and causes double-free with our g_shaders/destroy_shader/destroy_later flow; change the program creation to not transfer ownership (call createProgram with the _destroyShaders flag set to false) so g_shaders remains the sole owner, and verify that program teardown (wherever pipelines from g_pipelines are destroyed) does not attempt to free shader handles; update references around create_pipeline, bgfx::createProgram, g_shaders, destroy_shader, and destroy_later accordingly.engine/native/rendering/rhi/rhi_bgfx.cpp-367-417 (1)
367-417:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
PipelineState::PrimitiveTriStripis not mapped to bgfx state.The
PipelineStateenum definesPrimitiveTriStrip, butmap_state()never translates it toBGFX_STATE_PT_TRISTRIP, causing pipelines with this flag to render with the default triangle list topology instead.Missing mapping
if ((s & PipelineState::MSAA) != PipelineState::Default) state |= BGFX_STATE_MSAA; + + if ((s & PipelineState::PrimitiveTriStrip) != PipelineState::Default) + state |= BGFX_STATE_PT_TRISTRIP; return state;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 367 - 417, map_state currently omits mapping for the PipelineState::PrimitiveTriStrip flag, so when that bit is set the BGFX primitive type falls back to the default; update map_state (the function map_state(PipelineState s, ...)) to detect the PrimitiveTriStrip flag (similar to the MSAA/Write checks) and OR in BGFX_STATE_PT_TRISTRIP when present (e.g., check (s & PipelineState::PrimitiveTriStrip) != PipelineState::Default then state |= BGFX_STATE_PT_TRISTRIP) so pipelines using PrimitiveTriStrip render with the correct topology.engine/native/rendering/rhi/rhi_bgfx.cpp-432-446 (1)
432-446:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the view framebuffer when
desc.fbis absent.If this branch is skipped, the previous framebuffer bound to the same
ViewIDremains active. Reusing a view for backbuffer rendering after an offscreen pass can therefore keep drawing into the old target instead of the default framebuffer. Callbgfx::setViewFrameBuffer(view, BGFX_INVALID_HANDLE)whendesc.fb == InvalidFramebufferto explicitly reset to the default backbuffer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 432 - 446, In apply_view, ensure the view's framebuffer is explicitly reset when a ViewDesc has no framebuffer: when desc.fb == InvalidFramebuffer call bgfx::setViewFrameBuffer(view, BGFX_INVALID_HANDLE) so the previous framebuffer bound to that ViewID is cleared; keep the existing branch that sets the framebuffer when desc.fb is valid (using get_checked(g_framebuffers, desc.fb) and fb->fbh) and only add the explicit reset for the absent case to avoid leaking an old target into subsequent backbuffer rendering.
🟡 Minor comments (4)
engine/native/thirdparty/sdl-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSubmodule pinned to an untagged development commit instead of a stable release.
Commit
1aa7224is not associated with any release tag and appears as an intermediate development commit on SDL'smainbranch. Pinning to an untagged Git snapshot undermines build reproducibility and introduces risk of API or behavioral changes.Use a stable release tag instead (e.g., one of SDL's release tags like
release-3.4.x) for a known-good API surface and improved maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/thirdparty/sdl` at line 1, The SDL submodule at engine/native/thirdparty/sdl is pinned to an untagged commit (1aa7224); update it to a stable release tag (for example release-3.4.x) to ensure reproducible builds: check out the SDL repo in that submodule, fetch tags, git checkout the chosen release tag (e.g., release-3.4.x), commit the submodule update in the superproject, and, if present, update .gitmodules or any CI submodule references to the release tag/branch so future clones use the tagged release instead of the intermediate commit.engine/native/input/input.cpp-10-10 (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
is_down()againstKey::Invalidfor consistency withset_key().The
set_key()function guards againstKey::Invalid, butis_down()does not. WhileInvalidis enum value 8 (not a large sentinel) and fits safely ing_keys[256], the inconsistency is semantically problematic: callingis_down(Key::Invalid)reads from an array index thatset_key()explicitly prevents from being written. This leavesis_down(Key::Invalid)returning the zero-initialized state by accident rather than by design.Add the same guard to
is_down()for clarity and consistency:Suggested change
bool is_down(Key key) { + if (key == Key::Invalid) return false; return g_keys[(uint16_t)key]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/input/input.cpp` at line 10, is_down() currently reads g_keys using the passed Key without checking for Key::Invalid, creating an inconsistency with set_key() which explicitly guards against Key::Invalid; update is_down() to mirror set_key() by checking if the key equals Key::Invalid and return false immediately if so, then otherwise cast/index into g_keys as before — reference the is_down function, the Key::Invalid enum value, and the g_keys[256] array when making this change.engine/native/core/memory/slot_array.cppm-74-84 (1)
74-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGeneration counter wraparound can revive stale handles.
generationisuint16_t(line 16). After 65,535 destroys on the same slot the counter wraps to 0, at which point a long-held stale handle whose generation matches by coincidence will passvalid()and silently address a different live resource. Either widen touint32_t, or skip generation0on wrap (commonly done by pre-incrementing past 0 if the wrap reaches it) so wrap-collisions become detectable / less likely.🔧 Suggested fix
template<typename T> struct Slot { T value{}; - uint16_t generation = 0; + uint32_t generation = 0; bool alive = false; };Note: also widen the generation field/accessor on
Handle<Tag>accordingly soHandle::make(idx, slot.generation)andh.generation()stay consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/memory/slot_array.cppm` around lines 74 - 84, The generation counter for slots is a uint16_t and can wrap, letting stale handles become valid after 65,535 destroys; update the slot generation strategy in destroy()/slot storage to prevent wrap-collisions by either widening the generation field (e.g., change slot.generation from uint16_t to uint32_t and update any accessors) or ensure you never leave generation == 0 on wrap (increment again when wrapping to skip zero), and update all related symbols/uses (slots[], destroy(Handle h), valid(Handle h), Handle::make(idx, slot.generation), and h.generation()) so the type and semantics remain consistent across Handle<Tag> and slot storage.engine/native/rendering/renderer/renderer.cppm-20-35 (1)
20-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefault-initialize
Camera/SceneContextfields.
position,target,up,fov,near_plane,far_plane,screen_width, andscreen_heighthave no in-class initializers. Stack-instantiatedCamera/SceneContextvalues (e.g. a localCamera cam;passed tobegin_frame) will contain indeterminate data; reading them is UB and produces garbage view/projection matrices.🛠️ Proposed fix
struct Camera { - std::array<float, 3> position; - std::array<float, 3> target; - std::array<float, 3> up; - float fov; - float near_plane; - float far_plane; + std::array<float, 3> position{0.f, 0.f, 0.f}; + std::array<float, 3> target {0.f, 0.f, 0.f}; + std::array<float, 3> up {0.f, 1.f, 0.f}; + float fov = 60.f; + float near_plane = 0.1f; + float far_plane = 1000.f; }; struct SceneContext { - uint16_t screen_width; - uint16_t screen_height; + uint16_t screen_width = 0; + uint16_t screen_height = 0; Camera main_camera; draco::rendering::rendergraph::RenderGraph graph; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/renderer/renderer.cppm` around lines 20 - 35, Camera and SceneContext have non-initialized POD fields leading to UB when default-constructed; update the struct definitions (Camera and SceneContext) to provide in-class default initializers for position, target, up (e.g., zero or sensible defaults), fov, near_plane, far_plane, and for screen_width/screen_height so any stack-instantiated Camera/SceneContext (e.g., used in begin_frame) is always well-defined; ensure defaults are explicit for each member to avoid indeterminate data.
🧹 Nitpick comments (4)
engine/native/input/input.cpp (1)
53-69: 💤 Low valueConsider clearing key state on focus loss.
process_eventdoesn't handleSDL_EVENT_WINDOW_FOCUS_LOST(orSDL_EVENT_KEY_UPit never sees because the OS swallowed it after Alt-Tab). Without clearingg_keys, a key can appear "stuck down" until the user presses and releases it again in-window. Optional polish for a camera/input system.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/input/input.cpp` around lines 53 - 69, process_event currently ignores window focus loss so keys can remain stuck; update process_event to handle SDL_EVENT_WINDOW_FOCUS_LOST (and optionally SDL_EVENT_WINDOW_HIDDEN/SDL_EVENT_WINDOW_MINIMIZED) and on that event reset input state by clearing g_keys (or calling set_key(key, false) for every key) and zeroing mouse delta via set_mouse_delta(0,0); locate this logic near the existing switch in process_event and ensure map_sdl_key/set_key and any global g_keys or input state are referenced to perform the full reset.engine/native/core/io/filesystem.cpp (1)
24-28: 💤 Low valueTreat empty files as success, not failure.
size == 0is a legitimate state for an empty file and shouldn't necessarily be reported as an error. Consider gating the diagnostic onsize < 0(the failure return fromtellg) and returning an empty vector silently forsize == 0.♻️ Suggested change
- std::streamsize size = file.tellg(); - if (size <= 0) { - std::println("Error: File is empty or unreadable: {}", path); - return {}; - } + std::streamsize size = file.tellg(); + if (size < 0) { + std::println("Error: Could not determine file size: {}", path); + return {}; + } + if (size == 0) { + return {}; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/io/filesystem.cpp` around lines 24 - 28, The current check treats size == 0 as an error; change the condition that logs and returns on failure to only trigger when tellg() failed (i.e., size < 0). In the code around file.tellg(), replace the if (size <= 0) branch so that size < 0 produces the diagnostic via std::println("Error: ...", path) and returns {}, while size == 0 returns an empty buffer silently (no error log) so empty files are handled as successful reads.engine/native/core/memory/slot_array.cppm (1)
86-89: ⚡ Quick winAvoid exposing internal storage via
raw().
raw()returns a mutable reference to the underlyingslotsvector, which lets callers bypass alive/generation invariants (e.g.,pop_back, in-place mutation, resettingalive). If iteration is needed, afor_each_alive(callable)style accessor (orbegin()/end()over alive slots) preserves invariants without leaking storage. Ifraw()is only used for diagnostics, consider returningconst&instead.🔧 Suggested change
- std::vector<Slot<T>>& raw() + const std::vector<Slot<T>>& raw() const { return slots; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/memory/slot_array.cppm` around lines 86 - 89, The raw() accessor currently returns a mutable reference to the internal vector slots which allows callers to break alive/generation invariants; change raw() so it no longer exposes mutable storage — either make it return a const std::vector<Slot<T>>& (if callers only need read/diagnostic access) or remove raw() and add safe iteration helpers such as for_each_alive(callable) and/or begin_alive()/end_alive() that iterate only alive slots while preserving invariants; update callers of raw() to use the new const accessor or the safe iteration helpers and keep the Slot<T> and slots symbols unchanged.engine/native/rendering/renderer/renderer.cppm (1)
37-37: ⚖️ Poor tradeoffAvoid exposing
g_ctxas a mutable exported global.Defining
SceneContext g_ctx;at the top of an exported namespace in a module interface unit makes globally-observable mutable state part of the public surface and forces every importer to depend on its layout/ABI. Two issues to consider:
- Move the definition into
renderer.cppand expose access via functions (SceneContext& context()or per-field getters/setters). This keeps the module interface free of mutable globals and lets you control initialization order.- If you must keep it here, mark it
inlineto make the intent explicit and to avoid surprises if this ever ends up being included from a non-module path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/renderer/renderer.cppm` at line 37, The exported module interface currently defines a mutable global SceneContext g_ctx which exposes mutable state and ABI to importers; move the concrete definition of g_ctx out of the module interface into the implementation file (renderer.cpp) and provide controlled accessors such as a function SceneContext& context() or per-field getters/setters in the module interface (keep only the accessor declarations in renderer.cppm), or if you absolutely must keep the definition in the interface mark it inline (inline SceneContext g_ctx) to make the intent explicit—update all uses to call the accessor (context() or getters) instead of referencing g_ctx directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5b37717-4e79-4e8c-b017-590d9067c839
⛔ Files ignored due to path filters (2)
engine/native/rendering/shaders/fs_triangle.binis excluded by!**/*.binengine/native/rendering/shaders/vs_triangle.binis excluded by!**/*.bin
📒 Files selected for processing (42)
CMakeLists.txtengine/native/CMakeLists.txtengine/native/core/CMakeLists.txtengine/native/core/io/filesystem.cppengine/native/core/io/filesystem.cppmengine/native/core/io/image_loader.cppengine/native/core/io/image_loader.cppmengine/native/core/io/io.cppmengine/native/core/memory/handle.cppmengine/native/core/memory/handle_registry.cppmengine/native/core/memory/memory.cppmengine/native/core/memory/slot_array.cppmengine/native/input/CMakeLists.txtengine/native/input/input.cppengine/native/input/input.cppmengine/native/main/main.cppengine/native/platform/CMakeLists.txtengine/native/platform/linux/linux.cppengine/native/platform/mac/mac.mmengine/native/platform/platform.cppmengine/native/platform/win32/win32.cppengine/native/rendering/CMakeLists.txtengine/native/rendering/renderer/renderer.cppengine/native/rendering/renderer/renderer.cppmengine/native/rendering/rendergraph/rendergraph.cppengine/native/rendering/rendergraph/rendergraph.cppmengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/vertex.cppmengine/native/rendering/shaders/fs.scengine/native/rendering/shaders/varying.def.scengine/native/rendering/shaders/vs.scengine/native/scene/CMakeLists.txtengine/native/scene/camera/camera_controller.cppengine/native/scene/camera/camera_controller.cppmengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/bgfxengine/native/thirdparty/bimgengine/native/thirdparty/bxengine/native/thirdparty/sdlengine/native/thirdparty/stb/CMakeLists.txtengine/native/thirdparty/stb/stb_image.h
✅ Files skipped from review due to trivial changes (1)
- engine/native/thirdparty/bimg
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/native/thirdparty/bx
- engine/native/rendering/shaders/fs.sc
- Introduce multi-pass RenderGraph foundation - Move main pass creation into renderer lifecycle - Add material uniform binding system - Add RHI uniform registry + hash lookup - Decouple scene rendering into RenderPacket pipeline - Fix renderer/RHI uniform binding mismatch - Improve frame lifecycle ownership (renderer owns passes)
… into rendering-hardware-abstraction
JoltedJon
reviewed
May 15, 2026
Contributor
There was a problem hiding this comment.
What's with this file being called mac.mm versus mac.cppm?
Contributor
Author
There was a problem hiding this comment.
What's with this file being called mac.mm versus mac.cppm?
Mac uses Objective-C++ & this file being using the .mm extension allows us to mix Objective-C++ which is better on Apple devices with standard C++
Contributor
Author
There was a problem hiding this comment.
I still have to test it
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
engine/native/platform/linux/linux.cpp (1)
12-31:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard null SDL values and remove
SDL_Quit()from this utility.
SDL_GetCurrentVideoDriver()can be null, sostd::string_view(driver)is UB. Also, callingSDL_Quit()here tears down SDL globally from a query function. Return an invalid frame instead.🔧 Minimal safe fix
NativeWindowFrame get_native_handles(void* sdl_window_ptr) { - SDL_Window* window = static_cast<SDL_Window*>(sdl_window_ptr); - NativeWindowFrame frame; + NativeWindowFrame frame{}; + SDL_Window* window = static_cast<SDL_Window*>(sdl_window_ptr); + if (!window) { + return frame; + } const char* driver = SDL_GetCurrentVideoDriver(); + if (!driver) { + return frame; + } SDL_PropertiesID props = SDL_GetWindowProperties(window); + if (!props) { + return frame; + } + const std::string_view driver_sv{driver}; - if (std::string_view(driver) == "x11") { + if (driver_sv == "x11") { frame.ndt = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_X11_DISPLAY_POINTER, nullptr); frame.nwh = (void*)(uintptr_t)SDL_GetNumberProperty(props, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, 0); - } else if (std::string_view(driver) == "wayland") { + } else if (driver_sv == "wayland") { frame.ndt = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_WAYLAND_DISPLAY_POINTER, nullptr); frame.nwh = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_WAYLAND_SURFACE_POINTER, nullptr); } else { - std::println("No video driver was found"); - SDL_Quit(); + return frame; }SDL3 docs: what does SDL_GetCurrentVideoDriver() return when video isn't initialized, and what is the lifecycle impact of calling SDL_Quit() from a helper function?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/platform/linux/linux.cpp` around lines 12 - 31, The helper get_native_handles must not use std::string_view on a possibly-null driver or call SDL_Quit(); instead, check if SDL_GetCurrentVideoDriver() returned nullptr and if so return an invalid NativeWindowFrame (set frame.valid = false) without touching SDL global state; only compare driver strings when non-null. Also guard the property lookups: call SDL_GetPointerProperty/SDL_GetNumberProperty only when SDL_GetWindowProperties(window) yields valid props and treat any null pointer/zero result as absent (leave frame.ndt/frame.nwh null). Keep setting frame.width/frame.height via SDL_GetWindowSize and compute frame.valid as (frame.nwh != nullptr) before returning.
🟡 Minor comments (2)
engine/native/rendering/quad_renderer/quad_renderer.cpp-175-179 (1)
175-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard orthographic projection against zero dimensions.
build_ortho()divides bywidth/heightdirectly. Zero-sized windows/minimized states will produce invalid projection values.Suggested patch
- float rl = width; - float tb = height; + float rl = std::max(width, 1.0f); + float tb = std::max(height, 1.0f);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/quad_renderer/quad_renderer.cpp` around lines 175 - 179, The orthographic build (build_ortho) currently divides by width/height directly (rl = width, tb = height) causing invalid projections for zero-sized windows; fix by clamping width and height to a safe non-zero value before computing cam.proj (e.g. use float rl = max(width, 1e-6f) and float tb = max(height, 1e-6f)) and then compute cam.proj[0] and cam.proj[5] with those clamped values (cam.proj[0], cam.proj[5]) so division by zero cannot occur; optionally set a debug/log path or early-return with an identity/projection-safe result if both dimensions are effectively zero.engine/native/rendering/rhi/rhi_bgfx.cpp-578-581 (1)
578-581:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid invalid-handle warnings for non-indexed draws.
get_checked(g_buffers, p.index_buffer, ...)runs unconditionally. For packets that intentionally omit an index buffer, this can generate avoidable warnings each submit.Proposed fix
- auto* ib = get_checked(g_buffers, p.index_buffer, "IndexBuffer"); + Buffer* ib = nullptr; + if (p.index_buffer != InvalidBuffer) + ib = get_checked(g_buffers, p.index_buffer, "IndexBuffer");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 578 - 581, The get_checked call for the index buffer is unconditional and triggers warnings for packets that intentionally omit an index buffer; modify the code in the submit path so you only call get_checked(g_buffers, p.index_buffer, "IndexBuffer") when the packet's index buffer handle is valid (e.g. check p.index_buffer for the "no index" sentinel used in this codebase) and otherwise leave ib null/unused; keep get_checked for pipeline and vertex buffer unchanged and ensure any later use of ib accounts for the null/invalid case.
🧹 Nitpick comments (1)
engine/native/rendering/rhi/rhi_bgfx.cpp (1)
320-323: ⚡ Quick winRemove unconditional layout warning spam.
These warnings are emitted every time because the condition is hardcoded to
false. Gate them behind real checks (or debug-only instrumentation) to avoid noisy logs.Proposed cleanup
- RHI_WARN(false, "Calculated Stride: {} bytes (Expected: 24)", layout.getStride()); - RHI_WARN(false, "Position Offset: {}", layout.getOffset(map_attrib(Attrib::Position))); - RHI_WARN(false, "Color0 Offset: {}", layout.getOffset(map_attrib(Attrib::Color0))); - RHI_WARN(false, "TexCoord0 Offset: {}", layout.getOffset(map_attrib(Attrib::TexCoord0))); + // Optional debug diagnostics: + // RHI_WARN(layout.getStride() != 24, "Calculated Stride: {} bytes (Expected: 24)", layout.getStride());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 320 - 323, The RHI_WARN calls are unconditionally logging because they pass false as the condition; change these to only emit when appropriate by replacing the hardcoded false with a real check (e.g., a debug build flag or a validation condition that compares layout.getStride() vs expected stride and verifies offsets), or guard the block behind an `#ifdef` DEBUG/if (enable_rhi_debug) toggle; update the calls referencing layout.getStride(), layout.getOffset(map_attrib(Attrib::Position)), layout.getOffset(map_attrib(Attrib::Color0)), and layout.getOffset(map_attrib(Attrib::TexCoord0)) so they only execute when the validation/debug condition is true to prevent spammy logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMakeLists.txt`:
- Line 20: The global add_compile_options(-mavx2 -mfma) is being applied
unconditionally; change it to only add those flags for x86/x86_64 toolchains or
attach them to specific targets instead. Locate the add_compile_options call in
CMakeLists and either (a) wrap it in a conditional checking the
architecture/compiler (e.g. using CMAKE_SYSTEM_PROCESSOR or
CMAKE_CXX_COMPILER_ID/CMAKE_SYSTEM_NAME to detect x86_64) before calling
add_compile_options, or (b) remove the global call and use
target_compile_options(<your_target> PRIVATE -mavx2 -mfma) for the specific
targets that need AVX2/FMA so non-x86 toolchains (arm64/macos) are not affected.
- Around line 75-95: Replace the hardcoded bgfx/.build shaderc paths by using
CMake's find_program to locate the shaderc binary and set SHADERC accordingly,
and keep setting SHADER_PLATFORM and SHADER_PROFILE per platform; if
find_program fails, call message(FATAL_ERROR ...) with a clear diagnostic so the
configure stops early; update every block that currently sets SHADERC (the
sections that set SHADER_PLATFORM/SHADER_PROFILE and the variables SHADERC,
SHADER_PLATFORM, SHADER_PROFILE) to use find_program("shaderc" ...) and fallback
logic instead of the fixed path.
In `@engine/native/core/io/image_loader.cpp`:
- Around line 41-44: The translation unit uses
std::numeric_limits<size_t>::max() but does not include the header that declares
it; add an explicit `#include` <limits> near the top of
engine/native/core/io/image_loader.cpp so the use of std::numeric_limits in the
bounds check (the w/h calculation and the if that calls stbi_image_free(data))
is well-formed and portable across toolchains.
In `@engine/native/main/main.cpp`:
- Around line 175-179: Guard against zero-sized window dimensions reported by
SDL before calling the resize functions: after calling SDL_GetWindowSize(window,
&w, &h) check that w > 0 and h > 0 (or clamp to a minimum of 1) and only then
call draco::rendering::rhi::resize((uint16_t)w, (uint16_t)h) and
draco::rendering::renderer::resize((uint16_t)w, (uint16_t)h); if either
dimension is zero, skip the resize to avoid producing invalid render/view state.
- Line 104: The quad UI pipeline is created with
draco::rendering::rhi::create_pipeline using
draco::rendering::rhi::BlendMode::None which forces opaque rendering; update the
BlendMode argument in the pipeline creation call that constructs pipeline_quad
to an alpha blending mode (e.g., draco::rendering::rhi::BlendMode::Alpha or the
library's equivalent) so transparent UI textures render correctly while keeping
the existing PipelineState flags (WriteRGB, WriteAlpha, MSAA) and other pipeline
settings unchanged.
In `@engine/native/rendering/CMakeLists.txt`:
- Around line 94-103: The renderer target is linking to scene and transform
before those targets are defined, which causes CMake to treat them as plain link
items; fix by ensuring the scene (and transform) subdirectories are added before
rendering in the parent CMakeLists (i.e., move add_subdirectory(scene) and
add_subdirectory(transform) ahead of add_subdirectory(rendering)), or
alternatively adjust the renderer linking to consume the targets only after they
exist (e.g., perform target_link_libraries(renderer PUBLIC scene transform) in
the rendering/CMakeLists.txt only when those targets are guaranteed to be
defined), making sure to modify the parent CMakeLists ordering or move the
target_link_libraries call so that the named targets `scene` and `transform` are
actual CMake targets when referenced.
In `@engine/native/rendering/material/material.cppm`:
- Around line 12-17: Uniform::data currently stores a raw pointer which can
dangle when materials are persisted; change the representation to own the bytes
(e.g., replace const void* data with a small owned buffer or vector<uint8_t>
inside struct Uniform) and implement a typed setter on Material (e.g.,
Material::SetUniform or Uniform::SetData) that takes a name/hash, typed value
pointer and size (or template overloads) and performs a memcpy into the owned
buffer when creating or updating a Uniform in Material::uniforms; ensure count
and size are stored with the uniform and update any usages that read
Uniform::data to read from the owned buffer instead.
In `@engine/native/rendering/mesh/mesh.cpp`:
- Around line 69-80: g_mesh_layout is only initialized in create_cube(), causing
create_plane/sphere/cylinder/capsule to use an invalid layout; ensure the vertex
layout is initialized on all creation paths by moving the initialization check
into a shared place (e.g., at the start of Mesh::create() or a helper like
ensure_mesh_layout()), so that before any primitive function (create_plane,
create_sphere, create_cylinder, create_capsule, create_cube) allocates or
uploads vertex/index data it calls the check that sets g_mesh_layout via
rhi::create_vertex_layout(desc) when g_mesh_layout == rhi::InvalidLayout; update
create(), create_plane(), create_sphere(), create_cylinder(), and create_capsule
to call that helper (or inline the check) so the layout is guaranteed valid
regardless of which primitive is created first.
- Around line 47-59: The code sets mesh.valid = true before verifying GPU
resource creation; change logic so you only mark and register the mesh if both
rhi::create_vertex_buffer (mesh.vbh) and rhi::create_index_buffer (mesh.ibh)
succeeded. After calling rhi::create_vertex_buffer and rhi::create_index_buffer,
check their return/handle validity (e.g. non-null/invalid-handle sentinel) and
only then set mesh.layout, mesh.vertex_count, mesh.index_count, mesh.valid =
true and call g_meshes.create(mesh); if either creation failed, release any
created resource, leave mesh.valid false, and return an appropriate failure
result instead of registering the mesh.
- Around line 107-149: The geometry factories (create_sphere, create_cylinder,
create_capsule) do not validate their inputs so zero/negative segments or rings
(and non-positive height for cylinder/capsule) can trigger division-by-zero and
bad buffers; add input validation at the top of each function to require
segments and rings > 0 (and height > 0 where used), log or assert on invalid
parameters, and return an explicit invalid MeshHandle (e.g.,
default-constructed/empty) instead of calling gen::sphere_vertices /
gen::cylinder_vertices / gen::capsule_vertices or inserting into g_mesh_cache
when inputs are invalid. Ensure you reference the same cache key logic
(g_mesh_cache, hash_combine/hash_mesh_params) only after validation passes.
In `@engine/native/rendering/quad_renderer/quad_renderer.cpp`:
- Around line 67-74: The code currently drops quads when the batch state changes
because submit() returns on state_change; instead, detect state change (new_key
!= m_batch_key), call the batch flush routine (e.g., flushCurrentBatch() or
implement flushBatch() if missing) to submit the existing batch and update
m_batch_key to new_key, then proceed to queue the incoming quad into the (now
cleared) batch; modify the block around m_batch_key/new_key so it flushes and
continues rather than returning, ensuring the quad is enqueued after flushing.
In `@engine/native/rendering/renderer/renderer.cppm`:
- Line 47: The exported declaration for submit_entity must match its
implementation: change the declaration of submit_entity to accept a const
reference to RenderPacket and the same view parameter (i.e., void
submit_entity(const draco::rendering::rhi::RenderPacket& packet, uint16_t
view);) so the signature matches the implementation; update the declaration in
the module interface where submit_entity is declared (symbol: submit_entity) and
rebuild to ensure no signature mismatch errors.
In `@engine/native/rendering/rendergraph/rendergraph.cpp`:
- Line 3: Remove the unnecessary C++23 header by deleting the `#include` <print>
directive in rendergraph.cpp (the include shown in the diff); this TU does not
use any print APIs and the stray include breaks builds on toolchains without
<print> support—remove that include and recompile to confirm the build error is
resolved.
In `@engine/native/rendering/rendergraph/rendergraph.cppm`:
- Around line 43-50: The Pass struct leaves runtime fields view, framebuffer,
view_mtx and proj_mtx uninitialized and execute() reads them unconditionally;
add safe default initializers (either default member initializers or set in
Pass's constructor) so view and framebuffer are initialized to their
invalid/zero sentinel (e.g. rhi::ViewID{} / rhi::FramebufferHandle{}) and both
view_mtx and proj_mtx are zeroed or set to identity as appropriate before use;
update Pass's definition (the members view, framebuffer, view_mtx, proj_mtx) so
they have deterministic defaults to prevent undefined behavior when execute() is
called on a partially configured Pass.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 118-122: bgfx::init(init) can fail but the current
RHI_ASSERT(false, ...) aborts the process so the subsequent "return false" is
never reached; change this so failures are reported but still return control.
Replace or modify the RHI_ASSERT usage in the bgfx::init failure branch: do not
call the aborting assert—log or emit a non-fatal error (using the project's
logging facility) and then return false; if you still want validation-time
diagnostics, wrap a non-aborting diagnostic helper around RHI_ASSERT or call a
separate RHI_LOG_ERROR before returning. Ensure the change is applied in the
failing branch that currently contains bgfx::init(init) and RHI_ASSERT.
In `@engine/native/rendering/rhi/uniform_registry.cppm`:
- Around line 14-34: The uniform registry currently only offers
register_uniform/get_uniform and can return stale UniformHandle values when
resources are destroyed; add an unregister API (e.g.,
unregister_uniform(uint32_t hash)) that erases entries from g_uniform_map and a
clear_uniforms() that clears the map, expose these alongside
hash_uniform/register_uniform/get_uniform/InvalidUniform, and update the uniform
destruction and system shutdown paths to call unregister_uniform(hash) when a
uniform is destroyed and clear_uniforms() on renderer shutdown to prevent stale
lookups.
In `@engine/native/rendering/shaders/vs.sc`:
- Around line 8-11: v_normal is left in object space while gl_Position is
transformed; rotate/scale will break lighting. Transform and normalize a_normal
before writing to v_normal using the correct normal matrix: compute v_normal by
multiplying a_normal by the 3x3 normal transform (e.g.,
mat3(transpose(inverse(u_model))) or a provided u_normalMatrix) and normalize
the result so the fragment stage receives world/view-space normals consistent
with gl_Position.
In `@engine/native/scene/CMakeLists.txt`:
- Around line 50-56: The current CMake target is created with add_library(scene)
and then registers module sources via target_sources(...) with FILE_SET
CXX_MODULES (e.g., scene.cppm), which bypasses the project's module helper;
replace the plain add_library usage with the project helper
add_modules_library(scene) so the target is created with the expected module
wiring and directory handling for module sources (update occurrences around
add_library(scene) and target_sources(scene ...) to use
add_modules_library(scene) instead).
In `@engine/native/scene/transform/transform.cppm`:
- Around line 13-20: The Transform struct fields are left uninitialized; update
the Transform definition so position and rotation are default-initialized to
{0,0,0} and scale to {1,1,1} (retain dirty = true) to ensure a safe
default-constructed state (i.e., add in-struct initializers for position,
rotation, and scale in the Transform struct used by make_transform()).
---
Minor comments:
In `@engine/native/rendering/quad_renderer/quad_renderer.cpp`:
- Around line 175-179: The orthographic build (build_ortho) currently divides by
width/height directly (rl = width, tb = height) causing invalid projections for
zero-sized windows; fix by clamping width and height to a safe non-zero value
before computing cam.proj (e.g. use float rl = max(width, 1e-6f) and float tb =
max(height, 1e-6f)) and then compute cam.proj[0] and cam.proj[5] with those
clamped values (cam.proj[0], cam.proj[5]) so division by zero cannot occur;
optionally set a debug/log path or early-return with an identity/projection-safe
result if both dimensions are effectively zero.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 578-581: The get_checked call for the index buffer is
unconditional and triggers warnings for packets that intentionally omit an index
buffer; modify the code in the submit path so you only call
get_checked(g_buffers, p.index_buffer, "IndexBuffer") when the packet's index
buffer handle is valid (e.g. check p.index_buffer for the "no index" sentinel
used in this codebase) and otherwise leave ib null/unused; keep get_checked for
pipeline and vertex buffer unchanged and ensure any later use of ib accounts for
the null/invalid case.
---
Duplicate comments:
In `@engine/native/platform/linux/linux.cpp`:
- Around line 12-31: The helper get_native_handles must not use std::string_view
on a possibly-null driver or call SDL_Quit(); instead, check if
SDL_GetCurrentVideoDriver() returned nullptr and if so return an invalid
NativeWindowFrame (set frame.valid = false) without touching SDL global state;
only compare driver strings when non-null. Also guard the property lookups: call
SDL_GetPointerProperty/SDL_GetNumberProperty only when
SDL_GetWindowProperties(window) yields valid props and treat any null
pointer/zero result as absent (leave frame.ndt/frame.nwh null). Keep setting
frame.width/frame.height via SDL_GetWindowSize and compute frame.valid as
(frame.nwh != nullptr) before returning.
---
Nitpick comments:
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 320-323: The RHI_WARN calls are unconditionally logging because
they pass false as the condition; change these to only emit when appropriate by
replacing the hardcoded false with a real check (e.g., a debug build flag or a
validation condition that compares layout.getStride() vs expected stride and
verifies offsets), or guard the block behind an `#ifdef` DEBUG/if
(enable_rhi_debug) toggle; update the calls referencing layout.getStride(),
layout.getOffset(map_attrib(Attrib::Position)),
layout.getOffset(map_attrib(Attrib::Color0)), and
layout.getOffset(map_attrib(Attrib::TexCoord0)) so they only execute when the
validation/debug condition is true to prevent spammy logs.
🪄 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: 4675533a-1f6c-4267-9af1-d69c5e4b72d3
📒 Files selected for processing (39)
CMakeLists.txtcmake/Compiler.cmakeengine/native/CMakeLists.txtengine/native/core/CMakeLists.txtengine/native/core/io/filesystem.cppengine/native/core/io/image_loader.cppengine/native/core/io/image_loader.cppmengine/native/core/memory/handle.cppmengine/native/core/memory/slot_array.cppmengine/native/main/main.cppengine/native/platform/CMakeLists.txtengine/native/platform/linux/linux.cppengine/native/rendering/CMakeLists.txtengine/native/rendering/material/material.cppmengine/native/rendering/mesh/mesh.cppengine/native/rendering/mesh/mesh.cppmengine/native/rendering/quad_renderer/quad_renderer.cppengine/native/rendering/quad_renderer/quad_renderer.cppmengine/native/rendering/renderer/renderer.cppengine/native/rendering/renderer/renderer.cppmengine/native/rendering/rendergraph/rendergraph.cppengine/native/rendering/rendergraph/rendergraph.cppmengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/uniform_registry.cppmengine/native/rendering/rhi/vertex.cppmengine/native/rendering/shaders/fs.scengine/native/rendering/shaders/fs_quad.scengine/native/rendering/shaders/varying.def.scengine/native/rendering/shaders/varying_quad.def.scengine/native/rendering/shaders/vs.scengine/native/rendering/shaders/vs_quad.scengine/native/scene/CMakeLists.txtengine/native/scene/camera/camera_controller.cppmengine/native/scene/renderable/renderable.cppmengine/native/scene/scene.cppmengine/native/scene/transform/transform.cppengine/native/scene/transform/transform.cppmengine/native/thirdparty/CMakeLists.txt
✅ Files skipped from review due to trivial changes (2)
- engine/native/rendering/shaders/fs_quad.sc
- engine/native/scene/renderable/renderable.cppm
🚧 Files skipped from review as they are similar to previous changes (7)
- engine/native/core/io/image_loader.cppm
- engine/native/scene/camera/camera_controller.cppm
- engine/native/platform/CMakeLists.txt
- engine/native/core/memory/slot_array.cppm
- engine/native/core/CMakeLists.txt
- engine/native/rendering/rhi/rhi.cppm
- engine/native/core/memory/handle.cppm
Comment on lines
+8
to
+11
| gl_Position = mul(u_modelViewProj, vec4(a_position, 1.0)); | ||
|
|
||
| v_normal = a_normal; | ||
| v_texcoord0 = a_texcoord0; |
Contributor
There was a problem hiding this comment.
Transform normals before passing to the fragment stage.
gl_Position is transformed at Line 8, but v_normal at Line 10 stays in object space. Rotated/scaled meshes will shade incorrectly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/rendering/shaders/vs.sc` around lines 8 - 11, v_normal is left
in object space while gl_Position is transformed; rotate/scale will break
lighting. Transform and normalize a_normal before writing to v_normal using the
correct normal matrix: compute v_normal by multiplying a_normal by the 3x3
normal transform (e.g., mat3(transpose(inverse(u_model))) or a provided
u_normalMatrix) and normalize the result so the fragment stage receives
world/view-space normals consistent with gl_Position.
mcdubhghlas
reviewed
May 18, 2026
Member
mcdubhghlas
left a comment
mcdubhghlas
left a comment
There was a problem hiding this comment.
It doesn't compile for me. Here is the log: log.txt
If we can get this to compile, I'll hit approve and we can move on. A cursory glance over the actual code itself seemed fine to me.
mcdubhghlas
approved these changes
May 18, 2026
Member
mcdubhghlas
left a comment
mcdubhghlas
left a comment
There was a problem hiding this comment.
I was able to compile it via
cmake --preset release -DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld"
then
cmake --build build/release
BUT Old Gamer is going to review it a bit more, so if he says something seems off - Disregard my approval. Otherwise, we're solid!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Note
Contributed by 2LazyDevs.
Summary by CodeRabbit
Release Notes
New Features
Build Changes