Added a Shell abstraction + Migrated existing code to use it - #38
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThis PR replaces the Platform and Input modules with a Shell abstraction for windowing and input. It adds Null and SDL3 backends, updates RHI and camera input APIs, rewires build targets, and migrates the render sample to the shell lifecycle. ChangesShell abstraction replacing Platform/Input
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant RenderSample
participant SDL3Shell
participant SDL3WindowManager
participant SDL3InputManager
participant RHI
RenderSample->>SDL3Shell: createShell(WindowSettings)
SDL3Shell->>SDL3WindowManager: createWindow()
RenderSample->>RHI: init(native handles, window type)
loop main loop
RenderSample->>SDL3Shell: processEvents()
SDL3Shell->>SDL3InputManager: translate SDL input
RenderSample->>SDL3InputManager: query keyboard and mouse state
RenderSample->>RHI: render frame
end
RenderSample->>RHI: shutdown()
RenderSample->>SDL3Shell: destroyShell()
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
Engine/cpp/Runtime/Shell/Window.cppm (1)
121-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent const-correctness on
IWindowManagerquery methods.
mainWindow(),windows(), andgetWindow()are pure queries but are declared non-const(Lines 136, 139, 142), whileevents()correctly isconst(Line 146). This forces implementers to bypass constness for read-only access — e.g.NullShell::isRunning() const(NullShell.cppm, Line 183) has toconst_castthe window manager just to callmainWindow(). Making theseconst(returning non-const pointers is fine, same pattern asevents()/getWindow()) removes the need for that workaround entirely.♻️ Proposed fix
- [[nodiscard]] virtual std::span<IWindow* const> windows() noexcept = 0; + [[nodiscard]] virtual std::span<IWindow* const> windows() const noexcept = 0; // The first window created, or null if none are live. - [[nodiscard]] virtual IWindow* mainWindow() noexcept = 0; + [[nodiscard]] virtual IWindow* mainWindow() const noexcept = 0; // Look up a live window by id, or null if there is no such window. - [[nodiscard]] virtual IWindow* getWindow(draco::u32 id) noexcept = 0; + [[nodiscard]] virtual IWindow* getWindow(draco::u32 id) const noexcept = 0;🤖 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/cpp/Runtime/Shell/Window.cppm` around lines 121 - 151, The read-only query methods on IWindowManager are not const-correct: mainWindow(), windows(), and getWindow() should be callable on a const manager just like events(). Update the IWindowManager interface so these accessors are const-qualified while keeping their existing return types, then adjust all implementations and call sites such as NullShell::isRunning() to remove any const_cast workaround.Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm (1)
352-544: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
std::unique_ptrfor gamepad ownership.
m_gamepadsstores rawSDL3Gamepad*allocated withnewand freed manually inremoveGamepad()/releaseDevices(). The pairing looks correct today, butstd::vector<std::unique_ptr<SDL3Gamepad>>would remove the manual-delete risk if the code is touched again later (e.g., an early-return added before adelete).🤖 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/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm` around lines 352 - 544, The SDL3InputManager::m_gamepads container manually owns SDL3Gamepad instances with new/delete in addGamepad, removeGamepad, and releaseDevices, which is fragile. Refactor SDL3InputManager and its helper methods to store gamepads as std::unique_ptr<SDL3Gamepad> so ownership is automatic, and update findGamepadById, getGamepad, removeGamepad, and releaseDevices to work with smart pointers without manual delete calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm`:
- Around line 105-116: The sdl3WindowFlags() helper is applying
SDL_WINDOW_VULKAN to every non-dummy Linux window, which can break
SDL_CreateWindow() on systems without Vulkan support. Restrict the Vulkan flag
to the Wayland/libdecor path only, using the existing
SDL_GetCurrentVideoDriver() check in sdl3WindowFlags() and the SDL3Shell ctor
note as the reference point. Keep the headless dummy-driver exception intact,
but avoid enabling Vulkan for other Linux drivers.
In `@Engine/cpp/Runtime/Shell/Null/NullShell.cppm`:
- Around line 65-70: The NullShell destroyWindow override currently only guards
against null and will act on any foreign IWindow, which breaks the
IWindowManager contract; update NullShell::destroyWindow to verify the window is
owned/known by this manager before calling close() or pushing to
m_pendingDestroy, and otherwise return as a no-op. Use the existing bookkeeping
in NullShell (or the manager’s internal window lookup used by
flushDestroyed/creation paths) to reject unknown windows consistently with the
documented behavior.
- Line 53: The NullWindowManager currently treats m_live[0] as the main window,
so closing or flushing the original main window can reassign “main” to another
window and break isRunning() behavior. Update NullWindowManager/mainWindow() to
preserve the initial main window identity via a stable id or dedicated pointer,
and make window removal/flush logic avoid promoting a replacement main window.
Add a regression test covering closing or flushing the original main window
while another window is still open.
In `@Samples/cpp/Render/CMakeLists.txt`:
- Around line 16-23: Add the Windows/MSVC whole-archive equivalent in
RenderSample’s CMake logic so the static SDL3::SDL3-static library is
force-linked on Windows too. Update the platform branching around
target_link_options for RenderSample to include a WIN32/MSVC case that uses
/WHOLEARCHIVE:$<TARGET_FILE:SDL3::SDL3-static>, matching the existing UNIX and
APPLE handling, so constructor-registered input drivers are not stripped at link
time.
In `@Samples/cpp/Render/RenderSample.cpp`:
- Around line 148-177: The main loop in RenderSample.cpp busy-spins when the
window is minimized or has zero size because the `w == 0 || h == 0 ||
mainWindow->isMinimized()` branch just continues after `shell->processEvents()`.
Update the loop around `shell->isRunning()` to avoid a tight retry path by
adding a small sleep/yield or moving the resize/work logic behind a wait so
minimized state does not consume full CPU; keep the behavior localized to the
render loop and use the existing `shell`, `mainWindow`, and `processEvents()`
flow.
- Around line 168-186: The camera input path in RenderSample.cpp still forwards
mouse deltas to CameraController::update() even after capture is released, so
the camera can keep rotating when mouse capture is off. Gate the
`camInput.mouseDx` and `camInput.mouseDy` values (or skip calling
`camera.update(dt, camInput)`) based on the `mouse_captured` state used in the
input handling logic, so mouse movement only affects the camera while capture is
active.
---
Nitpick comments:
In `@Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm`:
- Around line 352-544: The SDL3InputManager::m_gamepads container manually owns
SDL3Gamepad instances with new/delete in addGamepad, removeGamepad, and
releaseDevices, which is fragile. Refactor SDL3InputManager and its helper
methods to store gamepads as std::unique_ptr<SDL3Gamepad> so ownership is
automatic, and update findGamepadById, getGamepad, removeGamepad, and
releaseDevices to work with smart pointers without manual delete calls.
In `@Engine/cpp/Runtime/Shell/Window.cppm`:
- Around line 121-151: The read-only query methods on IWindowManager are not
const-correct: mainWindow(), windows(), and getWindow() should be callable on a
const manager just like events(). Update the IWindowManager interface so these
accessors are const-qualified while keeping their existing return types, then
adjust all implementations and call sites such as NullShell::isRunning() to
remove any const_cast workaround.
🪄 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: eb5654ec-6c09-45f4-8d98-d7a56f54d13b
📒 Files selected for processing (29)
Engine/cpp/CMakeLists.txtEngine/cpp/Runtime/CMakeLists.txtEngine/cpp/Runtime/Draconic.cppmEngine/cpp/Runtime/Input/Input.cppEngine/cpp/Runtime/Input/Input.cppmEngine/cpp/Runtime/Platform/CMakeLists.txtEngine/cpp/Runtime/Platform/Platform.cppmEngine/cpp/Runtime/Platform/impl/CMakeLists.txtEngine/cpp/Runtime/Platform/impl/linux/linux.cppEngine/cpp/Runtime/Platform/impl/mac/mac.mmEngine/cpp/Runtime/Platform/impl/platform_impl.hEngine/cpp/Runtime/Platform/impl/win32/win32.cppEngine/cpp/Runtime/Rendering/CMakeLists.txtEngine/cpp/Runtime/Rendering/RHI/Core.cppEngine/cpp/Runtime/Rendering/RHI/RHI.cppmEngine/cpp/Runtime/Scene/CMakeLists.txtEngine/cpp/Runtime/Scene/Camera/CameraController.cppEngine/cpp/Runtime/Scene/Camera/CameraController.cppmEngine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppmEngine/cpp/Runtime/Shell/Desktop/SDL3Shell.test.cppEngine/cpp/Runtime/Shell/Input.cppmEngine/cpp/Runtime/Shell/Null/NullShell.cppmEngine/cpp/Runtime/Shell/Null/NullShell.test.cppEngine/cpp/Runtime/Shell/Shell.cppmEngine/cpp/Runtime/Shell/Window.cppmEngine/cpp/ThirdParty/CMakeLists.txtSamples/cpp/CMakeLists.txtSamples/cpp/Render/CMakeLists.txtSamples/cpp/Render/RenderSample.cpp
💤 Files with no reviewable changes (9)
- Engine/cpp/Runtime/Platform/CMakeLists.txt
- Engine/cpp/Runtime/Platform/impl/win32/win32.cpp
- Engine/cpp/Runtime/Platform/impl/platform_impl.h
- Engine/cpp/Runtime/Input/Input.cppm
- Engine/cpp/Runtime/Platform/impl/CMakeLists.txt
- Engine/cpp/Runtime/Platform/Platform.cppm
- Engine/cpp/Runtime/Platform/impl/mac/mac.mm
- Engine/cpp/Runtime/Platform/impl/linux/linux.cpp
- Engine/cpp/Runtime/Input/Input.cpp
Comment on lines
+105
to
+116
| // Builds SDL window-creation flags. On Wayland a Vulkan-backed window is | ||
| // needed for client-side decorations (see SDL3Shell ctor note); skipped | ||
| // under the headless "dummy" driver so tests still get a window. | ||
| [[nodiscard]] inline SDL_WindowFlags sdl3WindowFlags() noexcept | ||
| { | ||
| SDL_WindowFlags flags = SDL_WINDOW_RESIZABLE; | ||
| #if defined(__linux__) | ||
| const char* driver = SDL_GetCurrentVideoDriver(); | ||
| if (driver != nullptr && SDL_strcmp(driver, "dummy") != 0) { flags |= SDL_WINDOW_VULKAN; } | ||
| #endif | ||
| return flags; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does SDL3 SDL_WINDOW_VULKAN require Vulkan loader present even if the app renders with OpenGL via native handles?
💡 Result:
Yes, in SDL3, if you create a window with the SDL_WINDOW_VULKAN flag, SDL will attempt to load the Vulkan loader library at window creation time if one has not already been loaded [1][2][3]. Specifically, when a window is created with the SDL_WINDOW_VULKAN flag, SDL automatically calls SDL_Vulkan_LoadLibrary internally [3][4]. If this operation fails—for instance, if the Vulkan loader cannot be found or initialized on the system—the window creation itself will fail [3][4]. Therefore, the presence of a functional Vulkan loader is a strict requirement for creating a window with that flag, regardless of how you intend to use the window later [3][4]. If you intend to use the window for OpenGL rendering (or any other API) and do not need Vulkan, you should not include the SDL_WINDOW_VULKAN flag [3][4]. You can still obtain the necessary native window handles for other graphics APIs by using the appropriate platform-specific functions, such as SDL_GetProperty with SDL_PROP_WINDOW_X11_WINDOW_NUMBER, SDL_PROP_WINDOW_WIN32_HWND_POINTER, or SDL_PROP_WINDOW_COCOA_WINDOW_POINTER, without triggering the Vulkan loader dependency [5].
Citations:
- 1: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_vulkan.h
- 2: https://wiki.libsdl.org/SDL3/SDL_Vulkan_LoadLibrary
- 3: https://github.com/libsdl-org/sdlwiki/blob/0a70f26c323954589a8686c027bf9905ff1d5bee/SDL3/SDL_CreateWindow.md
- 4: https://manpages.debian.org/unstable/libsdl3-doc/SDL_CreateWindow.3.en.html
- 5: https://discourse.libsdl.org/t/does-sdl-window-opengl-lock-you-in-to-using-opengl/23004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file and nearby context.
git ls-files 'Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm'
wc -l Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm
sed -n '1,220p' Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm
# Search for the window flag helper and any Vulkan-related usage in the SDL shell.
rg -n "SDL_WINDOW_VULKAN|sdl3WindowFlags|SDL_Vulkan|GetCurrentVideoDriver|dummy|Wayland|client-side decorations" Engine/cpp/Runtime/Shell -SRepository: Redot-Engine/DraconicEngine
Length of output: 12066
Avoid setting SDL_WINDOW_VULKAN on every Linux window. SDL_CreateWindow() can fail outright when that flag is present but no Vulkan loader is available, so this makes the shell unusable on systems without Vulkan support even if the renderer uses a different backend. Narrow it to the Wayland/libdecor case instead of all Linux windows.
🤖 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/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm` around lines 105 - 116, The
sdl3WindowFlags() helper is applying SDL_WINDOW_VULKAN to every non-dummy Linux
window, which can break SDL_CreateWindow() on systems without Vulkan support.
Restrict the Vulkan flag to the Wayland/libdecor path only, using the existing
SDL_GetCurrentVideoDriver() check in sdl3WindowFlags() and the SDL3Shell ctor
note as the reference point. Keep the headless dummy-driver exception intact,
but avoid enabling Vulkan for other Linux drivers.
Contributor
Author
There was a problem hiding this comment.
Vulkan is a requirement for Draconic on Linux. BGFX will be replaced in a follow up commit. The new RHI will only support Vulkan on Linux. This can be revisited if that changes in the future.
Contributor
There was a problem hiding this comment.
@jayrulez, understood — thanks for the clarification. Since Vulkan is a hard requirement on Linux for Draconic (with the upcoming RHI replacement only supporting Vulkan there), applying SDL_WINDOW_VULKAN unconditionally on Linux (dummy driver excluded) is intentional and correct. I'll withdraw the original concern.
✏️ Learnings added
Learnt from: jayrulez
Repo: Redot-Engine/DraconicEngine PR: 38
File: Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm:105-116
Timestamp: 2026-07-05T11:53:11.704Z
Learning: In the Redot-Engine/DraconicEngine repository, Vulkan is a hard requirement on Linux for the Draconic engine/RHI. The current RHI uses BGFX but will be replaced by a new RHI in a follow-up commit that will only support Vulkan on Linux. Because of this, code such as `sdl3WindowFlags()` in `Engine/cpp/Runtime/Shell/Desktop/SDL3Shell.cppm` intentionally sets `SDL_WINDOW_VULKAN` unconditionally for all Linux windows (except under the SDL "dummy" video driver used for headless/CI), rather than restricting it to Wayland only. This should not be flagged as an issue unless the Vulkan-only requirement on Linux changes in the future.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mcdubhghlas
Repo: Redot-Engine/DraconicEngine PR: 1
File: core/math/vector4.cppm:136-140
Timestamp: 2026-03-17T14:51:36.674Z
Learning: For all C++ module files (.cppm), ensure the build flags cover AVX2/-mfma unconditionally (as in the project: -mavx2 -mfma). If you use BMI1/BMI2 intrinsics in a module, also enable them explicitly with -mbmi and -mbmi2, since BMI features are not guaranteed by AVX2 alone. Note that SSE3 and older SSEs are guaranteed via ISA backwards compatibility on AVX2 targets, so no SSE2/SSE3 fallbacks are needed.
Comment on lines
+148
to
177
| while (shell->isRunning()) | ||
| { | ||
| shell->processEvents(); | ||
|
|
||
| const auto nowTime = clock::now(); | ||
| const draco::f32 dt = std::chrono::duration<draco::f32>(nowTime - lastTime).count(); | ||
| lastTime = nowTime; | ||
| const draco::f32 elapsed = std::chrono::duration<draco::f32>(nowTime - startTime).count(); | ||
|
|
||
| if (e.type == SDL_EVENT_KEY_DOWN && | ||
| e.key.key == SDLK_ESCAPE) | ||
| { | ||
| mouse_captured = !mouse_captured; | ||
| draco::input::setMouseCaptured(window, mouse_captured); | ||
| } | ||
| IInputManager* input = shell->input(); | ||
| IKeyboard* kb = input->keyboard(); | ||
| IMouse* ms = input->mouse(); | ||
|
|
||
| draco::input::processEvent(e); | ||
| // Toggle mouse capture on Escape (edge-triggered). | ||
| if (kb->isKeyPressed(KeyCode::Escape)) | ||
| { | ||
| mouse_captured = !mouse_captured; | ||
| ms->setRelativeMode(mouse_captured); | ||
| } | ||
|
|
||
| int w, h; | ||
| SDL_GetWindowSize(window, &w, &h); | ||
| const draco::u32 w = mainWindow->width(); | ||
| const draco::u32 h = mainWindow->height(); | ||
|
|
||
| if (w <= 0 || h <= 0) | ||
| if (w == 0 || h == 0 || mainWindow->isMinimized()) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| draco::rendering::rhi::resize((draco::u16)w, (draco::u16)h); | ||
| draco::rendering::renderer::resize((draco::u16)w, (draco::u16)h); |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Busy-loops while minimized.
shell->processEvents() runs every iteration, and the minimized/zero-size branch just continues with no wait, so the loop spins at full CPU while the window is minimized.
🩹 Suggested fix
if (w == 0 || h == 0 || mainWindow->isMinimized())
{
+ std::this_thread::sleep_for(std::chrono::milliseconds(16));
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (shell->isRunning()) | |
| { | |
| shell->processEvents(); | |
| const auto nowTime = clock::now(); | |
| const draco::f32 dt = std::chrono::duration<draco::f32>(nowTime - lastTime).count(); | |
| lastTime = nowTime; | |
| const draco::f32 elapsed = std::chrono::duration<draco::f32>(nowTime - startTime).count(); | |
| if (e.type == SDL_EVENT_KEY_DOWN && | |
| e.key.key == SDLK_ESCAPE) | |
| { | |
| mouse_captured = !mouse_captured; | |
| draco::input::setMouseCaptured(window, mouse_captured); | |
| } | |
| IInputManager* input = shell->input(); | |
| IKeyboard* kb = input->keyboard(); | |
| IMouse* ms = input->mouse(); | |
| draco::input::processEvent(e); | |
| // Toggle mouse capture on Escape (edge-triggered). | |
| if (kb->isKeyPressed(KeyCode::Escape)) | |
| { | |
| mouse_captured = !mouse_captured; | |
| ms->setRelativeMode(mouse_captured); | |
| } | |
| int w, h; | |
| SDL_GetWindowSize(window, &w, &h); | |
| const draco::u32 w = mainWindow->width(); | |
| const draco::u32 h = mainWindow->height(); | |
| if (w <= 0 || h <= 0) | |
| if (w == 0 || h == 0 || mainWindow->isMinimized()) | |
| { | |
| continue; | |
| } | |
| draco::rendering::rhi::resize((draco::u16)w, (draco::u16)h); | |
| draco::rendering::renderer::resize((draco::u16)w, (draco::u16)h); | |
| while (shell->isRunning()) | |
| { | |
| shell->processEvents(); | |
| const auto nowTime = clock::now(); | |
| const draco::f32 dt = std::chrono::duration<draco::f32>(nowTime - lastTime).count(); | |
| lastTime = nowTime; | |
| const draco::f32 elapsed = std::chrono::duration<draco::f32>(nowTime - startTime).count(); | |
| IInputManager* input = shell->input(); | |
| IKeyboard* kb = input->keyboard(); | |
| IMouse* ms = input->mouse(); | |
| // Toggle mouse capture on Escape (edge-triggered). | |
| if (kb->isKeyPressed(KeyCode::Escape)) | |
| { | |
| mouse_captured = !mouse_captured; | |
| ms->setRelativeMode(mouse_captured); | |
| } | |
| const draco::u32 w = mainWindow->width(); | |
| const draco::u32 h = mainWindow->height(); | |
| if (w == 0 || h == 0 || mainWindow->isMinimized()) | |
| { | |
| std::this_thread::sleep_for(std::chrono::milliseconds(16)); | |
| continue; | |
| } | |
| draco::rendering::rhi::resize((draco::u16)w, (draco::u16)h); | |
| draco::rendering::renderer::resize((draco::u16)w, (draco::u16)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 `@Samples/cpp/Render/RenderSample.cpp` around lines 148 - 177, The main loop in
RenderSample.cpp busy-spins when the window is minimized or has zero size
because the `w == 0 || h == 0 || mainWindow->isMinimized()` branch just
continues after `shell->processEvents()`. Update the loop around
`shell->isRunning()` to avoid a tight retry path by adding a small sleep/yield
or moving the resize/work logic behind a wait so minimized state does not
consume full CPU; keep the behavior localized to the render loop and use the
existing `shell`, `mainWindow`, and `processEvents()` flow.
Contributor
Author
There was a problem hiding this comment.
This sample will be rewritten when the new RHI lands. It will be structured differently.
Contributor
There was a problem hiding this comment.
@jayrulez Understood — makes sense to hold off on this since the sample structure will change with the new RHI. I'll leave it as-is for now.
🧠 Learnings used
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 16
File: engine/native/rendering/rendergraph/rendergraph.cpp:3-3
Timestamp: 2026-05-17T09:15:49.434Z
Learning: DraconicEngine is intentionally C++23-only (GCC >= 14.0, Clang >= 18.0), with enforcement in cmake/Compiler.cmake. During code reviews, it’s OK to use C++23 standard library headers and facilities across the codebase (e.g., <print>) without raising compatibility concerns, since the build toolchain guarantees C++23 support. Avoid introducing C++ features or dependencies that require toolchains outside GCC/Clang versions enforced by the project.
Comment on lines
+168
to
+186
| const draco::u32 w = mainWindow->width(); | ||
| const draco::u32 h = mainWindow->height(); | ||
|
|
||
| if (w <= 0 || h <= 0) | ||
| if (w == 0 || h == 0 || mainWindow->isMinimized()) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| draco::rendering::rhi::resize((draco::u16)w, (draco::u16)h); | ||
| draco::rendering::renderer::resize((draco::u16)w, (draco::u16)h); | ||
|
|
||
| camera.update(dt); | ||
| draco::scene::CameraInput camInput{}; | ||
| camInput.mouseDx = ms->deltaX(); | ||
| camInput.mouseDy = ms->deltaY(); | ||
| camInput.moveForward = kb->isKeyDown(KeyCode::W); | ||
| camInput.moveBackward = kb->isKeyDown(KeyCode::S); | ||
| camInput.moveLeft = kb->isKeyDown(KeyCode::A); | ||
| camInput.moveRight = kb->isKeyDown(KeyCode::D); | ||
| camera.update(dt, camInput); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
git ls-files 'Samples/cpp/Render/RenderSample.cpp' 'Sources/**/CameraController.*' 'Samples/**/CameraController.*' '**/CameraInput.*' '**/Camera*.*'
# Show the relevant section of the sample with line numbers.
sed -n '1,260p' Samples/cpp/Render/RenderSample.cpp | cat -n
# Search for CameraInput / mouse-capture handling in the codebase.
rg -n "mouse_captured|setRelativeMode|CameraInput|deltaX\\(|deltaY\\(|camera\\.update\\(" Samples Sources -g '!**/build/**' -g '!**/dist/**'Repository: Redot-Engine/DraconicEngine
Length of output: 11157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the camera controller implementation and interface.
sed -n '1,260p' Engine/cpp/Runtime/Scene/Camera/CameraController.cpp | cat -n
printf '\n==== CameraController.cppm ====\n'
sed -n '1,260p' Engine/cpp/Runtime/Scene/Camera/CameraController.cppm | cat -n
# Show all references to CameraInput and capture-related logic.
rg -n "struct CameraInput|CameraInput|mouseDx|mouseDy|relativeMode|mouse_captured|setRelativeMode|update\\(" Engine/cpp/Runtime/Scene/Camera -g '!**/build/**'Repository: Redot-Engine/DraconicEngine
Length of output: 4876
Gate mouse deltas when capture is released.
CameraController::update() always applies mouseDx/mouseDy, so Escape only changes relative mode and the camera still rotates from mouse movement while capture is off. Zero the deltas or skip camera.update() when mouse_captured is 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 `@Samples/cpp/Render/RenderSample.cpp` around lines 168 - 186, The camera input
path in RenderSample.cpp still forwards mouse deltas to
CameraController::update() even after capture is released, so the camera can
keep rotating when mouse capture is off. Gate the `camInput.mouseDx` and
`camInput.mouseDy` values (or skip calling `camera.update(dt, camInput)`) based
on the `mouse_captured` state used in the input handling logic, so mouse
movement only affects the camera while capture is active.
Contributor
Author
There was a problem hiding this comment.
This sample will be rewritten when the new RHI lands. It will be structured differently.
Contributor
There was a problem hiding this comment.
@jayrulez, understood — thanks for the context. I'll hold off on pressing this further since the sample will be restructured once the new RHI lands. Feel free to revisit the mouse-capture gating then if it's still relevant.
🧠 Learnings used
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 16
File: engine/native/rendering/rendergraph/rendergraph.cpp:3-3
Timestamp: 2026-05-17T09:15:49.434Z
Learning: DraconicEngine is intentionally C++23-only (GCC >= 14.0, Clang >= 18.0), with enforcement in cmake/Compiler.cmake. During code reviews, it’s OK to use C++23 standard library headers and facilities across the codebase (e.g., <print>) without raising compatibility concerns, since the build toolchain guarantees C++23 support. Avoid introducing C++ features or dependencies that require toolchains outside GCC/Clang versions enforced by the project.
OldDev78
requested changes
Jul 5, 2026
OldDev78
left a comment
OldDev78
left a comment
Contributor
There was a problem hiding this comment.
Let's align it further. For one, the Shell library should still be engulfed into the Runtime, so that we keep our one draconic module exporting everything of the C++ code.
Introduce draconic::shell: the OS/windowing service (IShell, IWindow, IWindowManager, and the input device interfaces) as the primary module 'shell' with 'shell:window' and 'shell:input' partitions. Add two backends as separate libraries: - shell.null: a headless IShell (create/destroy/resize windows, no-op input) for tests, tools, and headless hosts. - shell.desktop: an SDL3-based IShell covering Win32/X11/Wayland/Cocoa windowing, input, and native-handle export for RHI surface creation. Each backend ships a doctest suite. Enable SDL_JOYSTICK and SDL_HIDAPI so the desktop backend's SDL_INIT_GAMEPAD (gamepad input) succeeds.
…ut modules Move windowing and input onto the new shell: the SDL3 desktop backend now owns window creation, native-handle export, and input, so the old draco::platform (SDL native-handle extraction) and draco::input (global SDL input) modules are removed. RHI no longer depends on platform: it defines its own rhi::NativeWindowType and init() takes it, keeping the renderer self-contained. RenderSample runs entirely on shell.desktop (createShell, mainWindow()->native() for RHI init, shell->input() for camera/mouse control, shell->processEvents()/isRunning() for the loop) and links Shell_Desktop. CameraController takes input by value (CameraInput) instead of reaching into a global input module. Draconic no longer re-exports platform/input, and the CMake wiring (Runtime, Rendering RHI, Scene Camera, Samples) is updated accordingly.
Give the sample its own subdirectory so more samples can be added as sibling folders. Samples/cpp/CMakeLists.txt now just dispatches via add_subdirectory; the RenderSample target and its shader/texture steps move to Samples/cpp/Render/CMakeLists.txt unchanged.
Track the main window by id instead of returning m_live[0]: destroying and flushing the main window while another window remains open no longer promotes that window to 'main' (which made isRunning() revive). mainWindow() now returns null once the initial window is gone, and NullShell::isRunning() stops when the main window is closed or destroyed, matching the documented contract. Make destroyWindow() a genuine no-op for windows the manager does not own, checked by pointer identity rather than id: windows from different managers can share an id, and acting on a foreign window would close it and make flushDestroyed() erase the wrong owned entry. Add headless regression tests for both: destroying the main window with a second window open (no promotion, no isRunning revival) and destroyWindow rejecting a foreign window whose id collides with an owned one.
Add the MSVC linker equivalent (/WHOLEARCHIVE:$<TARGET_FILE:SDL3::SDL3-static>) of the existing --whole-archive / -force_load handling. SDL3 is linked statically with joystick/HIDAPI enabled, so without whole-archive inclusion the linker can drop its constructor-registered input drivers and gamepad support disappears at runtime.
windows(), mainWindow(), and getWindow() are pure reads but were declared non-const while events() was const. Const-qualify them (they still return non-const IWindow* / span<IWindow* const>, the same pattern as events()), so read-only callers no longer have to defeat constness: NullShell::isRunning() and SDL3Shell::isRunning() drop their const_cast on the window manager and call mainWindow() directly.
m_gamepads held raw SDL3Gamepad* allocated with new and freed by hand in removeGamepad()/releaseDevices(), pairing each delete with an SDL_CloseGamepad(). Give SDL3Gamepad a destructor that closes its SDL_Gamepad and store std::vector<std::unique_ptr<SDL3Gamepad>>, so the container owns the whole device. removeGamepad() and releaseDevices() now just erase/clear, removing every manual free (and the risk that a future early-return skips one). Ordering is unchanged: releaseDevices() clears the list before SDL_Quit, and the later destructor sees an empty list.
Aligns the shell module with the engine's draco:: namespace convention (core, rhi). Module name and API are unchanged; only the enclosing namespace moves.
The shell is now exposed through the `draconic` module and the backends are implementation detail. - Draconic.cppm re-exports shell, and Runtime links Shell (+ the desktop backend) PUBLIC, so a consumer links Runtime and imports draconic once. - The shell interface now declares createShell()/destroyShell() (raw pointer, new/delete). Each backend supplies the definition via a small `module shell;` factory unit, so the backend is a compile-time (link) choice; neither shell.desktop nor shell.null is exported. - SDL no longer appears on the public surface: SDL3Window::handle() is a void* opaque handle, and no SDL types leak through `import draconic`. - Each backend is split by concern to mirror the interface (Shell / :window / :input), one interface .cppm plus a matching .cpp per file (SDL3Window/SDL3Input/SDL3Shell, NullWindow/NullInput/NullShell). The .cppm files are declarations only; all implementation moved to the .cpp files, with SDL confined to the desktop backend's translation units. - Drop the redundant `inline` on the constexpr KeyModifiers operators (constexpr already implies inline for functions). Both backends remain independently testable and the Runtime shared library links clean
createShell() now returns a raw IShell*, so pair it with destroyShell() on every exit path instead of relying on unique_ptr destruction. Import only `draconic` (it re-exports shell); the backend module is no longer imported directly.
OldDev78
previously approved these changes
Jul 9, 2026
OldDev78
left a comment
OldDev78
left a comment
Contributor
There was a problem hiding this comment.
lgtm. Just one line left to delete :)
Co-authored-by: Francisco Miguel Almeida <13934419+OldDev78@users.noreply.github.com>
mcdubhghlas
previously approved these changes
Jul 9, 2026
Shakai-Dev
suggested changes
Jul 9, 2026
Shakai-Dev
left a comment
Shakai-Dev
left a comment
Contributor
There was a problem hiding this comment.
Just a few nitpicks, overall lgtm
| { | ||
| // ---- SDL3Keyboard ---- | ||
|
|
||
| draco::u32 SDL3Keyboard::index(KeyCode key) noexcept |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u32, u32 is also fine. Same applies to rest of the file.
| export namespace draco::shell | ||
| { | ||
| // Double-buffered device-state sizes. | ||
| inline constexpr draco::u32 kKeyCount = static_cast<draco::u32>(KeyCode::Count); |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u32, u32 is also fine. Same applies to rest of the file.
| { | ||
| int w = 0, h = 0; | ||
| SDL_GetWindowSize(m_window, &w, &h); | ||
| m_width = static_cast<draco::u32>(w); |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u32, u32 is also fine. Same applies to rest of the file.
| SDL3Window(const SDL3Window&) = delete; | ||
| SDL3Window& operator=(const SDL3Window&) = delete; | ||
|
|
||
| [[nodiscard]] draco::u32 id() const noexcept override; |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u32, u32 is also fine. Same applies to rest of the file.
|
|
||
| // ---- NullMouse ---- | ||
|
|
||
| draco::f32 NullMouse::x() const { return 0.0f; } |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::f32, f32 is also fine. Same applies to rest of the file.
| { | ||
| // ---- NullWindow ---- | ||
|
|
||
| NullWindow::NullWindow(draco::u32 id, const WindowSettings& settings) noexcept |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u32, u32 is also fine. Same applies to rest of the file.
| class NullWindow final : public IWindow | ||
| { | ||
| public: | ||
| NullWindow(draco::u32 id, const WindowSettings& settings) noexcept; |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u32, u32 is also fine. Same applies to rest of the file.
| { | ||
| // ---- Keyboard --------------------------------------------------------- | ||
|
|
||
| enum class KeyCode : draco::u32 |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u32, u32 is also fine. Same applies to rest of the file.
| export namespace draco::shell | ||
| { | ||
| // The windowing system a native handle belongs to (see NativeWindow). | ||
| enum class WindowSystem : draco::u8 |
Contributor
There was a problem hiding this comment.
Since you're using namespace draco, you don't need to do draco::u8, u8 is also fine. Same applies to rest of the file.
| target_link_libraries(Core PUBLIC Definitions Math IO Memory Logging) | ||
| target_link_libraries(Input PRIVATE SDL3::SDL3-static Platform Core) | ||
| target_link_libraries(Shell PUBLIC Core) | ||
| # Shell backends: Null is headless, Desktop is SDL3-based. Neither is exported through |
Contributor
There was a problem hiding this comment.
Shouldn't there be docs on this as well which explain the Shell backend so we don't have to add unnecessary comments everywhere?
JoltedJon
reviewed
Jul 10, 2026
JoltedJon
approved these changes
Jul 10, 2026
mcdubhghlas
approved these changes
Jul 10, 2026
OldDev78
approved these changes
Jul 10, 2026
This was referenced Jul 11, 2026
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a Shell abstraction module meant to be the provider for engine windowing and input.
Second commit migrates existing code to use this new Shell abstraction
Moved Render sample into its own folder as we expect more samples to be added
Summary by CodeRabbit