Error check #32
Error check #32
Conversation
WalkthroughThis PR comprehensively refactors GPU resource management to employ std::expected-based error handling throughout. It introduces composite wrapper types for GPU contexts (DeviceHandle, WindowHandle, ClaimedWindow, GpuContext), updates BufferWrapper and PipelineWrapper public APIs to return expected-wrapped results with detailed error codes, expands UserApp initialization with staged GPU setup and Dear ImGui integration, and adds an interactive shader/vertex editor UI. Virtual keywords are removed from lifecycle methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as UserApp::init()
participant GpuCreate as GpuWrapper::create()
participant DevSetup as GPU Device/Window Setup
participant BuffCreate as GpuWrapper::create_buffer()
participant PipeCreate as GpuWrapper::create_pipeline_wrapper()
participant ShaderSet as PipelineWrapper::set_vertex_shader()
participant PipeSubmit as PipelineWrapper::submit()
participant Upload as BufferWrapper::upload()
participant ImGui as ImGui Init
Main ->> GpuCreate: create()
alt GPU Creation Success
GpuCreate ->> DevSetup: Initialize device/window
DevSetup -->> GpuCreate: GpuContext
GpuCreate -->> Main: Ok(GpuWrapper)
Main ->> BuffCreate: create_buffer()
alt Buffer Success
BuffCreate -->> Main: Ok(BufferWrapper)
Main ->> PipeCreate: create_pipeline_wrapper()
alt Pipeline Success
PipeCreate -->> Main: Ok(PipelineWrapper)
Main ->> ShaderSet: set_vertex_shader(src)
alt Shader Compile Success
ShaderSet ->> PipeSubmit: submit()
PipeSubmit -->> Main: Ok(())
else Shader Compile Failure
ShaderSet -->> Main: Err(GpuError)
end
else Pipeline Failure
PipeCreate -->> Main: Err(GpuError)
end
else Buffer Failure
BuffCreate -->> Main: Err(GpuError)
end
Main ->> Upload: upload(data, size, offset)
alt Upload Success
Upload -->> Main: Ok(())
else Upload Failure
Upload -->> Main: Err(GpuError)
end
Main ->> ImGui: Init ImGui + backends
else GPU Creation Failure
GpuCreate -->> Main: Err(GpuError)
end
sequenceDiagram
participant Editor as ImGui Editor UI
participant App as UserApp
participant PipeWrap as PipelineWrapper
participant BuffWrap as BufferWrapper
participant GPU as GpuWrapper
loop Each Frame
Editor ->> App: Vertex drag / Shader edit
alt Vertex Position Changed
App ->> BuffWrap: upload(new_vertex_data)
BuffWrap ->> GPU: acquire command buffer
BuffWrap ->> GPU: transfer buffer map/copy
alt Upload Success
BuffWrap -->> App: Ok(())
else Upload Fails
BuffWrap -->> App: Err(GpuError)
App ->> App: Log error, continue
end
else Shader Source Changed
App ->> PipeWrap: set_vertex_shader(src) or set_fragment_shader(src)
PipeWrap ->> PipeWrap: shaderc compile
alt Compile Success
PipeWrap ->> GPU: create_shader()
alt Shader Create Success
PipeWrap ->> GPU: release_shader(old)
PipeWrap ->> PipeWrap: Mark m_modified
PipeWrap -->> App: Ok(())
else Shader Create Fails
PipeWrap -->> App: Err(GpuError)
end
else Compile Fails
PipeWrap -->> App: Err(COMPILE_VERTEX_SHADER_FAILED)
end
end
App ->> PipeWrap: submit() if m_modified
alt Submit Success
PipeWrap ->> GPU: create_pipeline(info)
PipeWrap -->> App: Ok(())
else Submit Fails
PipeWrap -->> App: Err(GpuError)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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}
| #include <array> | ||
| #include <cmath> | ||
| #include <expected> | ||
| #include <iostream> |
There was a problem hiding this comment.
warning: included header expected is not used directly [misc-include-cleaner]
| #include <iostream> | |
| #include <iostream> |
| #include <expected> | ||
| #include <iostream> | ||
| #include <memory> | ||
| #include <numbers> |
There was a problem hiding this comment.
warning: included header memory is not used directly [misc-include-cleaner]
| #include <numbers> | |
| #include <numbers> |
| #include "SDL3/SDL_gpu.h" | ||
| #include "SDL3/SDL_keycode.h" | ||
|
|
||
| import sdl_wrapper; |
There was a problem hiding this comment.
warning: module 'sdl_wrapper' not found [clang-diagnostic-error]
import sdl_wrapper;
^| * components `(x, y, z)`. | ||
| */ | ||
| auto position() { return &x; } | ||
| float r, g, b, a; // vec4 color |
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
^| * components `(x, y, z)`. | ||
| */ | ||
| auto position() { return &x; } | ||
| float r, g, b, a; // vec4 color |
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
^| [&](auto& vertex_buffer) | ||
| { return vertex_buffer.upload(vertices.data(), static_cast<std::uint32_t>(sizeof(vertices)), 0); }); | ||
|
|
||
| if (!upload_result) |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to upload initial vertex data, error = %d",
^| [&](auto& vertex_buffer) | ||
| { return vertex_buffer.upload(vertices.data(), static_cast<std::uint32_t>(sizeof(vertices)), 0); }); | ||
|
|
||
| if (!upload_result) |
There was a problem hiding this comment.
warning: no header providing "SDL_LOG_CATEGORY_GPU" is directly included [misc-include-cleaner]
SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to upload initial vertex data, error = %d",
^| SDL_GetGPUSwapchainTextureFormat(gpu_wrapper->data(), gpu_wrapper->acquire_window()); | ||
| init_info.MSAASamples = SDL_GPU_SAMPLECOUNT_1; // Only used in multi-viewports mode. | ||
| init_info.SwapchainComposition = SDL_GPU_SWAPCHAINCOMPOSITION_SDR; // Only used in multi-viewports mode. | ||
|
|
There was a problem hiding this comment.
warning: no header providing "SDL_Window" is directly included [misc-include-cleaner]
if (SDL_Window* window = m_gpu->window())
^| SDL_GetGPUSwapchainTextureFormat(gpu_wrapper->data(), gpu_wrapper->acquire_window()); | ||
| init_info.MSAASamples = SDL_GPU_SAMPLECOUNT_1; // Only used in multi-viewports mode. | ||
| init_info.SwapchainComposition = SDL_GPU_SWAPCHAINCOMPOSITION_SDR; // Only used in multi-viewports mode. | ||
|
|
There was a problem hiding this comment.
warning: variable 'window' is not initialized [cppcoreguidelines-init-variables]
| if (SDL_Window* window = nullptr = m_gpu->window()) |
| { | ||
| ImGui_ImplSDL3_InitForSDLGPU(window); | ||
| } | ||
| else |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_LogError(SDL_LOG_CATEGORY_ERROR,
^
| * and sets up Dear ImGui and its SDL3/SDLGPU backends. | ||
| */ | ||
| virtual SDL_AppResult init(int argc, char** argv) override | ||
| SDL_AppResult init(int argc, char** argv) override |
There was a problem hiding this comment.
warning: method 'init' can be made static [readability-convert-member-functions-to-static]
| SDL_AppResult init(int argc, char** argv) override | |
| static SDL_AppResult init(int argc, char** argv) override |
| * and sets up Dear ImGui and its SDL3/SDLGPU backends. | ||
| */ | ||
| virtual SDL_AppResult init(int argc, char** argv) override | ||
| SDL_AppResult init(int argc, char** argv) override |
There was a problem hiding this comment.
warning: no header providing "SDL_AppResult" is directly included [misc-include-cleaner]
main.cpp:10:
- #include "imgui.h"
+ #include "SDL3/SDL_init.h"
+ #include "imgui.h"| * and sets up Dear ImGui and its SDL3/SDLGPU backends. | ||
| */ | ||
| virtual SDL_AppResult init(int argc, char** argv) override | ||
| SDL_AppResult init(int argc, char** argv) override |
There was a problem hiding this comment.
warning: parameter 'argc' is unused [misc-unused-parameters]
| SDL_AppResult init(int argc, char** argv) override | |
| SDL_AppResult init(int /*argc*/, char** argv) override |
| * and sets up Dear ImGui and its SDL3/SDLGPU backends. | ||
| */ | ||
| virtual SDL_AppResult init(int argc, char** argv) override | ||
| SDL_AppResult init(int argc, char** argv) override |
There was a problem hiding this comment.
warning: parameter 'argv' is unused [misc-unused-parameters]
| SDL_AppResult init(int argc, char** argv) override | |
| SDL_AppResult init(int argc, char** /*argv*/) override |
| auto gpu_result = sopho::GpuWrapper::create(); | ||
| if (!gpu_result) | ||
| { | ||
| SDL_LogError(SDL_LOG_CATEGORY_ERROR, "Failed to create GpuWrapper, error = %d", |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_LogError(SDL_LOG_CATEGORY_ERROR, "Failed to create GpuWrapper, error = %d",
^| if (!result) | ||
| { | ||
| pipeline_wrapper.set_fragment_shader(fragment_source); | ||
| SDL_LogError(SDL_LOG_CATEGORY_RENDER, "Failed to set fragment shader from editor, error = %d", |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_LogError(SDL_LOG_CATEGORY_RENDER, "Failed to set fragment shader from editor, error = %d",
^| m_pipeline_wrapper.and_then([](auto& pipeline_wrapper) { return pipeline_wrapper.submit(); }); | ||
| if (!pipeline_submit) | ||
| { | ||
| SDL_LogError(SDL_LOG_CATEGORY_GPU, "Pipeline submit failed, error = %d", |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_LogError(SDL_LOG_CATEGORY_GPU, "Pipeline submit failed, error = %d",
^| return SDL_APP_CONTINUE; | ||
| } | ||
|
|
||
| SDL_GPUDevice* device = m_gpu->device(); |
There was a problem hiding this comment.
warning: variable 'device' is not initialized [cppcoreguidelines-init-variables]
| SDL_GPUDevice* device = m_gpu->device(); | |
| SDL_GPUDevice* device = nullptr = m_gpu->device(); |
| } | ||
|
|
||
| SDL_GPUDevice* device = m_gpu->device(); | ||
| if (!device) |
There was a problem hiding this comment.
warning: implicit conversion 'SDL_GPUDevice *' -> 'bool' [readability-implicit-bool-conversion]
| if (!device) | |
| if (device == nullptr) |
| SDL_GPUDevice* device = m_gpu->device(); | ||
| if (!device) | ||
| { | ||
| SDL_LogError(SDL_LOG_CATEGORY_GPU, "GpuWrapper::device() returned null in draw()"); |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
SDL_LogError(SDL_LOG_CATEGORY_GPU, "GpuWrapper::device() returned null in draw()");
^![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.cpp (1)
361-427: Now let me verify the SDL3 GPU API documentation to confirm whetherSDL_BeginGPURenderPasscan returnnullptr:Let me search for more specific documentation on SDL_BeginGPURenderPass behavior:
Status after verification:
Based on my investigation, I found an important documentation discrepancy in SDL3:
SDL_CreateGPUTexture explicitly documents: "returns a texture object on success, or NULL on failure", but SDL_BeginGPURenderPass documentation only states: "returns a render pass handle" with no explicit failure case documented.
The review comment states that
SDL_BeginGPURenderPass"can returnnullptr" but this is not explicitly documented in the SDL3 API reference. However, SDL's general pattern is that "most SDL functions will return a specific value on failure. For functions that return pointers, this value is NULL".Confirmed issue in the codebase: The current
main.cppcode unconditionally usesrenderPasswithout checking for null, following the pattern already established forswapchainTexturenull checking in the same function.
Handle SDL_BeginGPURenderPass failure to avoid using a null render pass
The code unconditionally uses
renderPassfromSDL_BeginGPURenderPass(binding pipeline, binding vertex buffers, drawing), but following SDL's established pattern for pointer-returning functions, this should be guarded against null. The codebase already demonstrates careful null-checking for other GPU resources (command buffer, swapchain texture); the render pass should be treated consistently.Suggested fix:
SDL_GPURenderPass* renderPass = SDL_BeginGPURenderPass(commandBuffer, &colorTargetInfo, 1, nullptr); + if (!renderPass) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to begin GPU render pass: %s", SDL_GetError()); + SDL_SubmitGPUCommandBuffer(commandBuffer); + return SDL_APP_CONTINUE; + } // Bind pipeline if available. m_pipeline_wrapper.and_then(
🧹 Nitpick comments (4)
sdl_wrapper/sdl_wrapper.buffer.cpp (1)
42-135: Tighten bounds check and handle zero-size uploads explicitlyThe overall upload flow (bounds check → transfer buffer resize → map/copy → copy pass + submit →
std::expectederrors) is solid. Two small robustness tweaks would help:
- Avoid potential
std::uint32_toverflow inoffset + size.- Treat
size == 0as a no-op instead of running the full GPU path (and potentially mapping a null transfer buffer).For example:
- // Bounds check to avoid writing past the end of the GPU buffer. - if (offset + size > m_vertex_buffer_size) + // Early-out for zero-length uploads. + if (size == 0) + { + return std::monostate{}; + } + + // Bounds check to avoid writing past the end of the GPU buffer, without overflow. + if (offset > m_vertex_buffer_size || size > m_vertex_buffer_size - offset) { SDL_LogError(SDL_LOG_CATEGORY_GPU, "%s:%d buffer overflow: size=%u, offset=%u, buffer_size=%u", __FILE__, __LINE__, size, offset, m_vertex_buffer_size); return std::unexpected(GpuError::BUFFER_OVERFLOW); }The rest of the error reporting and GPU calls look consistent with the new
GpuErrormodel and the no-exceptions policy. Based on learnings.sdl_wrapper/sdl_wrapper.pipeline.cpp (2)
20-40: spv_result_to_bytes is correct; consider pre-sizing for fewer allocationsThe helper correctly walks the SPIR-V word range and appends bytes safely. If you ever want a micro-optimization, you could
bytes.resize(word_count * sizeof(std::uint32_t))once andstd::memcpyinto place instead of repeatedinsert, but the current version is perfectly fine for clarity.
42-246: Constructor and shader/pipeline methods compose cleanly with GpuWrapperThe constructor’s vertex layout, color target, and blend setup are internally consistent,
submit()refreshes vector-backed pointers before creating a new pipeline and swaps it with proper cleanup, andset_vertex_shader/set_fragment_shaderonly replace existing shaders after successful creation, marking the pipeline modified and returning preciseGpuErrors. Only minor nit: the “You may want to add this to your GpuError enum.” comments around the compile errors look stale now that the enum values are in use—either confirm they exist or update/remove the comments.sdl_wrapper/sdl_wrapper.gpu.ixx (1)
189-287: GpuWrapper factory and helpers align with the new error model (minor style nit)Using
create_gpu_context().transform(...)to build astd::shared_ptr<GpuWrapper>gives a neat,std::expected-friendly factory, and the resource helpers (create_pipeline,create_shader,release_*,get_texture_format) do the right logging and error mapping. If you care about style, you could switch the factory tostd::make_shared<GpuWrapper>(std::move(ctx))instead ofnew, but semantically this is already fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
main.cpp(11 hunks)sdl_wrapper/sdl_callback_implement.cpp(1 hunks)sdl_wrapper/sdl_wrapper.buffer.cpp(1 hunks)sdl_wrapper/sdl_wrapper.buffer.ixx(1 hunks)sdl_wrapper/sdl_wrapper.decl.ixx(1 hunks)sdl_wrapper/sdl_wrapper.gpu.cpp(1 hunks)sdl_wrapper/sdl_wrapper.gpu.ixx(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.cpp(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.ixx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.decl.ixxsdl_wrapper/sdl_wrapper.gpu.cppsdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/sdl_wrapper.buffer.ixxsdl_wrapper/sdl_callback_implement.cppsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.buffer.cppsdl_wrapper/sdl_wrapper.pipeline.ixxmain.cpp
📚 Learning: 2025-11-14T00:45:09.403Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.decl.ixxsdl_wrapper/sdl_wrapper.gpu.cppsdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/sdl_wrapper.buffer.ixxsdl_wrapper/sdl_callback_implement.cppsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.buffer.cppsdl_wrapper/sdl_wrapper.pipeline.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/sdl_wrapper.decl.ixxsdl_wrapper/sdl_wrapper.gpu.cppsdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/sdl_wrapper.buffer.ixxsdl_wrapper/sdl_callback_implement.cppsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.buffer.cppsdl_wrapper/sdl_wrapper.pipeline.ixxmain.cpp
📚 Learning: 2025-11-13T02:58:26.892Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.892Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
sdl_wrapper/sdl_wrapper.gpu.cppsdl_wrapper/sdl_callback_implement.cppsdl_wrapper/sdl_wrapper.gpu.ixx
🧬 Code graph analysis (2)
sdl_wrapper/sdl_wrapper.pipeline.cpp (1)
sdl_wrapper/sdl_wrapper.buffer.cpp (1)
nodiscard(42-135)
sdl_wrapper/sdl_wrapper.buffer.cpp (1)
sdl_wrapper/sdl_wrapper.pipeline.cpp (3)
nodiscard(133-173)nodiscard(175-209)nodiscard(211-246)
⏰ 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). (9)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: clang-tidy-review
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Debug, clang)
🔇 Additional comments (8)
sdl_wrapper/sdl_wrapper.decl.ixx (1)
9-28: GpuError enum covers the expected GPU failure modes coherentlyThe new
GpuErrorvalues line up with the failure cases inBufferWrapper::upload,GpuWrapper::create_buffer, and the pipeline shader compilation/submit paths, giving you a consistent error surface for the newstd::expectedAPIs. Looks good.sdl_wrapper/sdl_wrapper.buffer.cpp (1)
19-40: Destructor cleanup pattern aligns with move-only RAII semanticsThe destructor’s early return on
!m_gpuplus guarded releases form_vertex_bufferandm_transfer_bufferworks well with the move-only design ofBufferWrapper, avoiding double frees on moved-from instances while still cleaning up owned resources.sdl_wrapper/sdl_wrapper.gpu.cpp (1)
1-7: GpuWrapper factory methods integrate cleanly with the new expected-based API
create_bufferandcreate_pipeline_wrapperusestd::expected<..., GpuError>consistently, log failures via SDL, and return properly constructedBufferWrapper/PipelineWrapperinstances on success. The use ofget_texture_format().transform(...)keeps the pipeline creation path concise while propagating texture-format errors unchanged. This fits well with the PR’s error-handling design. Based on learnings.Also applies to: 13-28
sdl_wrapper/sdl_wrapper.buffer.ixx (1)
1-56: BufferWrapper’s move-only RAII design and upload API look well-structuredConstraining construction to
GpuWrapper, makingBufferWrappermove-only, trackingm_vertex_buffer_size, and exposing anuploadthat returnsstd::expected<std::monostate, GpuError>gives a clear, RAII-friendly interface that matches the implementation insdl_wrapper.buffer.cppand the overall error model.sdl_wrapper/sdl_wrapper.pipeline.ixx (1)
20-70: PipelineWrapper interface matches the new expected-based GPU APIPrivate construction via
GpuWrapper, deleted copy, move-only semantics,noexceptdestructor, and thesubmit/set_*_shadermethods returningstd::expectedall look coherent and consistent with the rest of the refactor. No issues from the interface side.sdl_wrapper/sdl_wrapper.gpu.ixx (1)
19-187: RAII handles correctly manage device/window/claim on all paths
DeviceHandle,WindowHandle, andClaimedWindowcleanly encapsulate their respective SDL* lifetimes, andcreate_gpu_context()uses them so that failures (device, window, or claim) all log and then release any partially-created resources via destructors. The resultingGpuContextis move-only and exposes raw device/window accessors, which fits the wrapper design; no issues here.main.cpp (2)
40-210: init()’s staged std::expected flow is solidCreating
GpuWrapper, buffer, pipeline wrapper, compiling shaders, submitting the pipeline, uploading vertices, and initializing ImGui all follow a clear step-by-step pattern with early returns on error and usefulSDL_LogErrormessages. The use ofstd::expectedand.and_then()here is consistent and keeps the control flow straightforward.
223-296: ImGui editor integration and buffer/shader updates handle failures gracefullyThe editor combo and the three modes (vertex positions, vertex shader, fragment shader) are wired so that:
- Vertex drags re-upload the buffer only when something changed, and upload errors are logged.
- Shader edits recompile via the pipeline wrapper, with compile/create errors logged without crashing.
The use ofstd::expected::and_thenkeeps these flows compact while still surfacing failures; I don’t see functional issues here.
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements