Reimplement background checkerboard rendering#4034
Reimplement background checkerboard rendering#4034timon-schelling wants to merge 18 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.
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.
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.
There was a problem hiding this comment.
1 issue found across 1 file (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/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:368">
P2: This merge uses `Vec::contains` inside the loop, making background merges quadratic. If background lists grow with graph size, this will scale poorly. Consider using a `HashSet` to track seen backgrounds while pushing into the vec.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| vector_data.extend(other.vector_data.iter().map(|(id, data)| (*id, data.clone()))); | ||
|
|
||
| // TODO: Find a better non O(n^2) way to merge backgrounds | ||
| for background in &other.backgrounds { |
There was a problem hiding this comment.
P2: This merge uses Vec::contains inside the loop, making background merges quadratic. If background lists grow with graph size, this will scale poorly. Consider using a HashSet to track seen backgrounds while pushing into the vec.
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 368:
<comment>This merge uses `Vec::contains` inside the loop, making background merges quadratic. If background lists grow with graph size, this will scale poorly. Consider using a `HashSet` to track seen backgrounds while pushing into the vec.</comment>
<file context>
@@ -363,9 +363,12 @@ impl RenderMetadata {
- *backgrounds = other.backgrounds.clone();
+
+ // TODO: Find a better non O(n^2) way to merge backgrounds
+ for background in &other.backgrounds {
+ if !backgrounds.contains(background) {
+ backgrounds.push(background.clone());
</file context>
a661a14 to
64feff4
Compare
| pub trait RenderBackground { | ||
| fn render_background_to_vello(&self, scene: &mut vello::Scene, transform: DAffine2, render_params: &RenderParams); | ||
| fn render_background_svg(&self, render: &mut SvgRender, render_params: &RenderParams); | ||
| } |
There was a problem hiding this comment.
It feels like this would be cleaner if we only had a single render function which takes one background as the argument and which has two structs implementing the trait, one SVG background render and a vello version. Those would then store the necessary scenes etc. It feels weird to have these leak into the trait definitions
There was a problem hiding this comment.
Then also change that for the normal render trait?
There was a problem hiding this comment.
Ah, good question, I was only thinking about the new code. If we were to refactor the main render code, that should happen in a separate pr
| for background in &other.backgrounds { | ||
| if !backgrounds.contains(background) { | ||
| backgrounds.push(background.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
You can sort both arrays and then iterate them in lock step. This gives you an O(nlogn) algorithm
There was a problem hiding this comment.
Sure, would need implementing CMP though, the DVec2 don't. Or use a HashSet or similar to make contains cheap.
There was a problem hiding this comment.
You are not using DVec2s you use IVecs, so this can just be derived.
|
This pr now introduces the same bind group creation regression as reported in #3910 |
|
Even though the bind groups are no longer used, the browser is bad at collecting garbage. And since they reference textures, those textures can't be deallocated until the bindgroup is destroyed, inflating the memory usage |
timon-schelling
force-pushed
the
redo-transparent-bg-render
branch
from
1dbea96 to
40e586a
Compare
|
I seem to have fixed this on my branch with a cached blitter impl |
3 participants
Improves the implementation from #4022. Closes #4021.