Texture #41
Texture #41
Conversation
WalkthroughRefactors SDL wrapper into builder-based Buffer/Transfer/Texture and RenderDataImpl submodules, moves shader reflection types into data_type, adds fragment reflection and tests, integrates stb and image-asset handling, implements image loading and TextureWrapper usage, and updates create_shader/create_data APIs and corresponding call sites. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant GLR as glsl_reflector
participant GW as GpuWrapper
participant BB as BufferWrapper::Builder
participant TB as TransferBufferWrapper
participant TW as TextureWrapper::Builder
participant GPU as SDL GPU
Note over App,GLR: Fragment reflection -> sampler count
App->>GLR: reflect_fragment(fragment_source)
GLR-->>App: FragmentReflection{sampler_count}
Note over App,GW: Shader creation with sampler count
App->>GW: create_shader(fragment_code, FRAGMENT, 0, sampler_count)
GW-->>App: fragment shader handle
Note over App,BB: Buffer builders create transfer buffers
App->>BB: configure & build vertex/index buffers
BB->>TB: Builder -> TransferBufferWrapper (create)
TB->>GPU: SDL_GPUCreateTransferBuffer()
GPU-->>TB: transfer handle
BB-->>App: checkable<BufferWrapper>
Note over App,TW: Texture build path uploads pixels
App->>TW: set_image_data(img) -> build(gpu)
TW->>TB: create transfer buffer & submit(pixel data)
TB->>GPU: map/memcpy/unmap
TW->>GPU: acquire command buffer -> copy to texture -> create sampler
GPU-->>TW: texture+sampler handles
TW-->>App: TextureWrapper
sequenceDiagram
participant App as Application
participant GW as GpuWrapper
participant RD as RenderDataImpl
App->>GW: create_data(proc, vertex_count, index_count)
GW->>RD: construct shared RenderDataImpl using BufferWrapper::Builder results
RD-->>GW: shared_ptr<RenderData>
GW-->>App: checkable<std::shared_ptr<RenderData>>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
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)
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}
| #define STB_IMAGE_IMPLEMENTATION | ||
| #include "stb_image.h" | ||
|
|
||
| import data_type; |
There was a problem hiding this comment.
warning: module 'data_type' not found [clang-diagnostic-error]
import data_type;
^| struct CameraUniform | ||
| { | ||
| std::array<float, 16> m{}; | ||
| std::array<float, 16> m{}; ///< 4x4 transformation matrix as a flat array |
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{}; ///< 4x4 transformation matrix as a flat array
^| * @return sopho::ImageData Structure containing the loaded image dimensions, channels, and pixel data. | ||
| * Returns an empty structure if loading fails. | ||
| */ | ||
| sopho::ImageData load_image() |
There was a problem hiding this comment.
warning: function 'load_image' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| sopho::ImageData load_image() | |
| static sopho::ImageData load_image() |
| */ | ||
| sopho::ImageData load_image() | ||
| { | ||
| stbi_set_flip_vertically_on_load(true); |
There was a problem hiding this comment.
warning: implicit conversion 'bool' -> 'int' [readability-implicit-bool-conversion]
| stbi_set_flip_vertically_on_load(true); | |
| stbi_set_flip_vertically_on_load(1); |
|
|
||
| if (!data) | ||
| { | ||
| SDL_Log("stbi_load failed for %s: %s", file_name.data(), stbi_failure_reason()); |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_Log("stbi_load failed for %s: %s", file_name.data(), stbi_failure_reason());
^![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 (12)
CMakeLists.txt (2)
91-91: stb integration looks fine; consider pinning revision for reproducible buildsUsing an INTERFACE
stbtarget andSYSTEMincludes is clean. To keep builds reproducible and avoid surprises from upstream changes, consider pinningstbto a specific commit/tag instead ofGIT_TAG master.Also applies to: 97-107, 125-125
132-147: Unconditional network download in configure step may hurt offline/CI buildsThe
file(DOWNLOAD ...)in configure-time logic will fail when offline or behind strict proxies and also doesn’t verify content. Consider:
- Guarding this behind an option (e.g.,
SDL_TEST_DOWNLOAD_ASSETS).- Allowing user-supplied assets if download fails.
- Optionally adding
EXPECTED_HASHfor integrity.sdl_wrapper/modules/sdl_wrapper.render_data.ixx (1)
16-32: Make the module self-contained by including<cstddef>
VertexView/IndexViewusesize_tandstd::bytebut the file doesn’t include<cstddef>. Relying on transitive includes from SDL headers is brittle; add the explicit include:-#include <SDL3/SDL_gpu.h> -#include <vector> -#include <expected> -#include <variant> +#include <SDL3/SDL_gpu.h> +#include <vector> +#include <expected> +#include <variant> +#include <cstddef>This keeps the module interface self-contained.
sdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx (1)
31-35: Clarifyusage_limitsemantics and align docs with implementation
m_usage_limitis commented as “Current usage count” while the Builder docs say it’s a maximum number of uploads before recycling. Also, other code (e.g., BufferWrapper::Builder) passes-1, which likely means “no limit”.To avoid confusion/bugs in
submit():
- Decide on the contract (e.g.,
usage_limit < 0⇒ unlimited;>= 0⇒ remaining uses).- Update the member comment and Builder docs to state that clearly.
- Ensure
submit()only decrements/enforces the limit according to that contract.Optionally,
submit(void* data_source)could takeconst void*if it never mutates the source data, which would make it easier to call with const buffers.Also applies to: 83-96
sdl_wrapper/modules/sdl_wrapper.ixx (1)
5-13: Top-level module now re-exports:textureinstead of:bufferSwitching the umbrella module to export
:texturechanges what clients see when theyimport sdl_wrapper;. If any existing code relied onBufferWrappervia the umbrella import, it will now need to importsdl_wrapper:bufferexplicitly, or you may want to re‑export both if backwards compatibility is desired.sdl_wrapper/src/sdl_wrapper.buffer.cpp (1)
52-65: Align upload docs/logging withTransferBufferWrapper::submitbehavior
upload()now delegates mapping/copy into the transfer buffer tom_transfer_buffer.submit(src_data), but the comment and log message still specifically talk about “failed to map transfer buffer”.If
submit()can fail for reasons other than mapping (e.g., usage limit reached, size mismatch), consider:
- Updating the function comment to mention that upload can fail with whatever
GpuErrorvaluessubmit()may return.- Generalizing the log message (or letting
submit()handle logging itself) so the text doesn’t assume a mapping failure.Functionally this is fine; it’s mainly a documentation/observability alignment issue.
Also applies to: 88-88
glsl_reflector/modules/glsl_reflector.ixx (1)
93-137: Good implementation of fragment reflection.The
reflect_fragmentfunction correctly counts sampler uniforms by checkingglslang::EbtSampler. The structure mirrorsreflect_vertexappropriately.Consider extracting the common parse/link boilerplate shared with
reflect_vertexinto a helper function to reduce duplication.sdl_wrapper/src/sdl_wrapper.texture.cpp (1)
101-106:max_anisotropy = 1.fwithenable_anisotropy = truehas no effect.Anisotropic filtering with a max anisotropy of 1 is equivalent to no anisotropic filtering. Either increase
max_anisotropyto a meaningful value (e.g., 4.0f or 16.0f) or setenable_anisotropy = false.- info.max_anisotropy = 1.f; + info.max_anisotropy = 4.f; // or 16.f for higher qualitysdl_wrapper/src/sdl_wrapper.gpu.cpp (1)
32-49: Consider parameterizing the index buffer size.The index buffer is hardcoded to
6 * sizeof(int)(6 indices for a quad), whilevertex_countis a parameter. This coupling may cause confusion if the caller expects a different primitive topology.Consider adding an
index_countparameter or documenting that this method specifically creates quad render data:- checkable<std::shared_ptr<RenderData>> GpuWrapper::create_data(const RenderProcedural& render_procedural, - uint32_t vertex_count) + checkable<std::shared_ptr<RenderData>> GpuWrapper::create_data(const RenderProcedural& render_procedural, + uint32_t vertex_count, + uint32_t index_count = 6)main.cpp (1)
49-69:load_image()silently returns empty data on failure.While the function logs an error, returning an empty
ImageDatacan lead to downstream issues if callers don't check for validity. The texture builder will attempt to create a 0x0 texture.Consider returning
std::expected<ImageData, ...>orstd::optional<ImageData>to make failure explicit:-sopho::ImageData load_image() +std::optional<sopho::ImageData> load_image() { ... if (!data) { SDL_Log("stbi_load failed for %s: %s", file_name.data(), stbi_failure_reason()); + return std::nullopt; } ... }sdl_wrapper/modules/sdl_wrapper.texture.ixx (2)
32-37: Consider breaking the long initializer list for readability.The move constructor initializer list is quite long on a single line, which impacts readability.
- TextureWrapper(TextureWrapper&& other) noexcept : m_gpu(std::move(other.m_gpu)), m_texture(other.m_texture), m_sampler(other.m_sampler),m_tex_binding(other.m_tex_binding) + TextureWrapper(TextureWrapper&& other) noexcept : + m_gpu(std::move(other.m_gpu)), + m_texture(other.m_texture), + m_sampler(other.m_sampler), + m_tex_binding(other.m_tex_binding) { other.m_texture = nullptr; other.m_sampler = nullptr; other.m_tex_binding = {}; }
69-73: Usestd::moveto avoid unnecessary copy.Since
image_datais taken by value, moving it intoimg_dataavoids a redundant copy whenImageDatacontains dynamically allocated resources (like pixel buffers).Builder& set_image_data(ImageData image_data) { - img_data = image_data; + img_data = std::move(image_data); return *this; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
CMakeLists.txt(2 hunks)README.md(1 hunks)data_type/modules/data_type.ixx(2 hunks)glsl_reflector/CMakeLists.txt(1 hunks)glsl_reflector/modules/glsl_reflector.ixx(2 hunks)glsl_reflector/modules/glsl_reflector.type.ixx(0 hunks)glsl_reflector/test/test.cpp(1 hunks)main.cpp(8 hunks)sdl_wrapper/CMakeLists.txt(1 hunks)sdl_wrapper/modules/sdl_wrapper.buffer.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.decl.ixx(0 hunks)sdl_wrapper/modules/sdl_wrapper.gpu.ixx(2 hunks)sdl_wrapper/modules/sdl_wrapper.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.render_data.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.texture.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx(1 hunks)sdl_wrapper/sdl_wrapper.render_data.ixx(0 hunks)sdl_wrapper/src/sdl_wrapper.buffer.cpp(3 hunks)sdl_wrapper/src/sdl_wrapper.gpu.cpp(2 hunks)sdl_wrapper/src/sdl_wrapper.render_procedural.cpp(2 hunks)sdl_wrapper/src/sdl_wrapper.texture.cpp(1 hunks)sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- sdl_wrapper/modules/sdl_wrapper.decl.ixx
- glsl_reflector/modules/glsl_reflector.type.ixx
- sdl_wrapper/sdl_wrapper.render_data.ixx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.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.
📚 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.render_data_impl.ixxsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/modules/sdl_wrapper.texture.ixxsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/modules/sdl_wrapper.ixxsdl_wrapper/src/sdl_wrapper.gpu.cppsdl_wrapper/src/sdl_wrapper.buffer.cppsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixxmain.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/modules/sdl_wrapper.render_data_impl.ixxsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/modules/sdl_wrapper.texture.ixxsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/modules/sdl_wrapper.ixxsdl_wrapper/src/sdl_wrapper.gpu.cppsdl_wrapper/src/sdl_wrapper.buffer.cppsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixxmain.cppsdl_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.render_data_impl.ixxsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/modules/sdl_wrapper.texture.ixxsdl_wrapper/src/sdl_wrapper.render_procedural.cppsdl_wrapper/modules/sdl_wrapper.ixxsdl_wrapper/src/sdl_wrapper.gpu.cppsdl_wrapper/src/sdl_wrapper.buffer.cppsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixxmain.cppsdl_wrapper/modules/sdl_wrapper.gpu.ixx
📚 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.txtsdl_wrapper/modules/sdl_wrapper.buffer.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp
🧬 Code graph analysis (3)
sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (2)
sdl_wrapper/src/sdl_wrapper.texture.cpp (2)
build(30-119)build(30-30)sdl_wrapper/src/sdl_wrapper.buffer.cpp (2)
build(108-128)build(108-108)
sdl_wrapper/src/sdl_wrapper.texture.cpp (2)
sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (4)
reset(30-37)reset(30-30)build(63-77)build(63-63)sdl_wrapper/src/sdl_wrapper.buffer.cpp (2)
build(108-128)build(108-108)
sdl_wrapper/src/sdl_wrapper.buffer.cpp (2)
sdl_wrapper/src/sdl_wrapper.texture.cpp (2)
build(30-119)build(30-30)sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (2)
build(63-77)build(63-63)
⏰ 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: clang-tidy-review
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: CodeQL C++ Analysis
🔇 Additional comments (27)
README.md (2)
1-22: Clean progress tracking documentation.The README clearly documents the project's learning objectives and current progress. The "Textures" milestone is appropriately marked as complete, reflecting the work introduced in this PR. The structure is readable and maintainable for future updates.
3-3: External link verified—no changes needed.The referenced resource learnopengl.com is a well-established, free online learning platform by Joey de Vries covering modern OpenGL tutorials with clear examples and supporting materials. The link in your README is valid and appropriate for this learning project.
glsl_reflector/CMakeLists.txt (1)
13-13: Linkingdata_typehere is appropriateAdding
data_typetotarget_link_libraries(glsl_reflector ...)matches the new module dependency and keeps the build graph consistent. No issues from this change.glsl_reflector/test/test.cpp (1)
41-76: New fragment reflection tests are clear and targetedBoth tests directly exercise
reflect_fragmentfor 1 and 2sampler2Duniforms and assertsampler_count, which is exactly what this API is supposed to expose. Looks good.sdl_wrapper/CMakeLists.txt (1)
10-21: Module/source reorganization looks coherentExposing the public module interfaces under
modules/and moving implementations tosrc/aligns well with the new render/texture/transfer abstractions. The FILE_SET and PRIVATE source lists are consistent and should work fine with the existing linkage.Also applies to: 23-28
glsl_reflector/modules/glsl_reflector.ixx (1)
85-89: Empty loop body appears to be incomplete.The loop iterates over uniform variables but discards each
varwithout any processing. If this is placeholder code for future uniform reflection, consider adding a TODO comment or removing the loop until it's needed.sdl_wrapper/src/sdl_wrapper.render_procedural.cpp (2)
222-222: LGTM!The vertex shader correctly specifies 1 uniform buffer and 0 samplers, which is the typical configuration for vertex shaders.
273-274: Good integration of fragment reflection.Using
reflect_fragmentto dynamically determine the sampler count before shader creation is a clean approach that avoids hardcoding resource counts.sdl_wrapper/src/sdl_wrapper.texture.cpp (1)
16-28: LGTM!The
reset()implementation correctly checks for both the resource and the GPU wrapper before releasing, and properly nullifies the pointers afterward.sdl_wrapper/modules/sdl_wrapper.gpu.ixx (2)
246-256: LGTM!The
create_shadermethod correctly acceptsnum_samplersas a parameter and propagates it to the shader creation info. This enables dynamic sampler count configuration based on shader reflection.
213-214: All callers correctly handle the newcheckable<std::shared_ptr<RenderData>>return type.The
checkabletype is defined asstd::expected<T, GpuError>indata_type/modules/data_type.ixx:37. Both call sites inmain.cppproperly manage the new return type:
- Line 163: Validates the result using
if (!render_data)before accessing- Line 347: Extracts the value using
.value()after a successful operation withstd::move()Both patterns are correct for
std::expectedand indicate the callers have been updated to handle the error-aware return type.sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (4)
17-29: Move assignment operator correctly handles self-assignment and resource transfer.The implementation properly checks for self-assignment, resets current resources before taking ownership, and nullifies the source's transfer buffer. Good practice.
30-37: LGTM - reset() safely releases resources.The method correctly checks all prerequisites (
m_transfer_buffer,m_gpu, andm_gpu->device()) before releasing the GPU transfer buffer.
63-77: Builder::build() implementation looks correct.Error handling properly returns
std::unexpectedon failure, and the wrapper is constructed with all necessary parameters on success.
39-62: Potential use-after-reset issue:submit()returns success afterreset()invalidates the wrapper.When
m_usage_limitreaches zero,reset()is called which nullifiesm_transfer_buffer. Subsequent calls tosubmit()will pass the assert (ifm_usage_limitis still 0) but then dereferencem_gpu->device()and use the nullm_transfer_buffer, causing undefined behavior.Consider either:
- Making the assert check
m_usage_limit > 0(or!= 0with additional null check)- Adding a guard for null
m_transfer_bufferat the startcheckable<std::monostate> TransferBufferWrapper::submit(void* data_source) { - assert(m_usage_limit != 0); + assert(m_usage_limit != 0 && "submit called on exhausted transfer buffer"); + if (!m_transfer_buffer || !m_gpu || !m_gpu->device()) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, "%s:%d transfer buffer not valid", __FILE__, __LINE__); + return std::unexpected(GpuError::UNINITIALIZED); + } void* dst = SDL_MapGPUTransferBuffer(m_gpu->device(), m_transfer_buffer, false);⛔ Skipped due to 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.sdl_wrapper/src/sdl_wrapper.gpu.cpp (1)
36-48: Builder pattern for buffer creation is clean and readable.The fluent builder interface for creating vertex and index buffers improves code clarity. Error propagation is handled correctly.
main.cpp (3)
496-503: Draw continues without texture binding, which may cause GPU validation errors or undefined rendering.When
m_texture_wrapperis null, the shader still expects a bound sampler (uTexture). This could cause driver validation errors or undefined rendering output.Consider either:
- Binding a fallback 1x1 white texture
- Skipping the draw call entirely when no texture is available
- Using a shader variant without texture sampling
111-116: Fragment shader samples texture usingv_color.xyas UV coordinates.This assumes vertex colors contain UV coordinates in their xy components, which is unconventional. Typically a separate
a_texcoordattribute would be used.Verify this is intentional. If vertex colors are meant to be actual colors (RGBA), the texture sampling will produce unexpected results.
347-350: Unchecked access tonew_data.value()may cause undefined behavior.If
create_datafails, calling.value()on an emptystd::expectedis undefined behavior (or throws if exceptions are enabled, which they are not per learnings).auto new_data = m_gpu->create_data(*m_renderable->procedural(), 4); + if (!new_data) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to create new render data, error = %d", + static_cast<int>(new_data.error())); + break; + } m_renderable->data() = std::move(new_data.value()); m_renderable->data()->upload();⛔ Skipped due to 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.data_type/modules/data_type.ixx (2)
39-51: New data structures look well-designed.
ImageData,BasicType, and the reflection structures are cleanly defined with appropriate default initialization.
13-33: GpuError enum extensions are comprehensive.The new error codes cover texture, sampler, command buffer, copy pass, and shader compilation failures appropriately.
sdl_wrapper/modules/sdl_wrapper.buffer.ixx (3)
23-34: Good refactor: Transfer buffer now uses RAII wrapper.Changing from raw
SDL_GPUTransferBuffer*toTransferBufferWrapperimproves resource management by ensuring proper cleanup. The move semantics in the constructor are correctly applied.
37-55: Builder pattern provides clean, fluent API for buffer construction.The builder with chainable setters and a
build()method returningcheckable<BufferWrapper>aligns well with the pattern used forTransferBufferWrapper::Builder.
57-60: Move operations correctly defaulted.With
TransferBufferWrapperimplementing proper move semantics, defaulting the move constructor and assignment operator forBufferWrapperis appropriate.sdl_wrapper/modules/sdl_wrapper.texture.ixx (3)
17-27: LGTM!The private constructor correctly takes the shared_ptr by value and moves it, and the member initialization is appropriate for managing SDL GPU resources.
56-59: LGTM!The accessor is appropriately marked
[[nodiscard]],const, andnoexcept.
61-63: LGTM!The destructor correctly delegates to
reset()for resource cleanup, following RAII principles.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
sdl_wrapper/src/sdl_wrapper.texture.cpp (1)
62-67: GPU texture leaks on command buffer acquisition failure.When
SDL_AcquireGPUCommandBufferfails, the texture created at line 42 is not released before returning, causing a resource leak.Apply this diff to fix the leak:
SDL_GPUCommandBuffer* cmd = SDL_AcquireGPUCommandBuffer(gpu.device()); if (!cmd) { SDL_Log("SDL_AcquireGPUCommandBuffer failed: %s", SDL_GetError()); + SDL_ReleaseGPUTexture(gpu.device(), texture); return std::unexpected{GpuError::ACQUIRE_COMMAND_BUFFER_FAILED}; }
🧹 Nitpick comments (8)
sdl_wrapper/src/sdl_wrapper.buffer.cpp (1)
108-129: Resource cleanup on failure is correctly implemented.The builder now properly releases
gpu_bufferat line 125 if transfer buffer creation fails, addressing the previously flagged leak. The pattern aligns withBufferWrapper::Builder::buildbest practices.One minor observation:
set_usage_limit(-1)at line 120 implies "unlimited usage." Consider adding a brief comment or named constant (e.g.,UNLIMITED_USAGE = -1) to document this convention for maintainability.sdl_wrapper/modules/sdl_wrapper.render_data.ixx (1)
29-30: Consider returning const references for buffer bindings.The current signatures return mutable references:
virtual std::vector<SDL_GPUBufferBinding>& get_vertex_buffer_binding() = 0; virtual SDL_GPUBufferBinding& get_index_buffer_binding() = 0;If callers don't need to modify these bindings, consider providing
constoverloads to express intent and prevent unintended mutations.sdl_wrapper/src/sdl_wrapper.texture.cpp (1)
100-111: Consider extracting sampler creation info to a configurable builder field.The sampler configuration is hardcoded with linear filtering and repeat addressing. If different textures need different sampling modes (e.g., clamp-to-edge for UI, point filtering for pixel art), consider making these configurable via the Builder.
sdl_wrapper/src/sdl_wrapper.gpu.cpp (1)
30-31: Incomplete Doxygen documentation.The
@param index_counttag is present but lacks a description. Add a brief explanation of the parameter's purpose.- * @param index_count + * @param index_count Number of indices the allocated index buffer must hold.main.cpp (2)
22-24: Consider moving STB implementation to a dedicated source file.Defining
STB_IMAGE_IMPLEMENTATIONin the main application file works but can cause issues if the header is included elsewhere. For better maintainability, consider placing this in a dedicatedstb_image.cppfile.
49-69: Caller cannot distinguish "load failed" from "empty image file".
load_image()returns a defaultImageDataon failure, but the caller has no explicit way to detect this other than checkingpixels.empty(). Consider returningstd::expected<ImageData, ...>orstd::optional<ImageData>to make failures explicit.Also, static analysis flagged this function should be
staticor in an anonymous namespace.-sopho::ImageData load_image() +static std::optional<sopho::ImageData> load_image() { stbi_set_flip_vertically_on_load(true); std::string file_name{"assets/test_texture.png"}; sopho::ImageData result; auto data = stbi_load(file_name.data(), &result.width, &result.height, &result.channels, 4); result.channels = 4; if (!data) { SDL_Log("stbi_load failed for %s: %s", file_name.data(), stbi_failure_reason()); + return std::nullopt; } - else - { + result.pixels.assign(reinterpret_cast<std::byte*>(data), reinterpret_cast<std::byte*>(data) + result.width * result.height * result.channels); stbi_image_free(data); SDL_Log("stbi_load succeeded, w: %d h:%d ch:%d", result.width, result.height, result.channels); - } return result; }sdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx (2)
83-95: Consider validating Builder parameters before build.The default
size{1024}is reasonable, butusage_limit{1}means the buffer is released after a single use. Ensure this aligns with typical usage patterns. Also, consider adding validation inbuild()to rejectsize == 0orusage_limit <= 0.Could you verify the intended semantics of
usage_limit? If it's meant to count down to zero, thenusage_limit <= 0at build time would be immediately invalid.
75-75: Clarifysubmit()behavior in documentation.The
submitmethod signature is declared but lacks documentation. Given this is a public API, consider adding a Doxygen comment explaining:
- What
data_sourcepoints to and expected size- When the buffer is automatically released (usage limit)
- Return value semantics
+ /* + * @brief Copies data from the source into the transfer buffer and submits it. + * @param data_source Pointer to the data to copy (must be at least size() bytes) + * @return checkable with std::monostate on success, or an error code on failure + * @note When usage_limit reaches zero after submit, the buffer is automatically recycled. + */ checkable<std::monostate> submit(const void* data_source);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
data_type/modules/data_type.ixx(2 hunks)main.cpp(8 hunks)sdl_wrapper/modules/sdl_wrapper.gpu.ixx(2 hunks)sdl_wrapper/modules/sdl_wrapper.render_data.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.texture.ixx(1 hunks)sdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx(1 hunks)sdl_wrapper/src/sdl_wrapper.buffer.cpp(3 hunks)sdl_wrapper/src/sdl_wrapper.gpu.cpp(2 hunks)sdl_wrapper/src/sdl_wrapper.texture.cpp(1 hunks)sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- sdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx
- data_type/modules/data_type.ixx
- sdl_wrapper/modules/sdl_wrapper.texture.ixx
- sdl_wrapper/modules/sdl_wrapper.gpu.ixx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.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.
📚 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.buffer.cppsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/src/sdl_wrapper.gpu.cppmain.cppsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/modules/sdl_wrapper.transfer_buffer.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.buffer.cppsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/src/sdl_wrapper.gpu.cppmain.cppsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/modules/sdl_wrapper.transfer_buffer.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/src/sdl_wrapper.buffer.cppsdl_wrapper/src/sdl_wrapper.texture.cppsdl_wrapper/src/sdl_wrapper.gpu.cppmain.cppsdl_wrapper/modules/sdl_wrapper.render_data.ixxsdl_wrapper/src/sdl_wrapper.transfer_buffer.cppsdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx
📚 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.cppsdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp
🧬 Code graph analysis (2)
sdl_wrapper/src/sdl_wrapper.texture.cpp (2)
sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (4)
reset(30-37)reset(30-30)build(63-77)build(63-63)sdl_wrapper/src/sdl_wrapper.buffer.cpp (2)
build(108-129)build(108-108)
sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (2)
sdl_wrapper/src/sdl_wrapper.texture.cpp (4)
reset(16-28)reset(16-16)build(30-122)build(30-30)sdl_wrapper/src/sdl_wrapper.buffer.cpp (2)
build(108-129)build(108-108)
⏰ 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). (19)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: CodeQL C++ Analysis
- GitHub Check: clang-tidy-review
- GitHub Check: build (Debug, clang)
- GitHub Check: CodeQL C++ Analysis
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
🔇 Additional comments (14)
sdl_wrapper/src/sdl_wrapper.buffer.cpp (2)
25-38: LGTM!The destructor properly releases the GPU buffer when the wrapper is valid, with appropriate null checks.
52-65: Clean error propagation with the new submit-based approach.The refactor to use
m_transfer_buffer.submit(src_data)simplifies the upload flow and properly propagates the error from the transfer buffer operation.sdl_wrapper/modules/sdl_wrapper.render_data.ixx (1)
14-34: Clean abstract interface for render data management.The
RenderDatainterface provides a clear contract for vertex/index buffer management with appropriate virtual destructor. TheVertexViewandIndexViewstructs effectively encapsulate the view semantics.sdl_wrapper/src/sdl_wrapper.texture.cpp (1)
16-28: LGTM!The
reset()method correctly releases both the texture and sampler resources with proper null checks.sdl_wrapper/src/sdl_wrapper.transfer_buffer.cpp (4)
17-29: LGTM!Move assignment correctly resets the current instance before taking ownership, and properly nullifies the source's transfer buffer to prevent double-free.
30-37: LGTM!The
reset()method properly validates all preconditions before releasing the transfer buffer.
39-62: Document or guard against post-exhaustion usage.When
m_usage_limitreaches 0,reset()is called at line 58, which nullifiesm_transfer_buffer. Subsequent calls tosubmit()would then fail at theSDL_MapGPUTransferBuffercall (line 42), returning an error—which is acceptable behavior.However, the
assert(m_usage_limit != 0)at line 41 will fire in debug builds ifsubmit()is called after exhaustion. Consider whether this is the intended behavior or if a graceful error return is preferable for a more defensive approach.
63-77: LGTM!The builder correctly creates the transfer buffer with the configured parameters and returns a properly initialized wrapper.
sdl_wrapper/src/sdl_wrapper.gpu.cpp (2)
6-6: LGTM!New imports are properly added to support the builder-based buffer creation pattern and the
RenderDataImpltype.Also applies to: 12-12, 15-16
49-51: LGTM!The return correctly constructs a
RenderDataImplwrapped in ashared_ptr, aligning with the updated interface.main.cpp (1)
239-250: Error handling for texture creation looks good.The texture creation now properly handles the failure case with logging. This addresses the previous review concern.
sdl_wrapper/modules/sdl_wrapper.transfer_buffer.ixx (3)
28-41: Private constructor and builder pattern are well-structured.The RAII design with a private constructor and Builder pattern ensures proper resource creation. The
noexceptspecifier on the constructor is appropriate.
43-51: Move constructor correctly transfers ownership.The move constructor properly transfers the
m_transfer_bufferpointer and nullifies the source to prevent double-free.
31-34: Consider[[nodiscard]]on accessors for clarity.
raw()andsize()already have[[nodiscard]], which is good. The internal members are appropriately encapsulated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx (1)
40-41: Move operations may invalidate buffer bindings.After a move,
m_bindingsandm_index_bindingstill reference the old buffer pointers from the moved-from object. The defaulted move constructor/assignment will movem_vertex_bufferandm_index_buffer, but the bindings won't be updated to point to the new buffer locations.Apply this diff to implement custom move operations that reinitialize the bindings:
- RenderDataImpl(RenderDataImpl&&) = default; - RenderDataImpl& operator=(RenderDataImpl&&) = default; + RenderDataImpl(RenderDataImpl&& other) noexcept : + m_vertex_buffer(std::move(other.m_vertex_buffer)), + m_index_buffer(std::move(other.m_index_buffer)), + m_layouts(other.m_layouts), + m_vertex_count(other.m_vertex_count), + m_index_count(other.m_index_count) + { + m_bindings.emplace_back(m_vertex_buffer.gpu_buffer(), 0); + m_index_binding.buffer = m_index_buffer.gpu_buffer(); + m_index_binding.offset = 0; + } + RenderDataImpl& operator=(RenderDataImpl&& other) noexcept + { + if (this != &other) + { + m_vertex_buffer = std::move(other.m_vertex_buffer); + m_index_buffer = std::move(other.m_index_buffer); + m_layouts = other.m_layouts; + m_vertex_count = other.m_vertex_count; + m_index_count = other.m_index_count; + m_bindings.clear(); + m_bindings.emplace_back(m_vertex_buffer.gpu_buffer(), 0); + m_index_binding.buffer = m_index_buffer.gpu_buffer(); + m_index_binding.offset = 0; + } + return *this; + }
🧹 Nitpick comments (2)
sdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx (1)
29-32: Consider consistent index count type.The
index_countparameter is declared asstd::uint32_t(line 30), but stored inm_index_countwhich issize_t(line 24). For consistency, consider using the same type for both.sdl_wrapper/src/sdl_wrapper.gpu.cpp (1)
43-46: Use explicit index type for size calculation.The index buffer size now correctly uses
index_count, butsizeof(int)is platform-dependent. Consider usingsizeof(std::uint32_t)to match theindex_countparameter type inRenderDataImpl's constructor and ensure consistent index size across platforms.Apply this diff:
auto index_buffer = BufferWrapper::Builder{} .set_flag(SDL_GPU_BUFFERUSAGE_INDEX) - .set_size(index_count * sizeof(int)) + .set_size(index_count * sizeof(std::uint32_t)) .build(*this);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx(1 hunks)sdl_wrapper/src/sdl_wrapper.gpu.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.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.
📚 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.gpu.cppsdl_wrapper/modules/sdl_wrapper.render_data_impl.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.gpu.cppsdl_wrapper/modules/sdl_wrapper.render_data_impl.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/src/sdl_wrapper.gpu.cppsdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx
⏰ 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). (16)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: clang-tidy-review
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, clang)
- GitHub Check: CodeQL C++ Analysis
🔇 Additional comments (4)
sdl_wrapper/modules/sdl_wrapper.render_data_impl.ixx (2)
48-51: LGTM: Index count fix applied.The
index_view()method now correctly usesm_index_countinstead ofm_vertex_count. This resolves the previous review comment.
52-61: LGTM: Upload logic is correct.The sequential upload of vertex and index buffers with proper error propagation is correct.
sdl_wrapper/src/sdl_wrapper.gpu.cpp (2)
37-42: LGTM: Vertex buffer creation is correct.The builder-based vertex buffer creation with proper size calculation and error propagation is well-implemented.
51-52: LGTM: Return statement is correct.The construction and return of
RenderDataImplviastd::make_sharedwith proper move semantics for buffers is well-implemented.
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.