Create the Rendering Hardware Abstraction (RHI)#16
Open
AR-DEV-1 wants to merge 2 commits intoRedot-Engine:masterfrom
Open
Create the Rendering Hardware Abstraction (RHI)#16AR-DEV-1 wants to merge 2 commits intoRedot-Engine:masterfrom
AR-DEV-1 wants to merge 2 commits intoRedot-Engine:masterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an SDL3 submodule and integrates a bgfx-backed renderer: new draconic executable, filesystem module, exported RHI API plus bgfx backend, shader sources, main SDL-driven entry, and CMake/submodule updates to build and link these components. Changes
Sequence Diagram(s)sequenceDiagram
participant App as main.cpp
participant SDL as SDL3
participant FS as Filesystem
participant RHI as RHI (bgfx)
participant GPU as GPU / bgfx
App->>SDL: SDL_Init / SDL_CreateWindow
SDL-->>App: Window handle / native display
App->>FS: load_binary("vs_triangle.bin"/"fs_triangle.bin")
FS-->>App: binary data
App->>RHI: init(display, window, 1280x720)
RHI->>GPU: bgfx::init / bgfx::reset
App->>RHI: create_shader(vs_data)
RHI->>GPU: bgfx::createShader(...)
RHI-->>App: ShaderHandle
App->>RHI: create_shader(fs_data)
RHI->>GPU: bgfx::createShader(...)
RHI-->>App: ShaderHandle
App->>RHI: create_pipeline(PipelineDesc{vs,fs})
RHI->>GPU: bgfx::createProgram(...)
RHI-->>App: PipelineHandle
App->>RHI: create_vertex_buffer(vertices)
RHI->>GPU: bgfx::createVertexBuffer(...)
RHI-->>App: BufferHandle
loop Main Loop
App->>SDL: SDL_PollEvent(...)
App->>RHI: resize(width,height) [on resize]
RHI->>GPU: bgfx::reset(...)
App->>RHI: begin_frame()
RHI->>GPU: setViewRect / set transforms
App->>RHI: submit(RenderPacket)
RHI->>GPU: setVertexBuffer / setState / submit
App->>RHI: end_frame()
RHI->>GPU: bgfx::frame()
end
App->>RHI: shutdown()
RHI->>GPU: bgfx::shutdown()
App->>SDL: SDL_DestroyWindow / SDL_Quit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AR-DEV-1
force-pushed
the
rendering-hardware-abstraction
branch
from
1928886 to
2e90dc6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
engine/native/core/filesystem/filesystem.cpp (2)
21-23: Error message references "shader" in generic function.Same issue as above - the message should be generic.
Proposed fix
if (size <= 0) { - std::println("Error: Shader file is empty: {}", path); + std::println("Error: File is empty or size unavailable: {}", path); return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/filesystem/filesystem.cpp` around lines 21 - 23, The error message in the empty-file check uses a shader-specific message; update the std::println call in filesystem.cpp (the block checking "if (size <= 0)" that references variables size and path) to use a generic message such as "Error: File is empty: {}" (or similar) so it no longer mentions "Shader"; ensure the change is applied to the std::println invocation that currently reads "Error: Shader file is empty: {}" and preserves the path interpolation.
14-18: Generic function has shader-specific error messages.
load_binaryis a general-purpose filesystem function, but the error messages reference "shader file". This will be confusing when the function is used for non-shader assets.Proposed fix
if (!file.is_open()) { - std::println("Error: Could not find shader file at: {}", path); + std::println("Error: Could not open file at: {}", path); // Return an empty vector return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/filesystem/filesystem.cpp` around lines 14 - 18, The error message inside load_binary currently says "shader file" which is misleading for a general-purpose function; change the std::println call in the load_binary function (the branch checking file.is_open()) to a generic message such as "Error: Could not open file at: {}" or "Error: Failed to open file: {}" and keep returning the empty vector; ensure you still include the path variable in the formatted output so callers see which file failed.engine/native/rendering/rhi/rhi_bgfx.cpp (1)
115-121: Vertex layout is duplicated fromvertex.cppm.This layout definition duplicates
PosColorVertex::init()invertex.cppm. If the vertex format changes, both locations must be updated, risking inconsistency.Consider using the centralized definition from
PosColorVertexor removing the unused code fromvertex.cppm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 115 - 121, The vertex layout in create_vertex_buffer duplicates PosColorVertex::init() from vertex.cppm; update create_vertex_buffer to reuse the centralized definition instead of redefining it: replace the inline bgfx::VertexLayout construction in create_vertex_buffer with a call to the shared initializer (e.g., PosColorVertex::init(layout) or use PosColorVertex::layout if a static layout exists), and remove the redundant layout code (or alternatively consolidate by moving the single authoritative layout to PosColorVertex and updating both call sites to reference it).CMakeLists.txt (2)
49-50: X11 pkg-config results are discovered but not used.
pkg_check_modulespopulates${X11_LIBS_LIBRARIES},${X11_LIBS_INCLUDE_DIRS}, and${X11_LIBS_LDFLAGS}, but lines 64-70 hardcode the library names instead. This loses pkg-config benefits (correct paths, linker flags, transitive dependencies).Proposed fix using pkg-config results
target_link_libraries(draconic PRIVATE rhi core bgfx bx bimg -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive c++ c++abi - X11 - Xext - Xcursor - Xrandr - Xrender - Xi - Xfixes + ${X11_LIBS_LIBRARIES} dl pthread m ) +target_include_directories(draconic PRIVATE ${X11_LIBS_INCLUDE_DIRS})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 49 - 50, The pkg-config results from pkg_check_modules(X11_LIBS ...) are discovered but not used; replace the hardcoded X11 library names with the pkg-config variables: add the include dirs from ${X11_LIBS_INCLUDE_DIRS} (via target_include_directories or include_directories), add ${X11_LIBS_LDFLAGS} to the target link/compile flags if needed (or use target_link_libraries with ${X11_LIBS_LIBRARIES}), and remove the hardcoded library list so transitive deps and correct paths from X11_LIBS are honored.
52-74: Linux-specific build logic lacks platform guards.The
--whole-archivelinker flags, explicitc++/c++abilinking, and X11 dependencies are Linux/Clang-specific. This will break builds on other platforms (macOS, Windows). Since this is WIP and Linux-focused, consider adding a guard or TODO.Example platform guard
+if(CMAKE_SYSTEM_NAME STREQUAL "Linux") add_executable(draconic engine/native/main/main.cpp) enable_modules(draconic) target_link_libraries(draconic PRIVATE rhi core ... ) +else() + message(WARNING "draconic executable is currently only supported on Linux") +endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 52 - 74, The Linux-specific linker and dependency lines in the add_executable("draconic" / engine/native/main/main.cpp) and target_link_libraries(draconic ...) block (the -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive, explicit c++ / c++abi, and X11-related libs) need to be guarded so they only apply on Linux; update the CMake logic to wrap these platform-specific link flags/dependencies with a conditional that detects Linux (e.g., UNIX and not APPLE) and keep the generic target_link_libraries entries for cross-platform deps outside that guard, leaving a TODO comment if this is WIP.engine/native/rendering/rhi/rhi.cppm (1)
7-10: Consider defining an explicit invalid handle constant.The implementation (
rhi_bgfx.cpp:150) usesUINT16_MAXas a sentinel for "no index buffer", but this isn't documented in the interface. Defining an explicit constant would make the API clearer:constexpr BufferHandle InvalidBuffer = UINT16_MAX;This is optional for WIP code but would improve API clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi.cppm` around lines 7 - 10, Define and document an explicit invalid-handle constant for BufferHandle (and optionally for PipelineHandle/ShaderHandle/ViewID) so callers and implementations don't rely on magic UINT16_MAX; add a constexpr like InvalidBuffer (using BufferHandle) and update uses (e.g., the place that checks for UINT16_MAX as "no index buffer" in rhi_bgfx.cpp where BufferHandle is handled) to compare against that named constant instead of UINT16_MAX, and update any comments/signatures to mention the sentinel.engine/native/main/main.cpp (1)
36-46: Only Linux/X11 is supported; consider adding a note for Wayland/other platforms.The native handle extraction only handles X11 via the
#if defined(__linux__)block. SDL3 also supports Wayland on Linux, and the engine will likely need macOS/Windows support. As this is WIP, consider adding a TODO or fallback diagnostic.Example Wayland placeholder
`#if` defined(__linux__) SDL_PropertiesID props = SDL_GetWindowProperties(window); if (driver && std::string_view(driver) == "x11") { ndt = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_X11_DISPLAY_POINTER, nullptr); nwh = (void*)(uintptr_t)SDL_GetNumberProperty(props, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, 0); } + else if (driver && std::string_view(driver) == "wayland") + { + // TODO: Add Wayland support + // ndt = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_WAYLAND_DISPLAY_POINTER, nullptr); + // nwh = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_WAYLAND_SURFACE_POINTER, nullptr); + std::println("Wayland support not yet implemented"); + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/main/main.cpp` around lines 36 - 46, The current Linux-only native handle extraction uses SDL_GetWindowProperties and X11-specific properties (SDL_GetPointerProperty with SDL_PROP_WINDOW_X11_DISPLAY_POINTER and SDL_PROP_WINDOW_X11_WINDOW_NUMBER) when driver == "x11"; add a short TODO and a fallback diagnostic path for Wayland/other platforms: detect when driver is not "x11" (or when SDL reports Wayland) and log or assert a clear message indicating unsupported backend, and leave a TODO in the SDL_GetWindowProperties/ndt/nwh handling to implement Wayland/macOS/Windows-specific property extraction later; reference SDL_GetWindowProperties, SDL_GetPointerProperty, SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, driver, ndt, and nwh so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 39-41: The C flag variable CMAKE_C_FLAGS incorrectly receives the
C++-only flag "-stdlib=libc++", causing C compiler warnings/errors; remove
"-stdlib=libc++" from CMAKE_C_FLAGS and ensure "-stdlib=libc++" (and "-lc++abi"
linker flags) are only appended to CMAKE_CXX_FLAGS and CMAKE_EXE_LINKER_FLAGS
(or conditionally added when the compiler is a C++ compiler) so that
CMAKE_CXX_FLAGS gets the C++ specific flag while CMAKE_C_FLAGS remains clean.
In `@engine/native/core/filesystem/filesystem.cpp`:
- Around line 29-33: When file.read(reinterpret_cast<char*>(buffer.data()),
size) fails the function currently returns an empty vector silently; change it
to emit a diagnostic before returning (e.g., log or throw) that includes the
filename/size and the system error (use errno/std::strerror(errno) or
std::error_code) so failures are visible; update the failure path that
references file.read and buffer to call the project's logger (or std::cerr) with
a clear message or throw a std::runtime_error containing the error details
instead of returning {}.
In `@engine/native/main/main.cpp`:
- Around line 125-127: The main function in main.cpp lacks an explicit success
return; add an explicit "return 0;" as the final statement in the main()
function (after the comment "// Fun fact: AR literally went mad & tis is the
result") so successful execution returns 0 consistently with the existing error
returns of -1 and improves clarity.
In `@engine/native/rendering/CMakeLists.txt`:
- Around line 1-3: The build currently adds the rhi target but never compiles
the shader source files; add add_custom_command rules that invoke shaderc on
engine/native/rendering/shaders/vs.sc, fs.sc and varying.def.sc to produce
vs_triangle.bin and fs_triangle.bin (and any varying/binary outputs) and place
them in the folder where the runtime loader expects them, then create a custom
target (e.g., rhi_shaders) that depends on those outputs and make the rhi target
depend on that custom target (use add_dependencies(rhi, rhi_shaders)) so the
shader binaries are generated before rhi is built; reference the shaderc
executable defined in thirdparty CMake (use the same variable/name) and ensure
the outputs are listed in the add_custom_command so CMake tracks them.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 50-53: The init() implementation in rhi_bgfx.cpp calls
bgfx::init(init) and returns early on failure without informing callers; change
the RHI init contract to return a bool (or otherwise signal failure) and
propagate that to main.cpp so callers can abort or handle the error: update the
rhi_bgfx.cpp init() to return false when bgfx::init(init) fails (instead of only
printing), update the RHI interface/signature used by main.cpp to accept the
bool return and check it, and ensure main.cpp stops initialization or reports
the error if init() returns false; alternatively, if changing the signature is
impossible, introduce a clearly named initialized flag (e.g.,
RHI_Bgfx::initialized) that init() sets to true/false and make callers
(main.cpp) check that flag before using the RHI.
- Around line 142-145: The submit() function dereferences handles without
validation which can cause out-of-bounds access when p.pipeline or
p.vertex_buffer are invalid; add defensive bounds checks before using
g_pipelines and g_buffers (check p.pipeline < g_pipelines.size() and
p.vertex_buffer < g_buffers.size()), and if a check fails, log or assert with
context (e.g., include the invalid index and ViewID) and early-return or use a
safe fallback pipeline/buffer so the function cannot crash; update submit(),
referencing the symbols submit, Pipeline, Buffer, g_pipelines, g_buffers,
p.pipeline and p.vertex_buffer.
In `@engine/native/rendering/rhi/vertex.cppm`:
- Around line 16-24: PosColorVertex::init() and its static layout are unused and
duplicate the bgfx::VertexLayout built inline in create_vertex_buffer(); either
delete both init() and the static layout from vertex.cppm to remove dead code,
or refactor create_vertex_buffer() to reuse PosColorVertex::layout by (a)
exposing a single initializer (e.g., keep PosColorVertex::init() or a function
that returns/ensures the static layout is initialized) and (b) replacing the
inline bgfx::VertexLayout in create_vertex_buffer() with a reference to
PosColorVertex::layout so the vertex format is defined in one place (ensure the
layout is initialized before use).
In `@engine/native/thirdparty/CMakeLists.txt`:
- Line 17: Move the set(CMAKE_POSITION_INDEPENDENT_CODE ON) line to before the
add_subdirectory() calls so that bx, bimg, and bgfx are built with
position-independent code; specifically, open the thirdparty CMakeLists (where
add_subdirectory(bx), add_subdirectory(bimg), add_subdirectory(bgfx) are
invoked) and place the CMAKE_POSITION_INDEPENDENT_CODE ON setting above those
add_subdirectory(...) lines to ensure static libraries used by the SHARED rhi
target are compiled PIC.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 49-50: The pkg-config results from pkg_check_modules(X11_LIBS ...)
are discovered but not used; replace the hardcoded X11 library names with the
pkg-config variables: add the include dirs from ${X11_LIBS_INCLUDE_DIRS} (via
target_include_directories or include_directories), add ${X11_LIBS_LDFLAGS} to
the target link/compile flags if needed (or use target_link_libraries with
${X11_LIBS_LIBRARIES}), and remove the hardcoded library list so transitive deps
and correct paths from X11_LIBS are honored.
- Around line 52-74: The Linux-specific linker and dependency lines in the
add_executable("draconic" / engine/native/main/main.cpp) and
target_link_libraries(draconic ...) block (the -Wl,--whole-archive
SDL3::SDL3-static -Wl,--no-whole-archive, explicit c++ / c++abi, and X11-related
libs) need to be guarded so they only apply on Linux; update the CMake logic to
wrap these platform-specific link flags/dependencies with a conditional that
detects Linux (e.g., UNIX and not APPLE) and keep the generic
target_link_libraries entries for cross-platform deps outside that guard,
leaving a TODO comment if this is WIP.
In `@engine/native/core/filesystem/filesystem.cpp`:
- Around line 21-23: The error message in the empty-file check uses a
shader-specific message; update the std::println call in filesystem.cpp (the
block checking "if (size <= 0)" that references variables size and path) to use
a generic message such as "Error: File is empty: {}" (or similar) so it no
longer mentions "Shader"; ensure the change is applied to the std::println
invocation that currently reads "Error: Shader file is empty: {}" and preserves
the path interpolation.
- Around line 14-18: The error message inside load_binary currently says "shader
file" which is misleading for a general-purpose function; change the
std::println call in the load_binary function (the branch checking
file.is_open()) to a generic message such as "Error: Could not open file at: {}"
or "Error: Failed to open file: {}" and keep returning the empty vector; ensure
you still include the path variable in the formatted output so callers see which
file failed.
In `@engine/native/main/main.cpp`:
- Around line 36-46: The current Linux-only native handle extraction uses
SDL_GetWindowProperties and X11-specific properties (SDL_GetPointerProperty with
SDL_PROP_WINDOW_X11_DISPLAY_POINTER and SDL_PROP_WINDOW_X11_WINDOW_NUMBER) when
driver == "x11"; add a short TODO and a fallback diagnostic path for
Wayland/other platforms: detect when driver is not "x11" (or when SDL reports
Wayland) and log or assert a clear message indicating unsupported backend, and
leave a TODO in the SDL_GetWindowProperties/ndt/nwh handling to implement
Wayland/macOS/Windows-specific property extraction later; reference
SDL_GetWindowProperties, SDL_GetPointerProperty,
SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, driver,
ndt, and nwh so the change is easy to locate.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 115-121: The vertex layout in create_vertex_buffer duplicates
PosColorVertex::init() from vertex.cppm; update create_vertex_buffer to reuse
the centralized definition instead of redefining it: replace the inline
bgfx::VertexLayout construction in create_vertex_buffer with a call to the
shared initializer (e.g., PosColorVertex::init(layout) or use
PosColorVertex::layout if a static layout exists), and remove the redundant
layout code (or alternatively consolidate by moving the single authoritative
layout to PosColorVertex and updating both call sites to reference it).
In `@engine/native/rendering/rhi/rhi.cppm`:
- Around line 7-10: Define and document an explicit invalid-handle constant for
BufferHandle (and optionally for PipelineHandle/ShaderHandle/ViewID) so callers
and implementations don't rely on magic UINT16_MAX; add a constexpr like
InvalidBuffer (using BufferHandle) and update uses (e.g., the place that checks
for UINT16_MAX as "no index buffer" in rhi_bgfx.cpp where BufferHandle is
handled) to compare against that named constant instead of UINT16_MAX, and
update any comments/signatures to mention the sentinel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c45da71e-d688-4600-ad17-e177e862e7ff
📒 Files selected for processing (19)
.gitmodulesCMakeLists.txtCMakePresets.jsoncmake/Compiler.cmakeengine/native/CMakeLists.txtengine/native/core/CMakeLists.txtengine/native/core/filesystem/filesystem.cppengine/native/core/filesystem/filesystem.cppmengine/native/main/main.cppengine/native/rendering/CMakeLists.txtengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/vertex.cppmengine/native/rendering/shaders/fs.scengine/native/rendering/shaders/varying.def.scengine/native/rendering/shaders/vs.scengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/bxengine/native/thirdparty/sdl
💤 Files with no reviewable changes (2)
- CMakePresets.json
- cmake/Compiler.cmake
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
CMakeLists.txt (1)
39-41:⚠️ Potential issue | 🟠 MajorGuard the libc++ flags by toolchain and remove them from
CMAKE_C_FLAGS.Line 40 still pushes the C++-only
-stdlib=libc++switch into C compilations, and Lines 39-41 are unconditional even though this setup only makes sense for a Clang/libc++ toolchain. Either fail fast on non-Clang toolchains or add these switches only under a Clang C++ branch.Suggested fix
+if (NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang") + message(FATAL_ERROR "DraconicEngine currently requires Clang/libc++") +endif() + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++") -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -stdlib=libc++") set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -stdlib=libc++ -lc++abi")Is `-stdlib=libc++` valid for C compilation, and which compilers/toolchains support that flag?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 39 - 41, The libc++ flags are being applied unconditionally (and mistakenly to C code via CMAKE_C_FLAGS); guard them by detecting the C++ compiler and only apply to Clang-like toolchains: wrap the set(...) calls in an if that checks CMAKE_CXX_COMPILER_ID for "Clang" or "AppleClang" (e.g. if(CMAKE_CXX_COMPILER_ID MATCHES "Clang|AppleClang")), remove the line setting CMAKE_C_FLAGS, and either add an else branch that emits a clear FATAL_ERROR when a non-Clang C++ compiler is detected or simply skip applying the flags for other toolchains so only set CMAKE_CXX_FLAGS and CMAKE_EXE_LINKER_FLAGS inside that Clang-specific block.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
61-61: Use CMake's native whole-archive handling here.Since this project requires CMake 4.0 (which supports the feature), replace the GNU-style linker flags with
$<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static>. This is the portable, standard CMake way to express whole-archive semantics and will automatically map to the correct linker syntax across different toolchains (GNU/Clang--whole-archive, MSVC/WHOLEARCHIVE, etc.).Suggested refactor
- -Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive + "$<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 61, Replace the GNU-specific linker flags used to force whole-archive linking with CMake's native generator expression: locate the target link libraries entry that currently uses "-Wl,--whole-archive SDL3::SDL3-static -Wl,--no-whole-archive" and swap it to use the CMake LINK_LIBRARY generator expression $<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static> so CMake emits the correct whole-archive directive for the active toolchain (reference SDL3::SDL3-static in the target_link_libraries or add_link_options usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 28-31: Replace the host-dependent -march=native option in the
Clang branch with the repository's explicit ISA baseline used in
cmake/Compiler.cmake (AVX2/FMA); update the add_compile_options call inside the
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") block to use the explicit flags
representing AVX2/FMA (e.g., -mavx2 and -mfma or the equivalent -march=haswell)
so the build is consistent across hosts and matches the project's declared
baseline.
- Around line 76-84: The runtime fails to find shader files because CMake copies
them into a "shaders" subfolder but the loader in engine/native/main/main.cpp
calls load_binary("vs_triangle.bin") and load_binary("fs_triangle.bin") with no
directory; either update those calls to load_binary("shaders/vs_triangle.bin")
and load_binary("shaders/fs_triangle.bin") in main.cpp, or change the CMake
target command (the add_custom_command that uses
$<TARGET_FILE_DIR:draconic>/shaders) to copy directly into
$<TARGET_FILE_DIR:draconic> (remove the "/shaders" suffix) so the copied files
match the existing load paths.
- Around line 49-50: The pkg-config call currently ignores discovered flags
because pkg_check_modules is not creating an imported target; change the
pkg_check_modules(X11_LIBS REQUIRED x11 xext xcursor xrandr xrender xi xfixes)
invocation to create an imported target (use the IMPORTED_TARGET option) so it
registers PkgConfig::X11_LIBS, then update the target_link_libraries invocation
that currently lists hardcoded library names (e.g., X11, Xext, Xcursor, Xrandr,
Xrender, Xi, Xfixes) to link against the PkgConfig::X11_LIBS imported target
instead so compiler/linker search paths and flags discovered by pkg-config are
used.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 39-41: The libc++ flags are being applied unconditionally (and
mistakenly to C code via CMAKE_C_FLAGS); guard them by detecting the C++
compiler and only apply to Clang-like toolchains: wrap the set(...) calls in an
if that checks CMAKE_CXX_COMPILER_ID for "Clang" or "AppleClang" (e.g.
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang|AppleClang")), remove the line setting
CMAKE_C_FLAGS, and either add an else branch that emits a clear FATAL_ERROR when
a non-Clang C++ compiler is detected or simply skip applying the flags for other
toolchains so only set CMAKE_CXX_FLAGS and CMAKE_EXE_LINKER_FLAGS inside that
Clang-specific block.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Line 61: Replace the GNU-specific linker flags used to force whole-archive
linking with CMake's native generator expression: locate the target link
libraries entry that currently uses "-Wl,--whole-archive SDL3::SDL3-static
-Wl,--no-whole-archive" and swap it to use the CMake LINK_LIBRARY generator
expression $<LINK_LIBRARY:WHOLE_ARCHIVE,SDL3::SDL3-static> so CMake emits the
correct whole-archive directive for the active toolchain (reference
SDL3::SDL3-static in the target_link_libraries or add_link_options usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17a47ca7-3f0c-44f0-b314-5fa3cd83dd70
⛔ Files ignored due to path filters (2)
engine/native/rendering/shaders/fs_triangle.binis excluded by!**/*.binengine/native/rendering/shaders/vs_triangle.binis excluded by!**/*.bin
📒 Files selected for processing (1)
CMakeLists.txt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
engine/native/core/filesystem/filesystem.cpp (1)
9-35: Error messages are shader-specific but function is generic.The error messages at lines 15 and 22 reference "shader file" but
load_binary()is a general-purpose function. Consider using generic messages like "Could not open file" and "File is empty" to match the function's broader purpose.Proposed fix
if (!file.is_open()) { - std::println("Error: Could not find shader file at: {}", path); + std::println("Error: Could not open file at: {}", path); // Return an empty vector return {}; } std::streamsize size = file.tellg(); if (size <= 0) { - std::println("Error: Shader file is empty: {}", path); + std::println("Error: File is empty or unreadable: {}", path); return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/filesystem/filesystem.cpp` around lines 9 - 35, The error messages in load_binary are shader-specific; change them to generic file messages: replace "Could not find shader file" and "Shader file is empty" with general phrases like "Could not open file" and "File is empty" (refer to the load_binary function and its local ifstream variable 'file' and the error paths that call std::println), and ensure the final read-failure message stays generic ("Failed to read file contents") so the function remains reusable for any binary file.engine/native/thirdparty/CMakeLists.txt (1)
16-31: SDL configuration looks appropriate for X11-only static linking.The settings correctly disable shared builds, Wayland, and shared audio backends while enabling X11. One minor note: the file is missing a newline at end of file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/thirdparty/CMakeLists.txt` around lines 16 - 31, The file is missing a trailing newline at EOF; open the CMakeLists.txt containing the SDL configuration (the block with set(SDL_MAIN_HANDLED ON ... set(SDL_X11 ON ...) and add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/sdl sdl-build)) and add a single newline character at the end of the file so the file ends with a newline.engine/native/rendering/rhi/rhi_bgfx.cpp (1)
106-115: Note:createProgramwithdestroyShaders=trueinvalidates input handles.The third parameter
truetobgfx::createProgram()causes the vertex and fragment shader handles to be destroyed after the program is created. This meansdesc.vsanddesc.fsbecome invalid after this call and cannot be reused for other pipelines.This is likely intentional for simple use cases, but if shader reuse across pipelines is ever needed, this behavior would need to change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 106 - 115, create_pipeline currently calls bgfx::createProgram(vs, fs, true) which destroys the shader handles and invalidates desc.vs/desc.fs; change this to not auto-destroy so shaders can be reused: in create_pipeline (and related code using PipelineDesc and desc.vs/desc.fs) call bgfx::createProgram(vs, fs, false) and ensure shader lifetime is managed explicitly (e.g., destroy shaders only when they are truly no longer needed or add a shader ownership/cleanup path) so desc.vs and desc.fs remain valid for reuse across pipelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/native/main/main.cpp`:
- Around line 58-59: draco::rhi::init(ndt, nwh, 1280, 720) now returns a bool
indicating success; update the call site to check that return value and handle
failure (e.g., log an error and abort/return non-zero) to avoid subsequent RHI
calls crashing. Specifically, in main where draco::rhi::init(ndt, nwh, 1280,
720) is invoked, capture the bool result, call your logging/error path (or
std::cerr/process exit) on false, and ensure you do not proceed to further
draco::rhi::* calls when init fails.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 156-161: The code accesses g_buffers[p.index_buffer] when
p.index_buffer != UINT16_MAX without verifying the index is within g_buffers
bounds; add a bounds check before indexing: ensure p.index_buffer <
g_buffers.size() (or the appropriate buffer count) before reading
g_buffers[p.index_buffer] and only then assign Buffer& ib and call
bgfx::setIndexBuffer(ib.ibh); keep the existing UINT16_MAX sentinel check and
handle the out-of-range case (skip setting the index buffer and optionally log
or assert) in the same block where p.index_buffer is tested.
---
Nitpick comments:
In `@engine/native/core/filesystem/filesystem.cpp`:
- Around line 9-35: The error messages in load_binary are shader-specific;
change them to generic file messages: replace "Could not find shader file" and
"Shader file is empty" with general phrases like "Could not open file" and "File
is empty" (refer to the load_binary function and its local ifstream variable
'file' and the error paths that call std::println), and ensure the final
read-failure message stays generic ("Failed to read file contents") so the
function remains reusable for any binary file.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 106-115: create_pipeline currently calls bgfx::createProgram(vs,
fs, true) which destroys the shader handles and invalidates desc.vs/desc.fs;
change this to not auto-destroy so shaders can be reused: in create_pipeline
(and related code using PipelineDesc and desc.vs/desc.fs) call
bgfx::createProgram(vs, fs, false) and ensure shader lifetime is managed
explicitly (e.g., destroy shaders only when they are truly no longer needed or
add a shader ownership/cleanup path) so desc.vs and desc.fs remain valid for
reuse across pipelines.
In `@engine/native/thirdparty/CMakeLists.txt`:
- Around line 16-31: The file is missing a trailing newline at EOF; open the
CMakeLists.txt containing the SDL configuration (the block with
set(SDL_MAIN_HANDLED ON ... set(SDL_X11 ON ...) and
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/sdl sdl-build)) and add a single
newline character at the end of the file so the file ends with a newline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4605d44-cf86-44ef-bfaf-24cf3828e471
📒 Files selected for processing (7)
CMakeLists.txtengine/native/core/filesystem/filesystem.cppengine/native/main/main.cppengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/vertex.cppmengine/native/thirdparty/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- engine/native/rendering/rhi/rhi.cppm
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/native/rendering/rhi/vertex.cppm
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
AR-DEV-1
force-pushed
the
rendering-hardware-abstraction
branch
from
08e0f83 to
6458806
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
engine/native/rendering/rhi/rhi_bgfx.cpp (1)
117-132: Consider making the vertex layout configurable.The vertex layout is hardcoded to
Position (3 floats) + Color0 (4 Uint8). While this matches the currentPosColorVertexformat invertex.cppm, this tight coupling may limit flexibility when additional vertex formats are needed (e.g., vertices with normals, UVs, or tangents).For now this is acceptable for the initial implementation, but consider parameterizing the layout or using a vertex format descriptor in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/rendering/rhi/rhi_bgfx.cpp` around lines 117 - 132, The create_vertex_buffer function hardcodes a bgfx::VertexLayout (Position + Color0) which couples vertex format to this implementation; modify create_vertex_buffer (or add an overload) to accept a vertex layout descriptor (e.g., a bgfx::VertexLayout or a lightweight vertex format enum/struct) as an argument instead of constructing layout internally, use that passed layout when calling bgfx::createVertexBuffer, and update callers that build PosColorVertex to pass the appropriate layout; ensure g_buffers still stores the returned vbh and behavior unchanged.engine/native/main/main.cpp (1)
36-46: Platform support is limited to Linux/X11.The native handle extraction only supports Linux with X11. Other platforms (Windows, macOS, Wayland) will result in null handles and early exit. This is acceptable for initial development but should be expanded.
Do you want me to open an issue to track adding Windows and macOS platform support for native window handles?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/main/main.cpp` around lines 36 - 46, Current code only extracts native handles for Linux/X11 using SDL_GetWindowProperties and X11-specific properties (SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER), causing null handles on other platforms; update the extraction to handle other drivers by branching on the SDL video driver (the existing driver variable) and adding platform-specific handle retrieval for "windows", "cocoa"/"macos", and "wayland". Use SDL's platform-agnostic helper (SDL_GetWindowWMInfo / SDL_version + SDL_VERSIONNUM checks) or the appropriate SDL functions for each backend to populate ndt and nwh, and keep the existing X11 branch that uses SDL_GetPointerProperty/SDL_GetNumberProperty for SDL_PROP_WINDOW_X11_*, so handles are non-null on Windows (HWND), macOS (NSWindow/NSView), and Wayland as available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/native/main/main.cpp`:
- Around line 80-85: The shader-load failure path returns early without cleaning
up RHI and SDL resources; update the block that checks vs_data.empty() ||
fs_data.empty() (where it currently prints "Failed to load shaders" and returns
-1) to first call draco::rhi::shutdown(), destroy the created SDL window (e.g.
SDL_DestroyWindow(window)) and perform SDL_Quit() (and any other local cleanup
like freeing context if used) before returning the error code so RHI and SDL are
properly torn down.
- Around line 116-121: The RenderPacket is default-initialized so index_buffer
ends up 0 and can be misinterpreted in submit(); explicitly mark "no index
buffer" by setting packet.index_buffer = UINT16_MAX after creating the packet
(before calling draco::rhi::submit) so submit()'s check (p.index_buffer !=
UINT16_MAX) correctly skips index handling; update the code that constructs the
packet (the RenderPacket packet{} block) to assign packet.index_buffer =
UINT16_MAX.
---
Nitpick comments:
In `@engine/native/main/main.cpp`:
- Around line 36-46: Current code only extracts native handles for Linux/X11
using SDL_GetWindowProperties and X11-specific properties
(SDL_PROP_WINDOW_X11_DISPLAY_POINTER, SDL_PROP_WINDOW_X11_WINDOW_NUMBER),
causing null handles on other platforms; update the extraction to handle other
drivers by branching on the SDL video driver (the existing driver variable) and
adding platform-specific handle retrieval for "windows", "cocoa"/"macos", and
"wayland". Use SDL's platform-agnostic helper (SDL_GetWindowWMInfo / SDL_version
+ SDL_VERSIONNUM checks) or the appropriate SDL functions for each backend to
populate ndt and nwh, and keep the existing X11 branch that uses
SDL_GetPointerProperty/SDL_GetNumberProperty for SDL_PROP_WINDOW_X11_*, so
handles are non-null on Windows (HWND), macOS (NSWindow/NSView), and Wayland as
available.
In `@engine/native/rendering/rhi/rhi_bgfx.cpp`:
- Around line 117-132: The create_vertex_buffer function hardcodes a
bgfx::VertexLayout (Position + Color0) which couples vertex format to this
implementation; modify create_vertex_buffer (or add an overload) to accept a
vertex layout descriptor (e.g., a bgfx::VertexLayout or a lightweight vertex
format enum/struct) as an argument instead of constructing layout internally,
use that passed layout when calling bgfx::createVertexBuffer, and update callers
that build PosColorVertex to pass the appropriate layout; ensure g_buffers still
stores the returned vbh and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ebcef73-8f7e-4fdc-9546-ca7a65447dfd
⛔ Files ignored due to path filters (2)
engine/native/rendering/shaders/fs_triangle.binis excluded by!**/*.binengine/native/rendering/shaders/vs_triangle.binis excluded by!**/*.bin
📒 Files selected for processing (19)
.gitmodulesCMakeLists.txtCMakePresets.jsoncmake/Compiler.cmakeengine/native/CMakeLists.txtengine/native/core/CMakeLists.txtengine/native/core/filesystem/filesystem.cppengine/native/core/filesystem/filesystem.cppmengine/native/main/main.cppengine/native/rendering/CMakeLists.txtengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/rhi_bgfx.cppengine/native/rendering/rhi/vertex.cppmengine/native/rendering/shaders/fs.scengine/native/rendering/shaders/varying.def.scengine/native/rendering/shaders/vs.scengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/bxengine/native/thirdparty/sdl
💤 Files with no reviewable changes (2)
- CMakePresets.json
- cmake/Compiler.cmake
✅ Files skipped from review due to trivial changes (8)
- engine/native/thirdparty/sdl
- .gitmodules
- engine/native/rendering/shaders/fs.sc
- engine/native/rendering/CMakeLists.txt
- engine/native/rendering/rhi/vertex.cppm
- engine/native/rendering/shaders/varying.def.sc
- engine/native/rendering/rhi/rhi.cppm
- engine/native/thirdparty/bx
🚧 Files skipped from review as they are similar to previous changes (6)
- engine/native/CMakeLists.txt
- engine/native/core/CMakeLists.txt
- engine/native/core/filesystem/filesystem.cppm
- engine/native/rendering/shaders/vs.sc
- engine/native/thirdparty/CMakeLists.txt
- CMakeLists.txt
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/native/main/main.cpp`:
- Around line 25-27: After successfully calling SDL_Init, the error paths return
without tearing down SDL resources; update the early returns so that when window
creation fails (the check on variable "window" after SDL_CreateWindow) you call
SDL_Quit() before returning, and for the native-handle failure branch(s) (the
code that checks the created window's native handle between the SDL_CreateWindow
and main loop) call SDL_DestroyWindow(window) then SDL_Quit() before returning;
use the SDL_DestroyWindow and SDL_Quit functions to ensure symmetric teardown
while retaining the existing SDL_GetError logging.
- Around line 4-5: main.cpp currently includes bgfx/bx headers and uses
BGFX_STATE_* flags; remove those includes and any direct use of BGFX symbols and
instead use the RHI public API: stop including <bgfx/bgfx.h> and <bx/math.h> in
main.cpp, initialize RenderPacket::model via an RHI-provided default matrix or
initialization helper (add or call something like RHI::defaultModelMatrix() or
RHI::InitRenderPacketModel(RenderPacket&)), and replace raw BGFX_STATE_* uses
with RHI-exposed pipeline state constants or an enum (e.g.
RHI::PipelineState::Opaque / RHI::STATE_DEFAULT) so main.cpp only references RHI
types; update code that sets pipeline state to use the new RHI constants and
ensure rhi_bgfx.cpp continues using bx::mtxIdentity internally to implement the
default model behavior.
- Around line 40-44: The current native-handle extraction only handles the "x11"
driver and leaves ndt/nwh null for other Linux drivers causing cryptic failures;
update the block that checks driver / std::string_view(driver) to either (a) add
explicit handling for other drivers you intend to support (e.g., check for
"wayland" and call the appropriate SDL_GetPointerProperty/SDL_GetNumberProperty
keys) or (b) add a clear fail-fast path that logs or returns an explicit
"unsupported video driver" error when driver is not "x11" (ensure ndt and nwh
are not used after that); modify the code around variables driver, ndt, nwh and
the SDL_GetPointerProperty/SDL_GetNumberProperty calls to implement the chosen
behavior so downstream null-handle assertions cannot occur silently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b14593cd-076d-4b40-adcb-ab607ac86df5
📒 Files selected for processing (1)
engine/native/main/main.cpp
Comment on lines
+4
to
+5
| #include <bgfx/bgfx.h> | ||
| #include <bx/math.h> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find engine/native -name "main.cpp" -o -name "rendering.h" -o -name "rhi.h" | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 98
🏁 Script executed:
find engine/native -type f -name "*.h" | grep -i rhi | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
find engine/native -type d -name "rhi" -o -type d -name "rendering" | head -10Repository: Redot-Engine/DraconicEngine
Length of output: 122
🏁 Script executed:
wc -l engine/native/main/main.cppRepository: Redot-Engine/DraconicEngine
Length of output: 102
🏁 Script executed:
head -30 engine/native/main/main.cpp | cat -nRepository: Redot-Engine/DraconicEngine
Length of output: 890
🏁 Script executed:
sed -n '90,105p' engine/native/main/main.cpp | cat -nRepository: Redot-Engine/DraconicEngine
Length of output: 736
🏁 Script executed:
sed -n '120,130p' engine/native/main/main.cpp | cat -nRepository: Redot-Engine/DraconicEngine
Length of output: 437
🏁 Script executed:
find engine/native/rendering/rhi -name "*.h" -o -name "*.ixx" | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
head -100 engine/native/rendering/rhi/rhi.ixx | cat -nRepository: Redot-Engine/DraconicEngine
Length of output: 165
🏁 Script executed:
find engine/native/rendering/rhi -type f | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 188
🏁 Script executed:
ls -la engine/native/rendering/rhi/Repository: Redot-Engine/DraconicEngine
Length of output: 363
🏁 Script executed:
find engine/native/rendering -type f -name "*.ixx" -o -name "*.h" -o -name "*.cpp" | head -30Repository: Redot-Engine/DraconicEngine
Length of output: 111
🏁 Script executed:
cat -n engine/native/rendering/rhi/rhi.cppmRepository: Redot-Engine/DraconicEngine
Length of output: 1481
🏁 Script executed:
cat -n engine/native/rendering/rhi/rhi_bgfx.cppRepository: Redot-Engine/DraconicEngine
Length of output: 6965
🏁 Script executed:
cat -n engine/native/rendering/rhi/vertex.cppmRepository: Redot-Engine/DraconicEngine
Length of output: 419
Remove bgfx/bx includes and backend state constants from main.cpp.
Lines 4-5, 94-99, and 123 directly depend on <bgfx/bgfx.h>, <bx/math.h>, and BGFX_STATE_* flags. The RHI abstraction doesn't expose these, forcing the app layer to know about the backend. This violates the abstraction boundary—changing RHI implementations would require editing app code.
The RHI layer internally uses bx::mtxIdentity (seen in rhi_bgfx.cpp line 176) and handles pipeline state, so wrap these capabilities: expose a default matrix or initialization function for RenderPacket::model, and define pipeline state constants/enum in the public RHI API instead of forcing raw uint64_t values from BGFX_STATE_*.
🤖 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 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.
Contributor
Author
There was a problem hiding this comment.
Aight, I was a bit lazy & missed tis.
Contributor
There was a problem hiding this comment.
@AR-DEV-1, no worries! Here's a concrete plan to clean up the abstraction leak:
Task: Remove bgfx/bx leakage from main.cpp
- Add pipeline state constants to
rendering.rhipublic API (rhi.cppm) - Add a
default_model_matrix(float[16])helper to the RHI public API - Implement both in
rhi_bgfx.cpp(bgfx-specific internals stay hidden) - Strip
<bgfx/bgfx.h>,<bx/math.h>, and allBGFX_STATE_*references frommain.cpp
🧠 Learnings used
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 16
File: engine/native/thirdparty/CMakeLists.txt:17-17
Timestamp: 2026-04-10T10:57:10.479Z
Learning: In `engine/native/thirdparty/CMakeLists.txt` (Redot-Engine/DraconicEngine), `CMAKE_POSITION_INDEPENDENT_CODE ON` must be placed **before** the bgfx/bx/bimg `add_subdirectory()` calls. The `rhi` target is a SHARED library that statically links bgfx/bx/bimg, so those static libs must be compiled as PIC to avoid relocation errors on Linux x86-64. SDL's PIC requirement is additionally covered by `SDL_STATIC_PIC ON`.
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 12
File: engine/native/thirdparty/cmake/bgfx/3rdparty/tint.cmake:57-68
Timestamp: 2026-03-31T03:45:52.137Z
Learning: In Redot-Engine/DraconicEngine, the CMake files under `engine/native/thirdparty/cmake/bgfx/` (including `3rdparty/tint.cmake`, `bgfx.cmake`, `shaderc.cmake`, etc.) are sourced from the upstream bgfx.cmake project and should not be modified. Do not suggest changes to these files as the maintainers intentionally keep them unmodified to stay in sync with upstream.
Failed to handle agent chat message. Please try again.
Comment on lines
+25
to
+27
| if (!window) { | ||
| std::println("Failed to create window: {}", SDL_GetError()); | ||
| return -1; |
Contributor
There was a problem hiding this comment.
Tear down SDL on these error paths too.
After SDL_Init(...) succeeds, Line 27 returns without SDL_Quit(). The native-handle failures on Lines 48-56 also skip SDL_DestroyWindow(window) and SDL_Quit(). It won’t matter much on process exit, but it leaves setup/teardown asymmetric and makes this entrypoint harder to reuse safely.
Proposed fix
if (!window) {
std::println("Failed to create window: {}", SDL_GetError());
+ SDL_Quit();
return -1;
}
...
if (!nwh) {
std::println("Failed to get native window handle");
+ SDL_DestroyWindow(window);
+ SDL_Quit();
return -1;
}
if(!ndt) {
std::println("Failed to get native display type");
+ SDL_DestroyWindow(window);
+ SDL_Quit();
return -1;
}Also applies to: 48-56
🤖 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 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.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat engine/native/main/main.cpp
Length of output: 3833
@AR-DEV-1, here are the three fixes needed across the early-return paths in engine/native/main/main.cpp:
- Fix window-creation failure path (missing
SDL_Quit()) - Fix native window handle failure path (missing
SDL_DestroyWindow+SDL_Quit()) - Fix native display type failure path (missing
SDL_DestroyWindow+SDL_Quit())
🧠 Learnings used
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 16
File: engine/native/thirdparty/CMakeLists.txt:17-17
Timestamp: 2026-04-10T10:57:10.479Z
Learning: In `engine/native/thirdparty/CMakeLists.txt` (Redot-Engine/DraconicEngine), `CMAKE_POSITION_INDEPENDENT_CODE ON` must be placed **before** the bgfx/bx/bimg `add_subdirectory()` calls. The `rhi` target is a SHARED library that statically links bgfx/bx/bimg, so those static libs must be compiled as PIC to avoid relocation errors on Linux x86-64. SDL's PIC requirement is additionally covered by `SDL_STATIC_PIC ON`.
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 12
File: engine/native/thirdparty/cmake/bgfx/3rdparty/tint.cmake:57-68
Timestamp: 2026-03-31T03:45:52.137Z
Learning: In Redot-Engine/DraconicEngine, the CMake files under `engine/native/thirdparty/cmake/bgfx/` (including `3rdparty/tint.cmake`, `bgfx.cmake`, `shaderc.cmake`, etc.) are sourced from the upstream bgfx.cmake project and should not be modified. Do not suggest changes to these files as the maintainers intentionally keep them unmodified to stay in sync with upstream.
Failed to handle agent chat message. Please try again.
Comment on lines
+40
to
+44
| if (driver && std::string_view(driver) == "x11") | ||
| { | ||
| ndt = SDL_GetPointerProperty(props, SDL_PROP_WINDOW_X11_DISPLAY_POINTER, nullptr); // Get the X11 display pointer | ||
| nwh = (void*)(uintptr_t)SDL_GetNumberProperty(props, SDL_PROP_WINDOW_X11_WINDOW_NUMBER, 0); // Get the X11 window number and cast it to a pointer | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In SDL3, what native window/display properties should be read from SDL_GetWindowProperties for the Wayland video backend, and what is the recommended way to pass those handles into a renderer initialization path?
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
# First, let's find and read the main.cpp file to understand the full context
fd -t f -i main.cpp | head -20Repository: Redot-Engine/DraconicEngine
Length of output: 141
🏁 Script executed:
# Search for Wayland mentions
rg -i "wayland" --type cpp --type cmakeRepository: Redot-Engine/DraconicEngine
Length of output: 595
🏁 Script executed:
# Search for SDL backend handling or driver selection
rg -i "driver|backend|x11" engine/native/main/main.cpp -A 5 -B 5Repository: Redot-Engine/DraconicEngine
Length of output: 855
🌐 Web query:
SDL3 Wayland native window handle SDL_GetWindowProperties
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
# Read the full main.cpp file to see the complete context and error handling
cat -n engine/native/main/main.cpp | sed -n '30,70p'Repository: Redot-Engine/DraconicEngine
Length of output: 1905
Add explicit fallback for unsupported Linux video drivers.
The code only handles X11 native handles on Linux; other drivers (including Wayland if ever enabled) silently fall through with null handles, triggering a cryptic failure in the checks at line 48. Either add support for additional drivers (e.g., Wayland) or fail fast with an explicit "unsupported video driver" error message instead of a generic null-handle assertion. Note: Wayland is currently disabled in the build (SDL_WAYLAND OFF in CMakeLists.txt), but this code will break if that changes without corresponding updates here.
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Note
Contributed by 2LazyDevs.
Summary by CodeRabbit
New Features
Chores