GL Edit #6
GL Edit #6
Conversation
WalkthroughRefactors procedural SDL/SDL_GPU app into a class-based Changes
Sequence DiagramsequenceDiagram
autonumber
participant Host
participant UserApp
participant SDL_GPU as SDL_GPU
participant Shaderc
participant ImGui
Host->>UserApp: create_app()
Host->>UserApp: init(argc, argv)
UserApp->>SDL_GPU: create window & device
UserApp->>Shaderc: compile GLSL -> SPIR‑V
Shaderc-->>UserApp: SPIR‑V blobs
UserApp->>SDL_GPU: create shader modules & graphics pipeline
UserApp->>SDL_GPU: create/upload vertex buffer
UserApp->>ImGui: initialize context & backends
loop main loop
Host->>UserApp: iterate()
UserApp->>SDL_GPU: acquire swap texture / begin frame
opt render available
UserApp->>SDL_GPU: bind pipeline & vertex buffer
UserApp->>SDL_GPU: draw vertices
UserApp->>ImGui: new frame / render UI
UserApp->>SDL_GPU: submit commands / present
end
Host->>UserApp: event(SDL_Event)
UserApp->>ImGui: forward/process event
end
Host->>UserApp: quit(result)
UserApp->>SDL_GPU: destroy pipeline, shaders, buffers, device, window
UserApp->>ImGui: shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Here's the code health analysis summary for commits Analysis Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
main.cpp
Outdated
| window = SDL_CreateWindow("Hello, Triangle!", 960, 540, SDL_WINDOW_RESIZABLE); | ||
|
|
||
| // create the device | ||
| device = SDL_CreateGPUDevice(SDL_GPU_SHADERFORMAT_SPIRV, true, NULL); | ||
| SDL_ClaimWindowForGPUDevice(device, window); | ||
|
|
||
| shaderc::Compiler compiler{}; |
There was a problem hiding this comment.
Handle null SDL window/device results before use.
If SDL_CreateWindow or SDL_CreateGPUDevice fails, the subsequent calls dereference null handles and crash the app during init. Bail out early and clean up when either call returns nullptr.
window = SDL_CreateWindow("Hello, Triangle!", 960, 540, SDL_WINDOW_RESIZABLE);
+ if (!window)
+ {
+ std::cerr << "SDL_CreateWindow failed: " << SDL_GetError() << std::endl;
+ return SDL_APP_FAILURE;
+ }
// create the device
device = SDL_CreateGPUDevice(SDL_GPU_SHADERFORMAT_SPIRV, true, NULL);
+ if (!device)
+ {
+ std::cerr << "SDL_CreateGPUDevice failed: " << SDL_GetError() << std::endl;
+ SDL_DestroyWindow(window);
+ window = nullptr;
+ return SDL_APP_FAILURE;
+ }📝 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.
| window = SDL_CreateWindow("Hello, Triangle!", 960, 540, SDL_WINDOW_RESIZABLE); | |
| // create the device | |
| device = SDL_CreateGPUDevice(SDL_GPU_SHADERFORMAT_SPIRV, true, NULL); | |
| SDL_ClaimWindowForGPUDevice(device, window); | |
| shaderc::Compiler compiler{}; | |
| window = SDL_CreateWindow("Hello, Triangle!", 960, 540, SDL_WINDOW_RESIZABLE); | |
| if (!window) | |
| { | |
| std::cerr << "SDL_CreateWindow failed: " << SDL_GetError() << std::endl; | |
| return SDL_APP_FAILURE; | |
| } | |
| // create the device | |
| device = SDL_CreateGPUDevice(SDL_GPU_SHADERFORMAT_SPIRV, true, NULL); | |
| if (!device) | |
| { | |
| std::cerr << "SDL_CreateGPUDevice failed: " << SDL_GetError() << std::endl; | |
| SDL_DestroyWindow(window); | |
| window = nullptr; | |
| return SDL_APP_FAILURE; | |
| } | |
| SDL_ClaimWindowForGPUDevice(device, window); | |
| shaderc::Compiler compiler{}; |
🤖 Prompt for AI Agents
In main.cpp around lines 62 to 68, the code calls SDL_CreateWindow and
SDL_CreateGPUDevice but does not check for nullptr; if either fails subsequent
calls (SDL_ClaimWindowForGPUDevice, later use) will dereference null handles and
crash. Fix by checking the return values immediately: if window is nullptr log
an error, call SDL_Quit and return a non‑zero exit; if device is nullptr log an
error, destroy the window (if created), call SDL_Quit and return non‑zero; only
call SDL_ClaimWindowForGPUDevice and proceed with shader/compiler setup when
both handles are valid. Also ensure any partially created resources are cleaned
up in the correct order before bailing out.
| if (result.GetCompilationStatus() != shaderc_compilation_status_success) | ||
| { | ||
| std::cerr << "[shaderc] compile error in " << "test.glsl" << ":\n" << result.GetErrorMessage() << std::endl; | ||
| } | ||
|
|
||
| // load the vertex shader code | ||
| std::vector<uint32_t> vertexCode{result.cbegin(), result.cend()}; | ||
|
|
||
| // create the vertex shader | ||
| SDL_GPUShaderCreateInfo vertexInfo{}; | ||
| vertexInfo.code = (Uint8*)vertexCode.data(); | ||
| vertexInfo.code_size = vertexCode.size() * 4; | ||
| vertexInfo.entrypoint = "main"; | ||
| vertexInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; | ||
| vertexInfo.stage = SDL_GPU_SHADERSTAGE_VERTEX; | ||
| vertexInfo.num_samplers = 0; | ||
| vertexInfo.num_storage_buffers = 0; | ||
| vertexInfo.num_storage_textures = 0; | ||
| vertexInfo.num_uniform_buffers = 0; | ||
|
|
||
| SDL_GPUShader* vertexShader = SDL_CreateGPUShader(device, &vertexInfo); | ||
|
|
||
| result = compiler.CompileGlslToSpv(fragment_source, shaderc_glsl_fragment_shader, "test.frag", options); | ||
|
|
||
| if (result.GetCompilationStatus() != shaderc_compilation_status_success) | ||
| { | ||
| std::cerr << "[shaderc] compile error in " << "test.frag" << ":\n" << result.GetErrorMessage() << std::endl; | ||
| } | ||
|
|
||
| // load the fragment shader code | ||
| std::vector<uint32_t> fragmentCode{result.cbegin(), result.cend()}; | ||
|
|
||
| // create the fragment shader |
There was a problem hiding this comment.
Abort init when shader compilation fails.
When shaderc reports an error you still proceed to build SDL_GPUShaderCreateInfo from empty bytecode, which yields null shaders/pipeline and crashes once SDL_BindGPUGraphicsPipeline or draw calls run. Treat the failure as fatal and return from init.
if (result.GetCompilationStatus() != shaderc_compilation_status_success)
{
std::cerr << "[shaderc] compile error in " << "test.glsl" << ":\n" << result.GetErrorMessage() << std::endl;
+ return SDL_APP_FAILURE;
}
@@
if (result.GetCompilationStatus() != shaderc_compilation_status_success)
{
std::cerr << "[shaderc] compile error in " << "test.frag" << ":\n" << result.GetErrorMessage() << std::endl;
+ return SDL_APP_FAILURE;
}🤖 Prompt for AI Agents
In main.cpp around lines 73 to 105, the code continues building
SDL_GPUShaderCreateInfo and creating shaders even when shaderc reports
compilation failures, which leads to null shaders and later crashes; modify the
init path so that immediately after each CompileGlslToSpv call you check
result.GetCompilationStatus(), log the error, and treat it as fatal by returning
from init (or throwing) before constructing or creating any shader; do this for
both the vertex and fragment compilation and ensure any partially created GPU
objects are cleaned up before returning.
main.cpp
Outdated
| // create the pipeline | ||
| graphicsPipeline = SDL_CreateGPUGraphicsPipeline(device, &pipelineInfo); | ||
|
|
||
| // we don't need to store the shaders after creating the pipeline | ||
| SDL_ReleaseGPUShader(device, vertexShader); | ||
| SDL_ReleaseGPUShader(device, fragmentShader); | ||
|
|
There was a problem hiding this comment.
Check SDL_CreateGPUGraphicsPipeline before using it.
A pipeline creation failure leaves graphicsPipeline null, but iterate() will still bind it, triggering an immediate crash. Verify the return value, release the temporary shaders, and abort init if creation fails.
// create the pipeline
graphicsPipeline = SDL_CreateGPUGraphicsPipeline(device, &pipelineInfo);
+ if (!graphicsPipeline)
+ {
+ SDL_ReleaseGPUShader(device, vertexShader);
+ SDL_ReleaseGPUShader(device, fragmentShader);
+ std::cerr << "SDL_CreateGPUGraphicsPipeline failed: " << SDL_GetError() << std::endl;
+ return SDL_APP_FAILURE;
+ }
// we don't need to store the shaders after creating the pipeline
SDL_ReleaseGPUShader(device, vertexShader);🤖 Prompt for AI Agents
In main.cpp around lines 168 to 174, SDL_CreateGPUGraphicsPipeline can return
null and the code currently continues, causing a crash when iterate() later
binds a null pipeline; after calling SDL_CreateGPUGraphicsPipeline check whether
graphicsPipeline is non-null, always release the temporary shaders with
SDL_ReleaseGPUShader regardless of success, and if creation failed abort
initialization (return an error code or set an init-failed flag and exit early)
so iterate() is never called with a null pipeline.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
main.cpp (3)
74-79: Guard window/device creation failures.
SDL_CreateWindow,SDL_CreateGPUDevice, andSDL_ClaimWindowForGPUDevicecan all return failure. Dereferencing those null handles immediately crashes (same blocker previously noted). Bail out and clean up right away when any of these calls fail.window = SDL_CreateWindow("Hello, Triangle!", 960, 540, SDL_WINDOW_RESIZABLE); + if (!window) + { + std::cerr << "SDL_CreateWindow failed: " << SDL_GetError() << std::endl; + return SDL_APP_FAILURE; + } // create the device device = SDL_CreateGPUDevice(SDL_GPU_SHADERFORMAT_SPIRV, true, NULL); - SDL_ClaimWindowForGPUDevice(device, window); + if (!device) + { + std::cerr << "SDL_CreateGPUDevice failed: " << SDL_GetError() << std::endl; + SDL_DestroyWindow(window); + window = nullptr; + return SDL_APP_FAILURE; + } + if (!SDL_ClaimWindowForGPUDevice(device, window)) + { + std::cerr << "SDL_ClaimWindowForGPUDevice failed: " << SDL_GetError() << std::endl; + SDL_DestroyGPUDevice(device); + device = nullptr; + SDL_DestroyWindow(window); + window = nullptr; + return SDL_APP_FAILURE; + }
174-174: Verify pipeline creation before use.
SDL_CreateGPUGraphicsPipelinemay return nullptr;iterate()then binds that null pipeline, causing an immediate crash. Release the temporary shaders, tear down, and fail init when pipeline creation fails.// create the pipeline graphicsPipeline = SDL_CreateGPUGraphicsPipeline(device, &pipelineInfo); + if (!graphicsPipeline) + { + std::cerr << "SDL_CreateGPUGraphicsPipeline failed: " << SDL_GetError() << std::endl; + SDL_ReleaseGPUShader(device, vertexShader); + vertexShader = nullptr; + SDL_ReleaseGPUShader(device, fragmentShader); + fragmentShader = nullptr; + SDL_DestroyGPUDevice(device); + device = nullptr; + SDL_DestroyWindow(window); + window = nullptr; + return SDL_APP_FAILURE; + }
83-110: Treat shader compilation failures as fatal.After shaderc signals an error you still feed empty bytecode into
SDL_CreateGPUShader, which propagates garbage down to pipeline creation and crashes. Abort init (and release the partially created device/window) as soon as compilation fails.if (result.GetCompilationStatus() != shaderc_compilation_status_success) { std::cerr << "[shaderc] compile error in " << "test.glsl" << ":\n" << result.GetErrorMessage() << std::endl; + SDL_DestroyGPUDevice(device); + device = nullptr; + SDL_DestroyWindow(window); + window = nullptr; + return SDL_APP_FAILURE; } @@ if (result.GetCompilationStatus() != shaderc_compilation_status_success) { std::cerr << "[shaderc] compile error in " << "test.frag" << ":\n" << result.GetErrorMessage() << std::endl; + SDL_DestroyGPUDevice(device); + device = nullptr; + SDL_DestroyWindow(window); + window = nullptr; + return SDL_APP_FAILURE; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(3 hunks)
⏰ 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). (20)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
| vertexShader = SDL_CreateGPUShader(device, &vertexInfo); | ||
|
|
||
| result = compiler.CompileGlslToSpv(fragment_source, shaderc_glsl_fragment_shader, "test.frag", options); | ||
|
|
||
| if (result.GetCompilationStatus() != shaderc_compilation_status_success) | ||
| { | ||
| std::cerr << "[shaderc] compile error in " << "test.frag" << ":\n" << result.GetErrorMessage() << std::endl; | ||
| } | ||
|
|
||
| // load the fragment shader code | ||
| std::vector<uint32_t> fragmentCode{result.cbegin(), result.cend()}; | ||
|
|
||
| // create the fragment shader | ||
| SDL_GPUShaderCreateInfo fragmentInfo{}; | ||
| fragmentInfo.code = (Uint8*)fragmentCode.data(); | ||
| fragmentInfo.code_size = fragmentCode.size() * 4; | ||
| fragmentInfo.entrypoint = "main"; | ||
| fragmentInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; | ||
| fragmentInfo.stage = SDL_GPU_SHADERSTAGE_FRAGMENT; | ||
| fragmentInfo.num_samplers = 0; | ||
| fragmentInfo.num_storage_buffers = 0; | ||
| fragmentInfo.num_storage_textures = 0; | ||
| fragmentInfo.num_uniform_buffers = 0; | ||
|
|
||
| fragmentShader = SDL_CreateGPUShader(device, &fragmentInfo); | ||
|
|
There was a problem hiding this comment.
Abort when shader module creation fails.
SDL_CreateGPUShader can return nullptr. Continuing would hand a null shader to the pipeline and later release calls. Check both creations, clean up, and stop init when they fail.
vertexShader = SDL_CreateGPUShader(device, &vertexInfo);
+ if (!vertexShader)
+ {
+ std::cerr << "SDL_CreateGPUShader (vertex) failed: " << SDL_GetError() << std::endl;
+ SDL_DestroyGPUDevice(device);
+ device = nullptr;
+ SDL_DestroyWindow(window);
+ window = nullptr;
+ return SDL_APP_FAILURE;
+ }
@@
fragmentShader = SDL_CreateGPUShader(device, &fragmentInfo);
+ if (!fragmentShader)
+ {
+ std::cerr << "SDL_CreateGPUShader (fragment) failed: " << SDL_GetError() << std::endl;
+ SDL_ReleaseGPUShader(device, vertexShader);
+ vertexShader = nullptr;
+ SDL_DestroyGPUDevice(device);
+ device = nullptr;
+ SDL_DestroyWindow(window);
+ window = nullptr;
+ return SDL_APP_FAILURE;
+ }🤖 Prompt for AI Agents
In main.cpp around lines 103 to 128, the code assumes SDL_CreateGPUShader always
succeeds; if it returns nullptr the program will continue with a null shader and
later crash on use/free. After each SDL_CreateGPUShader call check for nullptr,
log a descriptive error, clean up any resources that were successfully created
(destroy the other shader if created, free/clear any temporary shader code
buffers), set shader pointers to nullptr, and abort initialization (return an
error/false or exit) rather than proceeding; ensure both vertex and fragment
creation paths handle failure symmetrically so no dangling resources are left.
main.cpp
Outdated
| if (ImGui::InputTextMultiline("code editor", &vertex_source, ImVec2(0, 0), | ||
| ImGuiInputTextFlags_AllowTabInput)) | ||
| { | ||
| auto result = | ||
| compiler.CompileGlslToSpv(vertex_source, shaderc_glsl_vertex_shader, "test.glsl", options); | ||
|
|
||
| if (result.GetCompilationStatus() != shaderc_compilation_status_success) | ||
| { | ||
| std::cerr << "[shaderc] compile error in " << "test.glsl" << ":\n" | ||
| << result.GetErrorMessage() << std::endl; | ||
| } | ||
| else | ||
| { | ||
| // load the vertex shader code | ||
| std::vector<uint32_t> vertexCode{result.cbegin(), result.cend()}; | ||
|
|
||
| // create the vertex shader | ||
| SDL_GPUShaderCreateInfo vertexInfo{}; | ||
| vertexInfo.code = (Uint8*)vertexCode.data(); | ||
| vertexInfo.code_size = vertexCode.size() * 4; | ||
| vertexInfo.entrypoint = "main"; | ||
| vertexInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; | ||
| vertexInfo.stage = SDL_GPU_SHADERSTAGE_VERTEX; | ||
| vertexInfo.num_samplers = 0; | ||
| vertexInfo.num_storage_buffers = 0; | ||
| vertexInfo.num_storage_textures = 0; | ||
| vertexInfo.num_uniform_buffers = 0; | ||
|
|
||
|
|
||
| SDL_ReleaseGPUShader(device, vertexShader); | ||
| vertexShader = SDL_CreateGPUShader(device, &vertexInfo); | ||
|
|
||
| pipelineInfo.vertex_shader = vertexShader; | ||
| SDL_ReleaseGPUGraphicsPipeline(device, graphicsPipeline); | ||
| graphicsPipeline = SDL_CreateGPUGraphicsPipeline(device, &pipelineInfo); | ||
| } |
There was a problem hiding this comment.
Don’t drop the live pipeline before the replacement succeeds.
During hot-reload you destroy the current shader/pipeline before confirming the new ones were created. If either creation fails you end up with graphicsPipeline == nullptr, and the very next SDL_BindGPUGraphicsPipeline blows up. Create the replacements first, validate them, then swap and release the old handles only after success.
- SDL_ReleaseGPUShader(device, vertexShader);
- vertexShader = SDL_CreateGPUShader(device, &vertexInfo);
-
- pipelineInfo.vertex_shader = vertexShader;
- SDL_ReleaseGPUGraphicsPipeline(device, graphicsPipeline);
- graphicsPipeline = SDL_CreateGPUGraphicsPipeline(device, &pipelineInfo);
+ SDL_GPUShader* newVertexShader = SDL_CreateGPUShader(device, &vertexInfo);
+ if (!newVertexShader)
+ {
+ std::cerr << "SDL_CreateGPUShader (vertex hot-reload) failed: " << SDL_GetError() << std::endl;
+ return SDL_APP_CONTINUE;
+ }
+
+ SDL_GPUShader* oldVertexShader = vertexShader;
+ vertexShader = newVertexShader;
+ pipelineInfo.vertex_shader = vertexShader;
+
+ SDL_GPUGraphicsPipeline* newPipeline = SDL_CreateGPUGraphicsPipeline(device, &pipelineInfo);
+ if (!newPipeline)
+ {
+ std::cerr << "SDL_CreateGPUGraphicsPipeline (hot-reload) failed: " << SDL_GetError() << std::endl;
+ SDL_ReleaseGPUShader(device, vertexShader);
+ vertexShader = oldVertexShader;
+ return SDL_APP_CONTINUE;
+ }
+
+ SDL_GPUGraphicsPipeline* oldPipeline = graphicsPipeline;
+ graphicsPipeline = newPipeline;
+ SDL_ReleaseGPUGraphicsPipeline(device, oldPipeline);
+ SDL_ReleaseGPUShader(device, oldVertexShader);🤖 Prompt for AI Agents
In main.cpp around lines 237 to 272, you currently release the live vertexShader
and graphicsPipeline before creating the replacements which can leave
graphicsPipeline null on failure; instead, compile and create the new SDL GPU
shader and graphics pipeline into temporary local handles, check they are
non-null (and validate creation succeeded) and only then release the old
vertexShader and graphicsPipeline and assign the temp handles into vertexShader
and graphicsPipeline; if creation of either temp handle fails, release any temp
handles that were created and keep the existing live pipeline/shader unchanged,
and log the error.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
main.cpp (5)
74-78: Handle null SDL window/device results before use.If
SDL_CreateWindoworSDL_CreateGPUDevicefails, subsequent calls will dereference null handles and crash. Check both return values, log errors, clean up, and returnSDL_APP_FAILUREbefore proceeding.
81-103: Abort init when shader compilation or creation fails.Compilation errors are logged but execution continues with potentially empty bytecode, and
SDL_CreateGPUShaderis not null-checked. Either failure will cause crashes during pipeline creation or rendering.
105-127: Abort init when fragment shader compilation or creation fails.Same issue as vertex shader: compilation errors don't halt execution and shader creation isn't validated. This leads to null shaders being passed to pipeline creation.
174-174: Check pipeline creation before using it.
SDL_CreateGPUGraphicsPipelinecan return null. Without validation,iterate()will later bind a null pipeline and crash.
272-277: Don't drop the live pipeline before the replacement succeeds.You release
vertexShaderandgraphicsPipelinebefore validating the new creations. If eitherSDL_CreateGPUShaderorSDL_CreateGPUGraphicsPipelinefails,graphicsPipelinebecomes null and the nextSDL_BindGPUGraphicsPipelinecrashes.Create temporary handles, validate them, then swap and release the old ones only on success.
🧹 Nitpick comments (3)
main.cpp (3)
238-282: Consider adding fragment shader hot-reload.Only the vertex shader has hot-reload support in the editor. Adding the same functionality for the fragment shader would provide feature parity and allow fuller experimentation with shaders at runtime.
349-369: Consider guarding resource cleanup against partial init failures.If
init()fails midway, some pointers remain null. While SDL's release functions generally tolerate null, it's safer to check before cleanup or wrap resources in RAII types.
88-103: Extract common shader creation logic into a helper.The vertex shader compilation and creation code is duplicated between
init()and the hot-reload path. Consider extracting a helper method likeSDL_GPUShader* compileAndCreateShader(const std::string& source, shaderc_shader_kind kind, const char* name)to reduce duplication and centralize error handling.Also applies to: 256-273
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(3 hunks)
⏰ 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). (8)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, clang)
| ImGui::StyleColorsDark(); | ||
| // ImGui::StyleColorsLight(); | ||
|
|
||
| float main_scale = SDL_GetDisplayContentScale(SDL_GetPrimaryDisplay()); |
There was a problem hiding this comment.
Validate display and content scale.
SDL_GetPrimaryDisplay() may return null, and SDL_GetDisplayContentScale() could return 0 or invalid values, leading to unusable UI scaling. Check both return values and provide a sensible fallback (e.g., 1.0f).
🤖 Prompt for AI Agents
In main.cpp around line 196, the code calls SDL_GetPrimaryDisplay() and
SDL_GetDisplayContentScale() without validating results; modify it to first
check the display index is valid (non-negative), then retrieve the content scale
and validate it (finite and > 0); if either check fails, set main_scale to a
sensible fallback like 1.0f and log a warning. Use std::isfinite (or equivalent)
to detect NaN/inf and ensure main_scale is always a positive finite value.
| if (ImGui::InputTextMultiline("code editor", &vertex_source, size, ImGuiInputTextFlags_AllowTabInput)) | ||
| { | ||
| auto result = | ||
| compiler.CompileGlslToSpv(vertex_source, shaderc_glsl_vertex_shader, "test.glsl", options); | ||
|
|
||
| if (result.GetCompilationStatus() != shaderc_compilation_status_success) | ||
| { | ||
| std::cerr << "[shaderc] compile error in " << "test.glsl" << ":\n" | ||
| << result.GetErrorMessage() << std::endl; | ||
| } | ||
| else | ||
| { | ||
| // load the vertex shader code | ||
| std::vector<uint32_t> vertexCode{result.cbegin(), result.cend()}; | ||
|
|
||
| // create the vertex shader | ||
| SDL_GPUShaderCreateInfo vertexInfo{}; | ||
| vertexInfo.code = (Uint8*)vertexCode.data(); | ||
| vertexInfo.code_size = vertexCode.size() * 4; | ||
| vertexInfo.entrypoint = "main"; | ||
| vertexInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; | ||
| vertexInfo.stage = SDL_GPU_SHADERSTAGE_VERTEX; | ||
| vertexInfo.num_samplers = 0; | ||
| vertexInfo.num_storage_buffers = 0; | ||
| vertexInfo.num_storage_textures = 0; | ||
| vertexInfo.num_uniform_buffers = 0; | ||
|
|
||
|
|
||
| SDL_ReleaseGPUShader(device, vertexShader); | ||
| vertexShader = SDL_CreateGPUShader(device, &vertexInfo); | ||
|
|
||
| pipelineInfo.vertex_shader = vertexShader; | ||
| SDL_ReleaseGPUGraphicsPipeline(device, graphicsPipeline); | ||
| graphicsPipeline = SDL_CreateGPUGraphicsPipeline(device, &pipelineInfo); | ||
| } |
There was a problem hiding this comment.
Shader recompilation on every keystroke is inefficient.
InputTextMultiline triggers the callback on every character typed, causing full GLSL→SPIR-V recompilation and pipeline recreation for each keystroke. This blocks the render thread and causes frame drops during editing.
Consider debouncing (compile only after a pause in typing) or adding an explicit "Apply" button.
🤖 Prompt for AI Agents
In main.cpp around lines 244–278, the code recompiles GLSL and recreates the
pipeline directly inside the ImGui InputTextMultiline path on every keystroke;
change this to debounce or use an explicit "Apply" action so
compilation/pipeline recreation only happens after editing stops (e.g.,
start/refresh a timestamp or set a dirty flag on text change and only run
compile when now - last_edit >= 300–500ms or when the user clicks Apply). Move
the heavy work out of the immediate text callback into the main loop (or a
worker thread) so you don't block the render thread: on debounce expiry or
Apply, perform compilation (optionally on a background thread), then on success
schedule the SDL_Release/Create of shaders and pipelines back on the main thread
with proper resource synchronization and error handling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
main.cpp (9)
75-79: Duplicate: Handle null SDL window/device results before use.This critical issue was previously flagged. If either
SDL_CreateWindoworSDL_CreateGPUDevicereturnsnullptr, subsequent calls will crash.
84-87: Duplicate: Abort init when shader compilation fails.This critical issue was previously flagged. When shaderc reports an error, the code continues to create shaders from empty/invalid bytecode, leading to null shaders and crashes.
104-104: Duplicate: Abort when shader module creation fails.This major issue was previously flagged.
SDL_CreateGPUShadercan returnnullptr, and continuing would pass null to the pipeline and later calls.
108-111: Duplicate: Abort init when fragment shader compilation fails.This critical issue was previously flagged. Shader compilation failures should be treated as fatal errors during initialization.
128-128: Duplicate: Abort when fragment shader module creation fails.This major issue was previously flagged. Check the return value of
SDL_CreateGPUShaderand abort init if it returnsnullptr.
175-175: Duplicate: CheckSDL_CreateGPUGraphicsPipelinebefore using it.This critical issue was previously flagged. Pipeline creation can fail, leaving
graphicsPipelinenull and causing crashes initerate().
197-197: Duplicate: Validate display and content scale.This major issue was previously flagged.
SDL_GetPrimaryDisplay()may return null, andSDL_GetDisplayContentScale()could return invalid values.
245-280: Duplicate: Shader recompilation on every keystroke is inefficient.This major issue was previously flagged. The callback triggers full GLSL→SPIR-V recompilation and pipeline recreation for each keystroke, blocking the render thread.
Consider debouncing or adding an explicit "Apply" button.
273-278: Duplicate: Don't drop the live pipeline before the replacement succeeds.This critical issue was previously flagged. Lines 273-278 release the current shader and pipeline before validating the new ones, leaving
graphicsPipelinenull if creation fails. The nextSDL_BindGPUGraphicsPipelinecall will crash.Create the replacements first, validate them, then swap and release the old handles only after success.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(3 hunks)
⏰ 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). (20)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, clang)
🔇 Additional comments (5)
main.cpp (5)
1-21: LGTM: Clean includes and vertex structure.The includes are appropriate for the SDL3/GPU/ImGui/shaderc stack, and the
Vertexstruct is a straightforward POD with a convenientposition()accessor.
23-71: LGTM: Well-structured class members.The member layout is logical, grouping SDL resources, vertex data, shader sources, and pipeline configuration together. Default initialization ensures pointers start as
nullptr.
288-336: LGTM: Render loop with proper swapchain validation.The command buffer acquisition, swapchain texture handling (with proper null check at line 297), render pass setup, and submission are all correctly implemented.
338-348: LGTM: Clean event handling.The event method correctly forwards to ImGui and handles window close requests.
373-373: LGTM: Factory function.The
create_app()function correctly instantiates and returnsUserApp.
| // create the vertex buffer | ||
| SDL_GPUBufferCreateInfo bufferInfo{}; | ||
| bufferInfo.size = sizeof(vertices); | ||
| bufferInfo.usage = SDL_GPU_BUFFERUSAGE_VERTEX; | ||
| vertexBuffer.emplace(device, &bufferInfo); | ||
|
|
||
| vertexBuffer->upload(&vertices, sizeof(vertices), 0); | ||
|
|
There was a problem hiding this comment.
Check buffer creation and handle failures.
The BufferWrapper construction (line 181) can fail, but line 183 immediately dereferences vertexBuffer without validation. Additionally, if buffer creation fails after shaders and pipeline are created, those resources leak because init returns SDL_APP_CONTINUE regardless.
Check the vertexBuffer state after emplace (or handle construction exceptions), and ensure proper cleanup of already-created resources before returning failure.
🤖 Prompt for AI Agents
In main.cpp around lines 177 to 184, the BufferWrapper construction via
vertexBuffer.emplace(...) may fail but code immediately dereferences
vertexBuffer and proceeds; also, on any failure after creating shaders/pipeline
those resources are leaked because init still returns SDL_APP_CONTINUE. Fix by
validating vertexBuffer after emplace (or catch exceptions from emplacement),
only call vertexBuffer->upload(...) when the buffer exists and is valid, and on
any failure clean up previously created resources (destroy vertex/pixel shaders,
pipeline, etc.) and return an appropriate failure code instead of
SDL_APP_CONTINUE.
| ImGui_ImplSDL3_InitForSDLGPU(window); | ||
| ImGui_ImplSDLGPU3_InitInfo init_info = {}; | ||
| init_info.Device = device; | ||
| init_info.ColorTargetFormat = SDL_GetGPUSwapchainTextureFormat(device, window); | ||
| init_info.MSAASamples = SDL_GPU_SAMPLECOUNT_1; // Only used in multi-viewports mode. | ||
| init_info.SwapchainComposition = SDL_GPU_SWAPCHAINCOMPOSITION_SDR; // Only used in multi-viewports mode. | ||
| init_info.PresentMode = SDL_GPU_PRESENTMODE_VSYNC; | ||
| ImGui_ImplSDLGPU3_Init(&init_info); |
There was a problem hiding this comment.
Validate ImGui backend initialization.
Both ImGui_ImplSDL3_InitForSDLGPU and ImGui_ImplSDLGPU3_Init can fail (typically returning false). If they do, quit() will attempt to shut down uninitialized backends, which may crash or leak resources.
Check both return values and abort init if either fails.
🤖 Prompt for AI Agents
In main.cpp around lines 208 to 215, the ImGui/SDL GPU initialization calls are
not checked: both ImGui_ImplSDL3_InitForSDLGPU(window) and
ImGui_ImplSDLGPU3_Init(&init_info) can fail and must be validated. Modify the
code to check each function's return value, and if either returns false, log an
error, abort the remainder of initialization (call quit() or equivalent) without
attempting to shut down backends that were never successfully initialized, and
ensure any previously successfully initialized backend is properly shut down
before exiting; fail fast on init error.
| virtual void quit(SDL_AppResult result) override | ||
| { | ||
| ImGui_ImplSDL3_Shutdown(); | ||
| ImGui_ImplSDLGPU3_Shutdown(); | ||
| ImGui::DestroyContext(); | ||
| // release buffers | ||
| vertexBuffer = std::nullopt; | ||
|
|
||
| virtual void quit(SDL_AppResult result) override { | ||
| ImGui_ImplSDL3_Shutdown(); | ||
| ImGui_ImplSDLGPU3_Shutdown(); | ||
| ImGui::DestroyContext(); | ||
| // release buffers | ||
| vertexBuffer = std::nullopt; | ||
| // Release the shader | ||
| SDL_ReleaseGPUShader(device, vertexShader); | ||
| SDL_ReleaseGPUShader(device, fragmentShader); | ||
|
|
||
| // release the pipeline | ||
| SDL_ReleaseGPUGraphicsPipeline(device, graphicsPipeline); | ||
| // release the pipeline | ||
| SDL_ReleaseGPUGraphicsPipeline(device, graphicsPipeline); | ||
|
|
||
| // destroy the GPU device | ||
| SDL_DestroyGPUDevice(device); | ||
| // destroy the GPU device | ||
| SDL_DestroyGPUDevice(device); | ||
|
|
||
| // destroy the window | ||
| SDL_DestroyWindow(window); | ||
| } | ||
| // destroy the window | ||
| SDL_DestroyWindow(window); | ||
| } |
There was a problem hiding this comment.
Guard cleanup against partially-initialized state.
If init() fails partway through (e.g., shader compilation error, device creation failure), quit() will still execute and attempt to shut down ImGui and release potentially null resources. While SDL release functions may tolerate null pointers, ImGui shutdown functions do not guarantee this.
Consider adding null/validity checks before each cleanup operation, or maintain an initialization state flag to track what was successfully created.
🤖 Prompt for AI Agents
In main.cpp around lines 350 to 370, quit() unconditionally calls ImGui
shutdowns and SDL release/destroy functions which can be unsafe if init() failed
partway; guard each cleanup by checking whether the corresponding
subsystem/object was successfully created (e.g., track an initialization state
flag or individual booleans for ImGui context, GPU device, shaders, pipeline,
window, and vertexBuffer), only call
ImGui_ImplSDL3_Shutdown()/ImGui_ImplSDLGPU3_Shutdown()/ImGui::DestroyContext()
if ImGui was initialized, only release shaders/pipeline/device if those handles
are valid/non-null/engaged, and only destroy the window if it was created;
update quit() to conditionally perform each shutdown/release based on those
checks (or a single initialized flag) to avoid calling teardown on
partially-initialized resources.
Edit gl in imgui
Summary by CodeRabbit
New Features
Refactor