perf: GPU rendering pipeline overhaul — FrameLoop, Camera2D caching, save_layer elimination#594
Merged
softmarshmallow merged 23 commits intomainfrom Mar 21, 2026
Merged
perf: GPU rendering pipeline overhaul — FrameLoop, Camera2D caching, save_layer elimination#594softmarshmallow merged 23 commits intomainfrom
softmarshmallow merged 23 commits intomainfrom
Conversation
- Introduced a new `FrameLoop` struct to manage frame rendering and quality, decoupling it from the legacy frame scheduling approach. - Updated the `Camera2D` struct to use host-provided time for tracking changes, ensuring compatibility with WASM. - Modified the `Renderer` to support the new frame plan building and flushing mechanism. - Removed the deprecated `FrameScheduler` and associated files to streamline the codebase. - Updated application logic to utilize the new `FrameLoop` for rendering decisions, improving performance and responsiveness.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a host-driven FrameLoop, removes scheduler/timeout, introduces frame-quality lifecycle, prefilters compositor indices, extracts promoted blits, tightens opacity isolation with non-overlapping-fill computation, refactors painter/geometry for shape-level draw/clip and opacity/blend fast-paths, adds bench-report JSON tooling and many pixel-accuracy tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Host
participant FrameLoop as FrameLoop
participant Renderer as Renderer
participant Painter as Painter
participant Compositor as Compositor
Host->>FrameLoop: poll(now)
alt idle
FrameLoop-->>Host: None
else Unstable
FrameLoop-->>Host: Unstable
Host->>Renderer: build_frame_plan(...)
Renderer->>Compositor: prefilter compositor_indices
Renderer->>Painter: draw_render_commands(plan, promoted_blits)
Painter-->>Renderer: draw ops / promoted blits (draw_image_rect)
Renderer->>Renderer: mid-frame flush/submit
Renderer->>Compositor: update_compositor(plan.compositor_indices)
Compositor-->>Renderer: compositor updates / blits
Host->>FrameLoop: complete(Unstable)
else Stable
FrameLoop-->>Host: Stable
Host->>Renderer: build_frame_plan(stable=true)
Renderer->>Painter: draw_render_commands(...)
Renderer->>Compositor: update_compositor(...)
Host->>FrameLoop: complete(Stable)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
- Added caching for zoom and view matrix in the `Camera2D` struct to reduce computational overhead during rendering. - Implemented methods to invalidate and warm the cache, ensuring efficient updates on camera transformations. - Updated the application logic to call `warm_cache()` once per frame, improving performance for view-related calculations.
- Introduced a new `PromotedBlit` struct to store pre-extracted blit data for compositor-cached nodes, including image, source and destination rectangles, opacity, and blend mode. - Updated the `Painter` struct to utilize the `promoted_blits` map, allowing inline blitting of promoted nodes at their correct z-position during rendering. - Added a `build_promoted_blits` method in the `Renderer` to extract and prepare blit data for all promoted nodes, ensuring proper z-order and performance improvements. - Created a new test suite to verify that compositor-cached rendering matches live rendering for nodes with effects, ensuring visual consistency.
- Introduced a `RectConfig` struct to encapsulate rectangle properties such as opacity, blend mode, and effects. - Enhanced rectangle creation functions to utilize the new configuration, allowing for more flexible rendering options. - Implemented a benchmarking function to evaluate the performance of the new opacity folding optimization, reducing GPU surface allocations for semi-transparent nodes without effects. - Added tests to verify pixel-level accuracy of the opacity folding optimization, ensuring correct rendering behavior across different scenarios.
- Added benchmarking functions for opaque and normal blend modes at 1k and 10k nodes, optimizing rendering by eliminating unnecessary save_layer calls. - Updated the Painter implementation to recognize Normal blend mode as equivalent to PassThrough for leaf nodes, improving efficiency. - Introduced a pixel-level correctness test to verify that Normal blend mode produces identical results to PassThrough, ensuring rendering accuracy across different scenarios.
- Added a new method `draw_fills_with_opacity` to the `Painter` struct, allowing for direct multiplication of layer opacity into paint alpha for fills-only leaf nodes, eliminating unnecessary GPU surface allocations. - Updated the `draw_layer_standard` method to utilize this new fast path when conditions are met, improving rendering efficiency for simple blends without strokes or effects. - Enhanced tests to verify the correctness of the new rendering path, ensuring pixel-level accuracy for semi-transparent fills.
…imization - Introduced `stroke_overlaps_fill` and `non_overlapping_fill_path` fields in `PainterPictureShapeLayer` to optimize rendering when strokes overlap fills. - Added `compute_non_overlapping_fill_path` method to calculate fill paths with stroke regions subtracted, improving performance by allowing per-paint-alpha opacity folding. - Updated `Painter` methods to utilize the new fill path optimization, ensuring correct rendering behavior for overlapping strokes and fills. - Enhanced tests to verify pixel-level accuracy for the new rendering paths, ensuring visual consistency across different scenarios.
softmarshmallow
changed the title
- Refactored `render_noise_effect`, `draw_drop_shadow`, and other methods to utilize `draw_on_canvas` and `clip_on_canvas` for direct shape rendering, improving performance by avoiding unnecessary path creation. - Introduced new methods in `PainterShape` to leverage Skia's specialized draw calls for rect, rrect, and oval shapes, enhancing rendering efficiency. - Updated relevant methods in `Painter` to utilize the new shape methods, ensuring consistent performance improvements across rendering operations.
- Introduced a fast path in `sk_paint_stack` and `sk_paint_stack_without_images` to directly set color on the paint for single solid fills, avoiding shader allocation and improving performance. - Updated the `with_opacity` method in `Painter` to accept optional bounds, allowing for tighter GPU buffer allocations and further enhancing rendering efficiency. - Enhanced documentation to reflect the optimizations and their impact on rendering performance, particularly for opacity handling and solid color paints.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c9844c89a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+768
to
+772
| self.frame_loop.invalidate(now); | ||
|
|
||
| // Legacy path: also eager-queue an unstable frame on the renderer | ||
| // so that the existing `redraw()` flow continues to work when | ||
| // hosts call the old `redraw()` entry point. |
There was a problem hiding this comment.
This change removes the only app-level path that queued a full-quality frame after interaction, but the native host still renders through RedrawRequested -> app.redraw_requested() and its 240 Hz tick only calls tick_with_current_time() (crates/grida-dev/src/platform/native_application.rs). That means FrameLoop::poll() is never consulted on native, so after a pan/zoom the single unstable frame is flushed and no stable frame is ever requested. In practice, native windows can remain at the interactive render scale / stale compositor zoom until some unrelated input triggers another redraw.
Useful? React with 👍 / 👎.
Comment on lines
+1709
to
+1713
| &vector_layer.shape, | ||
| vec_effects, | ||
| &vector_layer.base.transform.matrix, | ||
| None, | ||
| f, |
There was a problem hiding this comment.
with_blendmode[_and_opacity] now uses bounded save-layers, but vector layers still pass None for the stroke bounds expansion even though PainterPictureVectorLayer::shape is built from geometry.get_world_bounds(id) (tight geometry bounds, not render bounds). For semi-transparent or non-normal-blend vectors with center/outside strokes or endpoint markers, the new layer bounds will be too small and clip the outer stroke/marker pixels that used to survive with the unbounded save_layer_alpha path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-canvas/src/runtime/scene.rs (1)
1823-1849:⚠️ Potential issue | 🟠 MajorPromoted images already contain the node opacity.
Both capture paths render with
painter.draw_layer(...), so the snapshot already hasbase.opacitybaked into its pixels. Persistingnode_opacityhere and applying it again in the promoted-blit paint makes promoted semi-transparent nodes render too faint (0.5becomes0.25, etc.). If the intent is to apply opacity at blit time, the capture step needs to normalize it away first.Also applies to: 1874-1889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 1823 - 1849, The captured atlas slot already has node opacity baked into its pixels (painter.draw_layer in the compositor_atlas.draw_into_slot path), so do not persist the original node_opacity for promoted images; instead store a neutral opacity (1.0) when calling scene_cache.compositor.insert_atlas so the blit-time paint doesn't double-apply opacity. Update the two insertion sites that call scene_cache.compositor.insert_atlas (the block using compositor_atlas.draw_into_slot and the similar block around the 1874-1889 region) to pass a neutral opacity or otherwise mark the stored entry as pre-multiplied/opaque so downstream promoted-blit code does not reapply node_opacity.
🧹 Nitpick comments (7)
.agents/skills/research/SKILL.md (1)
82-83: Optionally add guidance on verifying local clone currency.The guidance doesn't mention verifying that local clones are up to date or on the correct branch. While this is marked optional, stale local clones could lead to researching outdated code or patterns.
📋 Optional: Add verification guidance
-**Local clones (optional):** If `~/Documents/GitHub/` exists, it may contain default-style clones (sibling dirs named by repo, e.g. `skia`). Prefer searching there before cloning or using only the web. +**Local clones (optional):** If `~/Documents/GitHub/` exists, it may contain default-style clones (sibling dirs named by repo, e.g. `skia`). Prefer searching there before cloning or using only the web. When using a local clone, verify the branch and consider pulling latest commits to ensure currency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/research/SKILL.md around lines 82 - 83, Update the "Local clones (optional):" guidance to include a short checklist for verifying a local clone is current and on the correct branch: instruct to check the current branch (e.g., using git branch --show-current), fetch remote updates (git fetch --all), compare local vs remote (git status or git rev-list --count HEAD..@{u}), and pull or switch branches as needed (git pull or git checkout <branch>) so researchers avoid using stale code; keep the note concise and mark it optional alongside the existing Local clones text.crates/grida-dev/src/bench/runner.rs (4)
386-388: Consider documenting the#[allow(async_fn_in_trait)]usage.This attribute suppresses the warning about async functions in traits not being object-safe. Since
AsyncSceneLoaderis used withimpl Trait(notdyn Trait), this is fine. A brief comment would clarify this is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/runner.rs` around lines 386 - 388, The usage of #[allow(async_fn_in_trait)] on the AsyncSceneLoader trait is intentional to permit the async fn load(&self, source: &str) -> Result<Vec<Scene>> while the trait is only used via impl Trait (not dyn Trait); add a brief inline comment above the attribute referencing AsyncSceneLoader and load that explains this rationale (e.g., "Allow async fn in trait because this trait is used with impl Trait and not as a trait object, so object-safety is not required") so future readers understand the suppression is intentional.
18-33:count_effects_nodesonly checks Rectangle and Ellipse.Other node types like
Container,TextSpan, etc. also haveeffectsfields with potentially expensive effects. Consider extending the check or documenting why only these two types are counted.♻️ Example extension
fn count_effects_nodes(renderer: &cg::runtime::scene::Renderer) -> usize { renderer .scene .as_ref() .map(|s| { s.graph .nodes_iter() .filter(|(_, node)| match node { Node::Rectangle(r) => r.effects.has_expensive_effects(), Node::Ellipse(e) => e.effects.has_expensive_effects(), + Node::Container(c) => c.effects.has_expensive_effects(), + Node::TextSpan(t) => t.effects.has_expensive_effects(), _ => false, }) .count() }) .unwrap_or(0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/runner.rs` around lines 18 - 33, count_effects_nodes currently only inspects Rectangle and Ellipse for expensive effects; update it to include all Node variants that carry an effects field (for example Container, TextSpan, Image, Group, Clip, etc.) by extending the match in count_effects_nodes to call .effects.has_expensive_effects() for each of those variants, or alternatively add a helper method on Node (e.g., Node::has_expensive_effects()) and call that from count_effects_nodes so all node types with effects are counted consistently.
75-88: Percentile indexing is slightly imprecise for boundary cases.The current calculation uses
n * 95 / 100which forn=100yields index 95 (the 96th element in 0-indexed terms). The standard p95 is typically the value at or below which 95% of observations fall.For benchmark reporting this is acceptable, but consider using
(n - 1) * 95 / 100for more precise percentile semantics, or document the approximation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/runner.rs` around lines 75 - 88, The percentile indexing for PanStats is off-by-one for boundary cases: adjust how you compute indices into frame_times (and p50_us) to use the (n - 1) based index math (e.g., (n - 1) * 95 / 100 and (n - 1) * 99 / 100, and use (n - 1) / 2 for p50) so the returned p95_us/p99_us/p50_us correspond to the conventional percentiles; update the expressions that compute p50_us, p95_us, and p99_us in the PanStats construction (which currently reference frame_times[n / 2], frame_times[n * 95 / 100], frame_times[n * 99 / 100]) to use these (n - 1) adjusted indices and ensure you still handle n == 0 appropriately before indexing.
91-126: Zoom stage breakdown data is captured but discarded.
run_zoom_passreceivesFrameFlushResult::OK(stats)but only extracts total elapsed time. The stage breakdown (stats.draw,stats.mid_flush_duration, etc.) is available but not recorded, unlikerun_pan_pass.If this is intentional, consider adding a comment explaining why zoom doesn't need stage breakdown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/runner.rs` around lines 91 - 126, run_zoom_pass currently matches FrameFlushResult::OK(_) and only records total frame time, discarding the per-stage breakdown available as FrameFlushResult::OK(stats); update run_zoom_pass to capture the stats variable (FrameFlushResult::OK(stats)) and accumulate the same per-stage metrics that run_pan_pass records (e.g., stats.draw, stats.mid_flush_duration, stats.other_stage_fields) into ZoomStats (or into the same aggregate collectors run_pan_pass uses), so zoom reports p50/p95/p99 for those stages too; alternatively, if omitting stage breakdown is intentional, add a clear comment inside run_zoom_pass explaining why FrameFlushResult::OK(stats) stage metrics are ignored.crates/grida-dev/src/bench/args.rs (1)
13-15: Minor: Redundantdefault_value_tfor boolean flag.
default_value_t = falseis unnecessary for boolean flags — clap defaults them tofalseautomatically. Consider simplifying:♻️ Suggested simplification
/// List available scene names and exit. - #[arg(long = "list-scenes", default_value_t = false)] + #[arg(long = "list-scenes")] pub list_scenes: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/args.rs` around lines 13 - 15, The #[arg(...)] attribute on the boolean field list_scenes includes an unnecessary default_value_t = false; remove the default_value_t portion so the attribute becomes #[arg(long = "list-scenes")] and rely on clap's implicit false default for booleans; update the struct field pub list_scenes: bool accordingly in the Bench args (list_scenes) definition.crates/grida-dev/src/bench/report.rs (1)
40-47: Consider adding stage breakdown toZoomStatsfor consistency.
PanStatsincludes per-stage breakdown (draw_us,mid_flush_us,compositor_us,flush_us), butZoomStatsdoes not. If zoom benchmarks also captureFrameFlushResult::OK(stats), the stage data is available but discarded.Adding the breakdown would provide more actionable insights for zoom-specific optimizations. If this is intentional (e.g., zoom invalidates caches differently), a brief comment explaining the asymmetry would be helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/report.rs` around lines 40 - 47, ZoomStats currently lacks the per-stage timing fields present on PanStats, so update the ZoomStats struct to include the same stage breakdown fields (draw_us, mid_flush_us, compositor_us, flush_us) and keep serde::Serialize; then update any code that builds ZoomStats (e.g., where FrameFlushResult::OK(stats) is handled) to populate those new fields from stats to avoid discarding per-stage data, or if omitting was intentional add a brief comment on ZoomStats explaining why stage timings are excluded. Ensure field names match PanStats exactly so downstream consumers remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/research/SKILL.md:
- Around line 82-83: The README currently references a Unix/macOS-specific path
string `~/Documents/GitHub/`; update the guidance in SKILL.md to be
cross-platform by mentioning the equivalent Windows path (e.g.
`C:\Users\<user>\Documents\GitHub\`) and phrasing it generically (e.g. "your
Documents/GitHub folder or equivalent platform-specific location") so Windows
users aren’t confused; keep the optional/cautious language and prefer searching
that local folder before cloning.
In `@crates/grida-canvas/benches/bench_rectangles.rs`:
- Around line 265-509: The benchmarks in bench_opacity_folding (inside function
bench_opacity_folding) never initialize the raster surface, so semi-transparent
cases read uninitialized pixels; fix by clearing the canvas immediately after
creating the Renderer (right after Renderer::new / Backend::new_from_raster and
before renderer.load_scene) to a known opaque background color — either by
calling the renderer's clear method (e.g. renderer.clear_background or
renderer.clear(Color::WHITE) if available) or by drawing a full-size opaque
rectangle to the backend, so each iteration starts from a deterministic
background before renderer.load_scene / renderer.queue_unstable /
renderer.flush.
In `@crates/grida-canvas/src/cache/compositor/promotion.rs`:
- Around line 84-92: The early filter function has_promotable_effects currently
returns true for effects that are context-dependent and will later be rejected
by should_promote as ContextDependent; change has_promotable_effects to only
return true for effects that can be promoted regardless of rendering context by
excluding context-dependent effects (the same predicates that cause
should_promote to return ContextDependent). Concretely, update
has_promotable_effects(layer: &PainterPictureLayer) to check the promotable
effect flags (e.g., shadows, layer blur, noise) while explicitly checking and
omitting any context-dependent flags/variants (the ContextDependent path used by
should_promote) instead of simply delegating to has_expensive_effects. Ensure
you reference the same effect fields/enums used by should_promote so the early
filter aligns with final promotion decisions.
In `@crates/grida-canvas/src/node/schema.rs`:
- Around line 33-43: LayerEffects::is_empty currently checks structural presence
(Option::is_none / Vec::is_empty) and thus treats toggled-off effects as active;
change it to detect active effects instead: for each optional field (blur,
backdrop_blur, glass) use as_ref().map_or(true, |e| !e.is_active()) (or check
the specific active flag/method on that effect type) and for shadows/noises use
!self.shadows.iter().any(|s| s.is_active()) and !self.noises.iter().any(|n|
n.is_active()); return true only when none of the effects report active. Ensure
you call the actual active predicate provided by the effect types rather than
relying on Option/Vec emptiness.
In `@crates/grida-canvas/src/painter/layer.rs`:
- Around line 447-469: compute_non_overlapping_fill_path currently performs
PathOp::Difference for every inside/center fill+stroke shape, incurring
unnecessary CPU work for opaque cases; change the call sites (where flattening
inside/center fill+stroke shapes) to compute a needs_non_overlapping_fill_path
flag using the exact same conditions that enable the opacity-fold fast-path at
draw time, and only call compute_non_overlapping_fill_path when that flag is
true (or add a boolean parameter to compute_non_overlapping_fill_path and skip
skia_safe::op unless the flag indicates the layer can avoid isolation); ensure
you reference compute_non_overlapping_fill_path and the call sites that pass
stroke_path/stroke_overlaps_fill so PathOp::Difference is only executed when the
non-overlapping fill optimization will actually be used.
In `@crates/grida-canvas/src/painter/painter.rs`:
- Around line 1359-1372: The can_fold_into_paint predicate currently only checks
the layer blend mode (LayerBlendMode in the is_simple_blend check) but must also
ensure every individual paint in the shape's fills and strokes uses
BlendMode::Normal; update the condition used to compute can_fold_into_paint
(near the existing is_simple_blend and has_stroke_fill_overlap logic) to
additionally verify that all paint entries in shape_layer.fills and
shape_layer.strokes have their blend_mode == BlendMode::Normal (so folding node
opacity into paint alpha won't change compositing for per-paint blend modes);
reference paint::sk_paint_stack() and draw_*_with_opacity() behavior when adding
this check.
- Around line 1378-1419: The fast-path for pure translations skips calling
draw_stroke_decorations and thus drops markers; update the translation predicate
in painter.rs (the if that currently checks shape_layer.base.clip_path,
non_overlapping_fill_path and matrix m) to also ensure the shape has no stroke
markers (e.g., require stroke-markers field is None or call a
has_stroke_markers()/has_markers() helper on shape_layer) before taking the fast
path; if markers are present, fall through to the normal rendering path so
draw_stroke_decorations is executed after draw_stroke_path_with_opacity.
In `@crates/grida-canvas/src/runtime/camera.rs`:
- Around line 208-225: The zoom setter (_set_zoom()) mutates self.transform and
cached_zoom but does not invalidate the precomputed cached_view_matrix_inv
populated by warm_cache(), allowing subsequent calls (e.g., set_zoom_at() →
screen_to_canvas_point()) to use a stale inverse; update _set_zoom() to
clear/invalidate cached_view_matrix_inv (and do the same in the analogous block
at the second occurrence) immediately after mutating self.transform/cached_zoom
so callers compute a fresh inverse before any further projections; reference
cached_view_matrix_inv, _set_zoom(), warm_cache(), after_change(),
set_zoom_at(), and screen_to_canvas_point() when making the change.
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 1246-1263: The loop is currently treating any existing compositor
cache as promotable, which lets stale cached entries (e.g., after zoom) be
promoted and removed from live_indices so the frame blits old content; instead,
only treat a cache entry as promoted if the compositor cache entry is present
and fresh. Change the logic in the block that inspects
self.scene_cache.layers.layers (the loop using has_promotable_effects,
self.scene_cache.compositor.peek, promoted_ids, and live_indices) to: when
peek(&entry.id) returns Some(cache_entry) verify the cache_entry is not stale
(or has a freshness flag) before pushing entry.id into promoted_ids or removing
the index from live_indices; if the cache is stale, leave the index in
live_indices so update_compositor*_inner can recapture it. Ensure
has_promotable_effects behavior remains unchanged and only freshness gates
promotion.
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 794-806: process_image_queue() and process_font_queue() can
consume new resources while the app is otherwise idle, but because
frame_loop.poll(now) is called after them and returns None the freshly loaded
assets never trigger a render; change process_image_queue and process_font_queue
to return a bool indicating whether they consumed/decoded any new resources,
call them before polling, and if either returns true then force the frame loop
to schedule a render (e.g. call the frame loop wake/request method or treat that
as a non-idle by setting the poll result) so the newly loaded font/image is
presented immediately; update all callers and adjust
process_image_queue/process_font_queue signatures (and any helpers) accordingly
and ensure you use frame_loop.poll(now) only after accounting for those returned
booleans.
In `@crates/grida-canvas/tests/compositor_effects.rs`:
- Around line 117-131: The test helper draw_layer_composited() currently
captures the offscreen image and blits it to final_canvas using an unscaled dst
rect and a default SkPaint, so it doesn't model the runtime zoomed presentation
or cached opacity/blend; update the helper to multiply the destination Rect (dst
or dst_rect) by zoom (i.e., scale x,y,width,height by zoom) before calling
final_canvas.draw_image_rect and construct the SkPaint to mirror the runtime
blit (apply the layer's cached opacity and blend mode/alpha settings used by the
compositor) so the test exercises the same zoomed, composited path as the real
runtime.
In `@docs/wg/feat-2d/stroke-fill-opacity.md`:
- Around line 38-43: Several fenced code blocks in the document (for example the
pseudocode block showing save_layer_alpha/draw_fill/draw_stroke/restore and the
Chromium snippet blocks) lack explicit language tags which triggers
markdownlint; update each fenced code block flagged (including the blocks around
the save_layer_alpha example and the other ranges called out) to include an
explicit language tag—use "text" for pseudocode blocks and "cpp" for
Chromium/C++ snippets—so the fences become e.g. ```text and ```cpp respectively.
- Around line 1-3: The frontmatter block for the document titled "Stroke-Fill
Opacity Compositing" is missing the MDX opt-out; add a YAML key `format: md` to
the existing frontmatter (the top --- block) so the file is treated as plain
Markdown rather than MDX, preserving its raw comparison characters like `<` and
`>` during rendering.
---
Outside diff comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 1823-1849: The captured atlas slot already has node opacity baked
into its pixels (painter.draw_layer in the compositor_atlas.draw_into_slot
path), so do not persist the original node_opacity for promoted images; instead
store a neutral opacity (1.0) when calling scene_cache.compositor.insert_atlas
so the blit-time paint doesn't double-apply opacity. Update the two insertion
sites that call scene_cache.compositor.insert_atlas (the block using
compositor_atlas.draw_into_slot and the similar block around the 1874-1889
region) to pass a neutral opacity or otherwise mark the stored entry as
pre-multiplied/opaque so downstream promoted-blit code does not reapply
node_opacity.
---
Nitpick comments:
In @.agents/skills/research/SKILL.md:
- Around line 82-83: Update the "Local clones (optional):" guidance to include a
short checklist for verifying a local clone is current and on the correct
branch: instruct to check the current branch (e.g., using git branch
--show-current), fetch remote updates (git fetch --all), compare local vs remote
(git status or git rev-list --count HEAD..@{u}), and pull or switch branches as
needed (git pull or git checkout <branch>) so researchers avoid using stale
code; keep the note concise and mark it optional alongside the existing Local
clones text.
In `@crates/grida-dev/src/bench/args.rs`:
- Around line 13-15: The #[arg(...)] attribute on the boolean field list_scenes
includes an unnecessary default_value_t = false; remove the default_value_t
portion so the attribute becomes #[arg(long = "list-scenes")] and rely on clap's
implicit false default for booleans; update the struct field pub list_scenes:
bool accordingly in the Bench args (list_scenes) definition.
In `@crates/grida-dev/src/bench/report.rs`:
- Around line 40-47: ZoomStats currently lacks the per-stage timing fields
present on PanStats, so update the ZoomStats struct to include the same stage
breakdown fields (draw_us, mid_flush_us, compositor_us, flush_us) and keep
serde::Serialize; then update any code that builds ZoomStats (e.g., where
FrameFlushResult::OK(stats) is handled) to populate those new fields from stats
to avoid discarding per-stage data, or if omitting was intentional add a brief
comment on ZoomStats explaining why stage timings are excluded. Ensure field
names match PanStats exactly so downstream consumers remain consistent.
In `@crates/grida-dev/src/bench/runner.rs`:
- Around line 386-388: The usage of #[allow(async_fn_in_trait)] on the
AsyncSceneLoader trait is intentional to permit the async fn load(&self, source:
&str) -> Result<Vec<Scene>> while the trait is only used via impl Trait (not dyn
Trait); add a brief inline comment above the attribute referencing
AsyncSceneLoader and load that explains this rationale (e.g., "Allow async fn in
trait because this trait is used with impl Trait and not as a trait object, so
object-safety is not required") so future readers understand the suppression is
intentional.
- Around line 18-33: count_effects_nodes currently only inspects Rectangle and
Ellipse for expensive effects; update it to include all Node variants that carry
an effects field (for example Container, TextSpan, Image, Group, Clip, etc.) by
extending the match in count_effects_nodes to call
.effects.has_expensive_effects() for each of those variants, or alternatively
add a helper method on Node (e.g., Node::has_expensive_effects()) and call that
from count_effects_nodes so all node types with effects are counted
consistently.
- Around line 75-88: The percentile indexing for PanStats is off-by-one for
boundary cases: adjust how you compute indices into frame_times (and p50_us) to
use the (n - 1) based index math (e.g., (n - 1) * 95 / 100 and (n - 1) * 99 /
100, and use (n - 1) / 2 for p50) so the returned p95_us/p99_us/p50_us
correspond to the conventional percentiles; update the expressions that compute
p50_us, p95_us, and p99_us in the PanStats construction (which currently
reference frame_times[n / 2], frame_times[n * 95 / 100], frame_times[n * 99 /
100]) to use these (n - 1) adjusted indices and ensure you still handle n == 0
appropriately before indexing.
- Around line 91-126: run_zoom_pass currently matches FrameFlushResult::OK(_)
and only records total frame time, discarding the per-stage breakdown available
as FrameFlushResult::OK(stats); update run_zoom_pass to capture the stats
variable (FrameFlushResult::OK(stats)) and accumulate the same per-stage metrics
that run_pan_pass records (e.g., stats.draw, stats.mid_flush_duration,
stats.other_stage_fields) into ZoomStats (or into the same aggregate collectors
run_pan_pass uses), so zoom reports p50/p95/p99 for those stages too;
alternatively, if omitting stage breakdown is intentional, add a clear comment
inside run_zoom_pass explaining why FrameFlushResult::OK(stats) stage metrics
are ignored.
🪄 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
Run ID: b6c3ed80-f048-4e0a-a87c-8dbe7e32dc69
⛔ Files ignored due to path filters (1)
crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasmis excluded by!**/*.wasm
📒 Files selected for processing (36)
.agents/skills/cg-perf/SKILL.md.agents/skills/research/SKILL.mdcrates/grida-canvas-wasm/lib/bin/grida-canvas-wasm.jscrates/grida-canvas-wasm/package.jsoncrates/grida-canvas/benches/bench_rectangles.rscrates/grida-canvas/examples/tool_gen_bench_fixture.rscrates/grida-canvas/src/cache/compositor/promotion.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/src/painter/effects_noise.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/src/painter/paint.rscrates/grida-canvas/src/painter/painter.rscrates/grida-canvas/src/painter/painter_debug_node.rscrates/grida-canvas/src/painter/shadow.rscrates/grida-canvas/src/runtime/camera.rscrates/grida-canvas/src/runtime/frame_loop.rscrates/grida-canvas/src/runtime/mod.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/sys/mod.rscrates/grida-canvas/src/sys/scheduler.rscrates/grida-canvas/src/sys/timeout.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/window/application_emscripten.rscrates/grida-canvas/tests/compositor_effects.rscrates/grida-canvas/tests/translate_fold_accuracy.rscrates/grida-dev/examples/bench_cache_picture.rscrates/grida-dev/examples/sys_winit_raf.rscrates/grida-dev/src/bench/args.rscrates/grida-dev/src/bench/mod.rscrates/grida-dev/src/bench/report.rscrates/grida-dev/src/bench/runner.rscrates/grida-dev/src/main.rsdocs/wg/feat-2d/optimization.mddocs/wg/feat-2d/stroke-fill-opacity.mdfixtures/test-grida/bench.grida
💤 Files with no reviewable changes (4)
- crates/grida-canvas/src/sys/mod.rs
- crates/grida-canvas/src/sys/scheduler.rs
- crates/grida-dev/examples/sys_winit_raf.rs
- crates/grida-canvas/src/sys/timeout.rs
Comment on lines
+82
to
+83
| **Local clones (optional):** If `~/Documents/GitHub/` exists, it may contain default-style clones (sibling dirs named by repo, e.g. `skia`). Prefer searching there before cloning or using only the web. | ||
|
|
There was a problem hiding this comment.
Consider cross-platform path guidance.
The hard-coded path ~/Documents/GitHub/ is Unix/macOS-specific and may not match Windows conventions (typically C:\Users\<user>\Documents\GitHub\). Since this is marked optional and uses cautious language ("may contain"), the impact is minimal, but Windows users might be momentarily confused.
🌍 Optional: Make the path guidance more cross-platform
-**Local clones (optional):** If `~/Documents/GitHub/` exists, it may contain default-style clones (sibling dirs named by repo, e.g. `skia`). Prefer searching there before cloning or using only the web.
+**Local clones (optional):** Check for local repository clones in common locations (`~/Documents/GitHub/` on Unix/macOS, `%USERPROFILE%\Documents\GitHub\` on Windows). These may contain default-style clones (sibling dirs named by repo, e.g. `skia`). Prefer searching there before cloning or using only the web.📝 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
| **Local clones (optional):** If `~/Documents/GitHub/` exists, it may contain default-style clones (sibling dirs named by repo, e.g. `skia`). Prefer searching there before cloning or using only the web. | |
| **Local clones (optional):** Check for local repository clones in common locations (`~/Documents/GitHub/` on Unix/macOS, `%USERPROFILE%\Documents\GitHub\` on Windows). These may contain default-style clones (sibling dirs named by repo, e.g. `skia`). Prefer searching there before cloning or using only the web. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/research/SKILL.md around lines 82 - 83, The README currently
references a Unix/macOS-specific path string `~/Documents/GitHub/`; update the
guidance in SKILL.md to be cross-platform by mentioning the equivalent Windows
path (e.g. `C:\Users\<user>\Documents\GitHub\`) and phrasing it generically
(e.g. "your Documents/GitHub folder or equivalent platform-specific location")
so Windows users aren’t confused; keep the optional/cautious language and prefer
searching that local folder before cloning.
Comment on lines
+265
to
+509
| fn bench_opacity_folding(c: &mut Criterion) { | ||
| let width = 1000; | ||
| let height = 1000; | ||
|
|
||
| let mut group = c.benchmark_group("opacity_folding"); | ||
| group.sample_size(100); | ||
| group.measurement_time(std::time::Duration::from_secs(10)); | ||
|
|
||
| // --- 1K nodes --- | ||
|
|
||
| // Baseline: opaque, no save_layer needed | ||
| group.bench_function("1k_opaque_passthrough", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(1_000), | ||
| RectConfig { | ||
| opacity: 1.0, | ||
| blend_mode: LayerBlendMode::default(), | ||
| with_effects: false, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| // Optimized path: opacity folded into blend save_layer (1 save_layer) | ||
| group.bench_function("1k_semitransparent_normal", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(1_000), | ||
| RectConfig { | ||
| opacity: 0.8, | ||
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | ||
| with_effects: false, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| // Cannot fold: effects need separate opacity isolation (2+ save_layers) | ||
| group.bench_function("1k_semitransparent_normal_effects", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(1_000), | ||
| RectConfig { | ||
| opacity: 0.8, | ||
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | ||
| with_effects: true, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| // Opaque + Normal blend — previously wasted a save_layer, now zero overhead | ||
| group.bench_function("1k_opaque_normal", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(1_000), | ||
| RectConfig { | ||
| opacity: 1.0, | ||
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | ||
| with_effects: false, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| // --- 10K nodes --- | ||
|
|
||
| group.bench_function("10k_opaque_passthrough", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(10_000), | ||
| RectConfig { | ||
| opacity: 1.0, | ||
| blend_mode: LayerBlendMode::default(), | ||
| with_effects: false, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| // Opaque + Normal blend at 10k — should match opaque_passthrough now | ||
| group.bench_function("10k_opaque_normal", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(10_000), | ||
| RectConfig { | ||
| opacity: 1.0, | ||
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | ||
| with_effects: false, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| group.bench_function("10k_semitransparent_normal", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(10_000), | ||
| RectConfig { | ||
| opacity: 0.8, | ||
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | ||
| with_effects: false, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| group.bench_function("10k_semitransparent_normal_effects", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(10_000), | ||
| RectConfig { | ||
| opacity: 0.8, | ||
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | ||
| with_effects: true, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| // --- PassThrough + opacity (save_layer_alpha path) --- | ||
|
|
||
| group.bench_function("10k_semitransparent_passthrough", |b| { | ||
| b.iter(|| { | ||
| let mut renderer = Renderer::new( | ||
| Backend::new_from_raster(width, height), | ||
| None, | ||
| Camera2D::new(Size { | ||
| width: width as f32, | ||
| height: height as f32, | ||
| }), | ||
| ); | ||
| let scene = create_rectangles_cfg( | ||
| black_box(10_000), | ||
| RectConfig { | ||
| opacity: 0.8, | ||
| blend_mode: LayerBlendMode::default(), // PassThrough | ||
| with_effects: false, | ||
| }, | ||
| ); | ||
| renderer.load_scene(scene); | ||
| renderer.queue_unstable(); | ||
| renderer.flush(); | ||
| renderer.free(); | ||
| }) | ||
| }); | ||
|
|
||
| group.finish(); |
There was a problem hiding this comment.
Initialize the canvas in the new opacity-folding benchmarks.
Unlike bench_rectangles(), these cases never clear the raster surface to a known background. Several variants draw semi-transparent rectangles, so the measurements now depend on whatever pixels the fresh backend happens to contain, which makes the comparisons noisy and hard to line up with the existing benchmarks.
Suggested fix
b.iter(|| {
let mut renderer = Renderer::new(
Backend::new_from_raster(width, height),
None,
Camera2D::new(Size {
width: width as f32,
height: height as f32,
}),
);
+ let canvas = renderer.canvas();
+ let mut paint = skia_safe::Paint::default();
+ paint.set_color(skia_safe::Color::WHITE);
+ canvas.draw_rect(
+ skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32),
+ &paint,
+ );
let scene = create_rectangles_cfg(📝 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
| fn bench_opacity_folding(c: &mut Criterion) { | |
| let width = 1000; | |
| let height = 1000; | |
| let mut group = c.benchmark_group("opacity_folding"); | |
| group.sample_size(100); | |
| group.measurement_time(std::time::Duration::from_secs(10)); | |
| // --- 1K nodes --- | |
| // Baseline: opaque, no save_layer needed | |
| group.bench_function("1k_opaque_passthrough", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::default(), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Optimized path: opacity folded into blend save_layer (1 save_layer) | |
| group.bench_function("1k_semitransparent_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Cannot fold: effects need separate opacity isolation (2+ save_layers) | |
| group.bench_function("1k_semitransparent_normal_effects", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: true, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Opaque + Normal blend — previously wasted a save_layer, now zero overhead | |
| group.bench_function("1k_opaque_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // --- 10K nodes --- | |
| group.bench_function("10k_opaque_passthrough", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::default(), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Opaque + Normal blend at 10k — should match opaque_passthrough now | |
| group.bench_function("10k_opaque_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| group.bench_function("10k_semitransparent_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| group.bench_function("10k_semitransparent_normal_effects", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: true, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // --- PassThrough + opacity (save_layer_alpha path) --- | |
| group.bench_function("10k_semitransparent_passthrough", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::default(), // PassThrough | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| group.finish(); | |
| fn bench_opacity_folding(c: &mut Criterion) { | |
| let width = 1000; | |
| let height = 1000; | |
| let mut group = c.benchmark_group("opacity_folding"); | |
| group.sample_size(100); | |
| group.measurement_time(std::time::Duration::from_secs(10)); | |
| // --- 1K nodes --- | |
| // Baseline: opaque, no save_layer needed | |
| group.bench_function("1k_opaque_passthrough", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::default(), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Optimized path: opacity folded into blend save_layer (1 save_layer) | |
| group.bench_function("1k_semitransparent_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Cannot fold: effects need separate opacity isolation (2+ save_layers) | |
| group.bench_function("1k_semitransparent_normal_effects", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: true, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Opaque + Normal blend — previously wasted a save_layer, now zero overhead | |
| group.bench_function("1k_opaque_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(1_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // --- 10K nodes --- | |
| group.bench_function("10k_opaque_passthrough", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::default(), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // Opaque + Normal blend at 10k — should match opaque_passthrough now | |
| group.bench_function("10k_opaque_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 1.0, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| group.bench_function("10k_semitransparent_normal", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| group.bench_function("10k_semitransparent_normal_effects", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::Blend(BlendMode::Normal), | |
| with_effects: true, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| // --- PassThrough + opacity (save_layer_alpha path) --- | |
| group.bench_function("10k_semitransparent_passthrough", |b| { | |
| b.iter(|| { | |
| let mut renderer = Renderer::new( | |
| Backend::new_from_raster(width, height), | |
| None, | |
| Camera2D::new(Size { | |
| width: width as f32, | |
| height: height as f32, | |
| }), | |
| ); | |
| let canvas = renderer.canvas(); | |
| let mut paint = skia_safe::Paint::default(); | |
| paint.set_color(skia_safe::Color::WHITE); | |
| canvas.draw_rect( | |
| skia_safe::Rect::from_xywh(0.0, 0.0, width as f32, height as f32), | |
| &paint, | |
| ); | |
| let scene = create_rectangles_cfg( | |
| black_box(10_000), | |
| RectConfig { | |
| opacity: 0.8, | |
| blend_mode: LayerBlendMode::default(), // PassThrough | |
| with_effects: false, | |
| }, | |
| ); | |
| renderer.load_scene(scene); | |
| renderer.queue_unstable(); | |
| renderer.flush(); | |
| renderer.free(); | |
| }) | |
| }); | |
| group.finish(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/benches/bench_rectangles.rs` around lines 265 - 509, The
benchmarks in bench_opacity_folding (inside function bench_opacity_folding)
never initialize the raster surface, so semi-transparent cases read
uninitialized pixels; fix by clearing the canvas immediately after creating the
Renderer (right after Renderer::new / Backend::new_from_raster and before
renderer.load_scene) to a known opaque background color — either by calling the
renderer's clear method (e.g. renderer.clear_background or
renderer.clear(Color::WHITE) if available) or by drawing a full-size opaque
rectangle to the backend, so each iteration starts from a deterministic
background before renderer.load_scene / renderer.queue_unstable /
renderer.flush.
Comment on lines
+84
to
+92
| /// Returns true if the node has any effects that make it a candidate for | ||
| /// compositor promotion (shadows, layer blur, noise). | ||
| /// | ||
| /// This is a cheap check on struct fields (no HashMap lookups). Use it as | ||
| /// an early filter before the full `should_promote` evaluation to skip | ||
| /// nodes that will never be promoted. | ||
| pub fn has_promotable_effects(layer: &PainterPictureLayer) -> bool { | ||
| has_expensive_effects(layer) | ||
| } |
There was a problem hiding this comment.
Exclude context-dependent effects from the early promotable filter.
Line 90 currently returns true for some nodes (e.g., active glass) that should_promote will always reject as ContextDependent, so this early filter does extra work and reduces prefilter effectiveness.
Suggested fix
pub fn has_promotable_effects(layer: &PainterPictureLayer) -> bool {
- has_expensive_effects(layer)
+ has_expensive_effects(layer) && !has_context_dependent_effects(layer)
}📝 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
| /// Returns true if the node has any effects that make it a candidate for | |
| /// compositor promotion (shadows, layer blur, noise). | |
| /// | |
| /// This is a cheap check on struct fields (no HashMap lookups). Use it as | |
| /// an early filter before the full `should_promote` evaluation to skip | |
| /// nodes that will never be promoted. | |
| pub fn has_promotable_effects(layer: &PainterPictureLayer) -> bool { | |
| has_expensive_effects(layer) | |
| } | |
| /// Returns true if the node has any effects that make it a candidate for | |
| /// compositor promotion (shadows, layer blur, noise). | |
| /// | |
| /// This is a cheap check on struct fields (no HashMap lookups). Use it as | |
| /// an early filter before the full `should_promote` evaluation to skip | |
| /// nodes that will never be promoted. | |
| pub fn has_promotable_effects(layer: &PainterPictureLayer) -> bool { | |
| has_expensive_effects(layer) && !has_context_dependent_effects(layer) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/cache/compositor/promotion.rs` around lines 84 - 92,
The early filter function has_promotable_effects currently returns true for
effects that are context-dependent and will later be rejected by should_promote
as ContextDependent; change has_promotable_effects to only return true for
effects that can be promoted regardless of rendering context by excluding
context-dependent effects (the same predicates that cause should_promote to
return ContextDependent). Concretely, update has_promotable_effects(layer:
&PainterPictureLayer) to check the promotable effect flags (e.g., shadows, layer
blur, noise) while explicitly checking and omitting any context-dependent
flags/variants (the ContextDependent path used by should_promote) instead of
simply delegating to has_expensive_effects. Ensure you reference the same effect
fields/enums used by should_promote so the early filter aligns with final
promotion decisions.
Comment on lines
+33
to
+43
| /// Returns true when there are no effects at all (no shadows, blur, | ||
| /// backdrop blur, glass, or noise). Used for fast-path dispatch | ||
| /// to skip the effects pipeline entirely for simple nodes. | ||
| #[inline] | ||
| pub fn is_empty(&self) -> bool { | ||
| self.blur.is_none() | ||
| && self.backdrop_blur.is_none() | ||
| && self.glass.is_none() | ||
| && self.shadows.is_empty() | ||
| && self.noises.is_empty() | ||
| } |
There was a problem hiding this comment.
LayerEffects::is_empty should use active-effect checks, not structural presence.
At Lines 38-42, inactive effects still make is_empty() return false, which can incorrectly disable the “skip effects pipeline” fast path for nodes with toggled-off effects.
Suggested fix
#[inline]
pub fn is_empty(&self) -> bool {
- self.blur.is_none()
- && self.backdrop_blur.is_none()
- && self.glass.is_none()
- && self.shadows.is_empty()
- && self.noises.is_empty()
+ !self.blur.as_ref().is_some_and(|b| b.active)
+ && !self.backdrop_blur.as_ref().is_some_and(|b| b.active)
+ && !self.glass.as_ref().is_some_and(|g| g.active)
+ && !self.shadows.iter().any(|s| s.active())
+ && !self.noises.iter().any(|n| n.active)
}📝 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
| /// Returns true when there are no effects at all (no shadows, blur, | |
| /// backdrop blur, glass, or noise). Used for fast-path dispatch | |
| /// to skip the effects pipeline entirely for simple nodes. | |
| #[inline] | |
| pub fn is_empty(&self) -> bool { | |
| self.blur.is_none() | |
| && self.backdrop_blur.is_none() | |
| && self.glass.is_none() | |
| && self.shadows.is_empty() | |
| && self.noises.is_empty() | |
| } | |
| /// Returns true when there are no effects at all (no shadows, blur, | |
| /// backdrop blur, glass, or noise). Used for fast-path dispatch | |
| /// to skip the effects pipeline entirely for simple nodes. | |
| #[inline] | |
| pub fn is_empty(&self) -> bool { | |
| !self.blur.as_ref().is_some_and(|b| b.active) | |
| && !self.backdrop_blur.as_ref().is_some_and(|b| b.active) | |
| && !self.glass.as_ref().is_some_and(|g| g.active) | |
| && !self.shadows.iter().any(|s| s.active()) | |
| && !self.noises.iter().any(|n| n.active) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/node/schema.rs` around lines 33 - 43,
LayerEffects::is_empty currently checks structural presence (Option::is_none /
Vec::is_empty) and thus treats toggled-off effects as active; change it to
detect active effects instead: for each optional field (blur, backdrop_blur,
glass) use as_ref().map_or(true, |e| !e.is_active()) (or check the specific
active flag/method on that effect type) and for shadows/noises use
!self.shadows.iter().any(|s| s.is_active()) and !self.noises.iter().any(|n|
n.is_active()); return true only when none of the effects report active. Ensure
you call the actual active predicate provided by the effect types rather than
relying on Option/Vec emptiness.
Comment on lines
+447
to
+469
| /// Compute a fill path with the stroke region subtracted (PathOp::Difference). | ||
| /// | ||
| /// Returns `Some(path)` when stroke overlaps fill and both are present. | ||
| /// The resulting path, drawn with per-paint-alpha opacity, produces output | ||
| /// identical to `save_layer_alpha` group isolation — zero overlap, zero | ||
| /// GPU surface allocations. | ||
| /// | ||
| /// Returns `None` if no overlap, fills/strokes are empty, or PathOp fails | ||
| /// (degenerate geometry). The painter falls back to `save_layer_alpha`. | ||
| fn compute_non_overlapping_fill_path( | ||
| shape: &PainterShape, | ||
| stroke_path: Option<&skia_safe::Path>, | ||
| stroke_overlaps_fill: bool, | ||
| fills: &Paints, | ||
| strokes: &Paints, | ||
| ) -> Option<skia_safe::Path> { | ||
| if !stroke_overlaps_fill || fills.is_empty() || strokes.is_empty() { | ||
| return None; | ||
| } | ||
| stroke_path.and_then(|sp| { | ||
| skia_safe::op(&shape.to_path(), sp, skia_safe::PathOp::Difference) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Gate PathOp::Difference behind the actual opacity-fold fast path.
compute_non_overlapping_fill_path() is now invoked while flattening every inside/center fill+stroke shape, but most of those layers will never need the pre-subtracted fill path. That means the CPU cost of PathOp::Difference is paid on the display-list path even for the common opaque case where the result is dead data. Please only precompute this when the layer can actually avoid isolation by using the non-overlapping fill path.
Possible direction
- fn compute_non_overlapping_fill_path(
+ fn compute_non_overlapping_fill_path(
shape: &PainterShape,
stroke_path: Option<&skia_safe::Path>,
stroke_overlaps_fill: bool,
fills: &Paints,
strokes: &Paints,
+ needs_non_overlapping_fill_path: bool,
) -> Option<skia_safe::Path> {
- if !stroke_overlaps_fill || fills.is_empty() || strokes.is_empty() {
+ if !needs_non_overlapping_fill_path
+ || !stroke_overlaps_fill
+ || fills.is_empty()
+ || strokes.is_empty()
+ {
return None;
}Derive needs_non_overlapping_fill_path from the same conditions that actually enable the opacity-fold optimization at the call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/painter/layer.rs` around lines 447 - 469,
compute_non_overlapping_fill_path currently performs PathOp::Difference for
every inside/center fill+stroke shape, incurring unnecessary CPU work for opaque
cases; change the call sites (where flattening inside/center fill+stroke shapes)
to compute a needs_non_overlapping_fill_path flag using the exact same
conditions that enable the opacity-fold fast-path at draw time, and only call
compute_non_overlapping_fill_path when that flag is true (or add a boolean
parameter to compute_non_overlapping_fill_path and skip skia_safe::op unless the
flag indicates the layer can avoid isolation); ensure you reference
compute_non_overlapping_fill_path and the call sites that pass
stroke_path/stroke_overlaps_fill so PathOp::Difference is only executed when the
non-overlapping fill optimization will actually be used.
Comment on lines
+1246
to
+1263
| if effective_layer_compositing { | ||
| // Separate promoted (cached) nodes from live-drawn nodes. | ||
| let mut live_indices = Vec::new(); | ||
| for &idx in &indices { | ||
| if let Some(entry) = self.scene_cache.layers.layers.get(idx) { | ||
| if crate::cache::compositor::promotion::has_promotable_effects(&entry.layer) { | ||
| compositor_indices.push(idx); | ||
| } | ||
| if self.scene_cache.compositor.peek(&entry.id).is_some() { | ||
| promoted_ids.push(entry.id); | ||
| } else { | ||
| live_indices.push(idx); | ||
| } | ||
| } | ||
| if !live_indices.is_empty() { | ||
| regions.push((rect, live_indices)); | ||
| } | ||
| if !live_indices.is_empty() { | ||
| regions.push((bounds, live_indices)); | ||
| } |
There was a problem hiding this comment.
Only promote fresh compositor entries here.
Line 1254 promotes any existing cache entry, including entries marked stale after zoom. Those nodes are then removed from live_indices, but the recapture happens later in update_compositor*_inner(), so the current frame still blits the old cache. On the first stable-after-zoom frame that means the refinement work completes after draw and never becomes visible unless something else invalidates again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/runtime/scene.rs` around lines 1246 - 1263, The loop
is currently treating any existing compositor cache as promotable, which lets
stale cached entries (e.g., after zoom) be promoted and removed from
live_indices so the frame blits old content; instead, only treat a cache entry
as promoted if the compositor cache entry is present and fresh. Change the logic
in the block that inspects self.scene_cache.layers.layers (the loop using
has_promotable_effects, self.scene_cache.compositor.peek, promoted_ids, and
live_indices) to: when peek(&entry.id) returns Some(cache_entry) verify the
cache_entry is not stale (or has a freshness flag) before pushing entry.id into
promoted_ids or removing the index from live_indices; if the cache is stale,
leave the index in live_indices so update_compositor*_inner can recapture it.
Ensure has_promotable_effects behavior remains unchanged and only freshness
gates promotion.
Comment on lines
+794
to
+806
| // 3. Process async resources (native only) | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| { | ||
| self.process_image_queue(); | ||
| self.process_font_queue(); | ||
| } | ||
|
|
||
| let renderer_ptr: *mut Renderer = &mut self.renderer; | ||
| self.queue_stable_timer = Some(self.timer.set_timeout( | ||
| std::time::Duration::from_millis(self.queue_stable_debounce_millis), | ||
| move || unsafe { | ||
| (*renderer_ptr).queue_stable(); | ||
| }, | ||
| )); | ||
| // 4. Poll frame loop — should we render? | ||
| let now = self.clock.now(); | ||
| let quality = match self.frame_loop.poll(now) { | ||
| Some(q) => q, | ||
| None => return false, // idle | ||
| }; |
There was a problem hiding this comment.
Resource updates can get dropped while the app is idle.
process_image_queue() / process_font_queue() only invalidate renderer caches. If either queue changes state here and the app was otherwise idle, frame_loop.poll(now) still returns None, so the freshly loaded font/image will not be presented until some unrelated interaction queues another frame.
One way to keep resource arrivals visible
#[cfg(not(target_arch = "wasm32"))]
{
- self.process_image_queue();
- self.process_font_queue();
+ let resources_changed = self.process_image_queue() || self.process_font_queue();
+ if resources_changed {
+ self.frame_loop.invalidate(self.clock.now());
+ self.renderer.queue_unstable();
+ }
}That does require process_image_queue() / process_font_queue() to return whether they consumed any new resources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/window/application.rs` around lines 794 - 806,
process_image_queue() and process_font_queue() can consume new resources while
the app is otherwise idle, but because frame_loop.poll(now) is called after them
and returns None the freshly loaded assets never trigger a render; change
process_image_queue and process_font_queue to return a bool indicating whether
they consumed/decoded any new resources, call them before polling, and if either
returns true then force the frame loop to schedule a render (e.g. call the frame
loop wake/request method or treat that as a non-idle by setting the poll result)
so the newly loaded font/image is presented immediately; update all callers and
adjust process_image_queue/process_font_queue signatures (and any helpers)
accordingly and ensure you use frame_loop.poll(now) only after accounting for
those returned booleans.
Comment on lines
+117
to
+131
| let dst = Rect::from_xywh( | ||
| render_bounds.x, | ||
| render_bounds.y, | ||
| render_bounds.width, | ||
| render_bounds.height, | ||
| ); | ||
| let img = offscreen.image_snapshot(); | ||
| let src = Rect::new(0.0, 0.0, img.width() as f32, img.height() as f32); | ||
| let paint = SkPaint::default(); | ||
| final_canvas.draw_image_rect( | ||
| &img, | ||
| Some((&src, skia_safe::canvas::SrcRectConstraint::Fast)), | ||
| dst, | ||
| &paint, | ||
| ); |
There was a problem hiding this comment.
draw_layer_composited() doesn't model the zoomed presentation step.
Lines 90-91 scale the offscreen capture by zoom, but Lines 117-131 blit it onto an unscaled final canvas with an unscaled dst_rect. For zoom = 2.0, the helper still presents the layer at world size, so compositor_preserves_blur_at_zoom_2 is not exercising the real on-screen compositor path. The helper also uses a default paint, while the runtime blit path applies cached opacity/blend settings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/tests/compositor_effects.rs` around lines 117 - 131, The
test helper draw_layer_composited() currently captures the offscreen image and
blits it to final_canvas using an unscaled dst rect and a default SkPaint, so it
doesn't model the runtime zoomed presentation or cached opacity/blend; update
the helper to multiply the destination Rect (dst or dst_rect) by zoom (i.e.,
scale x,y,width,height by zoom) before calling final_canvas.draw_image_rect and
construct the SkPaint to mirror the runtime blit (apply the layer's cached
opacity and blend mode/alpha settings used by the compositor) so the test
exercises the same zoomed, composited path as the real runtime.
Comment on lines
+1
to
+3
| --- | ||
| title: "Stroke-Fill Opacity Compositing" | ||
| --- |
There was a problem hiding this comment.
Add format: md to the frontmatter.
This file is plain Markdown under docs/wg, and it contains multiple bare comparison expressions with < / > that are safer on the Markdown parser than on MDX.
Suggested fix
---
title: "Stroke-Fill Opacity Compositing"
+format: md
---As per coding guidelines, docs/{wg,reference}/**/*.{md,mdx}: For documentation files that don't use JSX/MDX features, opt out of MDX parsing by adding format: md to the frontmatter.
📝 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
| --- | |
| title: "Stroke-Fill Opacity Compositing" | |
| --- | |
| --- | |
| title: "Stroke-Fill Opacity Compositing" | |
| format: md | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-2d/stroke-fill-opacity.md` around lines 1 - 3, The frontmatter
block for the document titled "Stroke-Fill Opacity Compositing" is missing the
MDX opt-out; add a YAML key `format: md` to the existing frontmatter (the top
--- block) so the file is treated as plain Markdown rather than MDX, preserving
its raw comparison characters like `<` and `>` during rendering.
Comment on lines
+38
to
+43
| ``` | ||
| save_layer_alpha(bounds, opacity) ← offscreen surface | ||
| draw_fill(color, fill_opacity=1.0) | ||
| draw_stroke(color, stroke_opacity=1.0) | ||
| restore ← composite at `opacity` | ||
| ``` |
There was a problem hiding this comment.
Give each fenced block an explicit language.
markdownlint is already flagging all of these fences. Adding text for pseudocode and cpp for the Chromium snippets will clear the warnings and keep the doc consistent.
Also applies to: 55-58, 76-83, 87-90, 98-106, 158-160, 174-187, 198-203
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-2d/stroke-fill-opacity.md` around lines 38 - 43, Several fenced
code blocks in the document (for example the pseudocode block showing
save_layer_alpha/draw_fill/draw_stroke/restore and the Chromium snippet blocks)
lack explicit language tags which triggers markdownlint; update each fenced code
block flagged (including the blocks around the save_layer_alpha example and the
other ranges called out) to include an explicit language tag—use "text" for
pseudocode blocks and "cpp" for Chromium/C++ snippets—so the fences become e.g.
```text and ```cpp respectively.
- Introduced a `PanImageCache` structure to store GPU snapshots of composited frames, enabling a fast path for rendering during pan-only camera changes. - Added constants for tuning rasterization behavior, including `RASTER_ZOOM_RATIO` and `PAN_IMAGE_CACHE_MAX_OFFSET`. - Updated the rendering logic to utilize the cached image when panning, significantly reducing CPU and GPU workload by replacing multiple draw commands with a single texture blit. - Enhanced performance for interactive frames by minimizing re-rasterization and improving responsiveness during camera movements.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/grida-canvas/src/runtime/scene.rs (1)
1262-1267:⚠️ Potential issue | 🟠 MajorDon't promote dirty or stable-frame stale cache entries.
peek(...).is_some()turns any cache entry into a promoted blit. That means a dirty entry can still show old pixels for the current frame, stable-after-zoom frames can still present stretched content because recapture happens later, and entries that lost promotable effects never get evicted becausecompositor_indicesnow filters them out beforeupdate_compositor_inner()can remove them.🐛 Proposed fix
for &idx in &indices { if let Some(entry) = self.scene_cache.layers.layers.get(idx) { - if crate::cache::compositor::promotion::has_promotable_effects(&entry.layer) { - compositor_indices.push(idx); - } - if self.scene_cache.compositor.peek(&entry.id).is_some() { - promoted_ids.push(entry.id); - } else { - live_indices.push(idx); - } + let promotable = + crate::cache::compositor::promotion::has_promotable_effects(&entry.layer); + let cache_entry = self.scene_cache.compositor.peek(&entry.id); + + // Keep cached entries in the compositor scan so they can + // be refreshed or evicted even if the layer is no longer + // promotable. + if promotable || cache_entry.is_some() { + compositor_indices.push(idx); + } + + let can_promote = match cache_entry { + Some(img) => promotable && !img.dirty && (!stable || !img.stale), + None => false, + }; + + if can_promote { + promoted_ids.push(entry.id); + } else { + live_indices.push(idx); + } } }Also applies to: 1272-1280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 1262 - 1267, The code currently uses peek(...).is_some() to pre-filter promotable cache entries into compositor_indices, which inadvertently promotes dirty or stale stable-frame entries and prevents update_compositor_inner() from evicting entries that are no longer promotable; change the pre-filter so it only adds entries that are clean and still promotable (e.g., check the cache entry state for not-dirty and not-stale and that promotable effects still apply) instead of relying on peek(...).is_some(); ensure the same stricter checks are applied where compositor_indices is built (also at the 1272-1280 block) and keep update_compositor_inner() able to see and evict non-promotable entries by not filtering them out prematurely.
🧹 Nitpick comments (1)
crates/grida-canvas/src/runtime/scene.rs (1)
1005-1101: Extract the shared flush body.
flush()andflush_with_plan()now duplicate the same downscale/draw/compositor/submit sequence. Keeping that in one helper will reduce drift between the legacy andFrameLooppaths.♻️ Refactor sketch
+ fn flush_inner(&mut self, plan: FramePlan) -> Option<FrameFlushStats> { + // shared draw/compositor/submit body + } + pub fn flush(&mut self) -> FrameFlushResult { if !self.fc.has_pending() { return FrameFlushResult::NoPending; } @@ - let start = Instant::now(); - // ... - let stats = FrameFlushStats { /* ... */ }; - - self.fc.flush(); - self.plan = None; - - FrameFlushResult::OK(stats) + match self.flush_inner(frame) { + Some(stats) => { + self.fc.flush(); + self.plan = None; + FrameFlushResult::OK(stats) + } + None => FrameFlushResult::NoScene, + } } @@ pub fn flush_with_plan(&mut self, plan: FramePlan) -> Option<FrameFlushStats> { - let start = Instant::now(); - // ... - Some(FrameFlushStats { /* ... */ }) + self.flush_inner(plan) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 1005 - 1101, Extract the duplicated downscale/draw/compositor/submit/stat collection into a single private helper and call it from both flush() and flush_with_plan(): create a method like fn run_flush_plan(&mut self, surface: &mut SurfaceType, plan: FramePlan, scene: &Scene) -> Option<FrameFlushStats> (use the actual surface type returned by backend.get_surface()), move the code that computes width/height, manages self.downscale_surface and self.downscale_dims, calls self.draw(...), performs the mid-frame flush, compositor update (calling update_compositor or update_compositor_stable based on plan.stable), the final flush, and constructs FrameFlushStats into this helper, and then replace the duplicated blocks in both flush_with_plan() and flush() to obtain scene & surface as they already do and then call run_flush_plan(&mut surface, plan, scene) returning its result; ensure the helper updates self.downscale_surface when ds_taken is used and reuses self.draw, update_compositor, update_compositor_stable, config and backend exactly as before so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 1262-1267: The code currently uses peek(...).is_some() to
pre-filter promotable cache entries into compositor_indices, which inadvertently
promotes dirty or stale stable-frame entries and prevents
update_compositor_inner() from evicting entries that are no longer promotable;
change the pre-filter so it only adds entries that are clean and still
promotable (e.g., check the cache entry state for not-dirty and not-stale and
that promotable effects still apply) instead of relying on peek(...).is_some();
ensure the same stricter checks are applied where compositor_indices is built
(also at the 1272-1280 block) and keep update_compositor_inner() able to see and
evict non-promotable entries by not filtering them out prematurely.
---
Nitpick comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 1005-1101: Extract the duplicated
downscale/draw/compositor/submit/stat collection into a single private helper
and call it from both flush() and flush_with_plan(): create a method like fn
run_flush_plan(&mut self, surface: &mut SurfaceType, plan: FramePlan, scene:
&Scene) -> Option<FrameFlushStats> (use the actual surface type returned by
backend.get_surface()), move the code that computes width/height, manages
self.downscale_surface and self.downscale_dims, calls self.draw(...), performs
the mid-frame flush, compositor update (calling update_compositor or
update_compositor_stable based on plan.stable), the final flush, and constructs
FrameFlushStats into this helper, and then replace the duplicated blocks in both
flush_with_plan() and flush() to obtain scene & surface as they already do and
then call run_flush_plan(&mut surface, plan, scene) returning its result; ensure
the helper updates self.downscale_surface when ds_taken is used and reuses
self.draw, update_compositor, update_compositor_stable, config and backend
exactly as before so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b96da92e-3098-45ec-96fa-b394994030d2
📒 Files selected for processing (2)
AGENTS.mdcrates/grida-canvas/src/runtime/scene.rs
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
- Updated `CameraChangeKind` to include a boolean for zoom direction in `PanAndZoom`, improving clarity in camera state changes. - Refactored zoom change detection to handle floating-point precision issues, ensuring accurate zoom state reporting. - Introduced a `PanImageCache` for efficient rendering during pan-only interactions, reducing the need for full redraws and improving performance. - Enhanced the `Renderer` to utilize the pan image cache, allowing for a single texture blit instead of multiple draw commands during panning. - Added tests to validate camera change classifications and interactions, ensuring robustness in gesture handling and rendering behavior.
- Updated the rendering logic in the `Painter` to include stroke decorations when drawing shapes, ensuring that markers are considered during the rendering process. - Added a check for the presence of stroke markers to determine the appropriate rendering path, improving the visual fidelity of shapes with decorations. - Modified the camera's transformation update to invalidate the cached inverse view matrix, ensuring accurate projections after zoom changes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
crates/grida-canvas/src/window/application.rs (1)
794-806:⚠️ Potential issue | 🟠 MajorAsync resource loads still do not wake the FrameLoop.
process_image_queue()/process_font_queue()only mutate renderer state. If a font or image finishes loading while the app is otherwise idle,frame_loop.poll(now)still returnsNone, so the new asset is not presented until some unrelated interaction invalidates again.One way to fix it
#[cfg(not(target_arch = "wasm32"))] { - self.process_image_queue(); - self.process_font_queue(); + let resources_changed = + self.process_image_queue() || self.process_font_queue(); + if resources_changed { + self.frame_loop.invalidate(now); + self.renderer.queue_unstable(); + } }That does require
process_image_queue()andprocess_font_queue()to returnbool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/window/application.rs` around lines 794 - 806, process_image_queue() and process_font_queue() can finish loading assets without any user input but currently don't wake the frame loop, so change both functions to return a bool indicating whether they mutated renderer state (e.g., a "did_change" flag) and in the block where they're called (the #[cfg(not(target_arch = "wasm32"))] section in Application::... ), call them and if either returns true call self.frame_loop.wake() (or otherwise schedule a re-render) before calling self.frame_loop.poll(now) so newly-loaded images/fonts are presented immediately.crates/grida-canvas/src/runtime/scene.rs (1)
1287-1304:⚠️ Potential issue | 🟠 MajorOnly promote usable compositor entries into
promoted_ids.
peek().is_some()currently treats dirty/stale cache entries as drawable and removes them fromlive_indices. The draw pass then blits the old image beforeupdate_compositor*_inner()refreshes it. On the first stable-after-zoom frame, the refined capture is produced after draw and never becomes visible; dirty entries can also lag a frame.Possible fix
- if self.scene_cache.compositor.peek(&entry.id).is_some() { + let can_blit_cached = matches!( + self.scene_cache.compositor.peek(&entry.id), + Some(img) if !img.dirty && (!stable || !img.stale) + ); + if can_blit_cached { promoted_ids.push(entry.id); } else { live_indices.push(idx); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 1287 - 1304, The code promotes any peeked compositor entry into promoted_ids even if the cached entry is dirty/stale, causing old images to be blitted; change the promotion logic in the effective_layer_compositing block (around indices / live_indices / promoted_ids) to only push into promoted_ids when the compositor.peek(...) returns a usable/fresh entry (not just Some). Specifically, after calling self.scene_cache.compositor.peek(&entry.id) ensure the returned compositor entry indicates it is ready/valid (use the compositor API that exposes freshness/state or the peek result's validity) before pushing entry.id into promoted_ids; otherwise treat it as live by adding to live_indices so update_compositor*_inner() can refresh it before drawing.
🧹 Nitpick comments (1)
crates/grida-canvas/tests/camera_change_kind.rs (1)
59-79: Add one end-to-endframe()path regression case.These helpers still go straight through
Renderer::queue_unstable()/flush()and manualconsume_change(). The new regression surface in this PR isUnknownTargetApplication::command()+frame(), so this suite will miss dropped invalidations and stale pending-state issues on the FrameLoop path.Also applies to: 109-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/camera_change_kind.rs` around lines 59 - 79, The current helper queue_flush uses Renderer::queue_unstable() → Renderer::flush() and manual camera.consume_change(), which misses the new regression path through UnknownTargetApplication::command() + frame(); add an end-to-end test case that exercises the frame() path by driving the renderer via UnknownTargetApplication::command(...) and then calling frame() (or the test harness equivalent) so invalidations and pending-state drops are observed; update or add a new test in camera_change_kind.rs that mirrors queue_flush's intent but invokes UnknownTargetApplication::command and frame instead of calling Renderer::flush()/consume_change() directly, referencing queue_flush, Renderer::queue_unstable, Renderer::flush, camera.consume_change, UnknownTargetApplication::command, and frame to locate the related code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 1108-1118: flush_with_plan currently renders the supplied
FramePlan but doesn't clear the legacy pending state, leaving FrameCounter and
self.plan stale; after calling self.render_frame(plan) (or immediately before
returning), clear the legacy pending indicators by resetting self.plan = None
and clearing the frame counter pending flag (e.g., call a method like
self.fc.clear_pending() or self.fc.reset() / set any pending booleans to false)
so the legacy renderer will not remain permanently "pending" and future queue()
calls won't be suppressed or replay stale plans.
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 776-781: The WASM wrapper currently only exposes the old tick()
FFI, so the new Application::frame(time: f64) entrypoint is never called from
the host; add a matching FFI export named frame (e.g. #[no_mangle] pub unsafe
extern "C" fn frame(...)) in the wasm_application module that locates the same
Application instance used by tick() and forwards the call to app.frame(time);
keep the signature and safety pattern consistent with the existing tick() export
and update any JS bindings to call the new frame() export instead of tick().
In `@crates/grida-dev/src/platform/native_application.rs`:
- Around line 315-320: When setting the post-gesture settle_countdown
(settle_countdown = 12) ensure you schedule a stable settle frame instead of
leaving the renderer in the interactive path: replace or supplement the current
queue_unstable() call with queue_stable() (or the native equivalent) so the
compositor runs a full stable refresh; make the same change for the other
occurrence around settle_countdown in the 364-371 block. Reference:
settle_countdown, queue_unstable(), queue_stable().
---
Duplicate comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 1287-1304: The code promotes any peeked compositor entry into
promoted_ids even if the cached entry is dirty/stale, causing old images to be
blitted; change the promotion logic in the effective_layer_compositing block
(around indices / live_indices / promoted_ids) to only push into promoted_ids
when the compositor.peek(...) returns a usable/fresh entry (not just Some).
Specifically, after calling self.scene_cache.compositor.peek(&entry.id) ensure
the returned compositor entry indicates it is ready/valid (use the compositor
API that exposes freshness/state or the peek result's validity) before pushing
entry.id into promoted_ids; otherwise treat it as live by adding to live_indices
so update_compositor*_inner() can refresh it before drawing.
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 794-806: process_image_queue() and process_font_queue() can finish
loading assets without any user input but currently don't wake the frame loop,
so change both functions to return a bool indicating whether they mutated
renderer state (e.g., a "did_change" flag) and in the block where they're called
(the #[cfg(not(target_arch = "wasm32"))] section in Application::... ), call
them and if either returns true call self.frame_loop.wake() (or otherwise
schedule a re-render) before calling self.frame_loop.poll(now) so newly-loaded
images/fonts are presented immediately.
---
Nitpick comments:
In `@crates/grida-canvas/tests/camera_change_kind.rs`:
- Around line 59-79: The current helper queue_flush uses
Renderer::queue_unstable() → Renderer::flush() and manual
camera.consume_change(), which misses the new regression path through
UnknownTargetApplication::command() + frame(); add an end-to-end test case that
exercises the frame() path by driving the renderer via
UnknownTargetApplication::command(...) and then calling frame() (or the test
harness equivalent) so invalidations and pending-state drops are observed;
update or add a new test in camera_change_kind.rs that mirrors queue_flush's
intent but invokes UnknownTargetApplication::command and frame instead of
calling Renderer::flush()/consume_change() directly, referencing queue_flush,
Renderer::queue_unstable, Renderer::flush, camera.consume_change,
UnknownTargetApplication::command, and frame to locate the related code.
🪄 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
Run ID: c110c2ea-2151-4379-8e61-4056af5c9e33
⛔ Files ignored due to path filters (1)
crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasmis excluded by!**/*.wasm
📒 Files selected for processing (7)
crates/grida-canvas-wasm/lib/bin/grida-canvas-wasm.jscrates/grida-canvas/src/runtime/camera.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/tests/camera_change_kind.rscrates/grida-dev/src/platform/native_application.rsdocs/wg/feat-2d/optimization.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/grida-canvas/src/runtime/camera.rs
Comment on lines
+1108
to
+1118
| /// Flush a caller-provided plan: draw + GPU submit + compositor update. | ||
| /// | ||
| /// Returns `Some(stats)` on success, `None` if no scene is loaded. | ||
| /// Unlike the legacy `flush()`, this does NOT consult `FrameCounter` | ||
| /// — the caller (via `FrameLoop`) already decided to render. | ||
| pub fn flush_with_plan(&mut self, plan: FramePlan) -> Option<FrameFlushStats> { | ||
| if self.scene.is_none() { | ||
| return None; | ||
| } | ||
| Some(self.render_frame(plan)) | ||
| } |
There was a problem hiding this comment.
flush_with_plan() should clear the legacy pending state.
UnknownTargetApplication::queue() still seeds the legacy renderer queue for redraw-compat. This path renders a caller-supplied plan but never resets fc / self.plan, so the renderer stays permanently “pending”: later queues stop issuing request_redraw(), and any fallback flush() can replay the stale pre-frame() plan.
Possible fix
pub fn flush_with_plan(&mut self, plan: FramePlan) -> Option<FrameFlushStats> {
if self.scene.is_none() {
return None;
}
- Some(self.render_frame(plan))
+ let stats = self.render_frame(plan);
+ self.fc.flush();
+ self.plan = None;
+ Some(stats)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/runtime/scene.rs` around lines 1108 - 1118,
flush_with_plan currently renders the supplied FramePlan but doesn't clear the
legacy pending state, leaving FrameCounter and self.plan stale; after calling
self.render_frame(plan) (or immediately before returning), clear the legacy
pending indicators by resetting self.plan = None and clearing the frame counter
pending flag (e.g., call a method like self.fc.clear_pending() or
self.fc.reset() / set any pending booleans to false) so the legacy renderer will
not remain permanently "pending" and future queue() calls won't be suppressed or
replay stale plans.
Comment on lines
+776
to
+781
| /// Unified frame entry point. | ||
| /// | ||
| /// Called once per host frame (e.g. from RAF on WASM, RedrawRequested on | ||
| /// native). The engine decides whether to produce pixels and at what | ||
| /// quality. Returns `true` if a frame was rendered. | ||
| pub fn frame(&mut self, time: f64) -> bool { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type rust '\bapp\.(tick|frame)\s*\(' crates/grida-canvas-wasm crates/grida-canvas/src/windowRepository: gridaco/grida
Length of output: 302
🏁 Script executed:
cat -n crates/grida-canvas/src/window/application_emscripten.rs | sed -n '70,110p'Repository: gridaco/grida
Length of output: 1745
🏁 Script executed:
cat -n crates/grida-canvas-wasm/src/wasm_application.rs | sed -n '55,70p'Repository: gridaco/grida
Length of output: 518
Add FFI export for the new frame() entrypoint in the WASM wrapper.
The #[no_mangle] pub unsafe extern "C" fn tick() at crates/grida-canvas-wasm/src/wasm_application.rs:59-63 is the only FFI function exposed to the JavaScript host, and it still calls app.tick(time). The new unified frame() entrypoint introduced at application.rs:776-781 (which drives the FrameLoop lifecycle) is never invoked from the host. A corresponding #[no_mangle] pub unsafe extern "C" fn frame() export must be added so the JavaScript host can call the new entry point and benefit from the FrameLoop improvements documented in application_emscripten.rs:100-104.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/window/application.rs` around lines 776 - 781, The
WASM wrapper currently only exposes the old tick() FFI, so the new
Application::frame(time: f64) entrypoint is never called from the host; add a
matching FFI export named frame (e.g. #[no_mangle] pub unsafe extern "C" fn
frame(...)) in the wasm_application module that locates the same Application
instance used by tick() and forwards the call to app.frame(time); keep the
signature and safety pattern consistent with the existing tick() export and
update any JS bindings to call the new frame() export instead of tick().
Comment on lines
+315
to
+320
| if ok { | ||
| // Schedule a settle redraw ~50ms after the last interaction | ||
| // so the overlay shows "none" when the gesture ends. | ||
| // The 240Hz tick decrements the countdown (~12 ticks ≈ 50ms). | ||
| self.settle_countdown = 12; | ||
| } |
There was a problem hiding this comment.
Queue a stable settle frame here.
This countdown is supposed to produce the post-gesture refine frame, but it still calls queue_unstable(). On native that leaves the renderer on its interactive path, so reduced effects remain active and the compositor never runs its full stable refresh after pan/zoom.
Possible fix
- self.app.renderer_mut().queue_unstable();
+ self.app.renderer_mut().queue_stable();
self.window.request_redraw();Also applies to: 364-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/platform/native_application.rs` around lines 315 - 320,
When setting the post-gesture settle_countdown (settle_countdown = 12) ensure
you schedule a stable settle frame instead of leaving the renderer in the
interactive path: replace or supplement the current queue_unstable() call with
queue_stable() (or the native equivalent) so the compositor runs a full stable
refresh; make the same change for the other occurrence around settle_countdown
in the 364-371 block. Reference: settle_countdown, queue_unstable(),
queue_stable().
This was referenced Mar 24, 2026
This was referenced Mar 31, 2026
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
2 participants
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
Camera2D.tchanged fromstd::time::Instanttof64(host-time ms) for WASM compatibility. Removed deprecatedFrameSchedulerandTimeout.save_layer_alphaGPU surface allocations for simple leaf nodes.BlendMode::Normalas equivalent toPassThroughfor leaf nodes, eliminating unnecessarysave_layercalls.PathOp::Differenceat layer construction. Eliminates overlap artifacts withoutsave_layer_alpha— pixel-exact match with zero GPU overhead.Test plan
Summary by CodeRabbit
New Features
Performance Improvements
Visual Quality
System Improvements
Documentation & Tests