Skip to content

Conversation

@WSQS
Copy link
Owner

@WSQS WSQS commented Nov 19, 2025

Summary by CodeRabbit

  • New Features

    • Vertex editor now supports per‑vertex layout introspection (positions, colors) with live uploads and immediate visual updates.
  • Refactor

    • Rendering pipeline replaced by a unified renderable/data flow for simpler initialization, binding, and resource uploads; vertex layout handling improved.
  • Style

    • Shader path behavior changed to use a fixed color in the vertex path; init and error messages clarified.

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Walkthrough

A multi-file refactor replaces pipeline/buffer primitives with higher-level rendering abstractions: RenderProcedural, RenderData, Renderable, and VertexLayout. BufferWrapper::upload() now uploads an internal CPU buffer. main.cpp now builds and uses a Renderable, routing shader, data, and draw bindings through the new types.

Changes

Cohort / File(s) Summary
Application
main.cpp
Replaced m_vertex_buffer/m_pipeline_wrapper with m_renderable (std::shared_ptr<sopho::Renderable>). Initialization now calls create_render_procedural() and create_data(); vertex editing uses RenderData::vertex_view() and upload via m_renderable->data()->upload(); draw binds via procedural()->data() and data()->get_buffer_binding().
New rendering types
sdl_wrapper/sdl_wrapper.renderable.ixx, sdl_wrapper/sdl_wrapper.render_data.ixx, sdl_wrapper/sdl_wrapper.vertex_layout.ixx
Added Renderable (holds shared_ptrs to RenderProcedural & RenderData), RenderData (wraps BufferWrapper, VertexLayout, bindings, vertex_view/upload), and VertexLayout (format → attributes/stride computation and accessors).
Render procedural (pipeline) refactor
sdl_wrapper/sdl_wrapper.render_procedural.ixx, sdl_wrapper/sdl_wrapper.render_procedural.cpp
Renamed PipelineWrapperRenderProcedural; replaced manual vertex-attribute handling with VertexLayout; added set_vertex_attributes() and vertex_layout(); preserved shader/submit interfaces under new naming and updated internal state/stride handling.
Buffer wrapper redesign
sdl_wrapper/sdl_wrapper.buffer.ixx, sdl_wrapper/sdl_wrapper.buffer.cpp
Replaced m_vertex_buffer with m_gpu_buffer; added m_cpu_buffer and m_transfer_buffer constructor param; changed constructor signature; upload(const void*,size,offset)upload() (uploads internal CPU buffer); data()gpu_buffer() and added cpu_buffer() accessor; removed exported namespace on module.
GPU wrapper API updates
sdl_wrapper/sdl_wrapper.gpu.ixx, sdl_wrapper/sdl_wrapper.gpu.cpp
create_pipeline_wrapper()create_render_procedural() returning RenderProcedural; added create_data(const RenderProcedural&, uint32_t)RenderData; create_buffer() now creates/returns transfer buffer and returns updated error codes (CREATE_GPU_BUFFER_FAILED).
Module exports & build
sdl_wrapper/CMakeLists.txt, sdl_wrapper/sdl_wrapper.ixx, sdl_wrapper/sdl_wrapper.decl.ixx
Updated CMake to remove pipeline sources and add render_procedural/render_data/renderable/vertex_layout; replaced export import :pipeline with exports for render modules; GpuError enum member renamed (CREATE_BUFFER_FAILEDCREATE_GPU_BUFFER_FAILED); PipelineWrapper decl removed, RenderProcedural/RenderData added.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application (main.cpp)
    participant GPU as GpuWrapper
    participant RP as RenderProcedural
    participant RD as RenderData
    participant BW as BufferWrapper
    participant Rbl as Renderable

    rect rgba(200,240,200,0.3)
    Note over App,GPU: Initialization flow
    App->>GPU: create_render_procedural()
    GPU-->>App: RenderProcedural
    App->>GPU: create_data(RenderProcedural, vertex_count)
    GPU->>BW: create_buffer(size, transfer_buffer)
    GPU-->>RD: RenderData(BufferWrapper, VertexLayout, count)
    App->>Rbl: make Renderable(RP, RD)
    Rbl-->>App: store m_renderable
    end

    rect rgba(220,220,255,0.25)
    Note over App,Rbl: Edit & upload
    App->>Rbl: data()->vertex_view()
    Rbl-->>App: VertexView {layout, count, cpu_ptr}
    App->>App: modify cpu_ptr contents
    App->>Rbl: data()->upload()
    Rbl->>RD: upload()
    RD->>BW: upload()  -- copy cpu -> transfer -> gpu
    end

    rect rgba(255,230,200,0.25)
    Note over App,Rbl: Render frame
    App->>Rbl: procedural()->data()  (pipeline state)
    Rbl-->>App: RenderProcedural*
    App->>Rbl: data()->get_buffer_binding()
    Rbl-->>App: SDL_GPUBufferBinding[]
    App->>GPU: bind buffers & issue draw
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Cross-cutting architectural refactor touching modules, public APIs, and application code.
  • Heterogeneous edits: new modules, API renames, changed constructor/signature semantics, and altered runtime data flow.

Files/areas to review carefully:

  • Buffer upload path and transfer buffer mapping in sdl_wrapper/sdl_wrapper.buffer.cpp/ixx.
  • Stride/offset calculations in VertexLayout::set_vertex_attributes().
  • RenderData bindings initialization and lifetime (binding indices, GPU buffer references).
  • GpuWrapper::create_data() sizing (layout.get_stride() × vertex_count) and error propagation.
  • main.cpp pointer arithmetic and usage of vertex_view().raw when editing mixed-format vertex layouts.

Possibly related PRs

  • PR #6 — Touches main.cpp and application pipeline state; likely overlaps with the App→Renderable migration and state changes.
  • PR #3 — Earlier BufferWrapper and upload API modifications; overlaps with the buffer redesign and upload semantics here.
  • PR #7 — Introduced the prior PipelineWrapper pipeline flow that this change replaces with RenderProcedural/RenderData/Renderable.

