Grida Canvas - Outline Mode #511
Conversation
- Introduced a new example for rendering scenes in both normal and outline modes, generating a reference image. - Refactored the picture cache to support multiple render variants. - Implemented a comprehensive render policy system to manage rendering behaviors, including outline styles and effects. - Updated the painter to handle standard and outline rendering modes effectively.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
WalkthroughThis pull request introduces a render policy system that enables conditional rendering modes (standard vs. wireframe outline), threads this policy through the rendering pipeline, implements variant-based picture caching, and exposes outline mode toggling via the editor UI, hotkeys, and WASM bindings. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Editor UI<br/>(Hotkey/Menu)
participant Editor as Editor Surface
participant Canvas as Canvas Runtime
participant Painter as Painter &<br/>Renderer
participant Cache as Scene Cache
UI->>Editor: surfaceToggleOutlineMode()
Editor->>Editor: Dispatch outline-mode action
Editor->>Editor: Compute RenderPolicyFlags
Editor->>Canvas: __runtime_renderer_set_outline_mode(...)
Canvas->>Canvas: Convert flags to RenderPolicy
Canvas->>Painter: set_render_policy(policy)
Painter->>Painter: Compute variant_key from policy
Canvas->>Canvas: Queue redraw
Canvas->>Painter: draw_layer_list(layers, policy)
Painter->>Cache: get_node_picture_variant(id, variant_key)
alt Cache hit
Cache-->>Painter: Cached picture
else Cache miss
Painter->>Painter: Render with policy<br/>(wireframe/standard)
Painter->>Cache: set_node_picture_variant(id, variant_key, picture)
Cache-->>Painter: Stored
end
Painter->>Painter: Apply outline or fills/strokes<br/>based on policy
Painter-->>UI: ✓ Updated canvas
sequenceDiagram
participant Standard as Standard Render
participant Wireframe as Wireframe Render
participant Policy as RenderPolicy
Standard->>Policy: is_wireframe() == false
Policy-->>Standard: Render fills & strokes
Standard->>Standard: Apply texture, effects,<br/>compositing
Wireframe->>Policy: is_wireframe() == true
Policy-->>Wireframe: outline_style()
Wireframe->>Wireframe: Build glyph paths for text
Wireframe->>Wireframe: Draw layer outlines<br/>with stroke paint
Wireframe->>Policy: ignore_clips_content?
Policy-->>Wireframe: Skip clip application
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 functionality to ignore clip paths when outline mode is enabled, enhancing the debugging and inspection capabilities. - Introduced new actions and state management for toggling the clip path behavior in the editor. - Updated the render policy to accommodate the new outline mode preferences, ensuring consistent rendering behavior across different modes. - Refactored the painter to utilize helper methods for optional clipping, improving code readability and maintainability.
![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: 46a705090a
ℹ️ 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".
|
|
||
| for entry in &list.layers { | ||
| if let Some(scene_cache) = self.scene_cache { |
There was a problem hiding this comment.
In wireframe mode the renderer iterates list.layers directly, so it never executes PainterRenderCommand::MaskGroup. That means masks are always ignored in outline mode, even when the user disables “Ignore clips content” and expects masks/clips to apply. In masked scenes this will draw outlines outside the mask and diverge from the standard render; you likely need to route outline rendering through the command list or explicitly handle MaskGroup when ignore_clips_content is false.
Useful? React with 👍 / 👎.
| useHotkeys("meta+shift+o, ctrl+shift+o", () => { | ||
| const v = editor.surface.surfaceToggleOutlineMode(); | ||
| toast.success(`Outline Mode ${v === "on" ? "on" : "off"}`); |
There was a problem hiding this comment.
Cmd/Ctrl+Shift+O is bound by browsers to open the bookmarks manager. The new outline toggle handler doesn’t call preventDefault, so in Chrome/Edge/Firefox this shortcut will open the browser UI and the app hotkey will be unreliable. Add preventDefault: true (like the Cmd/Ctrl+Y handler just below) to keep the shortcut working inside the editor.
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: 0
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/window/application_emscripten.rs (1)
339-339: Address clippy warning: unreachable expression.The pipeline reports a warning about an unreachable expression on this line. The
unreachable!()macro is placed after areturnstatement in the#[cfg(target_os = "emscripten")]block (line 336), making it dead code when compiling for emscripten.Consider restructuring to avoid the warning:
🛠️ Suggested fix
#[cfg(target_os = "emscripten")] unsafe { // Register the animation frame callback with the leaked pointer. let app_ptr = Box::into_raw(_app); emscripten_request_animation_frame_loop( Some(request_animation_frame_callback), app_ptr as *mut _, ); // Reconstruct the box so the caller retains ownership. return Box::from_raw(app_ptr); } - unreachable!("emscipten cannot be initialized on native") + #[cfg(not(target_os = "emscripten"))] + unreachable!("emscripten cannot be initialized on native") }
🧹 Nitpick comments (4)
crates/grida-canvas/src/cache/picture.rs (1)
59-64: Consider removing unnecessary.clone()onNodeId.
NodeIdisu64, which implementsCopy. The.clone()call on line 63 is unnecessary sinceCopytypes are implicitly copied. This applies toset_node_picture_variantas well whereidis moved.♻️ Suggested simplification
pub fn get_node_picture_variant(&self, id: &NodeId, variant_key: u64) -> Option<&Picture> { if variant_key == 0 { return self.default_store.get(id); } - self.variant_store.get(&(id.clone(), variant_key)) + self.variant_store.get(&(*id, variant_key)) }crates/grida-canvas/src/runtime/render_policy.rs (1)
207-294: Add a doc comment in Rust referencing the TypeScript file to aid cross-language synchronization.Flag values are currently synchronized between Rust and TypeScript. However, the TypeScript file (
editor/grida-canvas/render-policy-flags.ts) already documents that Rust is the source of truth, while the Rust side has no reciprocal reference. Consider adding a module-level or section comment inrender_policy.rssimilar to the one in the TypeScript file to remind maintainers that these constants define an ABI boundary and must stay in sync across both files. For example:/// Render-policy bitflags used by the WASM renderer. /// /// **Note**: Keep these synchronized with `editor/grida-canvas/render-policy-flags.ts`. /// These constants define an ABI boundary between the Rust renderer and the TypeScript editor.This bidirectional cross-reference will make the synchronization requirement explicit in both files.
editor/grida-canvas-react/viewport/hotkeys.tsx (1)
472-483: RedundantpreventDefaultcall.The
e.preventDefault()call on line 476 is redundant sincepreventDefault: trueis already specified in the options object on line 481. Consider removing one of them for clarity.♻️ Suggested simplification
useHotkeys( "meta+y, ctrl+y", - (e) => { - // prevent default browser behavior (e.g. open history) - e.preventDefault(); + () => { const v = editor.surface.surfaceToggleOutlineMode(); toast.success(`Outline Mode ${v === "on" ? "on" : "off"}`); }, { preventDefault: true, } );crates/grida-canvas/src/painter/painter.rs (1)
1254-1281: Consider extracting glyph path building into a shared helper.The glyph path iteration logic in
build_text_glyph_path(lines 1260-1279) is nearly identical to the logic indraw_text_backdrop_blur(lines 462-481). Both visit paragraph glyphs, extract paths, apply position offsets, and accumulate into a PathBuilder.♻️ Suggested refactor to reduce duplication
Extract a shared helper method:
fn build_glyph_path_from_paragraph( paragraph: &Rc<RefCell<textlayout::Paragraph>>, y_offset: f32, ) -> Path { let mut builder = PathBuilder::new(); paragraph.borrow_mut().visit(|_, info| { if let Some(info) = info { let glyphs = info.glyphs(); let positions = info.positions(); let origin = info.origin(); let font = info.font(); for (glyph, pos) in glyphs.iter().zip(positions.iter()) { if let Some(glyph_path) = font.get_path(*glyph) { let offset = Point::new(pos.x + origin.x, pos.y + origin.y + y_offset); if offset.x != 0.0 || offset.y != 0.0 { let transformed = glyph_path.make_transform(&Matrix::translate((offset.x, offset.y))); builder.add_path(&transformed); } else { builder.add_path(&glyph_path); } } } } }); builder.detach() }Then both
draw_text_backdrop_blurandbuild_text_glyph_pathcan use this shared helper.Also applies to: 454-482
1 participant
day-315-grida-canvas-outline-mode.mp4
CtrlCmd + YorCtrlCmd + Shift + YRelease Notes