Add index buffer #40
Add index buffer #40
Conversation
WalkthroughIntroduces a new Changes
Sequence DiagramsequenceDiagram
participant App as SDL App
participant Callback as sdl_callback_implement
participant Factory as main.cpp<br/>(create_app)
participant AppObj as sopho::App
participant ET as data_type<br/>(checkable)
App->>Callback: SDL_AppInit()
Callback->>Factory: create_app()
Factory->>AppObj: new App(...)
AppObj->>ET: return checkable<App*>
Factory->>ET: return checkable<App*>
Callback->>ET: app = checkable<App*>
Callback->>ET: app.value()
ET->>Callback: unwrapped App*
Callback->>AppObj: appstate = app.value()
Callback->>AppObj: app.value()->init()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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}
| for (int index_index = 0; index_index < 2; ++index_index) | ||
| { | ||
| changed |= ImGui::InputInt3(std::format("index_{}", index_index).data(), | ||
| reinterpret_cast<int*>(index_ptr)); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
reinterpret_cast<int*>(index_ptr));
^| SDL_GPU_INDEXELEMENTSIZE_32BIT); | ||
|
|
||
| SDL_DrawGPUPrimitives(renderPass, 3, 1, 0, 0); | ||
| SDL_DrawGPUIndexedPrimitives(renderPass, 6, 1, 0, 0, 0); |
There was a problem hiding this comment.
warning: 6 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
SDL_DrawGPUIndexedPrimitives(renderPass, 6, 1, 0, 0, 0);
^| * deleting it. | ||
| */ | ||
| sopho::App* create_app(int argc, char** argv) { return new UserApp(); } | ||
| sopho::checkable<sopho::App*> create_app(int argc, char** argv) { return new UserApp(); } |
There was a problem hiding this comment.
warning: function 'create_app' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| sopho::checkable<sopho::App*> create_app(int argc, char** argv) { return new UserApp(); } | |
| static sopho::checkable<sopho::App*> create_app(int argc, char** argv) { return new UserApp(); } |
| * deleting it. | ||
| */ | ||
| sopho::App* create_app(int argc, char** argv) { return new UserApp(); } | ||
| sopho::checkable<sopho::App*> create_app(int argc, char** argv) { return new UserApp(); } |
There was a problem hiding this comment.
warning: parameter 'argc' is unused [misc-unused-parameters]
| sopho::checkable<sopho::App*> create_app(int argc, char** argv) { return new UserApp(); } | |
| sopho::checkable<sopho::App*> create_app(int /*argc*/, char** argv) { return new UserApp(); } |
| * deleting it. | ||
| */ | ||
| sopho::App* create_app(int argc, char** argv) { return new UserApp(); } | ||
| sopho::checkable<sopho::App*> create_app(int argc, char** argv) { return new UserApp(); } |
There was a problem hiding this comment.
warning: parameter 'argv' is unused [misc-unused-parameters]
| sopho::checkable<sopho::App*> create_app(int argc, char** argv) { return new UserApp(); } | |
| sopho::checkable<sopho::App*> create_app(int argc, char** /*argv*/) { return new UserApp(); } |
| #include <SDL3/SDL.h> | ||
| #include <SDL3/SDL_main.h> | ||
| #include "SDL3/SDL.h" | ||
| #include "SDL3/SDL_main.h" |
There was a problem hiding this comment.
warning: included header SDL.h is not used directly [misc-include-cleaner]
| #include "SDL3/SDL_main.h" | |
| #include "SDL3/SDL_main.h" |
| #include <SDL3/SDL_main.h> | ||
| #include "SDL3/SDL.h" | ||
| #include "SDL3/SDL_main.h" | ||
| import sdl_wrapper; |
There was a problem hiding this comment.
warning: module 'sdl_wrapper' not found [clang-diagnostic-error]
import sdl_wrapper;
^![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
sdl_wrapper/sdl_wrapper.render_procedural.ixx (1)
14-68: Good data_type import; consider switching tocheckableand updating commentPulling in
data_typehere is consistent with centralizingGpuErrorandcheckablein the new module, and the namespace merging keepsGpuErrorusable without qualification.Two small follow-ups you may want to do (no blockers):
- Use the new alias for consistency with the rest of the codebase, e.g.:
[[nodiscard]] std::expected<std::monostate, GpuError> submit();
[[nodiscard]] checkable<std::monostate> submit();and similarly for `set_vertex_shader` / `set_fragment_shader`.
- The comment on Line 15 (
// GpuError, forward declarations, etc.) is now slightly misleading sinceGpuErrorcomes fromdata_type; consider trimming or rewording it.sdl_wrapper/sdl_wrapper.gpu.cpp (1)
45-71: Doc comment and magic constants now lag behind the new vertex+index behaviorThe implementation of
GpuWrapper::create_datanow allocates both a vertex buffer and a fixed-size index buffer (6 * sizeof(int)), and returnsRenderDataowning both. The Doxygen above still only mentions a vertex buffer and doesn’t describe the index buffer at all, so it’s now misleading.You may also want to avoid scattering the implicit “6 indices” contract in multiple places (here, in the editor, and in
draw()); a shared constant or deriving the count fromRenderDatawould make future changes safer.sdl_wrapper/sdl_wrapper.buffer.ixx (1)
13-47: BufferWrapper visibility + explicit cpu_buffer() typeImporting
data_typehere is appropriate now thatGpuErrorlives there, and makingcpu_buffer()returnstd::byte*explicitly improves clarity overauto.Changing from
export namespace sophoto a non‑exported namespace in this partition effectively makesBufferWrapperan internal type ofsdl_wrapper:buffer. That’s fine if only othersdl_wrapperpartitions use it, but if any external code importssdl_wrapper:bufferdirectly and relies onsopho::BufferWrapper, this will be a breaking API change.main.cpp (4)
118-137: create_data usage with 4 vertices is consistent; minor nits on naming/loggingSwitching to
create_data(..., 4)matches a 4‑vertex rectangle driven by a 6‑index buffer, and wrapping this inRenderDatafits the new abstraction.Two small cleanups you might consider:
std::move(m_gpu->create_data(...))is redundant, ascreate_dataalready returns a prvalue.- Variable and log naming (
"Failed to create vertex buffer") still reference a single vertex buffer even thoughRenderDatanow encapsulates both vertex and index buffers; renaming to “render data” would better reflect reality.
231-268: Vertex/index editing block works; consider tightening a couple of detailsThe refactor to use a local
raw_ptrfor vertex data traversal keeps behavior while improving readability, and the new index editing viaindex_viewcleanly mirrors the fixed 2×int3index layout.Two optional robustness tweaks:
- Since indices are user‑editable, you may want to clamp or validate them against
editor_data.vertex_countbefore upload to avoid out‑of‑range indices causing undefined geometry.- The error log
"Failed to upload vertex buffer in tick()"now also covers index uploads; updating the message to “vertex/index data” would be less confusing when debugging.
441-447: Indexed draw path is wired correctly; avoid duplicating the “6 indices” contractBinding vertex buffers via
get_vertex_buffer_binding()and the index buffer viaget_index_buffer_binding()and then callingSDL_DrawGPUIndexedPrimitives(renderPass, 6, 1, 0, 0, 0);is consistent withGpuWrapper::create_dataallocating space for 6 indices and the editor’s twoInputInt3calls.To reduce the risk of these getting out of sync over time, consider having
RenderDataexpose anindex_count()(or a sharedconstexprindex count) and use that here instead of the hardcoded6, reusing the same value as increate_dataand the editor loop.
510-518: Update create_app’s documentation to match checkable<App> return type*The implementation now returns
sopho::checkable<sopho::App*>but the comment still documents a rawsopho::App*return and ownership semantics. The raw pointer part is still true, but it would be clearer to mention that the pointer is wrapped in acheckableresult (with an error channel) and that callers should treat a disengaged result as construction failure.You might also consider renaming the comment from “Creates a new application instance for the program” to mention that failure is now representable without throwing, aligning with the project’s no‑exceptions policy.
sdl_wrapper/sdl_wrapper.render_data.ixx (1)
49-51: Consider const-qualified accessors for more flexible usageRight now the accessors (
buffer(),get_vertex_buffer_binding(),get_index_buffer_binding(),vertex_view(),index_view()) are all non‑const, which can make it harder to useRenderDatathrough a const reference in purely read‑only render paths.If useful for your call sites, you could add const overloads, e.g.:
- auto& buffer() { return m_vertex_buffer; } + auto& buffer() { return m_vertex_buffer; } + const auto& buffer() const { return m_vertex_buffer; } - auto& get_vertex_buffer_binding() { return m_bindings; } + auto& get_vertex_buffer_binding() { return m_bindings; } + const auto& get_vertex_buffer_binding() const { return m_bindings; } - auto& get_index_buffer_binding() { return m_index_binding; } + auto& get_index_buffer_binding() { return m_index_binding; } + const auto& get_index_buffer_binding() const { return m_index_binding; } - auto vertex_view() + auto vertex_view() @@ - auto index_view() { return IndexView{.index_count = m_vertex_count, .raw = m_index_buffer.cpu_buffer()}; } + auto index_view() + { + return IndexView{.index_count = m_vertex_count, .raw = m_index_buffer.cpu_buffer()}; + } + + auto vertex_view() const + { + return VertexView{.layout = m_layouts, .vertex_count = m_vertex_count, .raw = m_vertex_buffer.cpu_buffer()}; + } + + auto index_view() const + { + return IndexView{.index_count = m_vertex_count, .raw = m_index_buffer.cpu_buffer()}; + }(Adjust the
index_countbits per your final design.) This is optional but improves API ergonomics.Also applies to: 54-56
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/clang-tidy-review.yml(1 hunks)CMakeLists.txt(1 hunks)data_type/CMakeLists.txt(1 hunks)data_type/modules/data_type.ixx(1 hunks)main.cpp(7 hunks)sdl_wrapper/CMakeLists.txt(1 hunks)sdl_wrapper/arch.md(1 hunks)sdl_wrapper/sdl_callback_implement.cpp(2 hunks)sdl_wrapper/sdl_wrapper.buffer.ixx(2 hunks)sdl_wrapper/sdl_wrapper.decl.ixx(0 hunks)sdl_wrapper/sdl_wrapper.gpu.cpp(1 hunks)sdl_wrapper/sdl_wrapper.gpu.ixx(1 hunks)sdl_wrapper/sdl_wrapper.render_data.ixx(2 hunks)sdl_wrapper/sdl_wrapper.render_procedural.ixx(1 hunks)sdl_wrapper/sdl_wrapper.renderable.ixx(1 hunks)
💤 Files with no reviewable changes (1)
- sdl_wrapper/sdl_wrapper.decl.ixx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.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: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.
📚 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/sdl_wrapper.buffer.ixxsdl_wrapper/sdl_callback_implement.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:
sdl_wrapper/sdl_wrapper.render_procedural.ixxsdl_wrapper/sdl_wrapper.gpu.cppsdl_wrapper/sdl_wrapper.buffer.ixxmain.cppsdl_wrapper/sdl_wrapper.renderable.ixxsdl_wrapper/sdl_callback_implement.cppsdl_wrapper/arch.mdsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.render_data.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.gpu.cppsdl_wrapper/sdl_wrapper.buffer.ixxmain.cppsdl_wrapper/sdl_callback_implement.cppsdl_wrapper/arch.mdsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.render_data.ixx
📚 Learning: 2025-11-14T00:45:09.403Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.403Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.gpu.cppsdl_wrapper/sdl_wrapper.buffer.ixxmain.cppsdl_wrapper/sdl_callback_implement.cppsdl_wrapper/sdl_wrapper.gpu.ixx
🧬 Code graph analysis (1)
sdl_wrapper/sdl_callback_implement.cpp (1)
main.cpp (2)
argc(84-197)argc(84-84)
⏰ 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 (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: CodeQL C++ Analysis
🔇 Additional comments (13)
.github/workflows/clang-tidy-review.yml (1)
37-42: clang-tidy-review action version bump looks safeOnly the action version changed to
v0.22.1with identical inputs, so behavior should remain the same aside from upstream fixes/features. Consider checking the action’s release notes before merging.sdl_wrapper/arch.md (1)
1-15: Architecture diagram is clear and aligned with code structureThe Mermaid diagram matches the current responsibilities and relationships (Renderable/RenderProcedural/RenderData/GpuWrapper) and is a useful high-level overview. No issues from a code-architecture perspective.
data_type/modules/data_type.ixx (1)
1-31: data_type module cleanly centralizes GPU error handlingThe
sopho::GpuErrorenum plusTErrorandcheckable<T>alias look consistent with existing error sites and create a single source of truth for GPU-related errors. The module/interface structure (module;+#include <expected>+export module data_type;) is also reasonable for C++23.data_type/CMakeLists.txt (1)
1-11: CMake setup for data_type module is appropriateThe
data_typestatic library with aCXX_MODULESfile set formodules/data_type.ixxmatches the project’s C++23 + CMake 3.30 setup and should integrate cleanly with consumers thatimport data_type;.CMakeLists.txt (1)
109-111: data_type subdirectory placement is correctAdding
add_subdirectory(data_type)beforesdl_wrapperensures thedata_typetarget exists whensdl_wrapperlinks against it. This wiring matches the new module import usage.sdl_wrapper/sdl_wrapper.renderable.ixx (1)
19-22: Explicitdata()return type improves API claritySwitching
data()to returnstd::shared_ptr<RenderData>&instead ofauto&makes the public interface clearer without changing semantics, and matches the member type.sdl_wrapper/CMakeLists.txt (1)
26-26: Linkingdata_typepublicly from sdl_wrapper matches the new module usageAdding
data_typeas a PUBLIC dependency is appropriate since sdl_wrapper’s exported interfaces depend on types from thedata_typemodule, and consumers will need it transitively.sdl_wrapper/sdl_wrapper.gpu.ixx (1)
212-214: Explicit window() return type is a good API tighteningReturning
SDL_Window*instead ofautomakes the public surface clearer while preserving behavior; call sites (e.g.,main.cppchecks for null) match this contract. No issues here.sdl_wrapper/sdl_callback_implement.cpp (2)
5-11: checkable-based create_app integration looks consistentImporting
data_typeand updating thecreate_appdeclaration to returnsopho::checkable<sopho::App*>aligns with the new error-handling model. InSDL_AppInit, usingif (!app)followed byapp.value()is the right pattern and keeps ownership semantics (the raw pointer is still handed to SDL via*appstateand deleted inSDL_AppQuit).Also applies to: 28-32
20-26: The original code is correct—review comment is based on SDL2 semantics, not SDL3.SDL3's
SDL_Initreturns a bool: true on success, false on failure. The code uses SDL3 (confirmed by#include "SDL3/SDL.h"), soif (!SDL_Init(SDL_INIT_VIDEO))correctly enters the error handler when the function returns false (failure).The review comment confuses this with SDL2's behavior, where
SDL_Initreturns an int (0 on success). The original code requires no changes.Likely an incorrect or invalid review comment.
main.cpp (2)
23-26: data_type import is appropriate for new checkable<App> usage*Pulling in
data_typehere is justified by the bottom‑of‑filecreate_appnow returningsopho::checkable<sopho::App*>; it keeps all definitions in a single translation unit without introducing extra headers.
45-61: Per‑vertex color propagation in vertex shader is a nice improvementChanging
v_colorto be assigned froma_colormakes the pipeline honor per‑vertex color data (which you edit in the UI) instead of a hardcoded value. This is consistent with the vertex layout and the ImGui “Node” editor.sdl_wrapper/sdl_wrapper.render_data.ixx (1)
15-16: Separation of vertex/index buffers and upload flow looks consistentOwning separate
m_vertex_buffer/m_index_bufferwith distinct vertex and index bindings, and uploading vertex first then index with early return on failure, is a clean, straightforward design for indexed rendering. I don’t see correctness issues here in isolation.Also applies to: 20-20, 23-31, 59-65
| struct IndexView | ||
| { | ||
| size_t index_count{}; | ||
| std::byte* raw{}; | ||
| }; |
There was a problem hiding this comment.
IndexView uses vertex_count, which can miscount indices
index_view() currently sets index_count = m_vertex_count. If the number of indices ever differs from the number of vertices (which is common), this will report the wrong count and can cause incorrect draw calls.
Consider tracking index count separately and wiring it through the constructor, e.g.:
- VertexLayout m_layouts{};
- size_t m_vertex_count{};
+ VertexLayout m_layouts{};
+ size_t m_vertex_count{};
+ size_t m_index_count{};
- explicit RenderData(BufferWrapper&& vertex_buffer_wrapper, BufferWrapper&& index_buffer_wrapper,
- const VertexLayout& layouts, size_t vertex_count) :
- m_vertex_buffer(std::move(vertex_buffer_wrapper)), m_index_buffer(std::move(index_buffer_wrapper)),
- m_layouts(layouts), m_vertex_count(vertex_count)
+ explicit RenderData(BufferWrapper&& vertex_buffer_wrapper, BufferWrapper&& index_buffer_wrapper,
+ const VertexLayout& layouts, size_t vertex_count, size_t index_count) :
+ m_vertex_buffer(std::move(vertex_buffer_wrapper)), m_index_buffer(std::move(index_buffer_wrapper)),
+ m_layouts(layouts), m_vertex_count(vertex_count), m_index_count(index_count)
@@
- auto index_view() { return IndexView{.index_count = m_vertex_count, .raw = m_index_buffer.cpu_buffer()}; }
+ auto index_view() { return IndexView{.index_count = m_index_count, .raw = m_index_buffer.cpu_buffer()}; }You’d then adjust call sites to pass the true index count.
Also applies to: 56-56
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.render_data.ixx around lines 40-44 (and also line
56), IndexView currently uses m_vertex_count to populate index_count which
misreports when index and vertex counts differ; change IndexView to store a
separate index_count member and add a constructor or parameter to set it from
callers, update index_view() to pass the true index count instead of
m_vertex_count, and update all call sites (including the spot around line 56) to
supply the actual index count so draw calls use the correct value.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.