Poem

🐰 I hopped through code, rearranged a stream,

Pipelines turned to render dreams, it seems.
New layouts stride, buffers hum in tune,
I munch a byte and tap the moon,
Hooray — the renderable carrot gleams! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Vertex layout' is overly vague and generic. While the PR does introduce a new VertexLayout class, it represents only one aspect of a much larger refactoring that replaces the pipeline-based architecture with a renderable-based approach across multiple files and components. Revise the title to reflect the primary architectural change, such as 'Refactor rendering pipeline to use renderable-based architecture' or 'Replace pipeline wrapper with renderable and render procedural abstractions'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch vertex_layout

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

This commit fixes the style issues introduced in 6726b98 according to the output
from ClangFormat.

Details: #34
@deepsource-io
Copy link
Contributor

deepsource-io bot commented Nov 19, 2025

Here's the code health analysis summary for commits 14491bc..a2112fc. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource C & C++ LogoC & C++❌ Failure
❗ 11 occurences introduced
🎯 94 occurences resolved
View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (9)
sdl_wrapper/sdl_wrapper.renderable.ixx (1)

10-23: Consider encapsulation for the Renderable class.

The Renderable class exposes m_render_procedural and m_render_data as public members. While this works for a simple aggregator, consider making them private and providing const accessors if the class might evolve to maintain invariants or add validation logic in the future.

Example refactor:

 export class Renderable
 {
+private:
     std::shared_ptr<RenderProcedural> m_render_procedural{};
     std::shared_ptr<RenderData> m_render_data{};
+public:
-    auto & procedural()
+    const auto& procedural() const
     {
         return m_render_procedural;
     }
-    auto & data()
+    const auto& data() const
     {
         return m_render_data;
     }
+    auto& procedural()
+    {
+        return m_render_procedural;
+    }
+    auto& data()
+    {
+        return m_render_data;
+    }
 };
sdl_wrapper/sdl_wrapper.vertex_layout.ixx (2)

95-95: Use size_t for loop counter to match vector size type.

The loop counter i is declared as int, but raw.size() returns size_t. This can cause signed/unsigned comparison warnings.

Apply this diff:

