fix: nested effects rendering#36
Conversation
📝 WalkthroughWalkthroughThe PR adds two new examples demonstrating box shadow and backdrop blur effects using analytical WGSL shaders. Core framework changes include exposing texture resources in PooledTexture and significantly refactoring the renderer to support backdrop-aware, segment-based rendering with separate offscreen passes for proper effect composition. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant BackdropContext
participant OffscreenTexture as Offscreen<br/>Textures
participant Shader as WGSL<br/>Shaders
participant Output as Final<br/>Output
Renderer->>BackdropContext: plan_traversal (subtree structure)
BackdropContext->>OffscreenTexture: create "behind" texture
Renderer->>OffscreenTexture: render scene behind group
Renderer->>OffscreenTexture: render group subtree (color_texture)
Renderer->>OffscreenTexture: copy output to snapshot
Renderer->>Shader: execute backdrop effect passes
Shader->>OffscreenTexture: ping-pong processing (if needed)
Renderer->>Output: stencil increment (prep mask)
Renderer->>Output: backdrop quad (apply effect)
Renderer->>Output: shape color composition
Output->>Output: final image with backdrop effect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/renderer.rs (1)
2466-2470: Prefer structured logging over println! in library code.Using the logging macros keeps output controllable and avoids polluting stdout.
♻️ Suggested change
- println!( - "Loading effect {} with {} passes", - effect_id, - pass_sources.len() - ); + log::debug!( + "Loading effect {} with {} passes", + effect_id, + pass_sources.len() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer.rs` around lines 2466 - 2470, Replace the stdout println! in the renderer (the call that prints "Loading effect {} with {} passes" using effect_id and pass_sources.len()) with a structured logging macro (e.g., log::info! or tracing::info!) so logs are controllable; update imports to use the chosen logging crate and ensure the application initializes a logger at startup. Keep the same message and variables (effect_id and pass_sources.len()) but use the logger macro inside the same function that currently contains the println! call.
🤖 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/renderer.rs`:
- Around line 3475-3504: subtree_has_backdrop_effects currently only scans
descendants and misses checking the root itself; update the function
(subtree_has_backdrop_effects) to first test whether
backdrop_effects.contains_key(&root_id) and return true if so before calling
scan, leaving scan and the descendant iteration unchanged so nodes that have
both group and backdrop effects are correctly detected.
- Around line 1330-1374: The effect-node ordering is unstable because
effect_node_ids is built from HashMap::keys() and only sorted by depth, causing
siblings at the same depth to be processed in nondeterministic order and miss
earlier siblings' effect_results in render_scene_behind_group; fix by adding a
secondary sort key that reflects deterministic tree draw order (e.g., a
pre-order/traversal index) when sorting effect_node_ids so ties on depth are
broken by traversal/draw order (ensure you reference the same traversal index or
ordering method used by your draw tree), guaranteeing effect_results contains
all previously drawn siblings before creating a behind snapshot for node_id.
---
Nitpick comments:
In `@src/renderer.rs`:
- Around line 2466-2470: Replace the stdout println! in the renderer (the call
that prints "Loading effect {} with {} passes" using effect_id and
pass_sources.len()) with a structured logging macro (e.g., log::info! or
tracing::info!) so logs are controllable; update imports to use the chosen
logging crate and ensure the application initializes a logger at startup. Keep
the same message and variables (effect_id and pass_sources.len()) but use the
logger macro inside the same function that currently contains the println! call.
| // Pre-render the scene content behind this group | ||
| let behind_tex = self.offscreen_texture_pool.acquire( | ||
| &self.device, | ||
| width, | ||
| height, | ||
| self.config.format, | ||
| self.msaa_sample_count, | ||
| ); | ||
| let behind_depth = create_and_depth_texture( | ||
| &self.device, | ||
| (width, height), | ||
| self.msaa_sample_count, | ||
| ); | ||
| let behind_depth_view = | ||
| behind_depth.create_view(&wgpu::TextureViewDescriptor::default()); | ||
|
|
||
| let (behind_color_view, behind_resolve_target) = if behind_tex.sample_count > 1 | ||
| { | ||
| ( | ||
| &behind_tex.color_view, | ||
| Some(behind_tex.resolve_view.as_ref().unwrap() as &wgpu::TextureView), | ||
| ) | ||
| } else { | ||
| (&behind_tex.color_view as &wgpu::TextureView, None) | ||
| }; | ||
|
|
||
| render_scene_behind_group( | ||
| &mut self.draw_tree, | ||
| &mut encoder, | ||
| &effect_results, | ||
| node_id, | ||
| behind_color_view, | ||
| behind_resolve_target, | ||
| &behind_depth_view, | ||
| self.composite_pipeline.as_ref(), | ||
| &pipelines, | ||
| &buffers, | ||
| ); | ||
|
|
||
| // The backdrop copy source is the resolved behind texture | ||
| let behind_copy_source = if behind_tex.sample_count > 1 { | ||
| behind_tex.resolve_texture.as_ref().unwrap() | ||
| } else { | ||
| &behind_tex.color_texture | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for effect_results ordering and traversal logic
rg -n "effect_results" src/renderer.rs | head -30Repository: antouhou/grafo
Length of output: 1719
🏁 Script executed:
#!/bin/bash
# Search for HashMap-related effect construction
rg -n "HashMap|effect_node_ids|group_effects|compute_node_depth" src/renderer.rs | head -40Repository: antouhou/grafo
Length of output: 2372
🏁 Script executed:
#!/bin/bash
# Search for traversal methods
rg -n "traverse" src/renderer.rs | head -30Repository: antouhou/grafo
Length of output: 766
🏁 Script executed:
#!/bin/bash
# Find render_scene_behind_group definition and examine it
rg -n "fn render_scene_behind_group" src/renderer.rs -A 50Repository: antouhou/grafo
Length of output: 2244
🏁 Script executed:
#!/bin/bash
# Check the context around line 1359 to see how render_scene_behind_group is called
sed -n '1355,1375p' src/renderer.rsRepository: antouhou/grafo
Length of output: 877
🏁 Script executed:
#!/bin/bash
# Check if there's any ordering logic related to siblings or draw order
rg -n "draw.?order|sibling|postorder|preorder|post_order|pre_order" src/renderer.rsRepository: antouhou/grafo
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Get the full context around line 1359 and the loop at 1295
sed -n '1284,1370p' src/renderer.rsRepository: antouhou/grafo
Length of output: 4052
🏁 Script executed:
#!/bin/bash
# Check what traverse_mut does - look at its signature and behavior
rg -n "fn traverse_mut|impl.*Tree" src/renderer.rs | head -20Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for the draw_tree definition to understand its traversal order
rg -n "draw_tree" src/renderer.rs | grep "new\|Tree" | head -10Repository: antouhou/grafo
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or implementation about traversal order
rg -n "easy_tree" src/renderer.rs | head -5Repository: antouhou/grafo
Length of output: 308
🏁 Script executed:
#!/bin/bash
# Look for where the draw tree is populated/built to understand its structure
rg -n "push_back\|append\|insert.*draw_tree" src/renderer.rs | head -20Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the full render_scene_behind_group to see what it actually renders
sed -n '4202,4350p' src/renderer.rsRepository: antouhou/grafo
Length of output: 5214
🏁 Script executed:
#!/bin/bash
# Check if easy_tree provides deterministic traversal by tree structure
rg -n "pub fn traverse" --type rust | head -10Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look for any post-order or draw-order sorting logic around effect_node_ids
sed -n '1280,1300p' src/renderer.rsRepository: antouhou/grafo
Length of output: 1006
🏁 Script executed:
#!/bin/bash
# Check how nodes are added to the tree to understand their natural order
rg -n "add_node\|add_child\|insert_node" src/renderer.rs | head -15Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Verify the easy_tree crate to understand traversal ordering
cat Cargo.toml | grep -A 2 "easy_tree"Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for how draw order is maintained or if there's a secondary sort key
rg -n "depth|draw_order|draw.order" src/renderer.rs | grep -E "(sort|cmp)" | head -20Repository: antouhou/grafo
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Check if there's a way to get siblings in order from the tree
rg -n "children|sibling|next_sibling" src/renderer.rs | head -20Repository: antouhou/grafo
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Look for postorder or depth-first traversal functions
rg -n "postorder|post_order|dfs|depth_first" src/renderer.rsRepository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check how the draw tree is built and if sibling insertion order is preserved
rg -n "fn handle.*group|DrawCommand::Group" src/renderer.rs | head -20Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the exact sorting logic and see if there's a secondary sort key
sed -n '1284,1295p' src/renderer.rsRepository: antouhou/grafo
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Look for any deterministic ordering that might be used elsewhere
rg -n "node_id.*sort\|sort.*node" src/renderer.rs | head -10Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check how plan_traversal orders nodes to see if that pattern could apply here
sed -n '3540,3610p' src/renderer.rsRepository: antouhou/grafo
Length of output: 2523
🏁 Script executed:
#!/bin/bash
# Look for how nodes are inserted into the tree to understand if insertion order equals draw order
rg -n "add_child\|push_child\|children_ref_mut" src/renderer.rs | head -20Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if easy_tree maintains insertion order (which would be draw order)
rg -n "easy_tree::" Cargo.lock | head -5Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Examine the loop structure more carefully to understand when effect_results is updated
sed -n '1295,1705p' src/renderer.rs | head -100Repository: antouhou/grafo
Length of output: 4712
🏁 Script executed:
#!/bin/bash
# Check where effect_results is updated during the loop
rg -n "effect_results.insert" src/renderer.rsRepository: antouhou/grafo
Length of output: 124
🏁 Script executed:
#!/bin/bash
# See the full context of effect result insertion to understand when it's updated
sed -n '1690,1710p' src/renderer.rsRepository: antouhou/grafo
Length of output: 913
🏁 Script executed:
#!/bin/bash
# Check if there's any logic that ensures all earlier effects are processed before later ones
sed -n '1275,1310p' src/renderer.rsRepository: antouhou/grafo
Length of output: 1726
🏁 Script executed:
#!/bin/bash
# Check what happens with the behind texture after it's created
sed -n '1360,1430p' src/renderer.rsRepository: antouhou/grafo
Length of output: 3682
🏁 Script executed:
#!/bin/bash
# Verify that easy_tree.traverse_mut respects tree structure (insertion order of children)
rg -n "struct Tree\|impl.*Tree" src/renderer.rs | head -5Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or pattern that guarantees sibling order
rg -n "insertion order\|draw order\|sibling" src/renderer.rs | head -10Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if node_id is assigned in a deterministic order (e.g., breadth-first or depth-first)
rg -n "node_id.*=" src/renderer.rs | grep -E "(push|insert|add)" | head -15Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look for tree traversal to understand if there's a natural tree iteration order
sed -n '4248,4270p' src/renderer.rsRepository: antouhou/grafo
Length of output: 1080
🏁 Script executed:
#!/bin/bash
# Understand the precise timing: when is behind texture created vs effect_results populated
sed -n '1320,1370p' src/renderer.rsRepository: antouhou/grafo
Length of output: 2470
Effect-result ordering can cause incorrect backdrop snapshots when siblings have backdrop effects.
When effect_node_ids is built from HashMap::keys() and sorted only by depth, siblings at the same depth retain HashMap's nondeterministic iteration order. If sibling A is processed before sibling B (both with effects), render_scene_behind_group for A uses effect_results that doesn't yet contain B's entry—so B's rendered content is omitted from A's behind snapshot. Add a secondary sort key (using tree traversal order) to ensure siblings are processed in deterministic draw order, guaranteeing all earlier-drawn siblings are in effect_results before their behind textures are created.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer.rs` around lines 1330 - 1374, The effect-node ordering is
unstable because effect_node_ids is built from HashMap::keys() and only sorted
by depth, causing siblings at the same depth to be processed in nondeterministic
order and miss earlier siblings' effect_results in render_scene_behind_group;
fix by adding a secondary sort key that reflects deterministic tree draw order
(e.g., a pre-order/traversal index) when sorting effect_node_ids so ties on
depth are broken by traversal/draw order (ensure you reference the same
traversal index or ordering method used by your draw tree), guaranteeing
effect_results contains all previously drawn siblings before creating a behind
snapshot for node_id.
| /// Check whether any descendant of `root_id` (exclusive) has a backdrop effect. | ||
| /// | ||
| /// Returns `true` if any child, grandchild, etc. of the given root node has a | ||
| /// backdrop effect attached. Used to decide whether Phase 1 subtree rendering | ||
| /// needs the segment-based render loop with pass-breaking support. | ||
| fn subtree_has_backdrop_effects( | ||
| tree: &easy_tree::Tree<DrawCommand>, | ||
| backdrop_effects: &HashMap<usize, EffectInstance>, | ||
| root_id: usize, | ||
| ) -> bool { | ||
| if backdrop_effects.is_empty() { | ||
| return false; | ||
| } | ||
| fn scan( | ||
| tree: &easy_tree::Tree<DrawCommand>, | ||
| backdrop_effects: &HashMap<usize, EffectInstance>, | ||
| node_id: usize, | ||
| ) -> bool { | ||
| for &child_id in tree.children(node_id) { | ||
| if backdrop_effects.contains_key(&child_id) { | ||
| return true; | ||
| } | ||
| if scan(tree, backdrop_effects, child_id) { | ||
| return true; | ||
| } | ||
| } | ||
| false | ||
| } | ||
| scan(tree, backdrop_effects, root_id) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for calls to subtree_has_backdrop_effects to understand its usage
rg -n "subtree_has_backdrop_effects" src/renderer.rs -A 3 -B 3Repository: antouhou/grafo
Length of output: 850
🏁 Script executed:
# Look for context around group effects and backdrop effects
rg -n "set_group_effect|set_shape_backdrop_effect" src/renderer.rs -A 2 -B 2Repository: antouhou/grafo
Length of output: 592
🏁 Script executed:
# Search for Phase 1 rendering logic and how backdrop effects are handled
rg -n "Phase 1|phase.1|phase1" src/renderer.rs -A 2 -B 2Repository: antouhou/grafo
Length of output: 1175
🏁 Script executed:
# Look for any validation or guards about combining group + backdrop effects
rg -n "group_effect.*backdrop|backdrop.*group_effect" src/renderer.rs -iRepository: antouhou/grafo
Length of output: 40
🏁 Script executed:
# Examine the Phase 1 rendering logic around where subtree_has_backdrop_effects is called
sed -n '1300,1350p' src/renderer.rsRepository: antouhou/grafo
Length of output: 2528
🏁 Script executed:
# Search for where group_effects and backdrop_effects are populated to understand if both can exist on same node
rg -n "group_effects.insert|backdrop_effects.insert" src/renderer.rs -B 2 -A 2Repository: antouhou/grafo
Length of output: 285
🏁 Script executed:
# Check the set_group_effect and set_shape_backdrop_effect methods for any validation
sed -n '2487,2530p' src/renderer.rsRepository: antouhou/grafo
Length of output: 1765
🏁 Script executed:
# Check the set_shape_backdrop_effect method
sed -n '2629,2660p' src/renderer.rsRepository: antouhou/grafo
Length of output: 1313
Check root node for backdrop effects in subtree_has_backdrop_effects.
The function only checks descendants, so if a node has both a group effect and a backdrop effect, Phase 1 will take the fast path and skip applying the backdrop effect on that node. Add a check for the root node before scanning descendants.
Suggested fix
fn subtree_has_backdrop_effects(
tree: &easy_tree::Tree<DrawCommand>,
backdrop_effects: &HashMap<usize, EffectInstance>,
root_id: usize,
) -> bool {
if backdrop_effects.is_empty() {
return false;
}
+ if backdrop_effects.contains_key(&root_id) {
+ return true;
+ }
fn scan(
tree: &easy_tree::Tree<DrawCommand>,
backdrop_effects: &HashMap<usize, EffectInstance>,
node_id: usize,
) -> bool {📝 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.
| /// Check whether any descendant of `root_id` (exclusive) has a backdrop effect. | |
| /// | |
| /// Returns `true` if any child, grandchild, etc. of the given root node has a | |
| /// backdrop effect attached. Used to decide whether Phase 1 subtree rendering | |
| /// needs the segment-based render loop with pass-breaking support. | |
| fn subtree_has_backdrop_effects( | |
| tree: &easy_tree::Tree<DrawCommand>, | |
| backdrop_effects: &HashMap<usize, EffectInstance>, | |
| root_id: usize, | |
| ) -> bool { | |
| if backdrop_effects.is_empty() { | |
| return false; | |
| } | |
| fn scan( | |
| tree: &easy_tree::Tree<DrawCommand>, | |
| backdrop_effects: &HashMap<usize, EffectInstance>, | |
| node_id: usize, | |
| ) -> bool { | |
| for &child_id in tree.children(node_id) { | |
| if backdrop_effects.contains_key(&child_id) { | |
| return true; | |
| } | |
| if scan(tree, backdrop_effects, child_id) { | |
| return true; | |
| } | |
| } | |
| false | |
| } | |
| scan(tree, backdrop_effects, root_id) | |
| } | |
| /// Check whether any descendant of `root_id` (exclusive) has a backdrop effect. | |
| /// | |
| /// Returns `true` if any child, grandchild, etc. of the given root node has a | |
| /// backdrop effect attached. Used to decide whether Phase 1 subtree rendering | |
| /// needs the segment-based render loop with pass-breaking support. | |
| fn subtree_has_backdrop_effects( | |
| tree: &easy_tree::Tree<DrawCommand>, | |
| backdrop_effects: &HashMap<usize, EffectInstance>, | |
| root_id: usize, | |
| ) -> bool { | |
| if backdrop_effects.is_empty() { | |
| return false; | |
| } | |
| if backdrop_effects.contains_key(&root_id) { | |
| return true; | |
| } | |
| fn scan( | |
| tree: &easy_tree::Tree<DrawCommand>, | |
| backdrop_effects: &HashMap<usize, EffectInstance>, | |
| node_id: usize, | |
| ) -> bool { | |
| for &child_id in tree.children(node_id) { | |
| if backdrop_effects.contains_key(&child_id) { | |
| return true; | |
| } | |
| if scan(tree, backdrop_effects, child_id) { | |
| return true; | |
| } | |
| } | |
| false | |
| } | |
| scan(tree, backdrop_effects, root_id) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer.rs` around lines 3475 - 3504, subtree_has_backdrop_effects
currently only scans descendants and misses checking the root itself; update the
function (subtree_has_backdrop_effects) to first test whether
backdrop_effects.contains_key(&root_id) and return true if so before calling
scan, leaving scan and the descendant iteration unchanged so nodes that have
both group and backdrop effects are correctly detected.
1 participant
Summary by CodeRabbit
New Features
Improvements