Add Camera add control rotate #30
Conversation
WalkthroughAdds a CameraUniform and yaw/pitch state to UserApp, computes a per-frame 4×4 view matrix and uploads it as a vertex uniform; extends GpuWrapper::create_shader to accept a uniform-buffer count and updates shader creation call sites to pass that count. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input Handler
participant App as UserApp
participant GPU as GPU / Shader
Input->>App: Arrow key events (LEFT/RIGHT/UP/DOWN)
App->>App: update yaw / pitch (clamp pitch)
Note right of App `#DDEBF7`: compute 4×4 view matrix (cam.m)
App->>GPU: SDL_PushGPUVertexUniformData(cam) and bind vertex uniform
GPU->>GPU: vertex shader multiplies positions by uView (cam)
GPU->>GPU: rasterize / draw
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Docstrings generation - SUCCESS |
|
Here's the code health analysis summary for commits Analysis Summary
|
Docstrings generation was requested by @WSQS. * #30 (comment) The following files were modified: * `main.cpp`
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include <SDL3/SDL.h> | ||
| #include <SDL3/SDL_gpu.h> | ||
| #include "SDL3/SDL_log.h" | ||
| import sdl_wrapper; |
There was a problem hiding this comment.
warning: module 'sdl_wrapper' not found [clang-diagnostic-error]
import sdl_wrapper;
^|
|
||
| struct CameraUniform | ||
| { | ||
| std::array<float, 16> m{}; |
There was a problem hiding this comment.
warning: 16 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
std::array<float, 16> m{};
^| sopho::BufferWrapper vertex_buffer{gpu_wrapper->create_buffer(SDL_GPU_BUFFERUSAGE_VERTEX, sizeof(vertices))}; | ||
| sopho::PipelineWrapper pipeline_wrapper{gpu_wrapper->create_pipeline()}; | ||
|
|
||
| float yaw = 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 = 0.0f; | |
| float yaw = 0.0F; |
| sopho::PipelineWrapper pipeline_wrapper{gpu_wrapper->create_pipeline()}; | ||
|
|
||
| float yaw = 0.0f; | ||
| float pitch = 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 = 0.0f; | |
| float pitch = 0.0F; |
|
|
||
| { | ||
|
|
||
| float cy = std::cos(yaw); |
There was a problem hiding this comment.
warning: variable 'cy' of type 'float' can be declared 'const' [misc-const-correctness]
| float cy = std::cos(yaw); | |
| float const cy = std::cos(yaw); |
main.cpp
Outdated
| float cp = std::cos(pitch); | ||
| float sp = std::sin(pitch); | ||
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0F, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
main.cpp
Outdated
| float cp = std::cos(pitch); | ||
| float sp = std::sin(pitch); | ||
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0F, 0.0f, 0.0f, 0.0f, 1.0f}; |
main.cpp
Outdated
| float cp = std::cos(pitch); | ||
| float sp = std::sin(pitch); | ||
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0F, 0.0f, 0.0f, 1.0f}; |
main.cpp
Outdated
| float cp = std::cos(pitch); | ||
| float sp = std::sin(pitch); | ||
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0F, 0.0f, 1.0f}; |
main.cpp
Outdated
| float cp = std::cos(pitch); | ||
| float sp = std::sin(pitch); | ||
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0F, 1.0f}; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
sdl_wrapper/sdl_wrapper.pipeline.cpp (1)
128-128: Consider using named constants for uniform buffer counts.The hardcoded values
1and0passed as the third parameter tocreate_shaderare not self-documenting. Using named constants would improve readability and maintainability.Consider defining constants at the class or namespace level:
// At class or namespace scope static constexpr uint32_t VERTEX_UNIFORM_BUFFER_COUNT = 1; static constexpr uint32_t FRAGMENT_UNIFORM_BUFFER_COUNT = 0;Then use them in the calls:
- m_vertex_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_VERTEX, 1); + m_vertex_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_VERTEX, VERTEX_UNIFORM_BUFFER_COUNT);- m_fragment_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_FRAGMENT, 0); + m_fragment_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_FRAGMENT, FRAGMENT_UNIFORM_BUFFER_COUNT);Also applies to: 157-157
main.cpp (3)
94-101: Consider simplifying identity matrix initialization.The manual identity matrix initialization is correct but verbose. You could simplify it with standard library utilities or a helper function.
Example using
std::fill:std::fill(cam.m.begin(), cam.m.end(), 0.0F); cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F;Or create a reusable helper:
static void setIdentity(std::array<float, 16>& mat) { std::fill(mat.begin(), mat.end(), 0.0F); mat[0] = mat[5] = mat[10] = mat[15] = 1.0F; }
262-288: Consider extracting matrix operations for reusability.The view matrix computation is mathematically correct, but the inline lambda for matrix multiplication adds complexity to the draw function. Extracting it would improve readability and enable reuse.
Consider moving matrix operations to helper functions:
// At class or namespace scope static void multiply4x4(const float* A, const float* B, float* result) { for (int col = 0; col < 4; ++col) { for (int row = 0; row < 4; ++row) { result[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] + A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3]; } } } static void makeRotationY(float angle, float* matrix) { float c = std::cos(angle); float s = std::sin(angle); float temp[16] = { c, 0.0f, s, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -s, 0.0f, c, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f }; std::copy(std::begin(temp), std::end(temp), matrix); }This would make the draw function more readable while keeping the matrix math testable and reusable.
337-356: Input handling looks good; consider angle normalization for long sessions.The keyboard controls correctly update yaw and pitch angles. The implementation is straightforward and functional.
For very long sessions, unbounded angle accumulation could lead to floating-point precision loss. Consider normalizing angles to the range [0, 2π) or [-π, π):
// After updating yaw or pitch: yaw = std::fmod(yaw, 2.0f * M_PI); pitch = std::fmod(pitch, 2.0f * M_PI);Or clamp pitch to prevent gimbal lock:
pitch = std::clamp(pitch, -M_PI/2.0f + 0.01f, M_PI/2.0f - 0.01f);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
main.cpp(5 hunks)sdl_wrapper/sdl_wrapper.gpu.ixx(2 hunks)sdl_wrapper/sdl_wrapper.pipeline.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-13T11:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
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.cppsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.pipeline.cpp
📚 Learning: 2025-11-14T00:45:09.377Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
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.cppsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.pipeline.cpp
📚 Learning: 2025-11-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
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.cppsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.pipeline.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: build (Release, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Debug, clang)
- GitHub Check: clang-tidy-review
- GitHub Check: build (windows-latest, Debug, cl)
🔇 Additional comments (4)
sdl_wrapper/sdl_wrapper.gpu.ixx (1)
51-63: LGTM! Clean API extension.The addition of the
p_num_uniform_buffersparameter cleanly extends the shader creation API to support uniform buffers. The parameter naming is consistent with the codebase conventions, and it's correctly propagated to the SDL GPU shader creation info.main.cpp (3)
23-26: LGTM! Simple and appropriate data structure.The
CameraUniformstruct correctly holds a 4×4 matrix as a 16-element float array, matching the shader'smat4 uViewuniform declaration.
34-35: LGTM! Appropriate camera state initialization.The camera control state (yaw, pitch) and uniform buffer are properly initialized as member variables with sensible default values.
Also applies to: 44-44
53-60: LGTM! Shader uniform integration is correct.The Camera uniform block declaration and view matrix transformation are properly implemented. The matrix multiplication order (
uView * vec4(a_position, 1.0f)) follows the correct column-major convention for Vulkan/OpenGL.
* 📝 Add docstrings to `camera` Docstrings generation was requested by @WSQS. * #30 (comment) The following files were modified: * `main.cpp` * style: format code with ClangFormat This commit fixes the style issues introduced in 10bd2e2 according to the output from ClangFormat. Details: #31 --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'a' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; // vec4 color
^| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'b' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; // vec4 color
^| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'g' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; // vec4 color
^| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'r' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; // vec4 color
^| auto position() { return &x; } | ||
| }; | ||
|
|
||
| struct CameraUniform |
There was a problem hiding this comment.
warning: destructor of 'UserApp' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
class UserApp : public sopho::App
^Additional context
main.cpp:27: make it public and virtual
class UserApp : public sopho::App
^|
|
||
| float cy = std::cos(yaw); | ||
| float sy = std::sin(yaw); | ||
| float cp = std::cos(pitch); |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float cp = std::cos(pitch); | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0F, 0.0f, 0.0f, 0.0f, 1.0f}; |
|
|
||
| float cy = std::cos(yaw); | ||
| float sy = std::sin(yaw); | ||
| float cp = std::cos(pitch); |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float cp = std::cos(pitch); | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0F, 0.0f, 0.0f, 1.0f}; |
|
|
||
| float cy = std::cos(yaw); | ||
| float sy = std::sin(yaw); | ||
| float cp = std::cos(pitch); |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float cp = std::cos(pitch); | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0F, 0.0f, 1.0f}; |
|
|
||
| float cy = std::cos(yaw); | ||
| float sy = std::sin(yaw); | ||
| float cp = std::cos(pitch); |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float cp = std::cos(pitch); | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0F, 1.0f}; |
|
|
||
| float cy = std::cos(yaw); | ||
| float sy = std::sin(yaw); | ||
| float cp = std::cos(pitch); |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float cp = std::cos(pitch); | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0F}; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
main.cpp (2)
94-101: Consider a more maintainable identity matrix initialization.The manual element-by-element initialization is correct but verbose and error-prone.
Apply this diff to use a clearer approach:
- { - // identity - cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F; - cam.m[1] = cam.m[2] = cam.m[3] = 0.0F; - cam.m[4] = cam.m[6] = cam.m[7] = 0.0F; - cam.m[8] = cam.m[9] = cam.m[11] = 0.0F; - cam.m[12] = cam.m[13] = cam.m[14] = 0.0F; - } + // Initialize camera to identity matrix + std::fill(cam.m.begin(), cam.m.end(), 0.0F); + cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F;
262-288: Optimize view matrix computation.The
mulMat4lambda is defined every frame, adding unnecessary overhead. Additionally, the view matrix is recomputed every frame even whenyawandpitchhaven't changed.Consider these improvements:
- Move the lambda to class scope or use a static helper function:
Add to the class:
static void mulMat4(const float* A, const float* B, float* C) { for (int col = 0; col < 4; ++col) { for (int row = 0; row < 4; ++row) { C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] + A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3]; } } }
- Track if camera changed and only recompute when needed:
Add a member:
bool camera_dirty = true;In
event(): Setcamera_dirty = true;after modifyingyaworpitch.In
draw(): Only recompute ifcamera_dirty, then set it tofalse.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
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:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
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-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
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.377Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
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: build (Debug, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: clang-tidy-review
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
🔇 Additional comments (5)
main.cpp (5)
2-2: LGTM!The
<cmath>include is necessary for the trigonometric functions andstd::clampused in the camera rotation logic.
23-26: LGTM!The
CameraUniformstruct correctly wraps a 4×4 matrix. The magic number warning for16is already flagged by static analysis.
34-35: LGTM!Camera state members are properly initialized. Style issues (lowercase
fsuffixes) are already flagged by static analysis.Also applies to: 44-44
337-358: LGTM!The arrow key handling correctly updates camera orientation with proper pitch clamping to prevent gimbal lock. The increment values provide reasonable rotation speed.
53-60: Uniform slot mapping is correct—no action needed.The shader correctly uses
set = 1, binding = 0for vertex uniform buffers, which aligns with SDL GPU's fixed resource set architecture where vertex uniforms are assigned to set 1. TheSDL_PushGPUVertexUniformDataslot parameter (0) refers to which of the 4 available uniform slots to write to, separate from the shader's set/binding designations. The mapping is valid.
| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'a' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; /**
^| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'b' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; /**
^| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'g' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; /**
^| { | ||
| float x, y, z; // vec3 position | ||
| float r, g, b, a; // vec4 color | ||
| float r, g, b, a; /** |
There was a problem hiding this comment.
warning: member variable 'r' has public visibility [misc-non-private-member-variables-in-classes]
float r, g, b, a; /**
^
main.cpp
Outdated
| float cp = std::cos(pitch); | ||
| float sp = std::sin(pitch); | ||
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0F}; |
main.cpp
Outdated
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | ||
|
|
||
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0F}; |
main.cpp
Outdated
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | ||
|
|
||
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: variable 'Rx' of type 'float[16]' can be declared 'const' [misc-const-correctness]
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | |
| float const Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
main.cpp
Outdated
|
|
||
| float Ry[16] = {cy, 0.0f, sy, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, -sy, 0.0f, cy, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | ||
|
|
||
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; |
There was a problem hiding this comment.
warning: variable name 'Rx' is too short, expected at least 3 characters [readability-identifier-length]
float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f};
^|
|
||
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | ||
|
|
||
| auto mulMat4 = [](const float* A, const float* B, float* C) |
There was a problem hiding this comment.
warning: parameter name 'A' is too short, expected at least 3 characters [readability-identifier-length]
auto mulMat4 = [](const float* A, const float* B, float* C)
^|
|
||
| float Rx[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, cp, sp, 0.0f, 0.0f, -sp, cp, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f}; | ||
|
|
||
| auto mulMat4 = [](const float* A, const float* B, float* C) |
There was a problem hiding this comment.
warning: parameter name 'B' is too short, expected at least 3 characters [readability-identifier-length]
auto mulMat4 = [](const float* A, const float* B, float* C)
^
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
main.cpp (2)
265-291: Verify rotation matrix correctness and consider extraction.The rotation matrices and multiplication logic should be verified to ensure correct camera behavior:
- Confirm that the matrix element ordering and signs produce the intended rotation (yaw around Y, pitch around X).
- Verify that the multiplication order
Rx * Ryachieves the desired pitch-then-yaw transformation.- Consider whether a view matrix should also include translation (camera position), or if rotation-only is intentional.
Consider extracting the matrix multiplication lambda and rotation matrix construction into named helper functions to improve readability and reusability:
+ auto createRotationY = [](float angle) -> std::array<float, 16> { + const float c = std::cos(angle); + const float s = std::sin(angle); + return {c, 0.0F, -s, 0.0F, + 0.0F, 1.0F, 0.0F, 0.0F, + s, 0.0F, c, 0.0F, + 0.0F, 0.0F, 0.0F, 1.0F}; + }; + + auto createRotationX = [](float angle) -> std::array<float, 16> { + const float c = std::cos(angle); + const float s = std::sin(angle); + return {1.0F, 0.0F, 0.0F, 0.0F, + 0.0F, c, s, 0.0F, + 0.0F, -s, c, 0.0F, + 0.0F, 0.0F, 0.0F, 1.0F}; + }; + + auto multiplyMat4 = [](const std::array<float, 16>& A, const std::array<float, 16>& B) -> std::array<float, 16> { + std::array<float, 16> C{}; + for (int col = 0; col < 4; ++col) { + for (int row = 0; row < 4; ++row) { + C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + + A[1 * 4 + row] * B[col * 4 + 1] + + A[2 * 4 + row] * B[col * 4 + 2] + + A[3 * 4 + row] * B[col * 4 + 3]; + } + } + return C; + };
345-356: Consider using named constants for rotation increments.The magic number
0.1fis used for rotation speed. While functional, consider extracting this to a named constant for clarity and easier tuning.+ static constexpr float ROTATION_SPEED = 0.1F; + if (event->type == SDL_EVENT_KEY_DOWN) { switch (event->key.key) { case SDLK_UP: - pitch += 0.1f; + pitch += ROTATION_SPEED; pitch = std::clamp<float>(pitch, -M_PI_2, M_PI_2); break; case SDLK_DOWN: - pitch -= 0.1f; + pitch -= ROTATION_SPEED; pitch = std::clamp<float>(pitch, -M_PI_2, M_PI_2); break; case SDLK_LEFT: - yaw += 0.1f; + yaw += ROTATION_SPEED; break; case SDLK_RIGHT: - yaw -= 0.1f; + yaw -= ROTATION_SPEED; break;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
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:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
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-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
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.377Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
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). (6)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: clang-tidy-review
- GitHub Check: build (Debug, gcc)
🔇 Additional comments (2)
main.cpp (2)
98-105: Camera initialization is correct.The explicit identity matrix initialization is clear and correct.
58-65: Assign num_uniform_buffers to shader create info—currently the parameter is passed but never used.In
sdl_wrapper/sdl_wrapper.gpu.ixxlines 51-63, thecreate_shaderfunction receivesp_num_uniform_buffersbut never assigns it to theSDL_GPUShaderCreateInfostructure. This causes SDL to incorrectly configure the shader descriptor layout, preventing the uniform buffer at set 1 from being bound.auto create_shader(const std::vector<uint8_t>& p_shader, SDL_GPUShaderStage p_stage, uint32_t p_num_uniform_buffers) { SDL_GPUShaderCreateInfo vertexInfo{}; vertexInfo.code = p_shader.data(); vertexInfo.code_size = p_shader.size(); vertexInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; vertexInfo.stage = p_stage; vertexInfo.num_uniform_buffers = p_num_uniform_buffers; // ← Missing assignment return SDL_CreateGPUShader(m_device, &vertexInfo); }Without this assignment, the shader will fail at runtime when
SDL_PushGPUVertexUniformDataattempts to push data to a descriptor set that SDL was never configured to expect.⛔ Skipped due to learnings
Learnt from: WSQS Repo: WSQS/sdl_test PR: 7 File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0 Timestamp: 2025-11-13T11:34:32.714Z 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:37-43 Timestamp: 2025-11-13T11:35:48.273Z 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.377Z 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.
| { | ||
| for (int col = 0; col < 4; ++col) | ||
| { | ||
| for (int row = 0; row < 4; ++row) |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
| for (int row = 0; row < 4; ++row) | |
| C[col * 4 + row] = A[(0 * 4) + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] + |
| { | ||
| for (int col = 0; col < 4; ++col) | ||
| { | ||
| for (int row = 0; row < 4; ++row) |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
| for (int row = 0; row < 4; ++row) | |
| C[col * 4 + row] = A[0 * 4 + row] * B[(col * 4) + 0] + A[1 * 4 + row] * B[col * 4 + 1] + |
| { | ||
| for (int col = 0; col < 4; ++col) | ||
| { | ||
| for (int row = 0; row < 4; ++row) |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
| for (int row = 0; row < 4; ++row) | |
| C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[(1 * 4) + row] * B[col * 4 + 1] + |
| { | ||
| for (int col = 0; col < 4; ++col) | ||
| { | ||
| for (int row = 0; row < 4; ++row) |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
| for (int row = 0; row < 4; ++row) | |
| C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[(col * 4) + 1] + |
| { | ||
| for (int col = 0; col < 4; ++col) | ||
| { | ||
| for (int row = 0; row < 4; ++row) |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] +
^
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
main.cpp (2)
100-107: Consider a more concise identity matrix initialization.The manual element-by-element initialization is correct but verbose and harder to verify. A loop-based approach would be clearer and less error-prone.
- { - // identity - cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F; - cam.m[1] = cam.m[2] = cam.m[3] = 0.0F; - cam.m[4] = cam.m[6] = cam.m[7] = 0.0F; - cam.m[8] = cam.m[9] = cam.m[11] = 0.0F; - cam.m[12] = cam.m[13] = cam.m[14] = 0.0F; - } + // Initialize camera to identity matrix + std::fill(cam.m.begin(), cam.m.end(), 0.0F); + cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F;
267-293: Consider extracting view matrix computation to a helper method.The per-frame view matrix computation (rotation from yaw/pitch) is self-contained logic that could be extracted to improve readability and testability.
Example refactor:
// Add as a private method in UserApp void update_view_matrix() { const float cy = std::cos(yaw); const float sy = std::sin(yaw); const float cp = std::cos(pitch); const float sp = std::sin(pitch); const std::array Ry = {cy, 0.0F, sy, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, -sy, 0.0F, cy, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F}; const std::array Rx = {1.0F, 0.0F, 0.0F, 0.0F, 0.0F, cp, sp, 0.0F, 0.0F, -sp, cp, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F}; auto mulMat4 = [](const float* A, const float* B, float* C) { for (int col = 0; col < 4; ++col) { for (int row = 0; row < 4; ++row) { C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] + A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3]; } } }; mulMat4(Rx.data(), Ry.data(), cam.m.data()); }Then in
draw():update_view_matrix(); SDL_PushGPUVertexUniformData(commandBuffer, 0, cam.m.data(), sizeof(cam.m));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
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:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
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-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
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.377Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
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
🔇 Additional comments (3)
main.cpp (3)
30-33: LGTM - CameraUniform structure is well-designed.The structure appropriately encapsulates the 4×4 view matrix as a contiguous array, suitable for uploading to GPU uniforms.
342-363: LGTM - Camera input handling is correct.The arrow key controls appropriately modify yaw and pitch, with pitch correctly clamped to prevent gimbal lock. The fixed increment is reasonable for interactive camera control.
267-293: ****The code is correct. In vertex shaders, the set 1 is used for uniforms, and the shader declaration
layout(std140, set = 1, binding = 0) uniform Cameraaligns with SDL3's predefined descriptor set for vertex uniforms. TheSDL_PushGPUVertexUniformData(commandBuffer, 0, ...)call correctly targets the first uniform slot within that set; you could write your vertex shaders to read a camera matrix from uniform binding slot 0, push the camera matrix at the start of the command buffer, and that data will be used for every subsequent draw call. The uniform slot indexing maps properly to the shader binding through SDL3's API abstraction.
| #include <algorithm> | ||
| #include <array> | ||
| #include <cmath> | ||
| #include <iostream> |
There was a problem hiding this comment.
warning: included header iostream is not used directly [misc-include-cleaner]
| #include <iostream> | |
| #include <algorithm> |
| #include <SDL3/SDL_gpu.h> | ||
| #include "SDL3/SDL_log.h" | ||
| #include "SDL3/SDL.h" | ||
| #include "SDL3/SDL_gpu.h" |
There was a problem hiding this comment.
warning: included header SDL.h is not used directly [misc-include-cleaner]
| #include "SDL3/SDL_gpu.h" | |
| #include "SDL3/SDL_gpu.h" |
| float cp = std::cos(pitch); | ||
| float sp = std::sin(pitch); | ||
|
|
||
| std::array Ry = {cy, 0.0F, sy, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, -sy, 0.0F, cy, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F}; |
There was a problem hiding this comment.
warning: variable name 'Ry' is too short, expected at least 3 characters [readability-identifier-length]
std::array Ry = {cy, 0.0F, sy, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, -sy, 0.0F, cy, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F};
^|
|
||
| std::array Ry = {cy, 0.0F, sy, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, -sy, 0.0F, cy, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F}; | ||
|
|
||
| std::array Rx = {1.0F, 0.0F, 0.0F, 0.0F, 0.0F, cp, sp, 0.0F, 0.0F, -sp, cp, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F}; |
There was a problem hiding this comment.
warning: variable name 'Rx' is too short, expected at least 3 characters [readability-identifier-length]
std::array Rx = {1.0F, 0.0F, 0.0F, 0.0F, 0.0F, cp, sp, 0.0F, 0.0F, -sp, cp, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F};
^|
|
||
| std::array Rx = {1.0F, 0.0F, 0.0F, 0.0F, 0.0F, cp, sp, 0.0F, 0.0F, -sp, cp, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F}; | ||
|
|
||
| auto mulMat4 = [](const float* A, const float* B, float* C) |
There was a problem hiding this comment.
warning: parameter name 'C' is too short, expected at least 3 characters [readability-identifier-length]
auto mulMat4 = [](const float* A, const float* B, float* C)
^| for (int row = 0; row < 4; ++row) | ||
| { | ||
| C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] + | ||
| A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3]; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3];
^| for (int row = 0; row < 4; ++row) | ||
| { | ||
| C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] + | ||
| A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3]; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3];
^| for (int row = 0; row < 4; ++row) | ||
| { | ||
| C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] + | ||
| A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3]; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3];
^| virtual SDL_AppResult event(SDL_Event* event) override | ||
| { | ||
| ImGui_ImplSDL3_ProcessEvent(event); | ||
| if (event->type == SDL_EVENT_KEY_DOWN) |
There was a problem hiding this comment.
warning: no header providing "SDL_EVENT_KEY_DOWN" is directly included [misc-include-cleaner]
main.cpp:5:
- #include "shaderc/shaderc.hpp"
+ #include "SDL3/SDL_events.h"
+ #include "shaderc/shaderc.hpp"| { | ||
| switch (event->key.key) | ||
| { | ||
| case SDLK_UP: |
There was a problem hiding this comment.
warning: no header providing "SDLK_UP" is directly included [misc-include-cleaner]
main.cpp:5:
- #include "shaderc/shaderc.hpp"
+ #include "SDL3/SDL_keycode.h"
+ #include "shaderc/shaderc.hpp"
| switch (event->key.key) | ||
| { | ||
| case SDLK_UP: | ||
| pitch += 0.1F; |
There was a problem hiding this comment.
warning: 0.1F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
pitch += 0.1F;
^| pitch += 0.1F; | ||
| pitch = std::clamp<float>(pitch, -std::numbers::pi / 2, std::numbers::pi / 2); | ||
| break; | ||
| case SDLK_DOWN: |
There was a problem hiding this comment.
warning: no header providing "SDLK_DOWN" is directly included [misc-include-cleaner]
case SDLK_DOWN:
^| pitch = std::clamp<float>(pitch, -std::numbers::pi / 2, std::numbers::pi / 2); | ||
| break; | ||
| case SDLK_DOWN: | ||
| pitch -= 0.1F; |
There was a problem hiding this comment.
warning: 0.1F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
pitch -= 0.1F;
^| pitch -= 0.1F; | ||
| pitch = std::clamp<float>(pitch, -std::numbers::pi / 2, std::numbers::pi / 2); | ||
| break; | ||
| case SDLK_LEFT: |
There was a problem hiding this comment.
warning: no header providing "SDLK_LEFT" is directly included [misc-include-cleaner]
case SDLK_LEFT:
^| pitch = std::clamp<float>(pitch, -std::numbers::pi / 2, std::numbers::pi / 2); | ||
| break; | ||
| case SDLK_LEFT: | ||
| yaw += 0.1F; |
There was a problem hiding this comment.
warning: 0.1F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
yaw += 0.1F;
^| case SDLK_LEFT: | ||
| yaw += 0.1F; | ||
| break; | ||
| case SDLK_RIGHT: |
There was a problem hiding this comment.
warning: no header providing "SDLK_RIGHT" is directly included [misc-include-cleaner]
case SDLK_RIGHT:
^| yaw += 0.1F; | ||
| break; | ||
| case SDLK_RIGHT: | ||
| yaw -= 0.1F; |
There was a problem hiding this comment.
warning: 0.1F is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
yaw -= 0.1F;
^
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
main.cpp (4)
22-28: Vertex::position() helper and doc look consistent with ImGui usage.Returning
&xmatches the contiguous(x, y, z)layout expected byImGui::DragFloat3, and the new comment accurately documents that contract. If you ever need const access, aconstoverload (returningconst float*) would be a minor readability win.
83-87: Identity initialization ofcam.mis correct but could be simplified.The manual assignments produce a proper 4×4 identity matrix and are only done once in
init(), so performance isn’t a concern. If you want a slightly clearer expression, you could replace the block with a single aggregate:cam.m = {1.0F, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, 0.0F, 0.0F, 1.0F};but the current code is functionally fine.
Also applies to: 101-108
221-225: View matrix computation is mathematically consistent with GLSL expectations.
cy/sy/cp/spare used to buildRy(yaw about Y) andRx(pitch about X) as column‑major matrices that match GLSL’smat4layout.mulMat4computesC = A * Bin column‑major form, souView = Rx * Ryis applied in the vertex shader asuView * vec4(a_position, 1.0f)as expected.- Pushing
cam.mviaSDL_PushGPUVertexUniformData(commandBuffer, 0, cam.m.data(), sizeof(cam.m));correctly targets vertex uniform slot 0 using std140 layout.This is a solid minimal camera implementation. If you later want to clean it up, consider factoring
mulMat4into a small math helper (or a tiny mat4 wrapper) to avoid raw pointer arithmetic in the render path.Also applies to: 268-294
343-364: Arrow‑key camera controls work; tighten the numeric types for pitch clamp.The SDL key handling integrates cleanly with ImGui and updates
yaw/pitchas expected. Two small numeric nits:
- You’re calling
std::clamp<float>(pitch, -std::numbers::pi / 2, std::numbers::pi / 2);wherestd::numbers::piisdouble. This relies on implicit narrowing tofloat.- For stricter type hygiene and to avoid surprises under warnings like
-Wdouble-promotion, you could use the float specialisation:pitch = std::clamp( pitch, -std::numbers::pi_v<float> / 2.0F, std::numbers::pi_v<float> / 2.0F);Behavior stays the same, and everything remains C++20/23‑conformant.
Please confirm your C++ standard is at least C++20 so
std::numbers::pi_v<float>is available; otherwise, aconstexpr float kHalfPi = 1.57079632679F;constant would be a portable fallback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
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:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
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-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
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.377Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
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). (3)
- GitHub Check: build (Debug, clang)
- GitHub Check: clang-tidy-review
- GitHub Check: build (Debug, gcc)
🔇 Additional comments (3)
main.cpp (3)
1-5: Includes now cover new usage; consider adding<string>directly.The added
<algorithm>,<cmath>,<numbers>, and SDL3 headers line up with new uses ofstd::clamp, trig, and key/event symbols. The file still relies on transitive inclusion forstd::string, though; adding#include <string>here would make the TU self‑contained.
[ suggest_optional_refactor ]Also applies to: 13-15
31-34: CameraUniform + yaw/pitch state are straightforward and well‑scoped.The
CameraUniformwrapper andcaminstance give a clear place to hang the 4×4 view matrix, and keepingyaw/pitchas members makes the camera behavior frame‑to‑frame predictable. This is a clean way to introduce camera state without touching the vertex format or buffer.Also applies to: 42-43, 52-53
61-64: Uniform block layout matches SDL GPU expectations for vertex uniforms.
layout(std140, set = 1, binding = 0) uniform Camera { mat4 uView; };combined withgl_Position = uView * vec4(a_position, 1.0f);is aligned with SDL’s convention that vertex uniform buffers live in set 1 and are addressed by binding index, whileSDL_PushGPUVertexUniformData(..., 0, ...)targets “slot 0” (binding 0) for the vertex stage. (wiki.libsdl.org)As long as your vertex shader is created with
num_uniform_buffers = 1(which the PR description says it is), this will bind correctly.If you want to double‑check, ensure the generated SPIR‑V really has the uniform at set 1, binding 0 and that your
create_shadercall for the vertex stage passesp_num_uniform_buffers = 1.Also applies to: 68-68
Summary by CodeRabbit