-        for (int i = 0; i < raw.size(); ++i)
+        for (size_t i = 0; i < raw.size(); ++i)
         {
             SDL_GPUVertexAttribute vertex_attribute{};
             vertex_attribute.location = i;

107-109: Consider marking accessor methods as const.

The accessor methods get_vertex_format(), get_vertex_attributes(), and get_stride() do not modify the object state and should be marked const for correctness.

Apply this diff:

-    const auto& get_vertex_format() { return raw; }
-    const auto& get_vertex_attributes() { return attributes; }
-    auto get_stride() const { return stride; }
+    const auto& get_vertex_format() const { return raw; }
+    const auto& get_vertex_attributes() const { return attributes; }
+    auto get_stride() const { return stride; }
sdl_wrapper/sdl_wrapper.render_procedural.cpp (1)

45-45: Consider making vertex layout configurable.

The vertex format is currently hardcoded to {SDL_GPU_VERTEXELEMENTFORMAT_FLOAT3, SDL_GPU_VERTEXELEMENTFORMAT_FLOAT4}. If different vertex layouts might be needed in the future, consider accepting the layout as a constructor parameter or providing a configuration method.

Example:

RenderProcedural::RenderProcedural(
    std::shared_ptr<GpuWrapper> gpu, 
    SDL_GPUTextureFormat swapchain_format,
    std::vector<SDL_GPUVertexElementFormat> vertex_formats = {
        SDL_GPU_VERTEXELEMENTFORMAT_FLOAT3, 
        SDL_GPU_VERTEXELEMENTFORMAT_FLOAT4
    }) noexcept
    : m_gpu(std::move(gpu))
{
    m_vertex_layout.set_vertex_attributes(std::move(vertex_formats));
    // ... rest of constructor
}
main.cpp (3)

42-58: Vertex color no longer driven by per-vertex data

Vertex shader now hardcodes v_color = vec4(1) (Line 57), while the editor UI still exposes per-vertex color (FLOAT4) controls in the “Vertex Edit” pane (Lines 223–230). Colors edited in the UI will not affect rendering anymore.

If this is intentional (e.g., focusing on layout mechanics only), consider updating the editor label/help text to avoid confusion; otherwise, restore use of the color attribute in the shader.

Also applies to: 212-237


91-105: Renderable / RenderData initialization mostly looks good, but initial CPU vertex data is never populated

The new flow:

  • Creates a RenderProcedural via create_render_procedural() (Line 92).
  • Sets shaders and submits via the and_then chain (Lines 101–104).
  • Allocates RenderData with create_data(pw_result.value(), 3) (Line 114).
  • Immediately calls upload() (Line 123) and wraps everything into m_renderable (Lines 132–134).

In this translation unit there is no code that initializes the CPU-side vertex buffer before that first upload(), so you effectively upload an all‑zero vertex buffer and draw a degenerate triangle until the user edits via the ImGui “Vertex Edit” panel.

If you still want a visible default triangle (as in the older code), you’ll need to seed m_renderable->data()->vertex_view().raw with initial positions/colors before the first upload(), or move that initialization into create_data. Otherwise, consider updating the comment at Line 122 (“Upload initial vertex data.”) to reflect that the buffer starts as zeros by design.

Also applies to: 113-135


212-237: Editing CPU vertex data via reinterpret_cast<float*>(std::byte*) is not alignment-safe

In the “Vertex Edit” case:

  • editor_data.raw is a std::byte* backed by std::vector<std::byte> in BufferWrapper (Lines 215–217).
  • You call ImGui::DragFloat3/4 on reinterpret_cast<float*>(ptr) (Lines 224–229).
  • ptr is incremented in bytes using sopho::get_size(format) (Lines 234–235).

Because the storage is std::vector<std::byte>, the base address is only guaranteed to be byte-aligned; reinterpreting that as float* is technically undefined behavior on platforms that require stricter alignment for float.

Given your likely desktop targets this will “work” in practice, but for portability and to avoid UB it would be better to:

  • Either store CPU vertex data in an aligned type (std::vector<float> / struct-of-arrays, or std::vector<std::byte, aligned_allocator<...>>), or
  • Use std::memcpy between the byte buffer and a local float[3]/float[4] when presenting/editing in ImGui, writing back only on change.

The latter keeps your raw-byte GPU layout but avoids alignment-sensitive aliasing.

Also applies to: 239-247

sdl_wrapper/sdl_wrapper.render_data.ixx (1)

13-47: RenderData ownership and API look good; consider tightening constness and documenting CPU buffer usage

RenderData nicely wraps:

  • A BufferWrapper (owning GPU/transfer/CPU buffers),
  • The VertexLayout, and
  • A single SDL_GPUBufferBinding in m_bindings (Lines 15–24).

A couple of small polish points:

  • get_buffer_binding() (Line 38) returns a non‑const reference; in current usage it’s only read (.data()/.size() in main.cpp). Exposing a const overload or returning const std::vector<SDL_GPUBufferBinding>& would better communicate intent.
  • VertexView::raw (Lines 28–33) is the primary way callers mutate CPU vertex data. It’s worth adding a brief comment that this pointer is only valid as long as the underlying RenderData (and its BufferWrapper) is alive, and that callers are expected to respect the vertex layout when writing.

These refinements aren’t blockers but would make the abstraction a bit clearer to future users.

sdl_wrapper/sdl_wrapper.buffer.ixx (1)

8-31: BufferWrapper CPU buffer design matches new flow; add explicit <cstddef> and brief usage doc

The updated BufferWrapper:

  • Tracks m_buffer_size and keeps a CPU mirror in m_cpu_buffer resized in the ctor (Lines 20–31).
  • Exposes a parameterless upload() (Line 41) that now works on the internal CPU buffer.
  • Provides gpu_buffer() and cpu_buffer() accessors (Lines 44–45), which are exactly what RenderData and the editor need.

Two minor polish items:

  • This file uses std::byte (Lines 23, 45) but doesn’t include <cstddef> directly. Relying on transitive includes from other headers can be brittle; consider adding:

    module;
    #include <expected>
    #include <memory>
    #include <variant>
    #include <vector>
  • #include // std::byte
    
    
  • A short comment on cpu_buffer() indicating that callers are expected to treat the memory as raw bytes matching the current VertexLayout (and that they should write before calling upload()) would help future readers understand the intended contract.

Functionally this integrates well with RenderData and the new vertex editing path.

Also applies to: 39-46

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14491bc and 2b08cd1.

📒 Files selected for processing (13)
  • main.cpp (12 hunks)
  • sdl_wrapper/CMakeLists.txt (1 hunks)
  • sdl_wrapper/sdl_wrapper.buffer.cpp (2 hunks)
  • sdl_wrapper/sdl_wrapper.buffer.ixx (2 hunks)
  • sdl_wrapper/sdl_wrapper.decl.ixx (2 hunks)
  • sdl_wrapper/sdl_wrapper.gpu.cpp (1 hunks)
  • sdl_wrapper/sdl_wrapper.gpu.ixx (3 hunks)
  • sdl_wrapper/sdl_wrapper.ixx (1 hunks)
  • sdl_wrapper/sdl_wrapper.render_data.ixx (1 hunks)
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp (8 hunks)
  • sdl_wrapper/sdl_wrapper.render_procedural.ixx (3 hunks)
  • sdl_wrapper/sdl_wrapper.renderable.ixx (1 hunks)
  • sdl_wrapper/sdl_wrapper.vertex_layout.ixx (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.

Applied to files:

  • sdl_wrapper/CMakeLists.txt
  • sdl_wrapper/sdl_wrapper.ixx
  • sdl_wrapper/sdl_wrapper.render_data.ixx
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp
  • sdl_wrapper/sdl_wrapper.decl.ixx
  • main.cpp
  • sdl_wrapper/sdl_wrapper.buffer.ixx
  • sdl_wrapper/sdl_wrapper.gpu.ixx
  • sdl_wrapper/sdl_wrapper.render_procedural.ixx
  • sdl_wrapper/sdl_wrapper.gpu.cpp
  • sdl_wrapper/sdl_wrapper.buffer.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.txt
  • sdl_wrapper/sdl_wrapper.ixx
  • sdl_wrapper/sdl_wrapper.render_data.ixx
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp
  • sdl_wrapper/sdl_wrapper.decl.ixx
  • main.cpp
  • sdl_wrapper/sdl_wrapper.buffer.ixx
  • sdl_wrapper/sdl_wrapper.gpu.ixx
  • sdl_wrapper/sdl_wrapper.render_procedural.ixx
  • sdl_wrapper/sdl_wrapper.gpu.cpp
  • sdl_wrapper/sdl_wrapper.buffer.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/CMakeLists.txt
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp
  • sdl_wrapper/sdl_wrapper.decl.ixx
  • sdl_wrapper/sdl_wrapper.gpu.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/CMakeLists.txt
  • sdl_wrapper/sdl_wrapper.ixx
  • sdl_wrapper/sdl_wrapper.render_data.ixx
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp
  • sdl_wrapper/sdl_wrapper.decl.ixx
  • main.cpp
  • sdl_wrapper/sdl_wrapper.vertex_layout.ixx
  • sdl_wrapper/sdl_wrapper.buffer.ixx
  • sdl_wrapper/sdl_wrapper.gpu.ixx
  • sdl_wrapper/sdl_wrapper.render_procedural.ixx
  • sdl_wrapper/sdl_wrapper.gpu.cpp
  • sdl_wrapper/sdl_wrapper.buffer.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). (4)
  • GitHub Check: clang-tidy-review
  • GitHub Check: build (windows-latest, Release, cl)
  • GitHub Check: build (windows-latest, Debug, gcc)
  • GitHub Check: build (Release, gcc)
🔇 Additional comments (16)
sdl_wrapper/sdl_wrapper.decl.ixx (1)

15-15: LGTM!

The enum rename to CREATE_GPU_BUFFER_FAILED and the addition of RenderProcedural and RenderData forward declarations properly support the architectural refactor from pipeline-centric to render-procedural/render-data abstractions.

Also applies to: 33-34

sdl_wrapper/CMakeLists.txt (1)

13-18: LGTM!

The CMake configuration correctly replaces the pipeline module with the new rendering modules (render_procedural, render_data, renderable, vertex_layout), aligning the build with the architectural refactor.

Also applies to: 21-21

sdl_wrapper/sdl_wrapper.ixx (1)

9-12: LGTM!

The module exports are correctly updated to reflect the new rendering architecture, replacing the buffer/pipeline modules with render_procedural, renderable, vertex_layout, and render_data modules.

sdl_wrapper/sdl_wrapper.gpu.ixx (1)

15-15: LGTM!

The GPU wrapper API is properly updated to use RenderProcedural and RenderData types. The new create_data method signature aligns well with the render-procedural architecture, and the import of :render_procedural ensures the necessary types are available.

Also applies to: 216-217, 227-227

sdl_wrapper/sdl_wrapper.buffer.cpp (2)

41-44: LGTM - Simplified upload API.

The upload() method signature is simplified to operate on internal buffers (m_cpu_buffer and m_gpu_buffer), removing the need for caller-provided data pointers and offset parameters. This encapsulation improves the API by centralizing buffer management within the BufferWrapper.


27-30: LGTM - Buffer references updated.

The buffer release logic and upload region setup correctly reference m_gpu_buffer instead of the previous vertex_buffer, aligning with the refactored buffer management approach.

Also applies to: 86-88

sdl_wrapper/sdl_wrapper.vertex_layout.ixx (1)

13-81: LGTM - Comprehensive format size mapping.

The get_size() function comprehensively covers all SDL_GPUVertexElementFormat enum values, correctly returning byte sizes for each format. The default case properly returns 0 for invalid formats.

sdl_wrapper/sdl_wrapper.render_procedural.cpp (3)

42-45: LGTM - Constructor and destructor renamed.

The constructor and destructor are properly renamed from PipelineWrapper to RenderProcedural, and the vertex layout initialization using m_vertex_layout.set_vertex_attributes() correctly integrates the new vertex layout abstraction.

Also applies to: 95-95


121-136: LGTM - Proper pointer refresh in submit().

The submit() method correctly refreshes pointers to internal vector data before creating the pipeline. This pattern ensures the pointers remain valid even if the vectors were reallocated, which is a good defensive programming practice.


163-197: LGTM - Shader compilation methods updated.

The set_vertex_shader() and set_fragment_shader() methods are properly renamed and maintain the correct error handling and resource management patterns established in the original implementation.

Also applies to: 200-234

main.cpp (2)

251-268: Shader editor re‑wiring to Renderable::procedural() looks consistent

The vertex and fragment shader editors now call m_renderable->procedural()->set_vertex_shader(...) and set_fragment_shader(...) (Lines 260, 279), which matches the new RenderProcedural API and keeps error propagation via std::expected.

This looks correct and preserves the previous editor behavior through the new abstraction.

Also applies to: 270-287


306-315: Draw path integration with RenderProcedural / RenderData looks coherent

In draw():

  • You call m_renderable->procedural()->submit() each frame (Line 307), relying on the procedural object to no-op if nothing changed.
  • You bind the graphics pipeline via m_renderable->procedural()->data() (Line 368).
  • You bind vertex buffers using m_renderable->data()->get_buffer_binding() for both pointer and count (Lines 400–401).

Assuming RenderProcedural::submit() correctly respects m_modified and RenderData::get_buffer_binding() keeps its buffer vector stable, this wiring is clean and matches SDL’s expectations.

Also applies to: 368-402

sdl_wrapper/sdl_wrapper.gpu.cpp (3)

47-52: create_render_procedural nicely encapsulates RenderProcedural construction

Using get_texture_format().transform(...) with a captured shared_from_this() (Lines 50–51) is a clean way to:

  • Propagate GpuErrors from the texture-format query, and
  • Construct RenderProcedural only on success.

This looks correct and aligns with the non‑throwing, std::expected-based error style used elsewhere.


36-46: Review comment is incorrect - no narrowing conversion occurs

The review flags a "size_t → uint32_t narrowing conversion," but the code actually involves only uint32_t types:

  • VertexLayout::get_stride() returns uint32_t (not size_t), as defined at line 109 of sdl_wrapper/sdl_wrapper.vertex_layout.ixx
  • vertex_count parameter is uint32_t
  • Therefore the computation get_stride() * vertex_count is uint32_t * uint32_t = uint32_t
  • No narrowing conversion to uint32_t occurs since the result is already uint32_t

The concern raised is based on a misidentification of the return type.

Likely an incorrect or invalid review comment.


15-35: GPU buffer leak on transfer buffer creation failure

In GpuWrapper::create_buffer (sdl_wrapper/sdl_wrapper.gpu.cpp, lines 15–35):

  • gpu_buffer is created on line 18
  • If transfer_buffer creation fails (lines 29–33), the method returns an error without releasing gpu_buffer
  • Since BufferWrapper is never constructed in this error path, its destructor does not run, and release_buffer() is never called

Release gpu_buffer via release_buffer() before returning the error on line 32.

⛔ 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/sdl_wrapper.render_procedural.ixx (1)

33-45: set_vertex_attributes must have explicit void return type; clarify vertex_layout() return type

Two API clarity issues confirmed:

  1. set_vertex_attributes (line ~69) uses auto return type with no return statement, which is non-idiomatic and confusing:

  •    auto set_vertex_attributes(std::vector<SDL_GPUVertexElementFormat> vertex_attributes)
    
  •    void set_vertex_attributes(std::vector<SDL_GPUVertexElementFormat> vertex_attributes)
       {
           m_vertex_layout.set_vertex_attributes(std::move(vertex_attributes));
           m_modified = true;
       }
    
    
    
  1. vertex_layout() (line ~75) uses auto& which is less explicit than:

  •    auto& vertex_layout() const { return m_vertex_layout; }
    
  •    [[nodiscard]] const VertexLayout& vertex_layout() const noexcept { return m_vertex_layout; }
    
    
    

The first fix is critical for unambiguous API semantics; the second improves readability and documentation.

⛔ 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.

Comment on lines +224 to +229
changed |= ImGui::DragFloat3(std::format("node{}", vertex_index).data(),
reinterpret_cast<float*>(ptr), 0.01f, -1.f, 1.f);
break;
case SDL_GPU_VERTEXELEMENTFORMAT_FLOAT4:
changed |= ImGui::DragFloat4(std::format("color{}", vertex_index).data(),
reinterpret_cast<float*>(ptr), 0.01f, -1.f, 1.f);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

std::format used without including <format>

std::format is used for the ImGui labels (Lines 224, 228), but <format> isn’t included at the top of this file. That will fail to compile on standard‑conforming toolchains.

Add the appropriate header:

 #include <algorithm>
 #include <array>
 #include <cmath>
 #include <expected>
 #include <iostream>
 #include <memory>
 #include <numbers>
 #include <string>
+ #include <format>

🏁 Script executed:

#!/bin/bash
# Check the main.cpp file for the code at lines 224-229
echo "=== Lines 224-229 in main.cpp ==="
sed -n '224,229p' main.cpp

echo ""
echo "=== Checking for #include <format> in main.cpp ==="
grep -n "#include <format>" main.cpp || echo "Not found"

echo ""
echo "=== Current include section (first 50 lines) ==="
head -50 main.cpp

Length of output: 307


🏁 Script executed:

#!/bin/bash
# Find main.cpp in the repository
echo "=== Finding main.cpp ==="
fd -t f "main\.cpp$"

echo ""
echo "=== Listing directory structure ==="
ls -la

Length of output: 868


🏁 Script executed:

#!/bin/bash
# Check lines 224-229 for std::format usage
echo "=== Lines 224-229 in main.cpp ==="
sed -n '224,229p' main.cpp

echo ""
echo "=== Searching for #include <format> ==="
grep -n "#include <format>" main.cpp || echo "NOT FOUND"

echo ""
echo "=== Include section (lines 1-20) ==="
sed -n '1,20p' main.cpp

Length of output: 1034


Add #include <format> header to fix compilation error

The review comment is verified as correct. std::format is used on lines 224 and 228 without the required <format> header included. This will cause a compilation failure on standard-conforming toolchains.

Add #include <format> to the standard library includes section at the top of main.cpp, as proposed in the original review comment.

🤖 Prompt for AI Agents
In main.cpp around lines 224 to 229, std::format is used but the <format> header
is not included; add #include <format> to the standard library includes at the
top of main.cpp (alongside other #include lines) so the calls to std::format
compile on standard-conforming toolchains.

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #35

coderabbitai bot added a commit that referenced this pull request Nov 19, 2025
Docstrings generation was requested by @WSQS.

* #34 (comment)

The following files were modified:

* `main.cpp`
* `sdl_wrapper/sdl_wrapper.buffer.cpp`
* `sdl_wrapper/sdl_wrapper.gpu.cpp`
* `sdl_wrapper/sdl_wrapper.render_procedural.cpp`
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
sdl_wrapper/sdl_wrapper.render_data.ixx (2)

21-25: Consider validating buffer size against vertex layout and count.

The constructor does not verify that the buffer's size matches the expected size based on vertex_count and the layout's stride. Additionally, the binding index is hardcoded to 0, which may limit flexibility if multiple vertex buffers per binding are needed in the future.

Consider adding validation:

 explicit RenderData(BufferWrapper&& buffer_wrapper, const VertexLayout& layouts, size_t vertex_count) :
     m_buffer(std::move(buffer_wrapper)), m_layouts(layouts), m_vertex_count(vertex_count)
 {
+    // Optional: validate buffer size matches layout stride * vertex count
     m_bindings.emplace_back(m_buffer.gpu_buffer(), 0);
 }

38-39: Exposing mutable references may break encapsulation.

Both buffer() and get_buffer_binding() return non-const references to internal state, allowing external code to modify m_buffer and m_bindings directly. This could lead to inconsistent state if bindings are modified without updating the buffer.

Consider returning const references or copying if mutation isn't required:

-auto& buffer() { return m_buffer; }
-auto& get_buffer_binding() { return m_bindings; }
+const auto& buffer() const { return m_buffer; }
+const auto& get_buffer_binding() const { return m_bindings; }
sdl_wrapper/sdl_wrapper.buffer.ixx (1)

26-31: Note: CPU buffer is zero-initialized on construction.

Line 30 uses resize(), which zero-initializes all bytes in m_cpu_buffer. If this overhead is unnecessary and the buffer will be immediately overwritten, consider using reserve() instead. However, if zero-initialization is intentional for safety, the current approach is fine.

If zero-initialization isn't required:

-m_cpu_buffer.resize(m_buffer_size);
+m_cpu_buffer.reserve(m_buffer_size);

Based on learnings

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b08cd1 and 5027eef.

📒 Files selected for processing (3)
  • sdl_wrapper/sdl_wrapper.buffer.ixx (3 hunks)
  • sdl_wrapper/sdl_wrapper.ixx (1 hunks)
  • sdl_wrapper/sdl_wrapper.render_data.ixx (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.

Applied to files:

  • sdl_wrapper/sdl_wrapper.ixx
  • sdl_wrapper/sdl_wrapper.render_data.ixx
  • sdl_wrapper/sdl_wrapper.buffer.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/sdl_wrapper.ixx
  • sdl_wrapper/sdl_wrapper.render_data.ixx
  • sdl_wrapper/sdl_wrapper.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/sdl_wrapper.ixx
  • sdl_wrapper/sdl_wrapper.render_data.ixx
  • sdl_wrapper/sdl_wrapper.buffer.ixx
🔇 Additional comments (7)
sdl_wrapper/sdl_wrapper.render_data.ixx (2)

28-33: LGTM!

The VertexView struct provides a clean, non-owning view of the vertex data with all necessary information for CPU-side inspection.


34-37: LGTM!

Move-only semantics are appropriate for a resource-owning wrapper class.

sdl_wrapper/sdl_wrapper.ixx (1)

10-13: LGTM!

The module exports have been correctly updated to reflect the new rendering abstractions, replacing the previous pipeline-based API with a more modular design.

sdl_wrapper/sdl_wrapper.buffer.ixx (4)

8-8: LGTM!

The <vector> include is necessary for the new m_cpu_buffer member.


20-23: LGTM!

The refactoring to use an internal CPU buffer (m_cpu_buffer) alongside the GPU buffer is a cleaner design that centralizes buffer management. The rename from m_vertex_buffer to m_gpu_buffer improves clarity.


39-41: LGTM! Significant API simplification.

The simplified upload() signature that operates on the internal CPU buffer is cleaner than the previous parameterized version. This enforces a clear pattern: modify via cpu_buffer(), then upload() to synchronize to GPU.


44-45: LGTM! Clear accessor naming.

The gpu_buffer() and cpu_buffer() naming clearly distinguishes between the GPU-side and CPU-side buffers. The raw pointer from cpu_buffer() is appropriate for direct memory access, and since the vector is sized at construction and presumably not resized, the pointer should remain valid.

* 📝 Add docstrings to `vertex_layout`

Docstrings generation was requested by @WSQS.

* #34 (comment)

The following files were modified:

* `main.cpp`
* `sdl_wrapper/sdl_wrapper.buffer.cpp`
* `sdl_wrapper/sdl_wrapper.gpu.cpp`
* `sdl_wrapper/sdl_wrapper.render_procedural.cpp`

* style: format code with ClangFormat

This commit fixes the style issues introduced in 79383bc according to the output
from ClangFormat.

Details: #35

---------

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>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

#include <expected>
#include "SDL3/SDL_gpu.h"
#include "SDL3/SDL_log.h"
module sdl_wrapper;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: module 'sdl_wrapper' not found [clang-diagnostic-error]

module sdl_wrapper;
       ^

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
main.cpp (1)

237-241: Missing #include <format> header for std::format usage.

std::format is used on lines 237 and 241, but the <format> header is not included. This will cause compilation failure on standard-conforming toolchains.

Add the missing header at the top of the file:

 #include <algorithm>
 #include <array>
 #include <cmath>
 #include <expected>
+#include <format>
 #include <iostream>
 #include <memory>
 #include <numbers>
 #include <string>
🧹 Nitpick comments (2)
main.cpp (2)

115-122: Reorder comments to match execution flow.

The comment "3. Create vertex buffer" appears after the code for step 4 (compile shaders and build pipeline). This makes the initialization flow harder to follow.

Apply this diff to fix the comment ordering:

-        // 3. Create vertex buffer.
         auto render_data = std::move(m_gpu->create_data(pw_result.value(), 3));

And update the subsequent comment:

-        // 5. Upload initial vertex data.
+        // 4. Upload initial vertex data.
         auto upload_result = render_data.and_then([&](auto& vertex_buffer) { return vertex_buffer.upload(); });

And update the camera initialization comment:

-        // 6. Initialize camera matrix to identity.
+        // 5. Initialize camera matrix to identity.

And subsequent steps (7, 8, 9 → 6, 7, 8).


234-249: Consider logging unsupported vertex formats in the default case.

The switch statement handles FLOAT3 and FLOAT4 formats but silently ignores others in the default case. If the vertex layout includes unsupported formats, they will be skipped without notification, which could make debugging difficult.

Consider adding a log message for unsupported formats:

                         default:
+                            SDL_LogWarn(SDL_LOG_CATEGORY_APPLICATION,
+                                       "Vertex editor does not support format: %d", static_cast<int>(format));
                             break;
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5027eef and a2112fc.

📒 Files selected for processing (4)
  • main.cpp (14 hunks)
  • sdl_wrapper/sdl_wrapper.buffer.cpp (2 hunks)
  • sdl_wrapper/sdl_wrapper.gpu.cpp (2 hunks)
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp (8 hunks)
🧰 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:

  • main.cpp
  • sdl_wrapper/sdl_wrapper.gpu.cpp
📚 Learning: 2025-11-13T11:35:48.289Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.289Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.

Applied to files:

  • main.cpp
  • sdl_wrapper/sdl_wrapper.buffer.cpp
  • sdl_wrapper/sdl_wrapper.gpu.cpp
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp
📚 Learning: 2025-11-13T11:34:32.732Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.732Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.

Applied to files:

  • main.cpp
  • sdl_wrapper/sdl_wrapper.buffer.cpp
  • sdl_wrapper/sdl_wrapper.gpu.cpp
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp
📚 Learning: 2025-11-14T00:45:09.403Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.

Applied to files:

  • main.cpp
  • sdl_wrapper/sdl_wrapper.buffer.cpp
  • sdl_wrapper/sdl_wrapper.gpu.cpp
  • sdl_wrapper/sdl_wrapper.render_procedural.cpp
🔇 Additional comments (11)
sdl_wrapper/sdl_wrapper.render_procedural.cpp (6)

20-48: LGTM! Well-documented SPIR-V byte conversion.

The function correctly converts shaderc's word-based SPIR-V output to a byte vector suitable for GPU shader creation. The documentation clearly explains the conversion process.


50-111: LGTM! Constructor properly initializes rendering pipeline state.

The constructor correctly configures the vertex layout, shader compiler options, vertex buffer descriptions, and color target state. The use of m_vertex_layout API ensures consistent stride and attribute setup throughout the pipeline.


113-144: LGTM! Destructor correctly releases GPU resources.

The destructor safely releases all owned GPU resources (pipeline and shaders) with appropriate null checks and pointer cleanup.


146-196: LGTM! Pipeline submission handles state updates correctly.

The method properly refreshes descriptor pointers after potential vector reallocations, creates a new pipeline when modifications are pending, and safely replaces the old pipeline. The early return optimization for unmodified state is a good practice.


198-242: LGTM! Vertex shader compilation and installation is well-structured.

The method correctly compiles GLSL to SPIR-V, handles compilation errors with appropriate logging, creates the GPU shader, and safely replaces the previous shader while marking the pipeline for rebuild.


244-290: LGTM! Fragment shader setup mirrors vertex shader implementation.

The method follows the same well-structured pattern as set_vertex_shader, ensuring consistent shader compilation, error handling, and resource management for fragment shaders.

sdl_wrapper/sdl_wrapper.buffer.cpp (2)

19-45: LGTM! Destructor correctly releases both GPU and transfer buffers.

The destructor safely releases both the GPU buffer and transfer buffer with appropriate null checks and pointer cleanup. The rename from m_vertex_buffer to m_gpu_buffer reflects the more general-purpose nature of the buffer.


47-118: LGTM! Simplified upload API uses internal CPU buffer.

The refactored upload() method correctly sources data from the internal m_cpu_buffer, simplifying the API surface. The GPU copy pass setup and command buffer submission are properly structured with appropriate error handling.

sdl_wrapper/sdl_wrapper.gpu.cpp (3)

15-43: LGTM! Buffer creation now includes transfer buffer upfront.

The expanded create_buffer method now creates both the GPU buffer and its associated transfer buffer in a single operation, improving API ergonomics and ensuring resources are allocated together. Error handling correctly distinguishes between GPU buffer and transfer buffer creation failures.


44-66: LGTM! New create_data method provides clean render data abstraction.

The method correctly computes buffer size from the vertex layout stride and vertex count, allocates the necessary GPU buffer, and encapsulates everything in a RenderData object. This higher-level abstraction simplifies render data management for users of the API.


67-80: LGTM! Renamed method constructs RenderProcedural with clean monadic flow.

The method leverages std::expected::transform to elegantly construct a RenderProcedural with the queried texture format, providing clean error propagation and a functional programming style.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

* Creates the GPU device, window/pipeline/buffer wrappers, compiles shaders,
* uploads the initial vertex data, initializes the camera to identity,
* and sets up Dear ImGui and its SDL3/SDLGPU backends.
* @return SDL_AppResult `SDL_APP_CONTINUE` on successful initialization, `SDL_APP_FAILURE` on error.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'init' can be made static [readability-convert-member-functions-to-static]

Suggested change
* @return SDL_AppResult `SDL_APP_CONTINUE` on successful initialization, `SDL_APP_FAILURE` on error.
static SDL_AppResult init(int argc, char** argv) override

* Creates the GPU device, window/pipeline/buffer wrappers, compiles shaders,
* uploads the initial vertex data, initializes the camera to identity,
* and sets up Dear ImGui and its SDL3/SDLGPU backends.
* @return SDL_AppResult `SDL_APP_CONTINUE` on successful initialization, `SDL_APP_FAILURE` on error.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "SDL_AppResult" is directly included [misc-include-cleaner]

main.cpp:12:

- #include "imgui.h"
+ #include "SDL3/SDL_init.h"
+ #include "imgui.h"

* Creates the GPU device, window/pipeline/buffer wrappers, compiles shaders,
* uploads the initial vertex data, initializes the camera to identity,
* and sets up Dear ImGui and its SDL3/SDLGPU backends.
* @return SDL_AppResult `SDL_APP_CONTINUE` on successful initialization, `SDL_APP_FAILURE` on error.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: parameter 'argc' is unused [misc-unused-parameters]

Suggested change
* @return SDL_AppResult `SDL_APP_CONTINUE` on successful initialization, `SDL_APP_FAILURE` on error.
SDL_AppResult init(int /*argc*/, char** argv) override

* Creates the GPU device, window/pipeline/buffer wrappers, compiles shaders,
* uploads the initial vertex data, initializes the camera to identity,
* and sets up Dear ImGui and its SDL3/SDLGPU backends.
* @return SDL_AppResult `SDL_APP_CONTINUE` on successful initialization, `SDL_APP_FAILURE` on error.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: parameter 'argv' is unused [misc-unused-parameters]

Suggested change
* @return SDL_AppResult `SDL_APP_CONTINUE` on successful initialization, `SDL_APP_FAILURE` on error.
SDL_AppResult init(int argc, char** /*argv*/) override


// 3. Create vertex buffer.
auto render_data = std::move(m_gpu->create_data(pw_result.value(), 3));
if (!render_data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]

            SDL_LogError(SDL_LOG_CATEGORY_ERROR, "Failed to create vertex buffer, error = %d",
            ^


/**
* @brief Advance the UI frame, present editors for triangle vertices and shader sources.
* @brief Advance the UI frame and present editors for vertex data and shader sources.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'tick' can be made static [readability-convert-member-functions-to-static]

Suggested change
* @brief Advance the UI frame and present editors for vertex data and shader sources.
static SDL_AppResult tick()

changed |= ImGui::DragFloat4(std::format("color{}", vertex_index).data(),
reinterpret_cast<float*>(ptr), 0.01f, -1.f, 1.f);
break;
default:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]

                        SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to upload vertex buffer in tick(), error = %d",
                        ^

*
* Performs pipeline submission if needed, prepares ImGui draw data, records GPU commands
* to clear and render the color target, uploads the camera uniform, binds vertex buffers
* and the graphics pipeline, issues the draw call, renders ImGui, and submits the command buffer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'device' is not initialized [cppcoreguidelines-init-variables]

Suggested change
* and the graphics pipeline, issues the draw call, renders ImGui, and submits the command buffer.
SDL_GPUDevice* device = nullptr = m_gpu->device();

* Performs pipeline submission if needed, prepares ImGui draw data, records GPU commands
* to clear and render the color target, uploads the camera uniform, binds vertex buffers
* and the graphics pipeline, issues the draw call, renders ImGui, and submits the command buffer.
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: implicit conversion 'SDL_GPUDevice *' -> 'bool' [readability-implicit-bool-conversion]

Suggested change
*
if (device == nullptr)

// Rebuild pipeline if needed.
auto pipeline_submit =
m_pipeline_wrapper.and_then([](auto& pipeline_wrapper) { return pipeline_wrapper.submit(); });
auto pipeline_submit = m_renderable->procedural()->submit();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]

            SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to acquire GPU command buffer");
            ^

@WSQS WSQS merged commit 3eee3af into dev Nov 19, 2025
25 of 26 checks passed
@WSQS WSQS deleted the vertex_layout branch November 20, 2025 00:41
WSQS added a commit that referenced this pull request Nov 20, 2025
* fix: add comment for main.cpp

* fix: add method set_vertex_attributes for PipelineWrapper

* fix: using m_vertex_layout to calculate vertex

* fix:remove vertex layout from forward declaration

* fix:rename pipeline wrapper to render procedural

* feat:add method vertex_layout

* fix: remove struct Vertex and using byte array

* feat: rename function

* feat: rename variable

* fix: remove unused variable

* doc: clean up doc

* fix: create Transfer Buffer when created

* feat: add class render data

* fix:rename variable

* fix:rename variable

* fix:rename function

* fix:change operation order

* feat:add method get_buffer_binding

* feat:add member variable m_render_data

* feat:add missing header

* fix:remove unused member variable

* fix:change shader interface

* fix:add property m_layouts for RenderData

* feat:change imgui based onto vertex layout

* fix:remove buffer function

* fix:hide buffer module

* style: format code with ClangFormat

This commit fixes the style issues introduced in 6726b98 according to the output
from ClangFormat.

Details: #34

* Revert "fix:hide buffer module"

This reverts commit 6726b98.

* 📝 Add docstrings to `vertex_layout` (#35)

* 📝 Add docstrings to `vertex_layout`

Docstrings generation was requested by @WSQS.

* #34 (comment)

The following files were modified:

* `main.cpp`
* `sdl_wrapper/sdl_wrapper.buffer.cpp`
* `sdl_wrapper/sdl_wrapper.gpu.cpp`
* `sdl_wrapper/sdl_wrapper.render_procedural.cpp`

* style: format code with ClangFormat

This commit fixes the style issues introduced in 79383bc according to the output
from ClangFormat.

Details: #35

---------

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>

---------

Co-authored-by: Sophomore <sophomore@duck.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@coderabbitai coderabbitai bot mentioned this pull request Nov 20, 2025
WSQS added a commit that referenced this pull request Nov 20, 2025
* feat: add module glsl reflector

* feat: implement glsl reflector

* Vertex layout (#34)

* fix: add comment for main.cpp

* fix: add method set_vertex_attributes for PipelineWrapper

* fix: using m_vertex_layout to calculate vertex

* fix:remove vertex layout from forward declaration

* fix:rename pipeline wrapper to render procedural

* feat:add method vertex_layout

* fix: remove struct Vertex and using byte array

* feat: rename function

* feat: rename variable

* fix: remove unused variable

* doc: clean up doc

* fix: create Transfer Buffer when created

* feat: add class render data

* fix:rename variable

* fix:rename variable

* fix:rename function

* fix:change operation order

* feat:add method get_buffer_binding

* feat:add member variable m_render_data

* feat:add missing header

* fix:remove unused member variable

* fix:change shader interface

* fix:add property m_layouts for RenderData

* feat:change imgui based onto vertex layout

* fix:remove buffer function

* fix:hide buffer module

* style: format code with ClangFormat

This commit fixes the style issues introduced in 6726b98 according to the output
from ClangFormat.

Details: #34

* Revert "fix:hide buffer module"

This reverts commit 6726b98.

* 📝 Add docstrings to `vertex_layout` (#35)

* 📝 Add docstrings to `vertex_layout`

Docstrings generation was requested by @WSQS.

* #34 (comment)

The following files were modified:

* `main.cpp`
* `sdl_wrapper/sdl_wrapper.buffer.cpp`
* `sdl_wrapper/sdl_wrapper.gpu.cpp`
* `sdl_wrapper/sdl_wrapper.render_procedural.cpp`

* style: format code with ClangFormat

This commit fixes the style issues introduced in 79383bc according to the output
from ClangFormat.

Details: #35

---------

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>

---------

Co-authored-by: Sophomore <sophomore@duck.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* feat: Implement glsl reflector into sdl wrapper

* feat: recreate data when shader update

* fix: add header file cstdint

* fix: import module

* fix: remove install for glslr

* fix: add time out for gtest_discover_tests

* fix:add head file to make in compile under mint

* fix: add and clean up header file

* style: format code with ClangFormat

This commit fixes the style issues introduced in 2061a92 according to the output
from ClangFormat.

Details: #36

---------

Co-authored-by: Sophomore <sophomore@duck.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Nov 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants