A wrapper for Cursors that allows us to write virtual write_to_cursor#967
Open
tdavidovicNV wants to merge 1 commit into
Open
A wrapper for Cursors that allows us to write virtual write_to_cursor#967tdavidovicNV wants to merge 1 commit into
tdavidovicNV wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
ccummingsNV
reviewed
Apr 30, 2026
Contributor
ccummingsNV
left a comment
ccummingsNV
left a comment
There was a problem hiding this comment.
Can we hold off on this until I've finished rewriting half the cursor code :)
![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 (1)
tests/sgl/device/test_cursors.cpp (1)
48-73: ⚡ Quick winUse
m_prefix for newly added class members in test structs.
ConcreteCursorTestStructandPolymorphicTestStructintroduce new members (f0,nested) that don’t follow the project member naming rule.As per coding guidelines
**/*.{cpp,cc,cxx,h,hpp,hh,hxx}: C++ classes use PascalCase, functions and variables use snake_case, member variables use m_ prefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sgl/device/test_cursors.cpp` around lines 48 - 73, The new test structs ConcreteCursorTestStruct and PolymorphicTestStruct violate the member-naming rule—rename their members to use the m_ prefix (e.g., f0 -> m_f0, nested -> m_nested), update their constructors/initializer lists to initialize m_f0 and m_nested, and update all uses inside write_to_cursor (both overloads in ConcreteCursorTestStruct and the override in PolymorphicTestStruct) and any write_test_struct_fields calls to pass m_f0 and m_nested.data; keep parameter names as you prefer but ensure all references use the new m_ member names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sgl/device/any_cursor.h`:
- Around line 23-121: Add Doxygen /// comments with `@param` and `@return` for the
new public API in AnyCursor and CursorWritable: document constructors
AnyCursor(ShaderCursor) and AnyCursor(BufferElementCursor), all query methods
(is_shader_cursor, is_buffer_element_cursor, is_valid, slang_type_layout,
to_string), conversion/accessors (as_shader_cursor, as_buffer_element_cursor),
subscript/find/has methods (operator[](string_view), operator[](uint32_t),
find_field, find_element, has_field, has_element), all setters (set_object,
set_buffer, set_buffer_view, set_texture, set_texture_view, set_sampler,
set_acceleration_structure, set_descriptor_handle, set_data,
set_cuda_tensor_view, set_pointer) and helpers
(_set_array/_set_scalar/_set_vector/_set_matrix) to describe parameters and
return values, and document CursorWritable::write_to_cursor (describe ownership
and whether cursor is copied or referenced); use /// style Doxygen comments
above each declaration and include `@param` for each parameter and `@return` where a
value is returned.
---
Nitpick comments:
In `@tests/sgl/device/test_cursors.cpp`:
- Around line 48-73: The new test structs ConcreteCursorTestStruct and
PolymorphicTestStruct violate the member-naming rule—rename their members to use
the m_ prefix (e.g., f0 -> m_f0, nested -> m_nested), update their
constructors/initializer lists to initialize m_f0 and m_nested, and update all
uses inside write_to_cursor (both overloads in ConcreteCursorTestStruct and the
override in PolymorphicTestStruct) and any write_test_struct_fields calls to
pass m_f0 and m_nested.data; keep parameter names as you prefer but ensure all
references use the new m_ member names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e1daf0e-59d5-4e2e-a460-d5b42a06afda
📒 Files selected for processing (7)
src/sgl/CMakeLists.txtsrc/sgl/device/any_cursor.cppsrc/sgl/device/any_cursor.hsrc/sgl/device/cursor_utils.hsrc/sgl/device/fwd.hsrc/sgl/device/shader_cursor.htests/sgl/device/test_cursors.cpp
Comment on lines
+23
to
+121
| AnyCursor(ShaderCursor cursor); | ||
| AnyCursor(BufferElementCursor cursor); | ||
|
|
||
| bool is_shader_cursor() const; | ||
| bool is_buffer_element_cursor() const; | ||
|
|
||
| ShaderCursor* as_shader_cursor(); | ||
| const ShaderCursor* as_shader_cursor() const; | ||
| BufferElementCursor* as_buffer_element_cursor(); | ||
| const BufferElementCursor* as_buffer_element_cursor() const; | ||
|
|
||
| bool is_valid() const; | ||
| std::string to_string() const; | ||
| slang::TypeLayoutReflection* slang_type_layout() const; | ||
|
|
||
| AnyCursor operator[](std::string_view name) const; | ||
| AnyCursor operator[](uint32_t index) const; | ||
|
|
||
| AnyCursor find_field(std::string_view name) const; | ||
| AnyCursor find_element(uint32_t index) const; | ||
|
|
||
| bool has_field(std::string_view name) const; | ||
| bool has_element(uint32_t index) const; | ||
|
|
||
| void set_object(const ref<ShaderObject>& object); | ||
|
|
||
| void set_buffer(const ref<Buffer>& buffer); | ||
| void set_buffer_view(const ref<BufferView>& buffer_view); | ||
| void set_texture(const ref<Texture>& texture); | ||
| void set_texture_view(const ref<TextureView>& texture_view); | ||
| void set_sampler(const ref<Sampler>& sampler); | ||
| void set_acceleration_structure(const ref<AccelerationStructure>& acceleration_structure); | ||
|
|
||
| void set_descriptor_handle(const DescriptorHandle& handle); | ||
|
|
||
| void set_data(const void* data, size_t size); | ||
| void set_cuda_tensor_view(const cuda::TensorView& tensor_view); | ||
| void set_pointer(uint64_t pointer_value); | ||
|
|
||
| template<typename T> | ||
| AnyCursor& operator=(const T& value) | ||
| { | ||
| set(value); | ||
| return *this; | ||
| } | ||
|
|
||
| template<typename T> | ||
| void set(const T& value) | ||
| { | ||
| using Value = std::decay_t<T>; | ||
| if constexpr (std::is_same_v<Value, ref<ShaderObject>>) { | ||
| set_object(value); | ||
| } else if constexpr (std::is_same_v<Value, ref<Buffer>>) { | ||
| set_buffer(value); | ||
| } else if constexpr (std::is_same_v<Value, ref<BufferView>>) { | ||
| set_buffer_view(value); | ||
| } else if constexpr (std::is_same_v<Value, ref<Texture>>) { | ||
| set_texture(value); | ||
| } else if constexpr (std::is_same_v<Value, ref<TextureView>>) { | ||
| set_texture_view(value); | ||
| } else if constexpr (std::is_same_v<Value, ref<Sampler>>) { | ||
| set_sampler(value); | ||
| } else if constexpr (std::is_same_v<Value, ref<AccelerationStructure>>) { | ||
| set_acceleration_structure(value); | ||
| } else if constexpr (std::is_same_v<Value, DescriptorHandle>) { | ||
| set_descriptor_handle(value); | ||
| } else if constexpr (std::is_same_v<Value, cuda::TensorView>) { | ||
| set_cuda_tensor_view(value); | ||
| } else if constexpr (HasWriteToCursor<T, AnyCursor>) { | ||
| value.write_to_cursor(*this); | ||
| } else { | ||
| std::visit( | ||
| [&](auto& cursor) | ||
| { | ||
| cursor.set(value); | ||
| }, | ||
| m_cursor | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| void _set_array(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, size_t element_count); | ||
| void _set_scalar(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type); | ||
| void _set_vector(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, int dimension); | ||
| void | ||
| _set_matrix(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, int rows, int cols); | ||
|
|
||
| private: | ||
| using CursorVariant = std::variant<ShaderCursor, BufferElementCursor>; | ||
|
|
||
| CursorVariant m_cursor; | ||
| }; | ||
|
|
||
| /// Interface for values that can write themselves to any supported cursor type. | ||
| class SGL_API CursorWritable { | ||
| public: | ||
| virtual ~CursorWritable() = default; | ||
| virtual void write_to_cursor(AnyCursor cursor) const = 0; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add Doxygen @param/@return docs for the new public API methods.
AnyCursor/CursorWritable introduce a broad public surface, but per-method contract docs are mostly missing.
As per coding guidelines **/*.{cpp,cc,cxx,h,hpp,hh,hxx}: C++ documentation must use Doxygen format with /// comments and @param/@return tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/device/any_cursor.h` around lines 23 - 121, Add Doxygen /// comments
with `@param` and `@return` for the new public API in AnyCursor and CursorWritable:
document constructors AnyCursor(ShaderCursor) and
AnyCursor(BufferElementCursor), all query methods (is_shader_cursor,
is_buffer_element_cursor, is_valid, slang_type_layout, to_string),
conversion/accessors (as_shader_cursor, as_buffer_element_cursor),
subscript/find/has methods (operator[](string_view), operator[](uint32_t),
find_field, find_element, has_field, has_element), all setters (set_object,
set_buffer, set_buffer_view, set_texture, set_texture_view, set_sampler,
set_acceleration_structure, set_descriptor_handle, set_data,
set_cuda_tensor_view, set_pointer) and helpers
(_set_array/_set_scalar/_set_vector/_set_matrix) to describe parameters and
return values, and document CursorWritable::write_to_cursor (describe ownership
and whether cursor is copied or referenced); use /// style Doxygen comments
above each declaration and include `@param` for each parameter and `@return` where a
value is returned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This wraps the common interface of BufferElementCursor and ShaderCursor, allowing us to a write one virtual interface that takes this generic cursor, rather than having to have two. This is a native code only, as Python can already do that.
Summary by CodeRabbit
Release Notes
New Features
Tests