Gl reflection #36
Gl reflection #36
Conversation
* 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>
WalkthroughAdds a C++20 module-based GLSL reflection library (glsl_reflector) using glslang, integrates GoogleTest, and replaces manual vertex-format plumbing with reflection-driven vertex attribute discovery and propagation through the SDL wrapper and application. Changes
Sequence Diagram(s)sequenceDiagram
%% Styling note: minor color accents via labels only
participant App as Application
participant RP as RenderProcedural
participant Reflect as glsl_reflector
participant VL as VertexLayout
participant SDL as SDL_GPU
App->>RP: provide/modify vertex shader source
RP->>Reflect: reflect_vertex(source)
Reflect->>Reflect: parse & link via glslang
Reflect-->>RP: VertexReflection(inputs...)
RP->>VL: set_vertex_reflection(reflection)
VL->>VL: map inputs -> SDL formats (to_sdl_format)
VL->>SDL: apply vertex attribute descriptions
App->>App: use get_vertex_reflection().inputs to build UI & strides
App->>SDL: upload/refresh vertex buffer when needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
glsl_reflector/modules/glsl_reflector.type.ixx (2)
12-16: Consider expanding BasicType to support additional shader input types.The enum currently only defines
NONEandFLOAT. Common vertex attributes also use integer types (int,uint), and future shaders may requiredoubleorbool. Consider adding these types now to avoid breaking changes later.enum class BasicType : std::uint8_t { NONE, - FLOAT + FLOAT, + INT, + UINT, + DOUBLE, + BOOL };
18-24: Consider using unsigned type for vector_size and adding validation.
vector_sizeisintbut represents a count that should be positive. Valid GLSL vector sizes are typically 1, 2, 3, or 4. Consider usingstd::uint8_tfor the type and documenting the valid range, or adding validation in the reflection code.struct VertexInfo { std::uint32_t location{}; std::string name{}; BasicType basic_type{}; - int vector_size{}; + std::uint8_t vector_size{}; // Valid values: 1, 2, 3, or 4 };glsl_reflector/test/test.cpp (2)
9-12: Remove unused glslang includes.Lines 9-12 include glslang headers that are not used in the test file. These includes suggest either incomplete test implementation or leftover imports from development.
-#include "glslang/Include/intermediate.h" -#include "glslang/MachineIndependent/localintermediate.h" -#include "glslang/Public/ResourceLimits.h" -#include "glslang/Public/ShaderLang.h" - #include <gtest/gtest.h>
20-45: Expand test coverage for reflection functionality.The current test only validates the happy path with
vec3float inputs. Consider adding tests for:
- Error cases: malformed GLSL, parse failures, empty source
- Different vector sizes:
vec2,vec4, scalarfloat- Other basic types when supported:
int,uint, etc.- Edge cases: no inputs, many inputs, duplicate locations
Do you want me to generate additional test cases to improve coverage?
sdl_wrapper/sdl_wrapper.render_procedural.ixx (1)
69-87: Consider explicit void return type for clarity.The method uses
autowith no return statement. While valid, an explicitvoidreturn type would improve readability and make the intent clear.- auto set_vertex_reflection(const VertexReflection& vertex_attributes) + void set_vertex_reflection(const VertexReflection& vertex_attributes)sdl_wrapper/sdl_wrapper.vertex_layout.ixx (1)
83-102: Extendto_sdl_formatmapping or document current limitationsRight now only
BasicType::FLOATwith vector sizes 3 and 4 map to valid SDL formats; everything else becomesSDL_GPU_VERTEXELEMENTFORMAT_INVALID. That meansvec2or scalar floats, and any future basic types, will silently become invalid attributes and produce zero size viaget_size. Consider at least handlingvector_size == 2and (if needed) 1, and/or asserting/logging when an unsupported combination is encountered instead of quietly returningINVALID.glsl_reflector/modules/glsl_reflector.ixx (1)
16-21: MakeGlslangProcessGuardstatic to ensure glslang initializes once per process
GlslangProcessGuardis instantiated as a local variable inreflect_vertex()(line 36), causingglslang::InitializeProcess()andglslang::FinalizeProcess()to be called on every invocation. glslang's documentation states these functions should be called once per process at initialization and shutdown, not per shader compilation.Move
GlslangProcessGuardto static or global scope to initialize glslang once at process startup and finalize once at teardown, eliminating repeated overhead and aligning with glslang's intended usage pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)glsl_reflector/CMakeLists.txt(1 hunks)glsl_reflector/modules/glsl_reflector.ixx(1 hunks)glsl_reflector/modules/glsl_reflector.type.ixx(1 hunks)glsl_reflector/test/test.cpp(1 hunks)main.cpp(3 hunks)sdl_wrapper/CMakeLists.txt(1 hunks)sdl_wrapper/sdl_wrapper.render_procedural.cpp(2 hunks)sdl_wrapper/sdl_wrapper.render_procedural.ixx(1 hunks)sdl_wrapper/sdl_wrapper.vertex_layout.ixx(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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.cppsdl_wrapper/sdl_wrapper.render_procedural.cppsdl_wrapper/sdl_wrapper.vertex_layout.ixxsdl_wrapper/CMakeLists.txtsdl_wrapper/sdl_wrapper.render_procedural.ixx
📚 Learning: 2025-11-13T02:58:26.892Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.892Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
sdl_wrapper/sdl_wrapper.render_procedural.cppsdl_wrapper/CMakeLists.txt
📚 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.render_procedural.cppsdl_wrapper/sdl_wrapper.vertex_layout.ixxsdl_wrapper/sdl_wrapper.render_procedural.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.vertex_layout.ixx
⏰ 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). (19)
- GitHub Check: clang-tidy-review
- GitHub Check: CodeQL C++ Analysis
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- 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, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, gcc)
- GitHub Check: CodeQL C++ Analysis
🔇 Additional comments (5)
sdl_wrapper/CMakeLists.txt (1)
26-26: LGTM!The public linkage to
glsl_reflectoris appropriate sincesdl_wrapperexportsVertexReflectiontypes through its public API.CMakeLists.txt (1)
97-110: LGTM!The GoogleTest infrastructure setup is standard and uses a recent stable version (v1.17.0). The integration with CTest and the new
glsl_reflectorsubdirectory is properly configured.glsl_reflector/CMakeLists.txt (1)
1-22: LGTM!The CMake configuration properly sets up the glsl_reflector library with C++23 module support, appropriate linkage to glslang, and GoogleTest integration with test discovery.
main.cpp (1)
286-291: Add error handling for render data creation and upload.Lines 289-290 create new render data and upload it, but the results are not checked. If
create_datareturns an error orupload()fails, the application will proceed with invalid state.Based on learnings: The sdl_wrapper library uses return codes for error handling (no exceptions).
Apply this diff to add error handling:
else { auto new_data = m_gpu->create_data(*m_renderable->procedural(), 3); - m_renderable->data() = std::make_shared<sopho::RenderData>(std::move(new_data.value())); - m_renderable->data()->upload(); + if (!new_data) + { + SDL_LogError(SDL_LOG_CATEGORY_RENDER, + "Failed to create render data after vertex shader update, error = %d", + static_cast<int>(new_data.error())); + } + else + { + m_renderable->data() = std::make_shared<sopho::RenderData>(std::move(new_data.value())); + auto upload_result = m_renderable->data()->upload(); + if (!upload_result) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, + "Failed to upload render data after vertex shader update, error = %d", + static_cast<int>(upload_result.error())); + } + } }⛔ 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.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.sdl_wrapper/sdl_wrapper.vertex_layout.ixx (1)
128-131: Reflection-based layout accessors look consistentStoring the
VertexReflectionand exposing it viaget_vertex_reflection, while drivingattributesandstrideoff that same source of truth, keeps the layout derivation centralized and consistent. This should make future reflection extensions (additional attributes, formats) easier to integrate.
| VertexReflection m_vertex_reflection{}; | ||
| std::vector<SDL_GPUVertexElementFormat> raw{}; | ||
| std::vector<SDL_GPUVertexAttribute> attributes{}; | ||
| uint32_t stride = 0; | ||
|
|
||
| public: | ||
| auto set_vertex_attributes(std::vector<SDL_GPUVertexElementFormat> vertex_attributes) | ||
| auto set_vertex_reflection(const VertexReflection& vertex_reflection) | ||
| { | ||
| raw.swap(vertex_attributes); | ||
| m_vertex_reflection = vertex_reflection; | ||
| stride = 0; | ||
| attributes.clear(); | ||
| for (int i = 0; i < raw.size(); ++i) | ||
| for (const auto& input : vertex_reflection.inputs) | ||
| { | ||
| SDL_GPUVertexAttribute vertex_attribute{}; | ||
| vertex_attribute.location = i; | ||
| vertex_attribute.location = input.location; | ||
| vertex_attribute.buffer_slot = 0; | ||
| vertex_attribute.format = raw[i]; | ||
| vertex_attribute.format = to_sdl_format(input.basic_type, input.vector_size); | ||
| vertex_attribute.offset = stride; | ||
| stride += get_size(vertex_attribute.format); | ||
| attributes.push_back(vertex_attribute); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix set_vertex_reflection return type and handle invalid attribute formats
-
set_vertex_reflectionis declared withautobut has noreturnstatement. In C++, a function with a deduced return type must return a value in all control paths; this will not compile as-is. It should be explicitlyvoid(orauto -> void). -
When
to_sdl_formatreturnsSDL_GPU_VERTEXELEMENTFORMAT_INVALID,get_sizereturns 0, so such attributes keepstrideunchanged and get pushed with an invalid format and duplicate offsets, which is likely unintended.
Suggested change:
- auto set_vertex_reflection(const VertexReflection& vertex_reflection)
+ void set_vertex_reflection(const VertexReflection& vertex_reflection)
{
m_vertex_reflection = vertex_reflection;
stride = 0;
attributes.clear();
for (const auto& input : vertex_reflection.inputs)
{
SDL_GPUVertexAttribute vertex_attribute{};
vertex_attribute.location = input.location;
vertex_attribute.buffer_slot = 0;
- vertex_attribute.format = to_sdl_format(input.basic_type, input.vector_size);
+ vertex_attribute.format = to_sdl_format(input.basic_type, input.vector_size);
+ if (vertex_attribute.format == SDL_GPU_VERTEXELEMENTFORMAT_INVALID)
+ {
+ // Either skip or assert/log here; adjust to your error-handling strategy.
+ continue;
+ }
vertex_attribute.offset = stride;
stride += get_size(vertex_attribute.format);
attributes.push_back(vertex_attribute);
}
}Adjust the invalid-format handling branch (continue vs assertion vs exception) to match the rest of your engine.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and inspect the vertex_layout.ixx file to verify the actual code state
find . -name "sdl_wrapper.vertex_layout.ixx" -type f 2>/dev/null | head -5
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for to_sdl_format implementation
rg -n -A5 "to_sdl_format" --type=cpp --type=ixx 2>/dev/null | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for get_size implementation
rg -n -A5 "get_size" --type=cpp --type=ixx 2>/dev/null | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for SDL_GPU_VERTEXELEMENTFORMAT_INVALID constant
rg -n "SDL_GPU_VERTEXELEMENTFORMAT_INVALID" --type=cpp --type=ixx 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for VertexReflection struct definition
rg -n -A10 "struct VertexReflection" --type=cpp --type=ixx 2>/dev/nullLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Read the actual file to see the current state
cat ./sdl_wrapper/sdl_wrapper.vertex_layout.ixx
Length of output: 4382
🏁 Script executed:
#!/bin/bash
# Search more broadly for format conversion and size functions
rg -n "format|size" ./sdl_wrapper/sdl_wrapper.vertex_layout.ixxLength of output: 373
🏁 Script executed:
#!/bin/bash
# Look at all function definitions in the file
rg -n "^\s*(void|auto|int|auto\*)" ./sdl_wrapper/sdl_wrapper.vertex_layout.ixx | head -20Length of output: 136
🏁 Script executed:
#!/bin/bash
# Search across all sdl_wrapper files for to_sdl_format and get_size
rg -n "to_sdl_format|get_size" ./sdl_wrapper/ 2>/dev/null | head -40Length of output: 482
🏁 Script executed:
#!/bin/bash
# Check if this is a C++20 module file - look for module/export keywords
head -20 ./sdl_wrapper/sdl_wrapper.vertex_layout.ixxLength of output: 472
Fix set_vertex_reflection return type and handle invalid attribute formats
Both issues identified in the review are confirmed:
-
set_vertex_reflection(line 112) is declared withautobut has no return statement. While this deduces tovoidin C++14+ (valid but confusing), explicitvoidis clearer and matches the codebase style. -
When
to_sdl_formatreturnsSDL_GPU_VERTEXELEMENTFORMAT_INVALID(lines 91, 97),get_sizereturns 0 (lines 18–19). This causes attributes with invalid formats to be added with duplicate offsets, which is unintended.
- auto set_vertex_reflection(const VertexReflection& vertex_reflection)
+ void set_vertex_reflection(const VertexReflection& vertex_reflection)
{
m_vertex_reflection = vertex_reflection;
stride = 0;
attributes.clear();
for (const auto& input : vertex_reflection.inputs)
{
SDL_GPUVertexAttribute vertex_attribute{};
vertex_attribute.location = input.location;
vertex_attribute.buffer_slot = 0;
vertex_attribute.format = to_sdl_format(input.basic_type, input.vector_size);
+ if (vertex_attribute.format == SDL_GPU_VERTEXELEMENTFORMAT_INVALID)
+ {
+ continue;
+ }
vertex_attribute.offset = stride;
stride += get_size(vertex_attribute.format);
attributes.push_back(vertex_attribute);
}
}🤖 Prompt for AI Agents
sdl_wrapper/sdl_wrapper.vertex_layout.ixx around lines 106-127: change the
set_vertex_reflection signature from auto to explicit void, then after computing
vertex_attribute.format call to_sdl_format, check for
SDL_GPU_VERTEXELEMENTFORMAT_INVALID (or size == 0) and skip adding that
attribute (optionally log or assert) so you do not push attributes with
duplicate offsets; only increment stride by get_size(format) and push_back when
the size > 0.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
glsl_reflector/modules/glsl_reflector.ixx (1)
36-86: Address previously flagged issues inreflect_vertex.Three issues from prior review remain unresolved:
Line 79: Remove debug output
std::cout << name << vector << std::endl;that unconditionally prints to stdout in production code.Lines 41-42: Replace unnecessary heap allocation with direct pointer usage:
std::vector sources{vertex_source.data()};shader.setStrings(sources.data(), sources.size());
const char* source_cstr = vertex_source.c_str();shader.setStrings(&source_cstr, 1);
- Error handling: Add a success indicator to
VertexReflectionso callers can distinguish parse/link failures from empty-but-valid results. Without this, both "shader has no inputs" and "parsing failed" return identical default-constructed objects.Additionally, lines 77-78 declare unused local variables
nameandvectorthat can be removed:- std::string name = var.name.c_str(); - auto vector = type->getVectorSize(); - std::cout << name << vector << std::endl; result.inputs.emplace_back(VertexInfo{.location = var.layoutLocation(), .name = var.name, .basic_type = to_basic_type(type->getBasicType()), .vector_size = type->getVectorSize()});
🧹 Nitpick comments (1)
glsl_reflector/modules/glsl_reflector.ixx (1)
18-23: Consider using shared/global glslang initialization for efficiency.The
GlslangProcessGuardinstantiation inreflect_vertexis functionally correct—glslang's InitializeProcess/FinalizeProcess are reference-counted, so repeated calls are safe. However, it introduces unnecessary overhead on every reflection call.Since
reflect_vertexis called during shader setup (not per-frame rendering), the impact is moderate. For better efficiency, consider initializing glslang once at application startup or using a shared/global RAII wrapper that performs initialization when first constructed and finalization when last destroyed.Also applies to: 38
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
glsl_reflector/modules/glsl_reflector.ixx(1 hunks)glsl_reflector/test/test.cpp(1 hunks)main.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- glsl_reflector/test/test.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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
📚 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
🔇 Additional comments (2)
main.cpp (2)
8-8: LGTM! Appropriate includes for the new reflection-based code.The
<format>header supports the dynamic label construction usingstd::format, and<variant>is needed forstd::monostatein the monadic pipeline operations.Also applies to: 13-13
288-293: Critical: Missing error checking before calling.value()oncreate_dataresult.The code directly calls
.value()onnew_datawithout verifying the operation succeeded, which will cause undefined behavior or termination ifcreate_datafails. This violates the error-checking pattern used consistently elsewhere (see lines 119-125, 128-135). Additionally, theupload()result is not checked.Apply this diff to add proper error handling:
else { auto new_data = m_gpu->create_data(*m_renderable->procedural(), 3); - m_renderable->data() = std::make_shared<sopho::RenderData>(std::move(new_data.value())); - m_renderable->data()->upload(); + if (!new_data) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to create render data after vertex shader update, error = %d", + static_cast<int>(new_data.error())); + } + else + { + m_renderable->data() = std::make_shared<sopho::RenderData>(std::move(new_data.value())); + auto upload_result = m_renderable->data()->upload(); + if (!upload_result) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, "Failed to upload render data after vertex shader update, error = %d", + static_cast<int>(upload_result.error())); + } + } }⛔ 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.
| auto to_basic_type(glslang::TBasicType gl_basic_type) | ||
| { | ||
| switch (gl_basic_type) | ||
| { | ||
| case glslang::EbtFloat: | ||
| return BasicType::FLOAT; | ||
| default: | ||
| return BasicType::NONE; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete type support in to_basic_type.
The function only handles glslang::EbtFloat, returning BasicType::NONE for all other types. Common GLSL types like int, uint, bool, and double are unhandled, which will cause vertex reflection to report incorrect types for non-float attributes.
If this is intentional (e.g., your shaders only use floats), add a comment explaining the limitation. Otherwise, expand support:
auto to_basic_type(glslang::TBasicType gl_basic_type)
{
switch (gl_basic_type)
{
case glslang::EbtFloat:
return BasicType::FLOAT;
+ case glslang::EbtInt:
+ return BasicType::INT;
+ case glslang::EbtUint:
+ return BasicType::UINT;
+ case glslang::EbtBool:
+ return BasicType::BOOL;
+ case glslang::EbtDouble:
+ return BasicType::DOUBLE;
default:
return BasicType::NONE;
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In glsl_reflector/modules/glsl_reflector.ixx around lines 25 to 34, the
to_basic_type function only maps glslang::EbtFloat and returns BasicType::NONE
for all other GLSL basic types, causing non-float attributes (int, uint, bool,
double, etc.) to be reported incorrectly; update the switch to map all relevant
glslang::TBasicType enum values (at minimum EbtInt -> BasicType::INT, EbtUint ->
BasicType::UINT, EbtBool -> BasicType::BOOL, EbtDouble -> BasicType::DOUBLE,
plus any sampler/struct types you need) to the corresponding BasicType entries,
and if the omission was intentional add a clear comment explaining the
limitation and keep an explicit default that logs or asserts on unsupported
types so it’s obvious at runtime.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include <cmath> | ||
| #include <expected> | ||
| #include <format> | ||
| #include <iostream> |
There was a problem hiding this comment.
warning: included header format is not used directly [misc-include-cleaner]
| #include <iostream> | |
| #include <iostream> |
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.