Support Nested Group and Custom Relationships - #18
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors boolean SDFs to use relationship-based operands, assigns deterministic parent-before-child group IDs during extraction, propagates transforms via the operand graph (including scale), emits parent metadata into GPU inputs, and replaces the shader composition with a stack-based parent-aware algorithm. ChangesNested Group Support with Operand Relationships
🎯 4 (Complex) | ⏱️ ~45 minutes sequenceDiagram
participant ExtractStage
participant InputBuilder
participant GPU
ExtractStage->>InputBuilder: emit SdfExtractedOperand
InputBuilder->>InputBuilder: deterministic DFS assign ids
InputBuilder->>GPU: upload SdfInput array with parent metadata
GPU->>GPU: eval_primitive then drain groups
🚥 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
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 |
Sheerwin02
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sdf.rs`:
- Around line 259-274: The early-exit uses q_changed and only watches
Changed<SdfGlobalTransform>, Changed<PrimitiveIndex>, and Added<PrimitiveIndex>,
which misses entity removals and operand changes; update the query `q_changed`
to also include RemovedComponents<PrimitiveIndex> and change detection for
operand data (e.g. Changed<SdfExtractedOperand>) so removals and
operand/boolean-order updates trigger the system; adjust any downstream logic
that assumed only add/change events (functions/blocks that reference q_changed,
SdfGlobalTransform, PrimitiveIndex, and SdfExtractedOperand) to handle removals
appropriately.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 641e8b27-7de7-47f3-9cba-a779ef8b60fa
📒 Files selected for processing (5)
assets/shaders/sdf_raymarch.wgslsrc/main.rssrc/sdf.rssrc/sdf/boolean.rssrc/sdf/transform.rs
…ionally inserting PrimitiveIndex
nixonyh
left a comment
nixonyh
left a comment
There was a problem hiding this comment.
The refactor from entity-index group IDs to DFS-based IDs is a solid correctness improvement, and the SdfOperandOf/SdfOperands relationship model is cleaner than the old approach. The CodeRabbit finding about missing RemovedComponents was addressed in the latest commit. Three issues to look at before merging:
1. Shader drain loop is incorrect for sibling groups (sdf_raymarch.wgsl, drain section)
The end-of-loop drain pops each stack frame and applies it to stack_acc[stack_top] (the frame immediately below), which is only correct when groups form a linear chain. When a parent has two or more child groups, the second child is merged into the first child's accumulator instead of the parent's.
Counterexample — Group A contains [P1, Group B, Group C], B contains P2, C contains P3. DFS pre-order assigns A=1, B=2, C=3. After the main loop the stack is [A(idx=0), B(idx=1), C(idx=2)]. The drain pops C and writes into stack_acc[stack_top] which is now stack_acc[1] (B's slot), even though C's parent_group_id correctly points to A. B and C end up merged together rather than both being applied to A independently.
The existing tests only exercise linear chains (A→B→C), so this is not caught. A fix requires either storing each frame's group_id alongside it and scanning for the matching parent slot during drain, or maintaining a group_id → stack_index map updated on push.
2. Silent geometry loss when stack depth is exceeded (sdf_raymarch.wgsl, "New group" branch)
if stack_top < i32(MAX_STACK_DEPTH) - 1 {
stack_top += 1;
...
cur_group_id = input.group_id;
}
// else: the first primitive of the new group (d) is silently discardedWhen the limit is hit, d is dropped and cur_group_id is not updated, so every subsequent primitive in that group also triggers this branch and is also dropped. Nesting beyond 8 levels silently loses entire sub-trees with no visual indication. At minimum, fall back to dist = min(dist, d) so geometry is at least preserved even if the boolean operation is approximated.
3. Redundant add_children in assign_sdf_group_children (src/sdf/boolean.rs)
commands.entity(group_entity).add_children(&child_entities);This inserts Bevy's built-in ChildOf/Children hierarchy on top of the new SdfOperandOf/SdfOperands relationship — looks like a leftover from the previous design. The main unintended side effect is Bevy's hierarchy-aware recursive despawn: despawning a group entity will also despawn all its primitive children. If that is not the intended behaviour, remove the add_children call; SdfOperandOf already maintains the operand list through SdfOperands.
Generated by Claude Code
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/sdf.rs (4)
330-385:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe packed input order cannot represent sibling-group transitions correctly.
Sorting by
(group_id, order)forces all direct operands of a parent group ahead of every descendant group, and it also makes sibling subgroups appear back-to-back. With the current shader stack logic, a sequence likeA, subgroup_1, Bis evaluated asA, B, subgroup_1, andsubgroup_2gets folded intosubgroup_1instead of their common parent. This needs either explicit close/depth metadata in the packed stream or shader-side draining back toinput.parent_group_idbefore pushing the next group.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdf.rs` around lines 330 - 385, Sorting the packed inputs by (group_id, order) in sorted_inputs loses sibling/parent transition information causing incorrect shader stack evaluation; modify the packing so SdfInput includes explicit close/depth metadata (e.g., add fields like depth or is_group_close) or emit an explicit "close-to-parent" sentinel between groups instead of relying solely on (group_id, order). Locate the loop building sorted_inputs (iterating q_primitives and calling SdfInput::new) and change the SdfInput constructor and pushed tuple to carry either a depth level or an explicit close flag derived from group_id_map and operand_of (and when absent default to 0/false), then adjust the sorting key to preserve the original traversal order (e.g., stable sort by traversal index) so shader-side stack draining can unambiguously pop to input.parent_group_id.
414-416:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe unchanged fast path never triggers for empty primitive buffers.
After the first no-primitive rebuild, Lines 423-435 leave only the sentinel element in the buffer.
buffer.len() > 1is then false forever, so absent primitive types still rewrite the same one-element GPU buffer every frame.Suggested fix
- if q_changed.is_empty() && removed.is_empty() && buffer.len() > 1 + if q_changed.is_empty() && removed.is_empty() && buffer.len() > 0 { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdf.rs` around lines 414 - 416, The fast-path check currently uses `if q_changed.is_empty() && removed.is_empty() && buffer.len() > 1 { return; }`, which skips returning when the GPU buffer only contains the sentinel (len == 1); change the length check to include the sentinel case (e.g. `buffer.len() > 0` or `buffer.len() >= 1`) so that when `q_changed` and `removed` are empty and the buffer has only the sentinel element the function returns early and avoids rewriting the one-element GPU buffer every frame.
391-395:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix stale
inputson the zero-primitive path (GPU data not rewritten)When
update_input_buffersclearsbuffers.input_bufferand skipswrite_bufferbecause it’s empty, the shader can still iterate using the old storage size/data:sdf_raymarch.wgsluseslen = arrayLength(&inputs)and loops overinputs[i]. Since the GPU buffer isn’t updated/resized on the empty path, the raymarch pass may keep using staleSdfInputvalues after the last primitive is removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdf.rs` around lines 391 - 395, The GPU can see stale inputs when buffers.input_buffer is cleared on the CPU but write_buffer is skipped; update the code so the GPU is explicitly informed of zero inputs (either by writing/updating the GPU buffer to a zero-length/zero-count or by updating a dedicated input count uniform) rather than skipping write_buffer. In practice modify the logic around buffers.input_buffer / update_input_buffers so that when input_buffer.is_empty() you still update the GPU (call the buffer write or update the SdfInput count uniform used by sdf_raymarch.wgsl) so the shader’s arrayLength(&inputs) / loop over inputs will see zero elements and not use stale SdfInput data.
259-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild
SdfInputwhenSdfExtractedOperandis removed, and write a cleared/empty input buffer
update_input_buffers’ early-return guard only considersChanged<SdfExtractedOperand>andRemovedComponents<PrimitiveIndex>. Operand removals can change the computedgroup_id/parent_group_id/parent_opfor existing primitives, but won’t trigger a rebuild—so stale nesting metadata can persist on the GPU.- When there are no primitives,
update_input_buffersclears the CPU-sidebuffers.input_bufferbut skipswrite_bufferif it’s empty, leaving the GPU storage buffer with the previousSdfInputcontents (unlikeupdate_primitive_buffers, which always writes a default/sentinel element).Suggested fix
fn update_input_buffers( q_primitives: Query<( &SdfGlobalTransform, &PrimitiveType, &PrimitiveIndex, Option<&SdfExtractedOperand>, )>, q_group_operands: Query<(&SdfExtractedOperand, &MainEntity)>, q_changed: Query< (), Or<( Changed<SdfGlobalTransform>, Changed<PrimitiveIndex>, Added<PrimitiveIndex>, Changed<SdfExtractedOperand>, )>, >, removed_primitives: RemovedComponents<PrimitiveIndex>, + removed_operands: RemovedComponents<SdfExtractedOperand>, mut buffers: ResMut<SdfBuffers>, render_device: Res<RenderDevice>, render_queue: Res<RenderQueue>, ) { - if q_changed.is_empty() && removed_primitives.is_empty() { + if q_changed.is_empty() + && removed_primitives.is_empty() + && removed_operands.is_empty() + { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdf.rs` around lines 259 - 275, The early-return in update_input_buffers misses operand removals and skips writing an empty SdfInput to the GPU; modify the change detection to also consider removals of SdfExtractedOperand (or add a RemovedComponents<SdfExtractedOperand> parameter and include it in the q_changed/guard) so a rebuild is triggered when operands are removed, and ensure that when buffers.input_buffer is empty you still call render_queue.write_buffer / render_device create_buffer_with_data to upload a default/cleared SdfInput (matching the sentinel behavior in update_primitive_buffers) so GPU state cannot remain stale; update references: update_input_buffers, SdfExtractedOperand, RemovedComponents<PrimitiveIndex>, buffers.input_buffer, write_buffer, update_primitive_buffers, and SdfInput.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/sdf.rs`:
- Around line 330-385: Sorting the packed inputs by (group_id, order) in
sorted_inputs loses sibling/parent transition information causing incorrect
shader stack evaluation; modify the packing so SdfInput includes explicit
close/depth metadata (e.g., add fields like depth or is_group_close) or emit an
explicit "close-to-parent" sentinel between groups instead of relying solely on
(group_id, order). Locate the loop building sorted_inputs (iterating
q_primitives and calling SdfInput::new) and change the SdfInput constructor and
pushed tuple to carry either a depth level or an explicit close flag derived
from group_id_map and operand_of (and when absent default to 0/false), then
adjust the sorting key to preserve the original traversal order (e.g., stable
sort by traversal index) so shader-side stack draining can unambiguously pop to
input.parent_group_id.
- Around line 414-416: The fast-path check currently uses `if
q_changed.is_empty() && removed.is_empty() && buffer.len() > 1 { return; }`,
which skips returning when the GPU buffer only contains the sentinel (len == 1);
change the length check to include the sentinel case (e.g. `buffer.len() > 0` or
`buffer.len() >= 1`) so that when `q_changed` and `removed` are empty and the
buffer has only the sentinel element the function returns early and avoids
rewriting the one-element GPU buffer every frame.
- Around line 391-395: The GPU can see stale inputs when buffers.input_buffer is
cleared on the CPU but write_buffer is skipped; update the code so the GPU is
explicitly informed of zero inputs (either by writing/updating the GPU buffer to
a zero-length/zero-count or by updating a dedicated input count uniform) rather
than skipping write_buffer. In practice modify the logic around
buffers.input_buffer / update_input_buffers so that when input_buffer.is_empty()
you still update the GPU (call the buffer write or update the SdfInput count
uniform used by sdf_raymarch.wgsl) so the shader’s arrayLength(&inputs) / loop
over inputs will see zero elements and not use stale SdfInput data.
- Around line 259-275: The early-return in update_input_buffers misses operand
removals and skips writing an empty SdfInput to the GPU; modify the change
detection to also consider removals of SdfExtractedOperand (or add a
RemovedComponents<SdfExtractedOperand> parameter and include it in the
q_changed/guard) so a rebuild is triggered when operands are removed, and ensure
that when buffers.input_buffer is empty you still call render_queue.write_buffer
/ render_device create_buffer_with_data to upload a default/cleared SdfInput
(matching the sentinel behavior in update_primitive_buffers) so GPU state cannot
remain stale; update references: update_input_buffers, SdfExtractedOperand,
RemovedComponents<PrimitiveIndex>, buffers.input_buffer, write_buffer,
update_primitive_buffers, and SdfInput.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84ea9d72-8541-403a-98f6-c13904b1ae8b
📒 Files selected for processing (2)
src/sdf.rssrc/sdf/boolean.rs
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdf/boolean.rs (1)
164-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear render-world
SdfExtractedOperandwhen removing operand membership from a group.
remove_from_grouponly removes(SdfOperandOf, SdfBooleanOp, SdfOrder)in the main world.extract_sdf_operandsonly insertsSdfExtractedOperandin the render world for entities that still match those main-world components (withChanged<...>gating), and there’s no shown render-world removal path forSdfExtractedOperand. Sinceupdate_input_buffersrelies onRemovedComponents<SdfExtractedOperand>and also builds group/operand maps fromQuery<(&SdfExtractedOperand, &MainEntity)>, a removed-from-group operand can leave stale parent/op/order data in the render world until despawn/reattachment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdf/boolean.rs` around lines 164 - 165, remove_from_group currently only removes the main-world markers (SdfOperandOf, SdfBooleanOp, SdfOrder) which leaves SdfExtractedOperand in the render world and causes stale data for extract_sdf_operands / update_input_buffers; modify the remove_from_group implementation to also clear SdfExtractedOperand in the render world for the same entity (use whatever render-world command API you have to remove SdfExtractedOperand from that entity) so that RemovedComponents<SdfExtractedOperand> and subsequent queries reflect the removal immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/sdf/boolean.rs`:
- Around line 164-165: remove_from_group currently only removes the main-world
markers (SdfOperandOf, SdfBooleanOp, SdfOrder) which leaves SdfExtractedOperand
in the render world and causes stale data for extract_sdf_operands /
update_input_buffers; modify the remove_from_group implementation to also clear
SdfExtractedOperand in the render world for the same entity (use whatever
render-world command API you have to remove SdfExtractedOperand from that
entity) so that RemovedComponents<SdfExtractedOperand> and subsequent queries
reflect the removal immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c40c6db-1f4b-4d7b-989e-19399f7d1ef7
📒 Files selected for processing (2)
src/sdf.rssrc/sdf/boolean.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdf.rs
90bf8f2 to
0d4ac3b
Compare
nixonyh
left a comment
nixonyh
left a comment
There was a problem hiding this comment.
Initial pass review! Will do a few more passes once these are addressed! Thanks :)
| } | ||
|
|
||
| // Evaluate the raw SDF distance for a single input at world-space `point`. | ||
| fn eval_primitive(input: SdfInput, point: vec3f) -> f32 { |
There was a problem hiding this comment.
Might want to move this and all the sd functions into a new file. (This can be a follow up though, so feel free to ignore this)
| /// The group entity owns references to all its operand children. | ||
| #[derive(Component, Default)] | ||
| #[relationship_target(relationship = SdfOperandOf, linked_spawn)] | ||
| pub struct SdfOperands(Vec<Entity>); |
There was a problem hiding this comment.
| pub struct SdfOperands(Vec<Entity>); | |
| pub struct SdfOperands(Entity); |
Can this be a one to one relationship, from what I know, boolean operation only involves 2 things (hence the name boolean)
| PartialOrd, | ||
| Ord, | ||
| )] | ||
| pub struct SdfOrder(pub usize); |
There was a problem hiding this comment.
If we can reduce down to 1 to 1 relationship, this shud be removed.
| let nested_inner_group = commands | ||
| .spawn(( | ||
| SdfGroup::new(nested_medium_sphere) | ||
| .difference(nested_small_sphere), | ||
| SdfTransform::default(), | ||
| )) | ||
| .id(); | ||
|
|
||
| commands.spawn(( | ||
| SdfGroup::new(nested_big_sphere) | ||
| .difference(nested_inner_group), | ||
| SdfTransform::default() | ||
| .with_translation(Vec3::new(0.0, 1.5, 0.0)), | ||
| )); |
There was a problem hiding this comment.
I envision the API to be sth like:
let boolean_op = SdfBoolean::new(big_sphere).difference(medium_sphere).difference(small_sphere);
// Then maybe..
boolean_op.build(); // Or sth like that?Cuz the current API is quite complex (not user friendly).
…ionally inserting PrimitiveIndex
0d4ac3b to
31eed6b
Compare
mrclputra
left a comment
mrclputra
left a comment
There was a problem hiding this comment.
lgtm
- commented from mobile phone
… into feature/node_graph
3 participants
Resolved #17
Replaces the rigid
ChildOfoperand approach with a customSdfOperandOf/SdfOperandsrelationship system, enabling fully nested SDF group hierarchies.Changes
1. Relationship Model (
src/sdf/boolean.rs)SdfOperandOf(Entity): Component on each operand entity pointing to its parent group.SdfOperands(Vec<Entity>): Component on the group entity listing its children (uses Bevy's#[relationship_target]withlinked_spawnfor automatic cleanup).SdfBooleanOp(BooleanOp)/SdfOrder(usize): Per-operand metadata for evaluation order and boolean operation.SdfExtractedOperand: Render-world copy of operand state for GPU buffer construction.SdfEntityCommandsExttrait: Ergonomicadd_to_group(),remove_from_group(),set_boolean_op(), andset_order()methods.2. Flat Preorder Buffer Layout (
src/sdf.rs)group_idassignment.emit_group(): Recursively writes a group header + its children (primitives and sub-groups) into a flatBufferVec<SdfInput>in DFS preorder.parent_indexis set to the header's buffer position, giving the GPU a stable tree to walk.update_input_buffers: Handles root-level primitives (no parent), group hierarchy collection, and order sorting.RemovedComponentstracking implemented for both primitives and operands.3. Stack-Based Shader Composition (
assets/shaders/sdf_raymarch.wgsl)parent_indexagainst stack entries.min()on stack overflow instead of silently dropping geometry.4. Transform Propagation (
src/sdf/transform.rs)SdfTransform: Local, user-authored transform with#[require(SdfGlobalTransform)].propagate_transformsystem: Walks the operand graph iteratively, recomputingworld_from_local(including scale viaAffine3A) only when local transforms change.SdfOperandOfand starts from root groups (Without<SdfOperandOf>).5. Builder API & Ergonomics
SdfGroup::new(entity).union(b).difference(c).intersect(d)allows fluent operand construction..translate(Vec3) -> SdfGroupBuilder,.build() -> impl Bundle.SdfTransform::from_xyz(x, y, z)shorthand added.SdfSphere, etc.) andSdfGrouphave#[require(SdfTransform)].6. Performance Optimizations
update_primitive_buffersandupdate_input_buffersearly-return when nothing changed.extract_sdf_operandsusesChanged<T>filters to avoid redundant extraction.input_count == 0.