Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ reviews:
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
base_branches:
- ".*"
ignore_usernames: []
finishing_touches:
docstrings:
Expand Down
76 changes: 7 additions & 69 deletions main.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "shaderc/shaderc.hpp"
#include <iostream>
#include <optional>

#include "imgui.h"
#include "imgui_impl_sdl3.h"
Expand Down Expand Up @@ -49,13 +50,10 @@ void main()
})WSQ";

class UserApp : public sopho::App {

sopho::BufferWrapper BufferWrapper{};
std::optional<sopho::BufferWrapper> vertexBuffer;

SDL_Window *window{};
SDL_GPUDevice *device{};
SDL_GPUBuffer *vertexBuffer{};
SDL_GPUTransferBuffer *transferBuffer{};
SDL_GPUGraphicsPipeline *graphicsPipeline{};

virtual SDL_AppResult init(int argc, char **argv) override {
Expand Down Expand Up @@ -177,42 +175,9 @@ class UserApp : public sopho::App {
SDL_GPUBufferCreateInfo bufferInfo{};
bufferInfo.size = sizeof(vertices);
bufferInfo.usage = SDL_GPU_BUFFERUSAGE_VERTEX;
vertexBuffer = SDL_CreateGPUBuffer(device, &bufferInfo);

// create a transfer buffer to upload to the vertex buffer
SDL_GPUTransferBufferCreateInfo transferInfo{};
transferInfo.size = sizeof(vertices);
transferInfo.usage = SDL_GPU_TRANSFERBUFFERUSAGE_UPLOAD;
transferBuffer = SDL_CreateGPUTransferBuffer(device, &transferInfo);

// fill the transfer buffer
Vertex *data = (Vertex *) SDL_MapGPUTransferBuffer(device, transferBuffer, false);

SDL_memcpy(data, (void *) vertices, sizeof(vertices));

SDL_UnmapGPUTransferBuffer(device, transferBuffer);

// start a copy pass
SDL_GPUCommandBuffer *commandBuffer = SDL_AcquireGPUCommandBuffer(device);
SDL_GPUCopyPass *copyPass = SDL_BeginGPUCopyPass(commandBuffer);

// where is the data
SDL_GPUTransferBufferLocation location{};
location.transfer_buffer = transferBuffer;
location.offset = 0;

// where to upload the data
SDL_GPUBufferRegion region{};
region.buffer = vertexBuffer;
region.size = sizeof(vertices);
region.offset = 0;
vertexBuffer.emplace(device, &bufferInfo);

// upload the data
SDL_UploadToGPUBuffer(copyPass, &location, &region, true);

// end the copy pass
SDL_EndGPUCopyPass(copyPass);
SDL_SubmitGPUCommandBuffer(commandBuffer);
vertexBuffer->upload(vertices, sizeof(vertices), 0);

// Setup Dear ImGui context
IMGUI_CHECKVERSION();
Expand Down Expand Up @@ -262,34 +227,8 @@ class UserApp : public sopho::App {
vertices[0].y = node[1];
vertices[0].z = node[2];

// fill the transfer buffer
Vertex *data = (Vertex *) SDL_MapGPUTransferBuffer(device, transferBuffer, false);

SDL_memcpy(data, (void *) vertices, sizeof(vertices));

SDL_UnmapGPUTransferBuffer(device, transferBuffer);

// start a copy pass
SDL_GPUCommandBuffer *commandBuffer = SDL_AcquireGPUCommandBuffer(device);
SDL_GPUCopyPass *copyPass = SDL_BeginGPUCopyPass(commandBuffer);
vertexBuffer->upload(vertices, sizeof(vertices), 0);

// where is the data
SDL_GPUTransferBufferLocation location{};
location.transfer_buffer = transferBuffer;
location.offset = 0;

// where to upload the data
SDL_GPUBufferRegion region{};
region.buffer = vertexBuffer;
region.size = sizeof(vertices);
region.offset = 0;

// upload the data
SDL_UploadToGPUBuffer(copyPass, &location, &region, true);

// end the copy pass
SDL_EndGPUCopyPass(copyPass);
SDL_SubmitGPUCommandBuffer(commandBuffer);
ImGui::End();
}

Expand Down Expand Up @@ -329,7 +268,7 @@ class UserApp : public sopho::App {

// bind the vertex buffer
SDL_GPUBufferBinding bufferBindings[1];
bufferBindings[0].buffer = vertexBuffer; // index 0 is slot 0 in this example
bufferBindings[0].buffer = vertexBuffer->data(); // index 0 is slot 0 in this example
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing optional validation before dereferencing.

vertexBuffer->data() is called without checking if the optional has a value. If initialization failed, this will crash during rendering.

Apply this diff:

         // bind the vertex buffer
         SDL_GPUBufferBinding bufferBindings[1];
-        bufferBindings[0].buffer = vertexBuffer->data(); // index 0 is slot 0 in this example
+        bufferBindings[0].buffer = vertexBuffer.has_value() ? vertexBuffer->data() : nullptr;
         bufferBindings[0].offset = 0; // start from the first byte
+        
+        if (!bufferBindings[0].buffer) {
+            SDL_EndGPURenderPass(renderPass);
+            SDL_SubmitGPUCommandBuffer(commandBuffer);
+            return SDL_APP_CONTINUE;
+        }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In main.cpp around line 305, vertexBuffer->data() is being dereferenced without
validating the optional; check the optional before use (e.g., if
(!vertexBuffer.has_value() or if (!vertexBuffer) ) and handle the failure path:
either log an error and abort/return early or provide a safe fallback so you do
not call ->data() on an empty optional. Ensure the bufferBindings assignment
only happens after the presence check.

bufferBindings[0].offset = 0; // start from the first byte

SDL_BindGPUVertexBuffers(renderPass, 0, bufferBindings, 1); // bind one buffer starting from slot 0
Expand Down Expand Up @@ -361,8 +300,7 @@ class UserApp : public sopho::App {
ImGui_ImplSDLGPU3_Shutdown();
ImGui::DestroyContext();
// release buffers
SDL_ReleaseGPUBuffer(device, vertexBuffer);
SDL_ReleaseGPUTransferBuffer(device, transferBuffer);
vertexBuffer = std::nullopt;

// release the pipeline
SDL_ReleaseGPUGraphicsPipeline(device, graphicsPipeline);
Expand Down
8 changes: 4 additions & 4 deletions sdl_wrapper/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ add_library(sdl_wrapper STATIC)
target_sources(sdl_wrapper
PUBLIC
FILE_SET cxx_modules TYPE CXX_MODULES FILES
${CMAKE_CURRENT_SOURCE_DIR}/sdl_wrapper.ixx
${CMAKE_CURRENT_SOURCE_DIR}/sdl_wrapper.buffer.ixx
${CMAKE_CURRENT_SOURCE_DIR}/sdl_wrapper.app.ixx
sdl_wrapper.ixx
sdl_wrapper.buffer.ixx
sdl_wrapper.app.ixx
PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}/sdl_callback_implement.cpp
sdl_callback_implement.cpp
)

target_link_libraries(sdl_wrapper PUBLIC SDL3::SDL3)
9 changes: 6 additions & 3 deletions sdl_wrapper/sdl_callback_implement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,24 @@ extern sopho::App *create_app();

SDL_AppResult SDL_AppInit(void **appstate, int argc, char **argv) {
auto app = create_app();
if (!app)
return SDL_APP_FAILURE;
*appstate = app;
return app->init(argc, argv);
}

SDL_AppResult SDL_AppIterate(void *appstate) {
auto *app = static_cast<sopho::App *>(appstate);
auto app = static_cast<sopho::App *>(appstate);
return app->iterate();
}

SDL_AppResult SDL_AppEvent(void *appstate, SDL_Event *event) {
auto *app = static_cast<sopho::App *>(appstate);
auto app = static_cast<sopho::App *>(appstate);
return app->event(event);
}

void SDL_AppQuit(void *appstate, SDL_AppResult result) {
auto *app = static_cast<sopho::App *>(appstate);
auto app = static_cast<sopho::App *>(appstate);
app->quit(result);
delete app;
}
60 changes: 59 additions & 1 deletion sdl_wrapper/sdl_wrapper.buffer.ixx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,64 @@ export module sdl_wrapper:buffer;

namespace sopho {
export class BufferWrapper {
SDL_GPUBuffer *vertexBuffer{};
SDL_GPUDevice *m_gpu{};
SDL_GPUBuffer *m_vertex_buffer{};
SDL_GPUTransferBuffer *m_transfer_buffer{};
uint32_t m_transfer_buffer_size{};

public:
BufferWrapper() = default;

BufferWrapper(SDL_GPUDevice *p_gpu, const SDL_GPUBufferCreateInfo *p_create_info)
: m_gpu(p_gpu), m_vertex_buffer(SDL_CreateGPUBuffer(p_gpu, p_create_info)) {
}
Comment on lines +18 to +20
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

Missing null check for buffer creation.

SDL_CreateGPUBuffer can return nullptr if buffer creation fails, but the constructor doesn't validate the result. This will cause crashes when upload() or the destructor attempts to use m_vertex_buffer.

Consider validating buffer creation or documenting that callers must check via a separate method:

 BufferWrapper(SDL_GPUDevice *p_gpu, const SDL_GPUBufferCreateInfo *p_create_info)
     : m_gpu(p_gpu), m_vertex_buffer(SDL_CreateGPUBuffer(p_gpu, p_create_info)) {
+    if (!m_vertex_buffer) {
+        SDL_Log("Failed to create GPU buffer");
+        // Consider throwing or setting an error state
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
BufferWrapper(SDL_GPUDevice *p_gpu, const SDL_GPUBufferCreateInfo *p_create_info)
: m_gpu(p_gpu), m_vertex_buffer(SDL_CreateGPUBuffer(p_gpu, p_create_info)) {
}
BufferWrapper(SDL_GPUDevice *p_gpu, const SDL_GPUBufferCreateInfo *p_create_info)
: m_gpu(p_gpu), m_vertex_buffer(SDL_CreateGPUBuffer(p_gpu, p_create_info)) {
if (!m_vertex_buffer) {
SDL_Log("Failed to create GPU buffer");
// Consider throwing or setting an error state
}
}
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.buffer.ixx around lines 18-20, the constructor
assigns m_vertex_buffer from SDL_CreateGPUBuffer without checking for nullptr;
add a null-check immediately after creation and either throw a descriptive
exception (e.g., std::runtime_error) so construction fails fast, or set a member
error flag and ensure all methods (upload(), destructor) guard against a null
m_vertex_buffer; also ensure the destructor and any cleanup only call SDL
functions when m_vertex_buffer is non-null and consider logging the failure with
contextual info (device/create_info) for debugging.


void upload(void *p_data, uint32_t p_size, uint32_t p_offset) {
if (p_size > m_transfer_buffer_size) {
if (m_transfer_buffer != nullptr) {
SDL_ReleaseGPUTransferBuffer(m_gpu, m_transfer_buffer);
}
SDL_GPUTransferBufferCreateInfo transfer_info{SDL_GPU_TRANSFERBUFFERUSAGE_UPLOAD, p_size, 0};
m_transfer_buffer = SDL_CreateGPUTransferBuffer(m_gpu, &transfer_info);
m_transfer_buffer_size = transfer_info.size;
}
Comment on lines +22 to +30
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

Missing null check for transfer buffer creation.

SDL_CreateGPUTransferBuffer (line 28) can return nullptr on failure, but there's no validation. If creation fails, subsequent SDL_MapGPUTransferBuffer (line 32) will crash.

Apply this diff:

             SDL_GPUTransferBufferCreateInfo transfer_info{SDL_GPU_TRANSFERBUFFERUSAGE_UPLOAD, p_size, 0};
             m_transfer_buffer = SDL_CreateGPUTransferBuffer(m_gpu, &transfer_info);
+            if (!m_transfer_buffer) {
+                SDL_Log("Failed to create GPU transfer buffer");
+                return; // or throw
+            }
             m_transfer_buffer_size = transfer_info.size;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void upload(void *p_data, uint32_t p_size, uint32_t p_offset) {
if (p_size > m_transfer_buffer_size) {
if (m_transfer_buffer != nullptr) {
SDL_ReleaseGPUTransferBuffer(m_gpu, m_transfer_buffer);
}
SDL_GPUTransferBufferCreateInfo transfer_info{SDL_GPU_TRANSFERBUFFERUSAGE_UPLOAD, p_size, 0};
m_transfer_buffer = SDL_CreateGPUTransferBuffer(m_gpu, &transfer_info);
m_transfer_buffer_size = transfer_info.size;
}
void upload(void *p_data, uint32_t p_size, uint32_t p_offset) {
if (p_size > m_transfer_buffer_size) {
if (m_transfer_buffer != nullptr) {
SDL_ReleaseGPUTransferBuffer(m_gpu, m_transfer_buffer);
}
SDL_GPUTransferBufferCreateInfo transfer_info{SDL_GPU_TRANSFERBUFFERUSAGE_UPLOAD, p_size, 0};
m_transfer_buffer = SDL_CreateGPUTransferBuffer(m_gpu, &transfer_info);
if (!m_transfer_buffer) {
SDL_Log("Failed to create GPU transfer buffer");
return; // or throw
}
m_transfer_buffer_size = transfer_info.size;
}
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.buffer.ixx around lines 22 to 30,
SDL_CreateGPUTransferBuffer may return nullptr but the code proceeds to use the
buffer; add a null check immediately after creation and handle failure by
logging or returning/throwing (and avoid calling SDL_MapGPUTransferBuffer or
updating m_transfer_buffer_size when m_transfer_buffer is null). If creation
failed, ensure previous buffer was already released and set m_transfer_buffer to
nullptr and m_transfer_buffer_size to 0 (or propagate an error) so upload() does
not dereference a null pointer.


auto data = SDL_MapGPUTransferBuffer(m_gpu, m_transfer_buffer, false);
SDL_memcpy(data, p_data, p_size);
SDL_UnmapGPUTransferBuffer(m_gpu, m_transfer_buffer);
Comment on lines +32 to +34
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

Missing null check after mapping transfer buffer.

SDL_MapGPUTransferBuffer can return nullptr if mapping fails, but there's no validation before the SDL_memcpy call. This will cause a crash on mapping failure.

Apply this diff:

         auto data = SDL_MapGPUTransferBuffer(m_gpu, m_transfer_buffer, false);
+        if (!data) {
+            SDL_Log("Failed to map GPU transfer buffer");
+            return;
+        }
         SDL_memcpy(data, p_data, p_size);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.buffer.ixx around lines 32 to 34, the code calls
SDL_MapGPUTransferBuffer and immediately SDL_memcpy without checking the
returned pointer; add a null-check for the mapped pointer, and if it is nullptr,
handle the failure (e.g., log an error, return/propagate failure, and skip the
memcpy) so you never call SDL_memcpy on a null pointer; ensure you still call
SDL_UnmapGPUTransferBuffer only when appropriate or otherwise avoid unmapping a
failed mapping.


// TODO: Delay submit command in collect
auto command_buffer = SDL_AcquireGPUCommandBuffer(m_gpu);
auto copy_pass = SDL_BeginGPUCopyPass(command_buffer);

SDL_GPUTransferBufferLocation location{};
location.transfer_buffer = m_transfer_buffer;
location.offset = 0;

SDL_GPUBufferRegion region{};
region.buffer = m_vertex_buffer;
region.size = p_size;
region.offset = p_offset;

SDL_UploadToGPUBuffer(copy_pass, &location, &region, false);

SDL_EndGPUCopyPass(copy_pass);
SDL_SubmitGPUCommandBuffer(command_buffer);
}

auto data() {
return m_vertex_buffer;
}

~BufferWrapper() {
// TODO: It's too late to release gpu buffer, gpu was released
SDL_ReleaseGPUBuffer(m_gpu, m_vertex_buffer);
m_vertex_buffer = nullptr;
Comment on lines +59 to +62
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

Missing null check before releasing vertex buffer.

SDL_ReleaseGPUBuffer is called on m_vertex_buffer without checking if it's nullptr. If buffer creation failed in the constructor, this will crash.

Apply this diff:

     ~BufferWrapper() {
         // TODO: It's too late to release gpu buffer, gpu was released
-        SDL_ReleaseGPUBuffer(m_gpu, m_vertex_buffer);
-        m_vertex_buffer = nullptr;
+        if (m_vertex_buffer) {
+            SDL_ReleaseGPUBuffer(m_gpu, m_vertex_buffer);
+            m_vertex_buffer = nullptr;
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
~BufferWrapper() {
// TODO: It's too late to release gpu buffer, gpu was released
SDL_ReleaseGPUBuffer(m_gpu, m_vertex_buffer);
m_vertex_buffer = nullptr;
~BufferWrapper() {
// TODO: It's too late to release gpu buffer, gpu was released
if (m_vertex_buffer) {
SDL_ReleaseGPUBuffer(m_gpu, m_vertex_buffer);
m_vertex_buffer = nullptr;
}
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.buffer.ixx around lines 59 to 62, the destructor
calls SDL_ReleaseGPUBuffer(m_gpu, m_vertex_buffer) without checking whether
m_vertex_buffer (or m_gpu) is nullptr which can crash if construction failed;
add a guard that checks if m_vertex_buffer is non-nullptr (and optionally m_gpu
is valid) before calling SDL_ReleaseGPUBuffer, then set m_vertex_buffer =
nullptr after releasing.

if (m_transfer_buffer) {
SDL_ReleaseGPUTransferBuffer(m_gpu, m_transfer_buffer);
m_transfer_buffer = nullptr;
m_transfer_buffer_size = 0;
}
}
};
}