Reimplement background checkerboard rendering#4034
Reimplement background checkerboard rendering#4034timon-schelling wants to merge 12 commits intomasterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 14 files
Confidence score: 2/5
- There is a high-confidence functional risk in
node-graph/nodes/gstd/src/render_cache.rs:store_regions()only evicts before insertion, so cache usage can remain aboveMAX_CACHE_MEMORY_BYTES, which can cause sustained memory pressure/regression. node-graph/libraries/wgpu-executor/src/lib.rsalso has a concrete runtime risk: zero-sized texture requests are not clamped to at least 1x1, which can lead to invalidcreate_texturecalls on edge-case inputs.- Other findings in
node-graph/libraries/wgpu-executor/src/texture_cache.rs(quadratic eviction-path scanning and unconditionalprintln!) plus the PR title wording issue are lower-severity, but they add avoidable performance/operational noise. - Pay close attention to
node-graph/nodes/gstd/src/render_cache.rs,node-graph/libraries/wgpu-executor/src/lib.rs, andnode-graph/libraries/wgpu-executor/src/texture_cache.rs- memory-limit enforcement and texture-size validation are the key correctness risks, with cache-path efficiency/logging cleanup as follow-up priorities.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/nodes/gstd/src/render_cache.rs">
<violation number="1" location="node-graph/nodes/gstd/src/render_cache.rs:186">
P1: `store_regions()` evicts before insertion but not after, so cache memory can exceed `MAX_CACHE_MEMORY_BYTES` and remain over budget.</violation>
</file>
<file name="node-graph/libraries/wgpu-executor/src/lib.rs">
<violation number="1" location="node-graph/libraries/wgpu-executor/src/lib.rs:1">
P2: Custom agent: **PR title enforcement**
PR title uses the non-standard abbreviation "bg", which violates the no-abbreviations-in-prose requirement.</violation>
<violation number="2" location="node-graph/libraries/wgpu-executor/src/lib.rs:100">
P2: Clamp requested texture size to at least 1x1 before forwarding to the cache; otherwise zero-sized inputs can trigger invalid `create_texture` calls.</violation>
</file>
<file name="node-graph/libraries/wgpu-executor/src/texture_cache.rs">
<violation number="1" location="node-graph/libraries/wgpu-executor/src/texture_cache.rs:57">
P2: Remove unconditional `println!` from the texture allocation path to avoid production log spam and I/O overhead.</violation>
<violation number="2" location="node-graph/libraries/wgpu-executor/src/texture_cache.rs:72">
P2: `evict_until_fits` has quadratic scanning in the hot eviction path; track free-bytes incrementally to avoid O(n²) work.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a texture caching system and a GPU-based texture blending mechanism to improve rendering performance and memory management. It refactors the rendering pipeline to use these new components, removes the hide_artboards parameter, and updates the node graph to support background composition. I have identified a high-severity issue in the blending shader where the formula incorrectly handles premultiplied alpha, as well as medium-severity issues regarding hardcoded pixel sizes and the use of println! in library code.
| let background = textureSample(t_background, s_linear, in.tex_coords); | ||
|
|
||
| let a = foreground.a + background.a * (1.0 - foreground.a); | ||
| let rgb = foreground.rgb * foreground.a + background.rgb * background.a * (1.0 - foreground.a); |
There was a problem hiding this comment.
The blending formula assumes non-premultiplied alpha by multiplying the RGB components by their respective alpha values. However, Vello (which produces these textures) outputs premultiplied alpha. If the input textures are already premultiplied, this formula will result in incorrect colors due to double-multiplication. The standard 'Over' operator for premultiplied alpha should be used instead.
let rgb = foreground.rgb + background.rgb * (1.0 - foreground.a);
timon-schelling
changed the title
Performance Benchmark Results
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/wgpu-executor/src/texture_cache.rs">
<violation number="1" location="node-graph/libraries/wgpu-executor/src/texture_cache.rs:79">
P2: `free_bytes -= entry.bytes` can underflow because `free_bytes` is a stale snapshot while `Arc::strong_count` is re-evaluated later per entry.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return true; | ||
| } | ||
| if Arc::strong_count(&entry.texture) == 1 { | ||
| free_bytes -= entry.bytes; |
There was a problem hiding this comment.
P2: free_bytes -= entry.bytes can underflow because free_bytes is a stale snapshot while Arc::strong_count is re-evaluated later per entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/texture_cache.rs, line 79:
<comment>`free_bytes -= entry.bytes` can underflow because `free_bytes` is a stale snapshot while `Arc::strong_count` is re-evaluated later per entry.</comment>
<file context>
@@ -69,13 +64,23 @@ impl TextureCache {
+ return true;
+ }
+ if Arc::strong_count(&entry.texture) == 1 {
+ free_bytes -= entry.bytes;
+ false
+ } else {
</file context>
| free_bytes -= entry.bytes; | |
| free_bytes = free_bytes.saturating_sub(entry.bytes); |
Performance Benchmark Results
|
Performance Benchmark Results
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/interpreted-executor/src/util.rs">
<violation number="1" location="node-graph/interpreted-executor/src/util.rs:54">
P1: `render_background_intermediate` still requires `FOOTPRINT`, but this wiring now declares only `VARARGS`. That can break context dependency tracking and cause incorrect/stale background rendering when viewport footprint changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Performance Benchmark Results
|
Performance Benchmark Results
|
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/rendering/src/background.rs">
<violation number="1" location="node-graph/libraries/rendering/src/background.rs:24">
P2: Checkerboard background rendering lost the `to_canvas()` guard, so non-canvas passes (e.g. thumbnail/mask) can now incorrectly include checkerboards.</violation>
</file>
<file name="node-graph/libraries/rendering/src/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:333">
P2: `backgrounds` merge logic can keep stale metadata when incoming data has the same length, causing outdated checkerboard placement after region merges.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| impl RenderBackground for Background { | ||
| fn render_background_to_vello(&self, scene: &mut vello::Scene, transform: DAffine2, render_params: &RenderParams) { | ||
| let rect = background_rect(self); | ||
| checkerboard_fill_vello(scene, transform, rect, DVec2::new(rect.x0, rect.y0), render_params.viewport_zoom); |
There was a problem hiding this comment.
P2: Checkerboard background rendering lost the to_canvas() guard, so non-canvas passes (e.g. thumbnail/mask) can now incorrectly include checkerboards.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/background.rs, line 24:
<comment>Checkerboard background rendering lost the `to_canvas()` guard, so non-canvas passes (e.g. thumbnail/mask) can now incorrectly include checkerboards.</comment>
<file context>
@@ -1,111 +1,59 @@
+impl RenderBackground for Background {
+ fn render_background_to_vello(&self, scene: &mut vello::Scene, transform: DAffine2, render_params: &RenderParams) {
+ let rect = background_rect(self);
+ checkerboard_fill_vello(scene, transform, rect, DVec2::new(rect.x0, rect.y0), render_params.viewport_zoom);
}
</file context>
| if other.backgrounds.len() > backgrounds.len() { | ||
| *backgrounds = other.backgrounds.clone(); | ||
| } |
There was a problem hiding this comment.
P2: backgrounds merge logic can keep stale metadata when incoming data has the same length, causing outdated checkerboard placement after region merges.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 333:
<comment>`backgrounds` merge logic can keep stale metadata when incoming data has the same length, causing outdated checkerboard placement after region merges.</comment>
<file context>
@@ -319,13 +321,18 @@ impl RenderMetadata {
clip_targets.extend(other.clip_targets.iter());
vector_data.extend(other.vector_data.iter().map(|(id, data)| (*id, data.clone())));
+ // TODO: Don't!
+ if other.backgrounds.len() > backgrounds.len() {
+ *backgrounds = other.backgrounds.clone();
+ }
</file context>
| if other.backgrounds.len() > backgrounds.len() { | |
| *backgrounds = other.backgrounds.clone(); | |
| } | |
| if !other.backgrounds.is_empty() { | |
| *backgrounds = other.backgrounds.clone(); | |
| } |
1 participant
Improves the implementation from #4022. Closes #4021.