Add rendering stuff, cross-platform support, camera controller & SDL input logic#16
Add rendering stuff, cross-platform support, camera controller & SDL input logic#16AR-DEV-1 wants to merge 16 commits intoRedot-Engine:masterfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 architecture for the Draconic Engine using C++20 modules and SDL3. It establishes foundational systems (IO, memory, input, platform integration), a BGFX-backed RHI layer, render graph orchestration, shader infrastructure, and a main executable with initialization and per-frame rendering loop. ChangesRendering Architecture Overhaul
Sequence DiagramsequenceDiagram
participant App as Application<br/>(main.cpp)
participant SDL3 as SDL3<br/>(Window)
participant Platform as Platform<br/>(Native Handles)
participant RHI as RHI/BGFX<br/>(GPU Interface)
participant RenderGraph as Render Graph<br/>(Pass Orchestration)
participant Renderer as Renderer<br/>(Frame Coordinator)
participant Camera as Camera<br/>(Input→View)
App->>SDL3: Create Window (1280×720)
SDL3-->>App: Window Handle
App->>Platform: get_native_handles(sdl_window)
Platform-->>App: NativeWindowFrame
App->>RHI: init(display, window, width, height)
RHI->>RHI: Configure BGFX, setup view 0
RHI-->>App: Initialized
App->>RHI: load_shader(vertex_data)
RHI->>RHI: Create BGFX Program
RHI-->>App: ShaderHandle
App->>RHI: create_vertex_buffer(quad_data)
RHI-->>App: BufferHandle
App->>RHI: create_pipeline(PipelineDesc)
RHI-->>App: PipelineHandle
loop Render Loop
App->>Camera: update(dt)
Camera->>Camera: Integrate Input (WASD, mouse)
Camera-->>App: Camera transform
App->>Renderer: begin_frame(camera)
Renderer->>RenderGraph: reset()
Renderer->>RenderGraph: add_pass()
Renderer-->>App: Ready for submission
App->>Renderer: submit_entity(packet, view)
Renderer->>RenderGraph: add packet to pass
App->>Renderer: end_frame()
Renderer->>RenderGraph: execute()
RenderGraph->>RHI: apply_view(view)
RenderGraph->>RHI: set_view_projection(proj)
RenderGraph->>RHI: submit(packet)
RHI->>RHI: Bind buffers/textures/uniforms
RHI->>RHI: Issue draw call via BGFX
RHI->>RHI: begin_frame() / end_frame()
end
App->>RHI: shutdown()
RHI->>RHI: Destroy resources, shutdown BGFX
App->>SDL3: Destroy Window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This PR introduces substantial new functionality across many interdependent systems: a complete modular architecture with C++ modules, a BGFX-backed rendering abstraction with resource management, platform integration for three OSs, input handling, scene components, and shader infrastructure. The density of logic spans RHI initialization (GPU state management with registries and deletion queues), render graph design (pass orchestration and view transforms), camera controller mathematics (angle/direction computation), and cross-module wiring. While individual files are moderately sized, the heterogeneity of implementation patterns (raw pointer handling in platform code, template-based memory management, shader compilation directives, per-platform conditionals) and the need to verify correct data flow through multiple abstraction layers increases review complexity. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
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
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
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
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>
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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
1 participant
TL;DR
Note
Contributed by 2LazyDevs.
Summary by CodeRabbit
Release Notes