Colors #45
Colors #45
Conversation
This reverts commit d69db40.
WalkthroughAdds a new sdl_raii module (PureRaii/BoundRaii and traits), migrates sdl_wrapper internals to RAII types, renames the app partition to lifecycle, extends RenderData/Renderable with vertex/index payloads and draw(RenderContext), and updates main.cpp for multi‑renderable rendering, per‑frame update, and input-driven camera motion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Points to review closely:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-11-13T11:35:48.289ZApplied to files:
📚 Learning: 2025-11-13T02:58:26.892ZApplied to files:
📚 Learning: 2025-11-13T11:34:32.732ZApplied to files:
📚 Learning: 2025-11-14T00:45:09.403ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
🔇 Additional comments (1)
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 |
|
Here's the code health analysis summary for commits Analysis Summary
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include <memory> | ||
| #include <numbers> | ||
| #include <span> | ||
| #include <string> |
There was a problem hiding this comment.
warning: included header span is not used directly [misc-include-cleaner]
| #include <string> | |
| #include <string> |
|
|
||
| #include "stb_image.h" | ||
|
|
||
| import lifecycle; |
There was a problem hiding this comment.
warning: module 'lifecycle' not found [clang-diagnostic-error]
import lifecycle;
^|
|
||
| // camera state | ||
| float yaw = 0.0f; | ||
| float yaw_speed = 0.0f; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float yaw_speed = 0.0f; | |
| float yaw_speed = 0.0F; |
| float yaw = 0.0f; | ||
| float yaw_speed = 0.0f; | ||
| float pitch = 0.0f; | ||
| float pitch_speed = 0.0f; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float pitch_speed = 0.0f; | |
| float pitch_speed = 0.0F; |
| discard; | ||
| })WSQ"; | ||
|
|
||
| std::string fragment_source2 = |
There was a problem hiding this comment.
warning: member variable 'fragment_source2' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::string fragment_source2 =
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdl_wrapper/src/sdl_wrapper.texture.cpp (1)
53-90: Missing GPU command buffer submission - texture upload will not execute.Similar to the buffer upload, the command buffer is created and used for the copy pass, but
SDL_SubmitGPUCommandBufferis never called afterSDL_EndGPUCopyPass. The texture data will not be uploaded to the GPU.Add submission after ending the copy pass:
SDL_EndGPUCopyPass(copy_pass); + if (!SDL_SubmitGPUCommandBuffer(command_buffer_raii.raw())) + { + SDL_Log("SDL_SubmitGPUCommandBuffer failed: %s", SDL_GetError()); + return std::unexpected{GpuError::SUBMIT_COMMAND_FAILED}; + } + SDL_GPUSamplerCreateInfo info{};
🧹 Nitpick comments (12)
main.cpp (5)
313-329: Consider extracting magic numbers and verifying accumulator reset behavior.The FPS calculation logic is sound. Minor suggestions:
- The
0.1multiplier on line 327 for speed scaling could be a named constant for clarity.- After printing FPS, resetting
m_fps_accumulatorto0.0discards any fractional second overflow, which is acceptable but slightly less accurate than subtracting 1.0.
358-482: Remove or restore the commented-out editor code.This large block of commented-out code (125+ lines) should either be removed if it's no longer needed, or restored if it's still required functionality. Leaving dead code hinders readability and maintenance.
585-595: Potential signed/unsigned comparison warning.Line 585 compares
int iwithm_renderables.size()which returnssize_t. Consider usingsize_torautofor the loop variable to avoid potential compiler warnings.- for (int i = 0; i < m_renderables.size(); i++) + for (size_t i = 0; i < m_renderables.size(); i++)
632-714: Consolidate duplicated key release logic.The
SDL_EVENT_KEY_UPhandler (lines 675-712) sets speeds to zero for multiple keys individually. This is correct, but the symmetric pairs (e.g., UP/DOWN both setpitch_speed = 0.F) could be consolidated using fallthrough or grouping.case SDLK_UP: + case SDLK_DOWN: pitch_speed = 0.F; break; - case SDLK_DOWN: - pitch_speed = 0.F; - break; case SDLK_LEFT: + case SDLK_RIGHT: yaw_speed = 0.F; break; - case SDLK_RIGHT: - yaw_speed = 0.F; - break;
749-754: Minor: Non-English comment.Line 749 contains a Chinese comment. Consider using English for consistency across the codebase.
sdl_wrapper/src/sdl_wrapper.render_data.cpp (1)
31-41: Preferstd::copyorstd::memcpyand fix signed/unsigned comparison.
- Loop variable
int icompared against.size()(returnssize_t) causes signed/unsigned comparison warnings.- Using
std::copyorstd::memcpyis more idiomatic and potentially more efficient.assert(vertex_data.size() <= size); - for (int i = 0; i < vertex_data.size(); ++i) - { - vertex_buffer->cpu_buffer()[i] = vertex_data[i]; - } + std::copy(vertex_data.begin(), vertex_data.end(), vertex_buffer->cpu_buffer()); assert(index_data.size() <= index_count * sizeof(int)); - for (int i = 0; i < index_data.size(); ++i) - { - index_buffer->cpu_buffer()[i] = index_data[i]; - } + std::copy(index_data.begin(), index_data.end(), index_buffer->cpu_buffer());Note: You may need to add
#include <algorithm>if not already included.sdl_wrapper/modules/sdl_wrapper.render_data.ixx (1)
47-62: Consider auto-populatingvertex_count/index_countfrom the data.The
set_verticesandset_indicesmethods populate the byte buffers but don't update the correspondingvertex_countandindex_countfields. This could lead to inconsistencies if callers forget to also callset_vertex_count/set_index_count.template <typename VertexType> Builder& set_vertices(std::span<VertexType> vertices) { static_assert(std::is_trivially_copyable_v<VertexType>, "VertexType must be trivially copyable"); vertex_data = std::vector<std::byte>(reinterpret_cast<const std::byte*>(vertices.data()), reinterpret_cast<const std::byte*>(vertices.data()) + vertices.size_bytes()); + vertex_count = static_cast<std::uint32_t>(vertices.size()); return *this; } Builder& set_indices(std::span<const uint32_t> indices) { index_data = std::vector<std::byte>(reinterpret_cast<const std::byte*>(indices.data()), reinterpret_cast<const std::byte*>(indices.data()) + indices.size_bytes()); + index_count = static_cast<std::uint32_t>(indices.size()); return *this; }sdl_wrapper/modules/sdl_raii/sdl_raii.bound_raii.ixx (1)
50-50: Minor formatting: missing space after comma.- void reset(Device* device = nullptr ,T* resource = nullptr) noexcept + void reset(Device* device = nullptr, T* resource = nullptr) noexceptsdl_wrapper/modules/sdl_wrapper.render_procedural.ixx (1)
44-46: Move assignment deleted while move construction allowed.The class allows move construction but deletes move assignment. This asymmetry may be intentional (e.g., to prevent accidental reassignment after initialization), but consider documenting the rationale or making it consistent.
sdl_wrapper/modules/sdl_wrapper.renderable.ixx (1)
15-21: Typo in struct name:RenderContex→RenderContext.The struct name is missing a 't'. Consider renaming for clarity and consistency.
- export struct RenderContex + export struct RenderContextAlso update the
drawmethod signature at line 30 to use the corrected name.sdl_wrapper/modules/sdl_raii/sdl_raii.ixx (1)
1-25: sdl_raii module wiring and public RAII aliases look solid; naming nit is optional.The primary interface cleanly re‑exports the RAII partitions and exposes focused aliases (
GpuBufferRaii,GpuTextureRaii, etc.); if you ever want to tighten API polish, you could normalize the alias names (Gpu*RaiivsGPU*Raii), but that’s purely cosmetic.sdl_wrapper/modules/sdl_raii/sdl_raii.bound_traits.ixx (1)
1-71: BoundTraits specializations cleanly map each resource to its SDL release hook.Each specialization consistently aliases
DevicetoSDL_GPUDeviceand forwardsreleaseto the matchingSDL_Release*function, which is exactly what a genericBoundRaiineeds; if you ever find these growing, you could factor out the commonDevicealias or a tiny helper, but at the current size keeping them explicit is perfectly fine and aligns with the existing non‑throwing style. Based on learnings, this matches the planned error‑handling approach.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
README.md(2 hunks)main.cpp(14 hunks)sdl_wrapper/CMakeLists.txt(2 hunks)sdl_wrapper/modules/lifecycle.ixx(1 hunks)sdl_wrapper/modules/sdl_raii/sdl_raii.bound_raii.ixx(1 hunks)sdl_wrapper/modules/sdl_raii/sdl_raii.bound_traits.ixx(1 hunks)sdl_wrapper/modules/sdl_raii/sdl_raii.ixx(1 hunks)sdl_wrapper/modules/sdl_raii/sdl_raii.pure_raii.ixx(1 hunks)sdl_wrapper/modules/sdl_raii/sdl_raii.pure_traits.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.buffer.ixx(3 hunks)sdl_wrapper/modules/sdl_wrapper.decl.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.gpu.ixx(3 hunks)sdl_wrapper/modules/sdl_wrapper.ixx(0 hunks)sdl_wrapper/modules/sdl_wrapper.render_data.ixx(3 hunks)sdl_wrapper/modules/sdl_wrapper.render_procedural.ixx(4 hunks)sdl_wrapper/modules/sdl_wrapper.renderable.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.texture.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx(3 hunks)sdl_wrapper/src/lifecycle.cpp(1 hunks)sdl_wrapper/src/sdl_wrapper.buffer.cpp(4 hunks)sdl_wrapper/src/sdl_wrapper.render_data.cpp(2 hunks)sdl_wrapper/src/sdl_wrapper.render_procedural.cpp(3 hunks)sdl_wrapper/src/sdl_wrapper.renderable.cpp(1 hunks)sdl_wrapper/src/sdl_wrapper.texture.cpp(6 hunks)sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp(3 hunks)
💤 Files with no reviewable changes (1)
- sdl_wrapper/modules/sdl_wrapper.ixx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
📚 Learning: 2025-11-13T02:58:26.892Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.892Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
README.mdsdl_wrapper/src/lifecycle.cppsdl_wrapper/CMakeLists.txtsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixxsdl_wrapper/modules/sdl_wrapper.texture.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/modules/sdl_wrapper.gpu.ixx
📚 Learning: 2025-11-14T00:45:09.403Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
sdl_wrapper/modules/sdl_wrapper.decl.ixxsdl_wrapper/src/sdl_wrapper.renderable.cppsdl_wrapper/CMakeLists.txtsdl_wrapper/modules/sdl_raii/sdl_raii.pure_traits.ixxsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/src/sdl_wrapper.render_data.cppmain.cppsdl_wrapper/modules/sdl_wrapper.renderable.ixxsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/src/sdl_wrapper.buffer.cppsdl_wrapper/modules/sdl_wrapper.render_procedural.ixxsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixxsdl_wrapper/modules/sdl_wrapper.texture.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/modules/sdl_raii/sdl_raii.ixxsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/modules/sdl_raii/sdl_raii.bound_traits.ixxsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/modules/sdl_wrapper.gpu.ixx
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/src/sdl_wrapper.renderable.cppsdl_wrapper/CMakeLists.txtsdl_wrapper/modules/sdl_raii/sdl_raii.pure_traits.ixxsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/src/sdl_wrapper.render_data.cppsdl_wrapper/modules/sdl_raii/sdl_raii.bound_raii.ixxmain.cppsdl_wrapper/modules/sdl_wrapper.renderable.ixxsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/src/sdl_wrapper.buffer.cppsdl_wrapper/modules/sdl_wrapper.render_procedural.ixxsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixxsdl_wrapper/modules/sdl_wrapper.texture.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/modules/sdl_raii/sdl_raii.ixxsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/modules/sdl_raii/sdl_raii.bound_traits.ixxsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/modules/sdl_wrapper.gpu.ixx
📚 Learning: 2025-11-13T11:34:32.732Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/src/sdl_wrapper.renderable.cppsdl_wrapper/CMakeLists.txtsdl_wrapper/modules/sdl_raii/sdl_raii.pure_traits.ixxsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/src/sdl_wrapper.render_data.cppsdl_wrapper/modules/sdl_raii/sdl_raii.bound_raii.ixxmain.cppsdl_wrapper/modules/sdl_wrapper.renderable.ixxsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/src/sdl_wrapper.buffer.cppsdl_wrapper/modules/sdl_wrapper.render_procedural.ixxsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixxsdl_wrapper/modules/sdl_wrapper.texture.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/modules/sdl_raii/sdl_raii.ixxsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/modules/sdl_raii/sdl_raii.bound_traits.ixxsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/modules/sdl_wrapper.gpu.ixx
🪛 markdownlint-cli2 (0.18.1)
README.md
53-53: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
60-60: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
63-63: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
66-66: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
67-67: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
68-68: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: CodeQL C++ Analysis
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Release, gcc)
🔇 Additional comments (33)
main.cpp (2)
37-40: LGTM!The
VertexTypestruct provides a clean, self-documenting vertex layout with position (x, y, z) and texture coordinates (u, v).
509-518: LGTM on RAII command buffer usage.Good adoption of
GpuCommandBufferRaiifor automatic resource management. Thereset()pattern correctly transfers ownership of the acquired command buffer.sdl_wrapper/modules/sdl_wrapper.decl.ixx (1)
12-12: LGTM!The forward declaration of
TextureWrapperaligns with the module restructuring whereApphas moved to thelifecyclemodule. This correctly exposesTextureWrapperfor dependent modules.README.md (2)
25-29: LGTM!The Mermaid diagram correctly documents the new module structure with
lifecycleandsdl_raiicomponents under thesdl_wrappersubgraph.
52-68: LGTM on track updates.The checklist correctly marks "Getting started", "Coordinate Systems", "Camera", and "Colors" as completed, reflecting the PR's progress.
sdl_wrapper/CMakeLists.txt (3)
10-15: LGTM!The new
lifecycleandsdl_raiimodule files are correctly added to the CXX_MODULES file set, establishing the RAII infrastructure for the project.
28-35: LGTM!The source file additions (
lifecycle.cpp,sdl_wrapper.renderable.cpp) correctly correspond to the new module interfaces.
38-38: Thelogoslibrary is properly defined in the project atlogos/CMakeLists.txtas a STATIC library and is available for linking. The PUBLIC dependency specification is appropriate sincelogosexposes C++ modules in its public interface (logos.ixx), consistent with howsdl_wrapperitself is structured with public C++ modules. No issues identified.sdl_wrapper/modules/sdl_raii/sdl_raii.pure_raii.ixx (1)
9-52: Clean RAII implementation.The
PureRaiiwrapper correctly implements move-only semantics with proper self-assignment guards, noexcept specifications, and traits-based resource release. The design is analogous tostd::unique_ptrand follows established RAII patterns.sdl_wrapper/modules/sdl_raii/sdl_raii.bound_raii.ixx (1)
9-64: Well-designed RAII wrapper for device-bound resources.The
BoundRaiicorrectly manages the device-resource pairing with proper move semantics and conditional release (only when both pointers are valid). This ensures SDL resources that require a device context for cleanup are handled safely.sdl_wrapper/modules/sdl_wrapper.gpu.ixx (2)
40-66: Correct RAII adoption in resource creation flow.The
create_gpu_context()function properly wraps raw SDL pointers into RAII wrappers immediately after successful creation, ensuring resources are automatically cleaned up on any subsequent failure or scope exit. TheClaimWindowRaiiis correctly created only after the claim succeeds.
96-129: The design is sound and does not require changes.create_pipeline()andcreate_shader()returning raw pointers is intentional—all call sites properly wrap the returned pointers in RAII types (GPUGraphicsPipelineRaiiandGpuShaderRaii) immediately after validation. The[[nodiscard]]attribute ensures developers notice the return value, and no resource leaks are possible with the current usage pattern.Likely an incorrect or invalid review comment.
sdl_wrapper/src/lifecycle.cpp (1)
7-7: LGTM!The import change correctly reflects the module rename from
sdl_wrapper:apptolifecycle, aligning with the broader module reorganization in this PR.sdl_wrapper/src/sdl_wrapper.renderable.cpp (1)
17-35: LGTM!The draw implementation correctly handles pipeline submission errors and follows the standard SDL GPU rendering workflow: bind pipeline, push uniforms, bind buffers, optionally bind samplers, then draw.
sdl_wrapper/src/sdl_wrapper.render_procedural.cpp (3)
160-163: LGTM! Clean RAII ownership transfer.The RAII wrapper correctly takes ownership of the new pipeline. The old pipeline (if any) is automatically released when the wrapper is reassigned.
199-203: Raw pointer stored after RAII assignment is safe here.The pattern of assigning the RAII wrapper then storing the raw pointer in
m_pipeline_infoworks correctly since both are class members with the same lifetime, andRenderProceduralis non-copyable.
246-250: Consistent RAII pattern for fragment shader.Same safe pattern as vertex shader - RAII wrapper owns the resource, raw pointer stored for pipeline creation.
sdl_wrapper/modules/sdl_wrapper.render_procedural.ixx (2)
24-26: LGTM! RAII wrappers replace raw pointers.The member types are correctly updated to use
GPUGraphicsPipelineRaiiandGpuShaderRaii, enabling automatic resource cleanup.
49-49: Good rename fromdata()toraw().The
raw()naming is more conventional and clearly indicates the method returns the underlying raw pointer.sdl_wrapper/modules/sdl_raii/sdl_raii.pure_traits.ixx (2)
14-18: Verify:SDL_SubmitGPUCommandBufferas RAII release may cause unintended submissions.Using
SDL_SubmitGPUCommandBufferin the RAII release method means the command buffer will be submitted to the GPU whenever the wrapper goes out of scope or is destroyed. This conflates resource cleanup with an action (submission). If the buffer was already submitted or shouldn't be submitted (e.g., on error paths), this could cause issues.Consider whether this is the intended behavior, or if a no-op/different cleanup is more appropriate for command buffers.
19-28: LGTM! Standard SDL resource cleanup.The
SDL_DestroyGPUDeviceandSDL_DestroyWindowrelease functions correctly match the expected cleanup operations for these SDL types.sdl_wrapper/modules/lifecycle.ixx (1)
6-6: Module renamed from partition to standalone module.The module declaration changed from
sdl_wrapper:app(a partition) tolifecycle(a standalone module). All consuming modules have been correctly updated—bothmain.cppandsdl_wrapper/src/lifecycle.cppimport the newlifecyclemodule, and no stale imports of the old partition remain.sdl_wrapper/modules/sdl_wrapper.renderable.ixx (1)
23-31: LGTM!The
Renderableclass structure is appropriate. PassingRenderContexby value is acceptable given its lightweight composition of pointers.sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (3)
17-23: LGTM!The
reset()method properly guards against null device before releasing the RAII resource.
25-48: LGTM!The
submit()method correctly handles buffer mapping, data copying, and usage tracking with proper error propagation.
49-64: LGTM!The builder correctly wraps the raw SDL transfer buffer in a RAII wrapper immediately after creation and transfers ownership via
std::move.sdl_wrapper/modules/sdl_wrapper.buffer.ixx (2)
20-35: LGTM!The
BufferWrapperclass correctly adopts RAII ownership. The constructor properly moves theGpuBufferRaiiwrapper, and member initialization order is correct.
58-74: LGTM!Defaulting move operations and destructor is the correct approach when using RAII wrappers for resource management.
sdl_wrapper/src/sdl_wrapper.buffer.cpp (2)
84-106: LGTM!The builder correctly wraps the raw GPU buffer in RAII immediately after creation. Error paths are now safe as the RAII wrapper handles cleanup automatically.
45-82: No action required - command buffer is automatically submitted via destructor.The
GpuCommandBufferRaiiis aPureRaii<SDL_GPUCommandBuffer>which automatically callsSDL_SubmitGPUCommandBufferin its destructor. When the command buffer goes out of scope, the destructor invokesPureTraits<SDL_GPUCommandBuffer>::release(), which submits the buffer to the GPU. The code is correct as written and requires no modification.Likely an incorrect or invalid review comment.
sdl_wrapper/src/sdl_wrapper.texture.cpp (1)
105-113: LGTM!Sampler creation and RAII wrapping is correct. The
TextureWrapperis properly constructed with moved RAII resources.sdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx (2)
21-21: RAII integration and defaulted moves look good; please confirm underlying move/reset semantics.Importing
sdl_raii, storingGpuTransferBufferRaiiby value, and defaulting the move ctor/assignment is a clean upgrade from the raw pointer; just double‑check thatGpuTransferBufferRaii’s move operations leave the source “empty” and thatTransferBufferWrapper::reset()only releases when owning a handle so moved‑from objects can’t trigger a second release. Based on learnings, keeping this non‑throwing and deferring extra SDL/null checks is consistent with the existing design.Also applies to: 33-41, 48-50
65-65: raw() forwarding through the RAII wrapper keeps the external API stable.Exposing
m_transfer_buffer.raw()preserves the originalSDL_GPUTransferBuffer*interface at call sites while centralizing ownership in the RAII member; no issues here.
| TextureWrapper(std::shared_ptr<GpuWrapper> gpu, GpuTextureRaii texture, GPUSamplerRaii sampler) noexcept : | ||
| m_gpu(std::move(gpu)), m_texture(std::move(texture)), m_sampler(std::move(sampler)) | ||
| { | ||
| m_tex_binding.texture = m_texture; | ||
| m_tex_binding.sampler = m_sampler; | ||
| m_tex_binding.texture = m_texture.raw(); | ||
| m_tex_binding.sampler = m_sampler.raw(); | ||
| } |
There was a problem hiding this comment.
Move operations will leave m_tex_binding with dangling pointers.
When TextureWrapper is moved, the defaulted move constructor/assignment will move m_texture and m_sampler RAII wrappers (invalidating their internal pointers in the source), but m_tex_binding is just shallow-copied. The moved-from object's m_tex_binding will contain dangling pointers, and the moved-to object's m_tex_binding will still point to the moved-from RAII wrappers' (now-invalid) raw pointers.
You need to update m_tex_binding after a move. Consider:
- TextureWrapper(TextureWrapper&& other) noexcept = default;
- TextureWrapper& operator=(TextureWrapper&& other) noexcept = default;
+ TextureWrapper(TextureWrapper&& other) noexcept :
+ m_gpu(std::move(other.m_gpu)),
+ m_texture(std::move(other.m_texture)),
+ m_sampler(std::move(other.m_sampler)),
+ m_tex_binding{}
+ {
+ m_tex_binding.texture = m_texture.raw();
+ m_tex_binding.sampler = m_sampler.raw();
+ }
+ TextureWrapper& operator=(TextureWrapper&& other) noexcept
+ {
+ if (this != &other)
+ {
+ m_gpu = std::move(other.m_gpu);
+ m_texture = std::move(other.m_texture);
+ m_sampler = std::move(other.m_sampler);
+ m_tex_binding.texture = m_texture.raw();
+ m_tex_binding.sampler = m_sampler.raw();
+ }
+ return *this;
+ }Also applies to: 34-35
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
sdl_wrapper/modules/sdl_wrapper.renderable.ixx (2)
15-21: Clarify ownership/validity expectations forRenderContextpointers
RenderContextcarries rawSDL_GPURenderPass*andSDL_GPUCommandBuffer*plus astd::shared_ptr<TextureWrapper>. It would help future readers to document (in a comment or docstring) whether these pointers are:
- always non‑null, and
- required to outlive the
draw()call.If they are required to be non‑null, consider using references or a
not_null‑style wrapper later; for now, even a brief comment on expected lifetime/validity would reduce ambiguity.
26-31: Consider const‑correct accessors and passingRenderContextby const reference (and fix the typo)As this is a public API, a couple of small tweaks would make it cleaner and cheaper to use:
- Add
constoverloads forprocedural()/data()soRenderablecan be used from const contexts.- Take
RenderContextbyconst&to avoid copying the matrix and shared_ptr whendrawdoesn’t need to modify it.- Fix the parameter name typo
render_contex→render_context.For example:
- auto& procedural() { return m_render_procedural; } - std::shared_ptr<RenderData>& data() { return m_render_data; } - checkable<std::monostate> draw(RenderContext render_contex); + auto& procedural() { return m_render_procedural; } + const auto& procedural() const { return m_render_procedural; } + + std::shared_ptr<RenderData>& data() { return m_render_data; } + const std::shared_ptr<RenderData>& data() const { return m_render_data; } + + checkable<std::monostate> draw(const RenderContext& render_context);(You’d also need to update the corresponding implementation in the
.cpp.)main.cpp (6)
25-28: Minor: Extraneous blank lines between includes.There are multiple consecutive blank lines between
<chrono>andstb_image.h. Consider consolidating for consistency.#include <chrono> - - #include "stb_image.h"
129-131: Consider using a slightly larger epsilon for alpha testing.The alpha threshold of
0.001is extremely small. Depending on texture formats and precision, you might want a slightly higher value (e.g.,0.01or0.1) to reliably discard transparent fragments.
326-327: Magic number for movement speed multiplier.The
0.1multiplier for location updates could be extracted to a named constant (e.g.,constexpr float kMovementScale = 0.1f;) for clarity and easier tuning.
358-482: Remove or track large commented-out code block.This ~120-line commented block clutters the codebase. Consider:
- Removing it entirely if no longer needed.
- Moving it to a separate branch/stash if it's work-in-progress.
- Adding a TODO comment explaining when it will be restored.
Would you like me to open an issue to track restoring or removing this editor functionality?
749-754: Consider extracting magic numbers and translating comments.The mouse sensitivity (
0.01) and pitch clamp values (1.55f) are magic numbers. The comments are in Chinese which may reduce accessibility for international contributors.+ // Example constants at class scope: + // static constexpr float kMouseSensitivity = 0.01f; + // static constexpr float kMaxPitch = 1.55f; // ~89 degrees - // 你自己的 yaw/pitch 变量（示例） - yaw += dx * 0.01; - pitch -= dy * 0.01; - - // 可选：限制 pitch 防止翻转 - pitch = std::clamp(pitch, -1.55f, 1.55f); // ~(-89°, 89°) + // Update yaw/pitch from mouse delta + yaw += dx * kMouseSensitivity; + pitch -= dy * kMouseSensitivity; + + // Clamp pitch to prevent camera flip + pitch = std::clamp(pitch, -kMaxPitch, kMaxPitch);
783-786: Consider returningstd::unique_ptr<sopho::App>instead of raw pointer.The current signature requires the caller to manually manage ownership. If the
sopho::checkableand framework constraints allow, returningstd::unique_ptr<sopho::App>would make ownership explicit and prevent memory leaks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
main.cpp(14 hunks)sdl_wrapper/modules/sdl_wrapper.renderable.ixx(1 hunks)sdl_wrapper/src/sdl_wrapper.renderable.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdl_wrapper/src/sdl_wrapper.renderable.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/modules/sdl_wrapper.renderable.ixxmain.cpp
📚 Learning: 2025-11-14T00:45:09.403Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
sdl_wrapper/modules/sdl_wrapper.renderable.ixxmain.cpp
📚 Learning: 2025-11-13T11:34:32.732Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/modules/sdl_wrapper.renderable.ixxmain.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: CodeQL C++ Analysis
- GitHub Check: build (windows-latest, Release, clang)
🔇 Additional comments (4)
sdl_wrapper/modules/sdl_wrapper.renderable.ixx (1)
5-10: Includes/imports look appropriate for the new API surface
<SDL3/SDL_gpu.h>and<variant>are justified bySDL_GPU*types andstd::monostateincheckable, and thelogos/data_typeimports match the exported types used below. No issues here.main.cpp (3)
37-40: LGTM!Clean POD struct for vertex data with appropriate default initialization.
225-237: Second pipeline reuses first renderable's data - verify intentional sharing.The second renderable shares render data with the first via
m_renderables[0]->data(). This means both renderables will render the same geometry. Ensure this is intentional and that updates to the sharedRenderDatawon't cause unexpected side effects.
768-775: LGTM!Proper cleanup order: GPU texture released before ImGui shutdown.
| for (int i = 0; i < m_renderables.size(); i++) | ||
| { | ||
| SDL_LogError(SDL_LOG_CATEGORY_GPU, "Texture not available, skip binding sampler"); | ||
| auto renderable = m_renderables[i]; | ||
| auto camera_mat = sopho::perspective(1, static_cast<float>(width) / height, 0.1, 10) * | ||
| sopho::rotation_x(-pitch) * sopho::rotation_y(yaw) * | ||
| sopho::translate(0 - location(0), 0.5 * i - location(1), -i - 5 - location(2)); | ||
| renderable->draw(sopho::RenderContext{.render_pass = renderPass, | ||
| .command_buffer = command_buffer_raii.raw(), | ||
| .camera_mat = camera_mat, | ||
| .texture_wrapper = i == 0 ? m_texture_wrapper : nullptr}); | ||
| } |
There was a problem hiding this comment.
Signed/unsigned comparison in loop.
m_renderables.size() returns size_t, but loop variable i is int. This causes a signed/unsigned comparison warning and could theoretically overflow for very large containers.
- for (int i = 0; i < m_renderables.size(); i++)
+ for (std::size_t i = 0; i < m_renderables.size(); ++i)Alternatively, use a range-based for with index:
for (auto i = 0uz; i < m_renderables.size(); ++i)🤖 Prompt for AI Agents
main.cpp lines 585-595: the for-loop uses int i while comparing against
m_renderables.size() (size_t), causing a signed/unsigned warning; change the
loop to use a size_t (or auto) for the index (e.g. size_t i = 0; i <
m_renderables.size(); ++i) or convert to a range-based loop with an explicit
index variable, and ensure any arithmetic using i that expects floating point
casts i to float/double as needed.
| auto now{std::chrono::steady_clock::now()}; | ||
| std::chrono::duration<double> delta = now - m_last_time; | ||
| m_last_time = now; | ||
| auto result = update(delta.count()); |
There was a problem hiding this comment.
Implicit narrowing from double to float.
delta.count() returns double (from std::chrono::duration<double>), but update() takes float. This is an implicit narrowing conversion.
- auto result = update(delta.count());
+ auto result = update(static_cast<float>(delta.count()));🤖 Prompt for AI Agents
main.cpp around lines 611-614: delta.count() yields a double but update()
accepts a float causing an implicit narrowing conversion; fix by making the
conversion explicit or widening the update API. Either change update(float) to
update(double) (and update all callers/signature) so the precise duration is
preserved, or replace the call with update(static_cast<float>(delta.count())) to
explicitly convert to float; choose one approach and apply it consistently
across the codebase.
|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^|
|
||
| std::vector<VertexType> vertices{ | ||
| {0.5, 0.5, 0.5, 0., 0.}, {-0.5, 0.5, 0.5, 1., 0.}, {0.5, -0.5, 0.5, 0., 1.}, {-0.5, -0.5, 0.5, 1., 1.}, | ||
| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, |
There was a problem hiding this comment.
warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
{0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.},
^| {0.5, 0.5, -0.5, 1., 1.}, {-0.5, 0.5, -0.5, 0., 1.}, {0.5, -0.5, -0.5, 1., 0.}, {-0.5, -0.5, -0.5, 0., 0.}, | ||
| }; | ||
|
|
||
| std::vector<std::uint32_t> indices{0, 1, 2, 1, 2, 3, 0, 1, 4, 1, 4, 5, 0, 2, 4, 2, 4, 6, |
There was a problem hiding this comment.
warning: 5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
std::vector<std::uint32_t> indices{0, 1, 2, 1, 2, 3, 0, 1, 4, 1, 4, 5, 0, 2, 4, 2, 4, 6,
^| return SDL_APP_CONTINUE; | ||
|
|
||
| SDL_GPUCommandBuffer* command_buffer = SDL_AcquireGPUCommandBuffer(device); | ||
| if (!command_buffer) |
There was a problem hiding this comment.
warning: implicit conversion 'SDL_GPUCommandBuffer *' -> 'bool' [readability-implicit-bool-conversion]
| if (!command_buffer) | |
| if (command_buffer == nullptr) |
| SDL_GPUCommandBuffer* command_buffer = SDL_AcquireGPUCommandBuffer(device); | ||
| if (!command_buffer) | ||
| { | ||
| SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to acquire GPU command buffer"); |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to acquire GPU command buffer");
^| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 135 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 135 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 135 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
main.cpp (4)
30-30: Module 'lifecycle' not found.This has already been flagged by static analysis but remains unresolved. Ensure the module is properly configured and available.
133-142: Public member variable flagged by static analysis.The
fragment_source2member has public visibility, which was flagged by cppcoreguidelines. Consider making shader sources private with accessor methods if needed.
585-595: Signed/unsigned comparison in loop.The loop variable type mismatch has already been flagged. The rendering logic itself is sound, with per-renderable camera matrices and conditional texture binding.
611-614: Implicit narrowing from double to float.This narrowing conversion has already been flagged by a previous review. The timing logic and control flow are otherwise correct.
🧹 Nitpick comments (1)
main.cpp (1)
717-758: Good mouse input handling with minor redundancy.The mouse drag implementation correctly respects ImGui's input capture. However, note that pitch clamping at line 754 duplicates the clamping in
update()at line 324. This is harmless but consider removing one for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(14 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cpp
📚 Learning: 2025-11-14T00:45:09.403Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
main.cpp
📚 Learning: 2025-11-13T11:34:32.732Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cpp
🔇 Additional comments (9)
main.cpp (9)
37-40: LGTM!The
VertexTypestruct is well-defined with proper default initialization for vertex position and texture coordinates.
75-77: LGTM!The new member variables properly support the enhanced functionality: FPS tracking, multiple renderables, and camera motion with input state.
Also applies to: 81-81, 89-96
188-194: LGTM!Good use of modern C++ with
std::vectorandstd::spanfor vertex and index data. The typed approach withVertexTypeimproves type safety.Also applies to: 201-202
313-329: LGTM!The update method properly handles per-frame timing, FPS tracking, and camera motion with appropriate pitch clamping to prevent gimbal lock.
509-518: LGTM!Proper use of RAII wrapper for command buffer management. The pattern of acquiring the buffer and wrapping it ensures proper cleanup.
632-715: LGTM with note on ImGui input handling.Proper use of
io.WantCaptureKeyboardto prevent input conflicts with ImGui. The keyboard controls for camera movement and rotation are well-structured.
538-554: LGTM!The dynamic depth texture resizing logic correctly detects dimension changes and recreates the depth texture with the new size.
601-602: LGTM!Proper use of RAII wrapper for ImGui rendering pass, maintaining consistency with the main rendering code.
783-784: LGTM!Documentation accurately reflects the function's behavior and ownership semantics.
| m_renderables.emplace_back(std::make_shared<sopho::Renderable>(sopho::Renderable{ | ||
| .m_render_procedural = std::make_shared<sopho::RenderProcedural>(std::move(pw_result2.value())), | ||
| .m_render_data = m_renderables[0]->data()})); |
There was a problem hiding this comment.
Consider the data sharing dependency.
The second renderable shares vertex data from m_renderables[0]->data(). If the first renderable is ever removed or reordered in the vector, this could cause issues. Consider documenting this dependency or using a shared pointer to the data directly.
🤖 Prompt for AI Agents
In main.cpp around lines 235 to 237, the second renderable is taking raw access
to m_renderables[0]->data(), creating a hidden lifetime/order dependency that
breaks if the first element is removed or reordered; change the code to share
the vertex data via a shared_ptr (e.g., make the data a std::shared_ptr and
pass/assign that shared_ptr into the new Renderable) or otherwise store the
shared_ptr in a local variable and use that instead of indexing
m_renderables[0], or add a clear comment documenting that the second renderable
depends on element 0 and prevent removal/reordering by design; ensure both
renderables hold a shared_ptr to the same data so lifetime is managed safely.
main.cpp
Outdated
| m_last_x = x; | ||
| m_last_y = y; | ||
|
|
||
| // 你自己的 yaw/pitch 变量（示例） |
There was a problem hiding this comment.
Replace Chinese comment with English.
The comment "你自己的 yaw/pitch 变量（示例）" should be translated to English for codebase consistency.
Apply this diff:
- // 你自己的 yaw/pitch 变量（示例）
+ // Update yaw/pitch based on mouse delta📝 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.
| // 你自己的 yaw/pitch 变量（示例） | |
| // Update yaw/pitch based on mouse delta |
🤖 Prompt for AI Agents
In main.cpp around line 749, replace the Chinese comment "你自己的 yaw/pitch 变量（示例）"
with an English equivalent to keep the codebase consistent; e.g. change it to
"your own yaw/pitch variables (example)" or similar clear English wording;
ensure punctuation and casing match surrounding comments.
| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 255 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 255.0F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 255.0F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 255.0F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^| // Create the color target. | ||
| SDL_GPUColorTargetInfo colorTargetInfo{}; | ||
| colorTargetInfo.clear_color = {240 / 255.0F, 240 / 255.0F, 240 / 255.0F, 255 / 255.0F}; | ||
| colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F}; |
There was a problem hiding this comment.
warning: 255.0F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
colorTargetInfo.clear_color = {135 / 255.0F, 135 / 255.0F, 135 / 255.0F, 255 / 255.0F};
^| switch (event->type) | ||
| { | ||
| case SDL_EVENT_MOUSE_BUTTON_DOWN: | ||
| if (event->button.button == SDL_BUTTON_LEFT) |
There was a problem hiding this comment.
warning: no header providing "SDL_BUTTON_LEFT" is directly included [misc-include-cleaner]
main.cpp:14:
- #include "imgui.h"
+ #include "SDL3/SDL_mouse.h"
+ #include "imgui.h"| case SDLK_RIGHT: | ||
| yaw += 0.1F; | ||
|
|
||
| case SDL_EVENT_MOUSE_BUTTON_UP: |
There was a problem hiding this comment.
warning: no header providing "SDL_EVENT_MOUSE_BUTTON_UP" is directly included [misc-include-cleaner]
case SDL_EVENT_MOUSE_BUTTON_UP:
^| break; | ||
| default: | ||
|
|
||
| case SDL_EVENT_MOUSE_MOTION: |
There was a problem hiding this comment.
warning: no header providing "SDL_EVENT_MOUSE_MOTION" is directly included [misc-include-cleaner]
case SDL_EVENT_MOUSE_MOTION:
^| case SDL_EVENT_MOUSE_MOTION: | ||
| if (m_dragging) | ||
| { | ||
| float x = event->motion.x; |
There was a problem hiding this comment.
warning: variable 'x' of type 'float' can be declared 'const' [misc-const-correctness]
| float x = event->motion.x; | |
| float const x = event->motion.x; |
| case SDL_EVENT_MOUSE_MOTION: | ||
| if (m_dragging) | ||
| { | ||
| float x = event->motion.x; |
There was a problem hiding this comment.
warning: variable name 'x' is too short, expected at least 3 characters [readability-identifier-length]
float x = event->motion.x;
^
| if (m_dragging) | ||
| { | ||
| float x = event->motion.x; | ||
| float y = event->motion.y; |
There was a problem hiding this comment.
warning: variable 'y' of type 'float' can be declared 'const' [misc-const-correctness]
| float y = event->motion.y; | |
| float const y = event->motion.y; |
| if (m_dragging) | ||
| { | ||
| float x = event->motion.x; | ||
| float y = event->motion.y; |
There was a problem hiding this comment.
warning: variable name 'y' is too short, expected at least 3 characters [readability-identifier-length]
float y = event->motion.y;
^| float x = event->motion.x; | ||
| float y = event->motion.y; | ||
|
|
||
| float dx = x - m_last_x; |
There was a problem hiding this comment.
warning: variable 'dx' of type 'float' can be declared 'const' [misc-const-correctness]
| float dx = x - m_last_x; | |
| float const dx = x - m_last_x; |
| float x = event->motion.x; | ||
| float y = event->motion.y; | ||
|
|
||
| float dx = x - m_last_x; |
There was a problem hiding this comment.
warning: variable name 'dx' is too short, expected at least 3 characters [readability-identifier-length]
float dx = x - m_last_x;
^| float y = event->motion.y; | ||
|
|
||
| float dx = x - m_last_x; | ||
| float dy = y - m_last_y; |
There was a problem hiding this comment.
warning: variable 'dy' of type 'float' can be declared 'const' [misc-const-correctness]
| float dy = y - m_last_y; | |
| float const dy = y - m_last_y; |
| // | ||
| module; | ||
| #include <SDL3/SDL_gpu.h> | ||
| #include <expected> |
There was a problem hiding this comment.
warning: included header SDL_gpu.h is not used directly [misc-include-cleaner]
| #include <expected> | |
| ; | |
| module; | ||
| #include <SDL3/SDL_gpu.h> | ||
| #include <expected> | ||
| #include <variant> |
There was a problem hiding this comment.
warning: included header expected is not used directly [misc-include-cleaner]
| #include <variant> | |
| > | |
| #include <SDL3/SDL_gpu.h> | ||
| #include <expected> | ||
| #include <variant> | ||
| module sdl_wrapper; |
There was a problem hiding this comment.
warning: included header variant is not used directly [misc-include-cleaner]
| module sdl_wrapper; | |
| > | |
| #include <SDL3/SDL_gpu.h> | ||
| #include <expected> | ||
| #include <variant> | ||
| module sdl_wrapper; |
There was a problem hiding this comment.
warning: module 'sdl_wrapper' not found [clang-diagnostic-error]
>
^| #include <expected> | ||
| #include "SDL3/SDL_gpu.h" | ||
| #include "SDL3/SDL_log.h" | ||
| module sdl_wrapper; |
There was a problem hiding this comment.
warning: module 'sdl_wrapper' not found [clang-diagnostic-error]
module sdl_wrapper;
^
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
main.cpp (5)
358-482: Large editor block is fully commented‑out; either remove it or gate it with a clear TODO.This big block for vertex/shader editing is still present but commented out, and
currentis now effectively unused. That’s dead code from a maintenance perspective.Either delete it if it’s no longer needed, or wrap it in
#if 0with a clear TODO / issue reference (or restore it so it builds), so future readers know whether it’s intentional and who owns re‑enabling it.
636-637: SDL event/mouse macros: consider including the dedicated headers to satisfy tools.You use
SDL_EVENT_KEY_DOWN,SDL_EVENT_MOUSE_BUTTON_DOWN,SDL_BUTTON_LEFT, etc., and rely onSDL3/SDL.hto pull them in transitively. Clang‑tidy’s include‑cleaner is still warning about missing direct headers; if you want a quiet static‑analysis run, you could follow its suggestion:-#include "imgui.h" +#include "SDL3/SDL_events.h" +#include "SDL3/SDL_mouse.h" +#include "imgui.h"Functionally it’s fine today; this is mostly about include hygiene.
Also applies to: 721-737
221-237: Hidden dependency onm_renderables[0]’s data in the second renderable.The second renderable pulls its render data from
m_renderables[0]->data(). This couples its lifetime and meaning to “whatever happens to be at index 0” and will break if you ever reorder, remove, or insert before element 0.It would be safer to hold the underlying data in a separate shared handle (or at least a named local) and pass that into both renderables, or to add a comment making this dependency explicit so future changes don’t accidentally violate the assumption.
585-595: Usestd::size_t(or range‑for) for indexingm_renderables.The loop currently mixes
intwithm_renderables.size()(asize_t), which generates signed/unsigned comparison warnings and is slightly brittle:for (int i = 0; i < m_renderables.size(); i++)Consider:
- for (int i = 0; i < m_renderables.size(); i++) + for (std::size_t i = 0; i < m_renderables.size(); ++i)(or a range‑for with an explicit index if you prefer). This aligns types and keeps static analysis happy.
611-619: Makeupdate’s time parameter type consistent withdelta.count().
deltais astd::chrono::duration<double>, sodelta.count()isdouble, butupdatetakes afloat. Right now you rely on an implicit narrowing conversion:auto result = update(delta.count());Either change
updateto takedoubleor make the conversion explicit:- auto result = update(delta.count()); + auto result = update(static_cast<float>(delta.count()));This makes the intentional precision loss clear and avoids the narrowing warning.
🧹 Nitpick comments (6)
main.cpp (6)
37-40: Vertex layout and cube buffers look consistent; consider deriving counts from container sizes.
VertexType’s{x, y, z, u, v}layout lines up with the GLSLvec3 a_position/vec2 a_uv, and the 8 vertices + 36 indices form a sane cube. To reduce future mismatch risk if you tweak the vectors, you could derive the counts directly:- auto render_data = sopho::RenderData::Builder{} - .set_vertex_layout(pw_result.value().vertex_layout()) - .set_vertex_count(8) - .set_index_count(36) - .set_vertices(std::span(vertices)) - .set_indices(std::span(indices)) + auto render_data = sopho::RenderData::Builder{} + .set_vertex_layout(pw_result.value().vertex_layout()) + .set_vertex_count(static_cast<std::uint32_t>(vertices.size())) + .set_index_count(static_cast<std::uint32_t>(indices.size())) + .set_vertices(std::span(vertices)) + .set_indices(std::span(indices))Also applies to: 188-195, 201-202
129-131: Fragment shader tweaks look fine; consider naming/documenting the “solid white” variant.The alpha discard in
fragment_sourceand the always‑whitefragment_source2are straightforward and match the new dual‑pipeline setup. Iffragment_source2is intended as a debug/outline or “unlit” shader, a short comment or a more descriptive variable name (e.g.fragment_source_unlit) would make its purpose clearer.Also applies to: 133-142
509-519: RAII command buffer integration is good; consider handling depth texture recreation failure.The
sopho::GpuCommandBufferRaiiwrapper nicely guarantees submission even on early returns, and the swapchain/resize path looks reasonable.In the resize branch you recreate
SceneDepthTexturewithout checking for failure; ifSDL_CreateGPUTexturereturnsnullptryou’ll proceed with a null depth target:SceneDepthTexture = SDL_CreateGPUTexture(m_gpu->device(), &ci);You might want to guard this, e.g.:
- SDL_ReleaseGPUTexture(m_gpu->device(), SceneDepthTexture); - SceneDepthTexture = SDL_CreateGPUTexture(m_gpu->device(), &ci); + SDL_GPUTexture* new_depth = SDL_CreateGPUTexture(m_gpu->device(), &ci); + if (!new_depth) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to recreate depth texture on resize"); + // Optionally keep old depth texture instead of releasing it. + } + else + { + SDL_ReleaseGPUTexture(m_gpu->device(), SceneDepthTexture); + SceneDepthTexture = new_depth; + }Also applies to: 531-554, 581-583, 601-602
566-567: Background clear color is a hard‑coded magic value; optional: lift to a named constant.
135 / 255.0Frepeated three times plus the alpha literal work, but are opaque at the call site. If you care about readability here, you could define a named color:constexpr SDL_FColor kBackgroundColor{135.0F / 255.0F, 135.0F / 255.0F, 135.0F / 255.0F, 1.0F}; ... colorTargetInfo.clear_color = kBackgroundColor;
630-715: Keyboard camera controls and ImGui capture gating look good.Conditioning the key handling on
!io.WantCaptureKeyboardavoids fighting ImGui, and the mapping from arrow/WASD/EQ keys topitch_speed,yaw_speed, andspeedcomponents is straightforward.If you later expand controls, you might want to centralize the
0.5Fand1.Fliterals into configurable constants, but the current code is clear enough.
717-753: Mouse drag‑to‑look logic is simple and correct; optional small cleanups.The drag state (
m_dragging,m_last_x/y) plusdx/dydeltas and pitch clamping form a clean FPS‑style mouse look, and gating on!io.WantCaptureMouseis the right pattern.You could optionally:
- Make
x,y,dx,dyconst floatsince they’re not reassigned.- Extract the
0.01sensitivity into a named constant for easier tuning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(14 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cpp
📚 Learning: 2025-11-13T02:58:26.892Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.892Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
main.cpp
📚 Learning: 2025-11-13T11:34:32.732Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cpp
📚 Learning: 2025-11-14T00:45:09.403Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
main.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: CodeQL C++ Analysis
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: clang-tidy-review
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: CodeQL C++ Analysis
🔇 Additional comments (2)
main.cpp (2)
129-131: Alpha discard threshold is fine; ensure it matches your asset expectations.
if (FragColor.a <= 0.001) discard;is a reasonable epsilon for treating texels as fully transparent. Just confirm your textures don’t rely on very low non‑zero alpha values being visible; if they do, you might want a slightly lower or configurable threshold.
781-782: Ownership note increate_appdoc is helpful.Clarifying that the caller owns and must delete the
App*returned bycreate_appmatches the currentnew UserApp()implementation and will help avoid confusion for integrators.
Summary by CodeRabbit
New Features
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.