Dev bindgroup set - #26
Merged
Merged
Conversation
added 28 commits
April 8, 2026 09:28
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant CW as compute_wrapper
participant CPI as compute_wrapper::pimpl
participant BGS as bindgroup_set
participant Pass as ComputePass
App->>CW: compute(bindgroup_set, width, height, encoder)
CW->>CPI: compute(set, width, height, encoder)
CPI->>BGS: access m_pimpl->m_bindgroups
loop for each (group_index, bindgroup_wrapper)
CPI->>Pass: SetBindGroup(group_index, bindgroup)
end
CPI->>Pass: Dispatch(width, height, 1)
CPI->>Pass: End()
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![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
🤖 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/bindgroup_set_impl.hpp`:
- Around line 24-27: The class currently keeps m_bindgroups private with no
accessor, preventing consumers (e.g., compute(bindgroup_set, ...) and
render(bindgroup_set, ...)) from reading stored bind groups; add a const
accessor on the bindgroup_set_impl class such as const std::map<unsigned,
bindgroup_wrapper>& bindgroups() const (or an equivalent read-only view/iterator
interface) so callers can iterate/lookup bind groups without mutating them;
update call sites to use bindgroups() where they need to bind or inspect
entries.
In `@src/compute_wrapper_impl.hpp`:
- Around line 39-44: The compute(bindgroup_set set, unsigned width, unsigned
height, encoder_wrapper encoder) function currently returns immediately;
implement the dispatch path by creating a compute pass from the provided
encoder_wrapper (e.g., begin/create a compute pass via encoder), bind the
provided bindgroup_set to that pass (use bindgroup_set's method that attaches
its groups to a pass), call the dispatch call with the given width and height to
issue workgroups, end/close the compute pass on the encoder, and return true on
success (return false only on failure). Ensure you reference and use the
existing symbols compute, bindgroup_set, and encoder_wrapper when locating and
wiring up the pass creation, binding, dispatchWorkgroups, and pass finish/close
steps.
In `@src/dawn_wrapper.hpp`:
- Around line 90-95: The default constructor for bindgroup_set leaves the PIMPL
pointer (m_pimpl) null while add_bindgroup(bindgroup_wrapper bg, unsigned group)
dereferences it; initialize m_pimpl in the default constructor or remove the
public default ctor. Modify the bindgroup_set() constructor to allocate and set
up the PIMPL used by DAWN_WRAPPER_PIMPL_DEC (so add_bindgroup can safely use
m_pimpl), or make the default ctor private/deleted and provide a
factory/explicit constructor that initializes the PIMPL; ensure add_bindgroup
assumes a valid m_pimpl only after these changes.
In `@src/render_wrapper_impl.hpp`:
- Around line 53-57: The public render(bindgroup_set set, encoder_wrapper
encoder) is currently a no-op; implement it to execute the render path by
starting a render pass with the provided encoder, binding the pipeline and the
provided bindgroup_set, issuing the appropriate draw/dispatch calls, ending the
pass, submitting the encoder/command buffer, and presenting the output. Locate
the render function named render(bindgroup_set set, encoder_wrapper encoder) in
render_wrapper_impl.hpp and forward or reuse the existing internal render logic
(the pipeline setup, bind group bindings, draw calls, command submission and
present steps) so that providing a bindgroup_set actually performs rendering
rather than returning immediately. Ensure proper use of the encoder_wrapper API
to begin/end the pass and to submit/present.
In `@test/build_and_run`:
- Around line 5-6: The build step was left without its configure step: re-enable
the configuration command (restore the "cmake -B build" line) before the "cmake
--build build -j 8" invocation so the build/ directory is created and CI no
longer fails; ensure the script runs the configure step (cmake -B build) and
only then runs the build step (cmake --build build -j 8).
🪄 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: e0856a60-9f2d-41de-a156-8d5908e33d9d
📒 Files selected for processing (9)
CMakeLists.txtsrc/bindgroup_set.cppsrc/bindgroup_set_impl.hppsrc/compute_wrapper.cppsrc/compute_wrapper_impl.hppsrc/dawn_wrapper.hppsrc/render_wrapper.cppsrc/render_wrapper_impl.hpptest/build_and_run
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/render_wrapper_impl.hpp (1)
54-78: Extract common render-pass body to avoid overload drift.This overload duplicates pass setup/draw/submit/present logic already present in other
render(...)overloads. A shared helper would reduce maintenance risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render_wrapper_impl.hpp` around lines 54 - 78, The render(bindgroup_set, encoder_wrapper) overload duplicates the render-pass setup, draw and submit/present sequence; extract that common sequence into a shared private helper (e.g., a method like execute_render_pass_or_finish(RenderPassEncoder& pass, const bindgroup_set& set, encoder_wrapper& encoder) or renderPassBody(const bindgroup_set& set, dawn::RenderPassEncoder& pass, encoder_wrapper& encoder)) that does ASSERT(m_bindGroupLayout), calls dawn_utils::begin_render_pass/getCurrentTextureView as needed, SetPipeline(get_pipeline()), iterates set.m_pimpl->m_bindgroups to call make_bindgroup(m_device), sets vertex/index buffers, calls DrawIndexed and End, then does encoder.submit_command_buffer() and m_surface.present() guarded by the __EMSCRIPTEN__ macro; replace the duplicated body in this render(...) and the other render(...) overloads to a single call to that helper so behavior and assertions remain identical.
🤖 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/render_wrapper_impl.hpp`:
- Around line 54-67: The render(bindgroup_set set, encoder_wrapper encoder)
function dereferences set.m_pimpl without validating it which can crash for
default/invalid bindgroup_set; before using set.m_pimpl (and before iterating
set.m_pimpl->m_bindgroups) add a guard (e.g. ASSERT or if-check) to ensure
set.m_pimpl is non-null and return/handle the error early if it is null,
mirroring the safety used for bindgroup_wrapper, so subsequent calls like
make_bindgroup(m_device) and access to m_bindgroups are only performed when
set.m_pimpl is valid.
---
Nitpick comments:
In `@src/render_wrapper_impl.hpp`:
- Around line 54-78: The render(bindgroup_set, encoder_wrapper) overload
duplicates the render-pass setup, draw and submit/present sequence; extract that
common sequence into a shared private helper (e.g., a method like
execute_render_pass_or_finish(RenderPassEncoder& pass, const bindgroup_set& set,
encoder_wrapper& encoder) or renderPassBody(const bindgroup_set& set,
dawn::RenderPassEncoder& pass, encoder_wrapper& encoder)) that does
ASSERT(m_bindGroupLayout), calls
dawn_utils::begin_render_pass/getCurrentTextureView as needed,
SetPipeline(get_pipeline()), iterates set.m_pimpl->m_bindgroups to call
make_bindgroup(m_device), sets vertex/index buffers, calls DrawIndexed and End,
then does encoder.submit_command_buffer() and m_surface.present() guarded by the
__EMSCRIPTEN__ macro; replace the duplicated body in this render(...) and the
other render(...) overloads to a single call to that helper so behavior and
assertions remain identical.
🪄 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: 998e00b7-189f-49c1-9dde-0ac3795113c8
📒 Files selected for processing (5)
src/bindgroup_set_impl.hppsrc/compute_wrapper.cppsrc/compute_wrapper_impl.hppsrc/dawn_wrapper.hppsrc/render_wrapper_impl.hpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/bindgroup_set_impl.hpp
- src/compute_wrapper_impl.hpp
- src/dawn_wrapper.hpp
- src/compute_wrapper.cpp
Comment on lines
+54
to
+67
| void render(bindgroup_set set, encoder_wrapper encoder) | ||
| { | ||
| ASSERT(m_bindGroupLayout); | ||
|
|
||
| auto textureView = getCurrentTextureView(); | ||
| ASSERT(textureView); | ||
|
|
||
| auto pass = dawn_utils::begin_render_pass(encoder.m_pimpl->m_encoder, textureView); | ||
| pass.SetPipeline(get_pipeline()); | ||
|
|
||
| for (auto entry : set.m_pimpl->m_bindgroups) | ||
| { | ||
| pass.SetBindGroup(entry.first, entry.second.m_pimpl->make_bindgroup(m_device)); | ||
| } |
There was a problem hiding this comment.
Guard bindgroup_set validity before dereferencing m_pimpl.
Line 64 dereferences set.m_pimpl without validation. Unlike the bindgroup_wrapper path (Line 83), this can crash on invalid/default bindgroup_set.
🛠️ Proposed fix
void render(bindgroup_set set, encoder_wrapper encoder)
{
ASSERT(m_bindGroupLayout);
+ ASSERT(set.is_valid());
auto textureView = getCurrentTextureView();
ASSERT(textureView);
auto pass = dawn_utils::begin_render_pass(encoder.m_pimpl->m_encoder, textureView);
pass.SetPipeline(get_pipeline());
- for (auto entry : set.m_pimpl->m_bindgroups)
+ for (const auto& entry : set.m_pimpl->m_bindgroups)
{
+ ASSERT(entry.second.is_valid());
pass.SetBindGroup(entry.first, entry.second.m_pimpl->make_bindgroup(m_device));
}📝 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 render(bindgroup_set set, encoder_wrapper encoder) | |
| { | |
| ASSERT(m_bindGroupLayout); | |
| auto textureView = getCurrentTextureView(); | |
| ASSERT(textureView); | |
| auto pass = dawn_utils::begin_render_pass(encoder.m_pimpl->m_encoder, textureView); | |
| pass.SetPipeline(get_pipeline()); | |
| for (auto entry : set.m_pimpl->m_bindgroups) | |
| { | |
| pass.SetBindGroup(entry.first, entry.second.m_pimpl->make_bindgroup(m_device)); | |
| } | |
| void render(bindgroup_set set, encoder_wrapper encoder) | |
| { | |
| ASSERT(m_bindGroupLayout); | |
| ASSERT(set.is_valid()); | |
| auto textureView = getCurrentTextureView(); | |
| ASSERT(textureView); | |
| auto pass = dawn_utils::begin_render_pass(encoder.m_pimpl->m_encoder, textureView); | |
| pass.SetPipeline(get_pipeline()); | |
| for (const auto& entry : set.m_pimpl->m_bindgroups) | |
| { | |
| ASSERT(entry.second.is_valid()); | |
| pass.SetBindGroup(entry.first, entry.second.m_pimpl->make_bindgroup(m_device)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render_wrapper_impl.hpp` around lines 54 - 67, The render(bindgroup_set
set, encoder_wrapper encoder) function dereferences set.m_pimpl without
validating it which can crash for default/invalid bindgroup_set; before using
set.m_pimpl (and before iterating set.m_pimpl->m_bindgroups) add a guard (e.g.
ASSERT or if-check) to ensure set.m_pimpl is non-null and return/handle the
error early if it is null, mirroring the safety used for bindgroup_wrapper, so
subsequent calls like make_bindgroup(m_device) and access to m_bindgroups are
only performed when set.m_pimpl is valid.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/compute_wrapper_impl.hpp (1)
40-57: Implementation looks correct; consider using const reference in range-for.The
bindgroup_setoverload properly iterates the bind groups and sets each at its designated group index. One minor optimization:auto entrycopies each map entry. Sincebindgroup_wrapperlikely contains ashared_ptr, the copy is cheap, but usingconst auto&is idiomatic and avoids any unnecessary overhead.♻️ Suggested change
- for (auto entry : set.m_pimpl->m_bindgroups) + for (const auto& entry : set.m_pimpl->m_bindgroups) { ASSERT(entry.second.m_pimpl); computePass.SetBindGroup(entry.first, entry.second.m_pimpl->make_bindgroup(m_device)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compute_wrapper_impl.hpp` around lines 40 - 57, In compute(bindgroup_set set, unsigned width, unsigned height, encoder_wrapper encoder) the range-for loop copies each map entry; change the loop header over set.m_pimpl->m_bindgroups to use a const reference (e.g., const auto&) so entries are not copied unnecessarily, keeping the rest of the body (ASSERT(entry.second.m_pimpl); computePass.SetBindGroup(...)) unchanged.src/bindgroup_set.cpp (1)
1-3: Unused includes can be removed.
<iostream>and<map>are included but not used in this translation unit. The<map>is used inbindgroup_set_impl.hppand<memory>is needed forstd::make_shared, but<iostream>appears entirely unused.🧹 Proposed cleanup
-#include <iostream> -#include <map> `#include` <memory> `#include` "bindgroup_set_impl.hpp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bindgroup_set.cpp` around lines 1 - 3, Remove the unused includes from this translation unit: delete `#include` <iostream> and `#include` <map> from src/bindgroup_set.cpp, leaving `#include` <memory> (needed for std::make_shared) and any required project headers (e.g., bindgroup_set_impl.hpp) in place; ensure any <map> usage remains in bindgroup_set_impl.hpp so the map include stays where it's actually needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/bindgroup_set.cpp`:
- Around line 1-3: Remove the unused includes from this translation unit: delete
`#include` <iostream> and `#include` <map> from src/bindgroup_set.cpp, leaving
`#include` <memory> (needed for std::make_shared) and any required project headers
(e.g., bindgroup_set_impl.hpp) in place; ensure any <map> usage remains in
bindgroup_set_impl.hpp so the map include stays where it's actually needed.
In `@src/compute_wrapper_impl.hpp`:
- Around line 40-57: In compute(bindgroup_set set, unsigned width, unsigned
height, encoder_wrapper encoder) the range-for loop copies each map entry;
change the loop header over set.m_pimpl->m_bindgroups to use a const reference
(e.g., const auto&) so entries are not copied unnecessarily, keeping the rest of
the body (ASSERT(entry.second.m_pimpl); computePass.SetBindGroup(...))
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42a086d0-c9c5-4525-8636-3b7edee0f4af
📒 Files selected for processing (5)
src/bindgroup_set.cppsrc/bindgroup_set_impl.hppsrc/compute_wrapper_impl.hppsrc/dawn_wrapper.hppsrc/render_wrapper_impl.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/bindgroup_set_impl.hpp
- src/render_wrapper_impl.hpp
- src/dawn_wrapper.hpp
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
1 participant
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.
Summary by CodeRabbit
Release Notes
bindgroup_settype for managing multiple bindgroups with a fluentadd_bindgroupAPIbindgroup_setalongside individual bindgroup usage