Coordinate systems #44
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughSwaps FetchContent Git fetches for ZIP URLs, redesigns the logos::Mat matrix API (row/column semantics, new ops, Rodrigues rotation, projection/translate/scale), adds depth-texture rendering and perspective in main app, and refactors RenderData creation to a Builder pattern with GPU buffer allocation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as UserApp
participant Builder as RenderData::Builder
participant Gpu as GpuWrapper
participant GPU as GPU/Driver
participant Pass as RenderPass
App->>Builder: configure(layout, vertex_count, index_count)
App->>Builder: build(gpu)
Builder->>Gpu: allocate vertex buffer(size)
Gpu->>GPU: create vertex buffer resource
GPU-->>Gpu: vertex buffer handle / error
Builder->>Gpu: allocate index buffer(size)
Gpu->>GPU: create index buffer resource
GPU-->>Gpu: index buffer handle / error
Gpu-->>Builder: buffers(ok) / error
Builder-->>App: checkable<RenderData> (ready) / error
App->>Pass: begin(depth-enabled)
App->>Gpu: bind RenderData (vertex/index), bind depth texture
App->>Pass: draw(call)
Pass->>GPU: issue draw with depth test
GPU-->>Pass: draw complete
Pass-->>App: end
App->>Pass: begin(ImGui pass)
Pass->>GPU: draw UI
Pass-->>App: end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (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}
| .type = SDL_GPU_TEXTURETYPE_2D, | ||
| .format = SDL_GPU_TEXTUREFORMAT_D16_UNORM, | ||
| .usage = SDL_GPU_TEXTUREUSAGE_DEPTH_STENCIL_TARGET, | ||
| .width = static_cast<std::uint32_t>(win_w), |
There was a problem hiding this comment.
warning: no header providing "std::uint32_t" is directly included [misc-include-cleaner]
main.cpp:6:
- #include <expected>
+ #include <cstdint>
+ #include <expected>| reinterpret_cast<float*>(raw_ptr), 0.01f, -1.f, 1.f); | ||
| default: | ||
| SDL_Log("Not implemented size"); | ||
| assert(false); |
There was a problem hiding this comment.
warning: no header providing "assert" is directly included [misc-include-cleaner]
main.cpp:5:
- #include <cmath>
+ #include <cassert>
+ #include <cmath>|
|
||
| if (win_w != width || win_h != height) | ||
| { | ||
| SDL_GPUTextureCreateInfo ci = { |
There was a problem hiding this comment.
warning: variable name 'ci' is too short, expected at least 3 characters [readability-identifier-length]
SDL_GPUTextureCreateInfo ci = {
^| }; | ||
| SDL_ReleaseGPUTexture(m_gpu->device(), SceneDepthTexture); | ||
| SceneDepthTexture = SDL_CreateGPUTexture(m_gpu->device(), &ci); | ||
| win_w = width; |
There was a problem hiding this comment.
warning: narrowing conversion from 'Uint32' (aka 'unsigned int') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
win_w = width;
^| break; | ||
| case SDLK_LEFT: | ||
| yaw += 0.1F; | ||
| 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;
^| break; | ||
| case SDLK_RIGHT: | ||
| yaw -= 0.1F; | ||
| 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;
^| // | ||
| module; | ||
| #include <expected> | ||
| #include <memory> |
There was a problem hiding this comment.
warning: included header expected is not used directly [misc-include-cleaner]
| #include <memory> | |
| #include <memory> |
| module; | ||
| #include <expected> | ||
| #include <memory> | ||
| #include "SDL3/SDL_gpu.h" |
There was a problem hiding this comment.
warning: included header memory is not used directly [misc-include-cleaner]
| #include "SDL3/SDL_gpu.h" | |
| #include "SDL3/SDL_gpu.h" |
| #include <expected> | ||
| #include <memory> | ||
| #include "SDL3/SDL_gpu.h" | ||
| module sdl_wrapper; |
There was a problem hiding this comment.
warning: module 'sdl_wrapper' not found [clang-diagnostic-error]
module sdl_wrapper;
^![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
CMakeLists.txt (1)
79-82: Consider pinning stb to a specific commit or tag for reproducible builds.Using
refs/heads/master.zipmeans builds may fetch different code over time, potentially introducing breaking changes or build failures. Unlike the other dependencies which use tagged releases, stb uses the unstable master branch.FetchContent_Declare( stb - URL https://github.com/nothings/stb/archive/refs/heads/master.zip + URL https://github.com/nothings/stb/archive/f0569113c93ad095470c54bf34a17b36646bbbb5.zip )Note: Replace the commit hash with the specific commit you want to pin to. You can find recent commits at https://github.com/nothings/stb/commits/master.
README.md (1)
48-56: Inconsistent list indentation.Static analysis flags 4-space indentation where 2 spaces is expected (MD007). If your project enforces markdownlint rules, consider adjusting:
- [ ] Getting started - - [x] OpenGL - - [x] Creating a window - - [x] Hello Window - - [x] Hello Triangle - - [x] Shaders - - [x] Textures - - [x] Transformations - - [ ] Coordinate Systems - - [ ] Camera + - [x] OpenGL + - [x] Creating a window + - [x] Hello Window + - [x] Hello Triangle + - [x] Shaders + - [x] Textures + - [x] Transformations + - [ ] Coordinate Systems + - [ ] Camerasdl_wrapper/src/sdl_wrapper.render_data.cpp (1)
30-71: Hardcoded vertex data reduces reusability.The vertex positions and UV coordinates are hardcoded directly in the builder. This couples the builder to specific geometry (appears to be a cube), limiting its usefulness as a general-purpose abstraction.
Consider accepting vertex data as a parameter to the builder or moving this test data elsewhere if it's temporary.
main.cpp (2)
71-71: Inconsistent member naming.
SceneDepthTexturedoesn't follow them_prefix convention used by other member variables (e.g.,m_gpu,m_renderable).- SDL_GPUTexture* SceneDepthTexture{}; + SDL_GPUTexture* m_scene_depth_texture{};
156-160: Duplicated magic numbers for vertex/index counts.
vertex_count(8)andindex_count(36)are hardcoded in multiple places. Consider extracting these as constants:static constexpr std::uint32_t CUBE_VERTEX_COUNT = 8; static constexpr std::uint32_t CUBE_INDEX_COUNT = 36;This improves maintainability when geometry changes.
Also applies to: 363-367
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CMakeLists.txt(5 hunks)README.md(1 hunks)logos/modules/logos.ixx(3 hunks)main.cpp(12 hunks)sdl_wrapper/CMakeLists.txt(1 hunks)sdl_wrapper/modules/sdl_wrapper.gpu.ixx(0 hunks)sdl_wrapper/modules/sdl_wrapper.render_data.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.vertex_layout.ixx(2 hunks)sdl_wrapper/src/sdl_wrapper.gpu.cpp(0 hunks)sdl_wrapper/src/sdl_wrapper.render_data.cpp(1 hunks)sdl_wrapper/src/sdl_wrapper.render_procedural.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- sdl_wrapper/modules/sdl_wrapper.gpu.ixx
- sdl_wrapper/src/sdl_wrapper.gpu.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 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:
sdl_wrapper/CMakeLists.txt
📚 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/CMakeLists.txtsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/src/sdl_wrapper.render_data.cppsdl_wrapper/modules/sdl_wrapper.vertex_layout.ixxsdl_wrapper/modules/sdl_wrapper.render_data.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/CMakeLists.txtsdl_wrapper/src/sdl_wrapper.render_data.cppsdl_wrapper/modules/sdl_wrapper.vertex_layout.ixxsdl_wrapper/modules/sdl_wrapper.render_data.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/CMakeLists.txtsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/src/sdl_wrapper.render_data.cppsdl_wrapper/modules/sdl_wrapper.vertex_layout.ixxsdl_wrapper/modules/sdl_wrapper.render_data.ixxmain.cpp
🪛 markdownlint-cli2 (0.18.1)
README.md
48-48: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
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)
⏰ 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). (12)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: CodeQL C++ Analysis
- GitHub Check: clang-tidy-review
🔇 Additional comments (11)
sdl_wrapper/CMakeLists.txt (1)
29-29: LGTM!The new source file is correctly added to the PRIVATE sources list, consistent with the existing build structure for the sdl_wrapper static library.
sdl_wrapper/src/sdl_wrapper.render_procedural.cpp (1)
103-112: LGTM!The depth/stencil configuration is well-structured:
- D16_UNORM is an appropriate 16-bit depth format.
SDL_GPU_COMPAREOP_LESScorrectly passes fragments closer to the camera.- Depth test and write enabled with stencil disabled is standard for basic depth buffering.
CMakeLists.txt (1)
91-91: LGTM!Disabling gtest installation is good practice to avoid polluting the install target with test framework binaries.
sdl_wrapper/modules/sdl_wrapper.vertex_layout.ixx (1)
93-108: LGTM!Good additions:
- FLOAT2 support expands the valid vertex formats.
- Error handling via
SDL_LogError+assertprovides debug-time checks while respecting the project's no-exceptions policy. TheINVALIDreturn value ensures graceful degradation in release builds.Based on learnings, this aligns with the project's error handling conventions.
logos/modules/logos.ixx (4)
14-32: LGTM on the Mat class template redesign.The swapped
<Col, Row>template parameters with column-major storage (m_data[col][row]) is a clean design that matches GPU shader conventions. The matrix multiplication dimensions and indexing are correct.
50-132: LGTM on the new operators and utilities.
operator+,operator*(TScalar),transpose(),resize(),length(), and the initializer list constructor are all correctly implemented.- The
requires(Col == 1)constraints appropriately restrict vector-specific operations.
160-188: LGTM on the Rodrigues rotation implementation.The formula
R = cos(θ)I + (1-cos(θ))kk^T + sin(θ)[k]×is correctly implemented:
k_k_tcomputes the outer product for thekk^Tterm.k_mcorrectly builds the skew-symmetric cross-product matrix.- Using
scale()to apply coefficients to the identity and zero blocks is elegant.
190-213: LGTM on the translation matrix.Translation values are correctly placed in column 3 (indices
(3, 0),(3, 1),(3, 2)), which is the standard position for homogeneous translation in column-major matrices.README.md (1)
5-43: LGTM! Clear module dependency diagram.The Mermaid diagram effectively documents the relationships between third-party libraries and internal modules, which aids onboarding and understanding the project architecture.
sdl_wrapper/modules/sdl_wrapper.render_data.ixx (1)
20-45: LGTM! Well-structured Builder pattern.The fluent API with value-initialized members and the
build(GpuWrapper&)method provides a clean interface for constructingRenderDatainstances. The design appropriately defers resource allocation to the build step.main.cpp (1)
493-504: LGTM! Proper depth-stencil and multi-pass rendering setup.The depth-stencil target configuration with clear values and the separation of the 3D scene pass from the ImGui pass is correctly implemented. Using
SDL_GPU_LOADOP_LOADfor the ImGui pass preserves the previously rendered content.Also applies to: 532-538
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.