feat(cg, grida-dev): native surface UI + dev-only editor with translate/resize#617
feat(cg, grida-dev): native surface UI + dev-only editor with translate/resize#617softmarshmallow merged 8 commits intomainfrom
Conversation
The emscripten `add_image` and `add_image_with_rid` methods were not calling `mark_changed(ChangeFlags::IMAGE_LOADED)` after inserting images into the repository. Without this flag, `apply_changes()` never invalidated the PictureCache or compositor cache, so stale pictures recorded before the image loaded (containing no image shader) were replayed indefinitely. This caused image fills to appear only during zoom interactions (where a cache-miss on the unstable variant key forced fresh recording) but disappear on settle and pan frames. Also fix the L0-image fixture: the Transform variant used out-of-range box-relative coordinates (tx=10, ty=20) producing a 1500px offset in a 150px container. Replace with valid values and add a rotation test case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds incremental edit gestures and selection-handle UI: introduces resize/rotate cursor and gesture types, computes/draws screen-space selection handles with hit-testing, maps gestures to editor mutations (translate/resize) that update scene geometry and flush to renderer; removes legacy hit-overlay, exposes new app query APIs, and adds integration tests and examples. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Surface as SurfaceState
participant Editor as EditorDocument
participant Scene as SceneGraph
participant Renderer as Renderer
User->>Surface: Pointer Down (on node)
Surface->>Surface: store pending_pointer_down / maybe select
User->>Surface: Pointer Move (delta)
Surface->>Surface: promote to Translate / start Resize/Rotate
Surface->>Editor: handle_gesture_delta(gesture_before)
Editor->>Editor: compute MutationCommand (Translate/Resize)
Editor->>Scene: apply(MutationCommand)
Scene->>Scene: refresh_node_geo_data(id)
Editor->>Renderer: flush() -> load_scene()
Renderer->>Renderer: rebuild caches
User->>Surface: Pointer Up
Surface->>Surface: end gesture (Idle)
Renderer->>User: redraw updated scene
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
This new document provides a comprehensive guide on estimating GPU render costs for 2D scene operations, focusing on memory bandwidth and fill rate dominance. It includes core principles, effect cost constants, and a per-node cost formula, along with insights on cache efficiency and implicit save_layer triggers. This resource aims to assist developers in optimizing rendering performance.
…te/resize Surface UI (cg): - Selection handles: 8 resize knobs + 4 rotation hit zones - Hover/selection highlight model with bounding rect overlay - Directional resize + rotation cursors - Gesture state: Translate, Resize, Rotate (incremental prev_* model) - Deferred selection for drag-on-selected-node - readonly flag (default true, web unaffected) - show_selection_handles overlay config flag - Query API on UTA: hit_test_point, get_node_bounds, get_union_bounds, point_in_node_bounds, view_matrix - Remove legacy hit_overlay (replaced by surface overlay hover) Editor (grida-dev): - New editor/ module: document.rs, mutation.rs - EditorDocument owns a Scene, applies MutationCommand, flushes via load_scene() - Translate + Resize commands with per-node-type dispatch - Text resize: sets paragraph width, preserves auto axes (Option per axis) - Vector resize: explicitly unsupported (no-op, documented) - Resize geometry helpers: axis detection, origin-shift for NW/N/W handles - Gesture-to-mutation translation in NativeApplication Architecture: - cg is renderer-only, zero mutation code - grida-dev editor owns document state, reads via UTA query API, writes via load_scene() - Node::transform_mut/size_mut moved from cg to grida-dev - refresh_node_geo_data stays on SceneGraph (required for geo_data sync)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 532d53280c
ℹ️ 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".
| let mutated = match (gesture_before, &gesture_after) { | ||
| // ── Translate ──────────────────────────────────────────── | ||
| ( | ||
| SurfaceGesture::Translate { | ||
| prev_canvas: old_pt, |
There was a problem hiding this comment.
Apply first drag delta when entering Translate
This matcher only handles Translate -> Translate, so the first pointer-move that transitions Idle -> Translate is dropped. In the current flow (native_application snapshots gesture_before before dispatch), a drag starting on a selected node promotes to Translate on the first move, but no mutation is emitted until the second move; the selection therefore lags behind the cursor by the initial drag segment on every translate start.
Useful? React with 👍 / 👎.
| for (id, w, h) in &node_sizes { | ||
| mutated |= self.apply(&MutationCommand::Resize { | ||
| id: *id, | ||
| width: if affects_w { | ||
| Some((w * scale_x).max(1.0)) |
There was a problem hiding this comment.
Reposition nodes while resizing multi-selection
Group resize currently scales each node’s width/height but never scales each node’s offset within the selection bounds; only a single global tx/ty translation is applied. For multi-selection (e.g., two separated rectangles), dragging an edge handle should move nodes relative to the group anchor, but this implementation keeps their spacing mostly fixed and produces incorrect geometry for the resized group.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-dev/src/platform/native_application.rs (1)
659-665:⚠️ Potential issue | 🟠 MajorReset the surface interaction state when loading a different scene.
These branches replace the editor document and renderer scene, but keep the previous
selection,hover, andgesturealive onself.app.surface(). That leaves staleNodeIds active across page/file switches and can route export/gesture logic at the wrong scene until the user manually clears state.Also applies to: 710-719, 747-757
🤖 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 659 - 665, When replacing the EditorDocument and loading a new scene (the block that calls EditorDocument::new, renderer.load_scene, fit_camera_to_scene, renderer.queue_unstable, and self.window.request_redraw), also clear the surface interaction state on self.app.surface() so stale NodeId state isn’t carried over; explicitly reset selection, hover and gesture (via the surface mutator / clear methods on the surface object) immediately after swapping the document and before requesting redraw so interactions/export logic are tied to the new scene.
🧹 Nitpick comments (2)
crates/grida-canvas/examples/fixtures/l0_image.rs (1)
37-39: Make transform coefficients self-documenting in the fixture.This works, but naming the coefficients would make fixture intent clearer and easier to tweak later.
♻️ Proposed readability refactor
- ImagePaintFit::Transform(AffineTransform { - matrix: [[1.5, 0.0, -0.25], [0.0, 1.5, -0.15]], - }), + const SCALE: f32 = 1.5; + const OFFSET_X: f32 = -0.25; + const OFFSET_Y: f32 = -0.15; + ImagePaintFit::Transform(AffineTransform { + matrix: [[SCALE, 0.0, OFFSET_X], [0.0, SCALE, OFFSET_Y]], + }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/fixtures/l0_image.rs` around lines 37 - 39, The AffineTransform matrix in ImagePaintFit::Transform is numeric and not self-documenting; update the fixture to construct the transform with named parameters or a small helper so each coefficient is labeled (e.g. scale_x/scale_y, skew_x/skew_y, translate_x/translate_y) instead of a raw matrix array. Locate ImagePaintFit::Transform and AffineTransform in the fixture and replace the matrix literal with either an AffineTransform::new(...) or AffineTransform { scale_x: ..., scale_y: ..., translate_x: ..., translate_y: ... } (or a local helper like make_affine(scale_x, scale_y, tx, ty)) so the intent of 1.5, -0.25, -0.15 is explicit and easier to tweak later.crates/grida-canvas/src/node/scene_graph.rs (1)
808-813: Prefer explicit failure when the node ID is invalid.Line 808 currently no-ops for missing nodes. Returning
SceneGraphResult<()>would make invalid refresh calls visible to callers and easier to debug.♻️ Suggested change
- pub fn refresh_node_geo_data(&mut self, id: &NodeId) { - if let Some(node) = self.nodes.get(id) { - let geo = extract_geo_data(node); - self.geo_data.insert(*id, geo); - } - } + pub fn refresh_node_geo_data(&mut self, id: &NodeId) -> SceneGraphResult<()> { + let node = self + .nodes + .get(id) + .ok_or_else(|| SceneGraphError::NodeNotFound(*id))?; + let geo = extract_geo_data(node); + self.geo_data.insert(*id, geo); + Ok(()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/node/scene_graph.rs` around lines 808 - 813, Change refresh_node_geo_data to return SceneGraphResult<()> and make it fail when the node ID is missing: in refresh_node_geo_data(&mut self, id: &NodeId) call self.nodes.get(id) and if Some proceed to extract_geo_data(node) and insert into self.geo_data, then return Ok(()); if None return an appropriate error (e.g., SceneGraphError::InvalidNodeId(*id) or similar) wrapped in Err(...). Update the function signature and any callers to handle the SceneGraphResult, and use the existing types NodeId, SceneGraphResult, SceneGraphError, extract_geo_data, self.nodes, and self.geo_data to locate and implement the change.
🤖 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/changes.rs`:
- Around line 60-66: The NODE_TRANSFORM change flag
(ChangeFlags::NODE_TRANSFORM) is declared but never handled—add a branch in the
scene apply_changes method that detects ChangeFlags::NODE_TRANSFORM and for each
affected node invalidates the compositor blit destination and
updates/invalidates the spatial index (i.e., the same paths used for other
spatial/compositor invalidations), and ensure code paths that mutate node
transforms call mark_changed(ChangeFlags::NODE_TRANSFORM) so the flag is set;
alternatively, if you intend it to be reserved, update the NODE_TRANSFORM doc
comment to state it is unused and remove expected behavior references.
In `@crates/grida-canvas/src/surface/state.rs`:
- Around line 438-473: The pending translate anchor (self.pending_pointer_down)
is left set when a double-click opens text edit (response.double_clicked_node),
which causes future PointerMove to start a ghost translate; clear that pending
state when handling a double-click. After you set response.double_clicked_node
(in the click_count >= 2 block inside SurfaceState::handle_*), assign
self.pending_pointer_down = None so the PendingPointerDown/DeferredSelectionOp
anchor is cleared; ensure this happens regardless of the earlier
already_selected vs unselected branch so no stale anchor remains after
double-click handoff to UnknownTargetApplication::handle_surface_event.
- Around line 248-271: The promotion from Idle to SurfaceGesture::Translate in
the pending_pointer_down handling immediately gets overwritten by the Translate
arm, so prev_canvas (the anchor) never reaches the editor and the initial delta
is lost; modify the logic in the same block (the pending_pointer_down handling
that sets self.gesture = SurfaceGesture::Translate { prev_canvas:
pending.anchor_canvas }) so it does not fall through to the Translate match arm
on the same event — e.g. after setting prev_canvas and updating
self.cursor/response.cursor_changed, early-return or set a short-lived flag to
skip the Translate arm update for this event; change code around
self.pending_pointer_down, SurfaceGesture::Translate and prev_canvas to ensure
the anchor_canvas value is preserved and sent to
EditorDocument::handle_gesture_delta() as the "before" gesture.
In `@crates/grida-canvas/src/surface/ui/hit_region.rs`:
- Around line 10-13: Doc comment for the ResizeHandle enum variant is outdated:
update the comment on ResizeHandle(ResizeDirection) in hit_region.rs to reflect
that a pointer-down in editable mode does begin a resize/rotate gesture (and
that the UI should show directional cursor and start the gesture), rather than
saying it should not begin a gesture; mention the editable-mode exception and
keep ResizeDirection reference so reviewers can find the enum variant.
In `@crates/grida-canvas/src/surface/ui/render.rs`:
- Around line 225-241: The loop registers hit regions for all handles even when
they are hidden; update the logic so hit regions are only added when the
selection handles are visible: check SelectionHandles::visible (i.e., the
`handles.visible` flag returned by SelectionHandles::from_screen_rect) before
iterating `handles.resize` and `handles.rotation`, or ensure hidden handles
return no hit info so the match on `HandleHit::{Resize,Rotate}` won't produce
hit regions; this will prevent adding OverlayAction::ResizeHandle /
OverlayAction::RotateHandle entries to `hit_regions` when `handles` are not
visible.
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 355-357: TryCopyAsPNG fails when scenes are loaded without
rebuilding the reverse id map because surface.selection contains internal
NodeIds but internal_id_to_user relies on id_mapping_reverse; fix by ensuring
the reverse map is rebuilt after scene loads and before export: call the
existing reverse-map construction (or populate id_mapping_reverse from
id_mapping_forward) whenever HostEvent::LoadScene, load_dummy_scene(), or
load_benchmark_scene() finish loading, or alternatively make
TryCopyAsPNG/internal_id_to_user lazily reconstruct id_mapping_reverse from
id_mapping_forward if missing so that selections in surface.selection can be
translated to user IDs for export.
- Around line 842-907: The public methods hit_test_point, hit_test_rect,
get_node_bounds, get_union_bounds, and point_in_node_bounds currently expose
internal NodeId; change their public surface to use UserNodeId (String) instead,
or make these methods pub(crate) if they must remain internal. For the
UserNodeId option, have each method convert between internal NodeId and
UserNodeId using the application's bidirectional mapping (the mapping owned by
UnknownTargetApplication) so inputs and outputs use UserNodeId; for methods that
return NodeId(s) (hit_test_point, hit_test_rect), map internal NodeId ->
UserNodeId before returning; for methods that accept ids (get_union_bounds,
point_in_node_bounds) accept &[UserNodeId] and map to internal NodeId for the
internal calls. If you choose non-public, change the signatures to pub(crate) to
avoid leaking the unstable internal ID scheme.
In `@crates/grida-dev/src/editor/document.rs`:
- Around line 67-94: The match on (gesture_before, &gesture_after) ignores
SurfaceGesture::Rotate so rotation gestures never mutate the scene; add a branch
for (SurfaceGesture::Rotate { prev_angle: old_a, center: old_c },
SurfaceGesture::Rotate { prev_angle: new_a, .. }) that computes the angle delta
(new_a - old_a), collects selected ids via
app.surface_selected_nodes().to_vec(), and calls self.apply with the appropriate
rotation mutation (e.g., MutationCommand::Rotate { ids, delta_angle, center:
*old_c } or whatever fields your MutationCommand::Rotate expects) so rotating
the surface updates the document; if you prefer, factor logic into a helper like
handle_incremental_rotate similar to handle_incremental_resize and call that
from the match arm.
- Around line 149-189: The resize currently computes a single tx/ty and applies
it to all nodes and clamps new_w/new_h after computing tx/ty, which causes
incorrect position scaling for multi-selection and overshoot when clamping;
update the logic in document.rs so that after calling compute_resize_geometry
you clamp new_w/new_h and recompute tx/ty against the clamped sizes (use
compute_resize_geometry or the same math) and, instead of a single
MutationCommand::Translate for all ids, compute per-node translated offsets from
the selection union origin scaled by scale_x/scale_y (use each node's original
bounds from node_sizes and union origin/width/height) and apply per-node
Translate/Resize mutations (or a combined per-node MutationCommand) so positions
are scaled relative to union bounds and respect clamping; reference
compute_resize_geometry, resize_affected_axes, resizable_ids, node_sizes,
MutationCommand::Translate, and MutationCommand::Resize to locate changes.
In `@crates/grida-dev/src/editor/mutation.rs`:
- Around line 59-62: node_supports_resize currently returns true for every
non-Vector node, causing many node kinds (Group, BooleanOperation, Path,
Polygon, InitialContainer, etc.) to advertise resize support even though
resize_node only mutates containers/trays, text, and variants that actually have
a size field. Update node_supports_resize to return true only for the concrete
Node variants that resize_node can change (e.g., Node::Container, Node::Tray,
Node::Text, and the specific Node::Variant variant(s) that contain a real size
field) and false for all others; reference the node_supports_resize function and
resize_node to ensure the predicate matches the exact variants handled by
resize_node.
In `@docs/wg/feat-2d/render-cost-prediction.md`:
- Around line 35-37: Several fenced code blocks in
docs/wg/feat-2d/render-cost-prediction.md are missing a language identifier and
trigger MD040; update each triple-backtick fence for the shown snippets (the
blocks containing "frame_cost ≈ total_pixels_touched / memory_bandwidth", "large
sigma → downsample 2× → ... → blur at reduced size → upsample", "save_layer_cost
= layer_bounds_area × zoom² × 2 (write to offscreen + read back)", the
save/restore offscreen sequence block beginning with "save_layer ← offscreen A",
and "pixels_per_ms = (screen_width × screen_height) / render_time_ms") to use a
language tag (use text) so they become ```text fences, and apply the same change
to the other mentioned ranges (78-80, 91-93, 97-105, 193-195).
- Line 119: Update the table row that currently reads "Blur / backdrop filter |
Reads from dst, needs snapshot" to use correct grammar by changing "needs
snapshot" to "needs a snapshot" so the entry becomes "Blur / backdrop filter |
Reads from dst, needs a snapshot"; locate the row text in the document and
replace only that phrase to preserve formatting and spacing.
- Line 229: Replace the machine-specific absolute path string
"/Users/softmarshmallow/Documents/Github/chromium/cc/" in
render-cost-prediction.md with a portable, repo-relative reference (for example
use inline code like `crates/...` or `packages/...` or `../path/to/cc` as
appropriate) so the docs do not link outside the docs directory; ensure the
replacement follows the docs rule for docs/**/*.{md,mdx} by using an inline
repo-relative pointer or documented upstream path instead of the local absolute
path.
---
Outside diff comments:
In `@crates/grida-dev/src/platform/native_application.rs`:
- Around line 659-665: When replacing the EditorDocument and loading a new scene
(the block that calls EditorDocument::new, renderer.load_scene,
fit_camera_to_scene, renderer.queue_unstable, and self.window.request_redraw),
also clear the surface interaction state on self.app.surface() so stale NodeId
state isn’t carried over; explicitly reset selection, hover and gesture (via the
surface mutator / clear methods on the surface object) immediately after
swapping the document and before requesting redraw so interactions/export logic
are tied to the new scene.
---
Nitpick comments:
In `@crates/grida-canvas/examples/fixtures/l0_image.rs`:
- Around line 37-39: The AffineTransform matrix in ImagePaintFit::Transform is
numeric and not self-documenting; update the fixture to construct the transform
with named parameters or a small helper so each coefficient is labeled (e.g.
scale_x/scale_y, skew_x/skew_y, translate_x/translate_y) instead of a raw matrix
array. Locate ImagePaintFit::Transform and AffineTransform in the fixture and
replace the matrix literal with either an AffineTransform::new(...) or
AffineTransform { scale_x: ..., scale_y: ..., translate_x: ..., translate_y: ...
} (or a local helper like make_affine(scale_x, scale_y, tx, ty)) so the intent
of 1.5, -0.25, -0.15 is explicit and easier to tweak later.
In `@crates/grida-canvas/src/node/scene_graph.rs`:
- Around line 808-813: Change refresh_node_geo_data to return
SceneGraphResult<()> and make it fail when the node ID is missing: in
refresh_node_geo_data(&mut self, id: &NodeId) call self.nodes.get(id) and if
Some proceed to extract_geo_data(node) and insert into self.geo_data, then
return Ok(()); if None return an appropriate error (e.g.,
SceneGraphError::InvalidNodeId(*id) or similar) wrapped in Err(...). Update the
function signature and any callers to handle the SceneGraphResult, and use the
existing types NodeId, SceneGraphResult, SceneGraphError, extract_geo_data,
self.nodes, and self.geo_data to locate and implement the change.
🪄 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: fcb415e7-d664-4738-964b-99f17c9a7fef
⛔ Files ignored due to path filters (1)
crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasmis excluded by!**/*.wasm
📒 Files selected for processing (30)
crates/grida-canvas-wasm/lib/bin/grida-canvas-wasm.jscrates/grida-canvas-wasm/package.jsoncrates/grida-canvas-wasm/src/wasm_application.rscrates/grida-canvas/examples/fixtures/l0_image.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/devtools/hit_overlay.rscrates/grida-canvas/src/devtools/mod.rscrates/grida-canvas/src/devtools/surface_overlay.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/src/runtime/changes.rscrates/grida-canvas/src/surface/cursor.rscrates/grida-canvas/src/surface/gesture.rscrates/grida-canvas/src/surface/mod.rscrates/grida-canvas/src/surface/state.rscrates/grida-canvas/src/surface/ui/handles.rscrates/grida-canvas/src/surface/ui/hit_region.rscrates/grida-canvas/src/surface/ui/mod.rscrates/grida-canvas/src/surface/ui/render.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/window/application_emscripten.rscrates/grida-canvas/tests/surface_interaction.rscrates/grida-dev/src/bench/runner.rscrates/grida-dev/src/editor/document.rscrates/grida-dev/src/editor/mod.rscrates/grida-dev/src/editor/mutation.rscrates/grida-dev/src/lib.rscrates/grida-dev/src/platform/native_application.rsdocs/wg/feat-2d/render-cost-prediction.mdfixtures/test-grida/L0.grida
💤 Files with no reviewable changes (2)
- crates/grida-canvas/src/devtools/mod.rs
- crates/grida-canvas/src/devtools/hit_overlay.rs
| // ── Check for pending pointer-down → translate promotion ───────── | ||
| // Only in editing mode (!readonly). If the gesture is still Idle | ||
| // and we have a pending pointer-down, the user is dragging from a | ||
| // node. Cancel any deferred selection and start a Translate gesture. | ||
| if !self.readonly { | ||
| if let Some(pending) = self.pending_pointer_down.take() { | ||
| if matches!(self.gesture, SurfaceGesture::Idle) { | ||
| // Start translate. Set prev_canvas to the anchor so the | ||
| // application layer can compute the first delta as | ||
| // (current_point - anchor). Then immediately fall | ||
| // through to the Translate match arm which updates | ||
| // prev_canvas to canvas_point. | ||
| self.gesture = SurfaceGesture::Translate { | ||
| prev_canvas: pending.anchor_canvas, | ||
| }; | ||
| let new_cursor = CursorIcon::Move; | ||
| if new_cursor != self.cursor { | ||
| self.cursor = new_cursor; | ||
| response.cursor_changed = true; | ||
| } | ||
| // Don't return — fall through to the Translate arm so | ||
| // prev_canvas gets updated to canvas_point on the same | ||
| // event that promoted the gesture. | ||
| } |
There was a problem hiding this comment.
The first translate move is dropped.
This branch promotes Idle -> Translate and then immediately overwrites prev_canvas with canvas_point. Because the native host snapshots gesture_before before dispatch and EditorDocument::handle_gesture_delta() computes dx/dy from the before/after gestures, the pointer-down anchor never reaches the editor and the initial drag delta is lost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/surface/state.rs` around lines 248 - 271, The
promotion from Idle to SurfaceGesture::Translate in the pending_pointer_down
handling immediately gets overwritten by the Translate arm, so prev_canvas (the
anchor) never reaches the editor and the initial delta is lost; modify the logic
in the same block (the pending_pointer_down handling that sets self.gesture =
SurfaceGesture::Translate { prev_canvas: pending.anchor_canvas }) so it does not
fall through to the Translate match arm on the same event — e.g. after setting
prev_canvas and updating self.cursor/response.cursor_changed, early-return or
set a short-lived flag to skip the Translate arm update for this event; change
code around self.pending_pointer_down, SurfaceGesture::Translate and prev_canvas
to ensure the anchor_canvas value is preserved and sent to
EditorDocument::handle_gesture_delta() as the "before" gesture.
| // Pure geometry. | ||
| let (new_w, new_h, tx, ty) = | ||
| compute_resize_geometry(direction, dx, dy, union.width, union.height); | ||
| let new_w = new_w.max(1.0); | ||
| let new_h = new_h.max(1.0); | ||
| let scale_x = new_w / union.width; | ||
| let scale_y = new_h / union.height; | ||
| let (affects_w, affects_h) = resize_affected_axes(direction); | ||
|
|
||
| // Collect per-node sizes before mutation. | ||
| let node_sizes: Vec<_> = resizable_ids | ||
| .iter() | ||
| .filter_map(|id| app.get_node_bounds(id).map(|b| (*id, b.width, b.height))) | ||
| .collect(); | ||
|
|
||
| let mut mutated = false; | ||
|
|
||
| // Origin shift. | ||
| if tx.abs() > 0.0001 || ty.abs() > 0.0001 { | ||
| mutated |= self.apply(&MutationCommand::Translate { | ||
| ids: resizable_ids.clone(), | ||
| dx: tx, | ||
| dy: ty, | ||
| }); | ||
| } | ||
| // Per-node resize. | ||
| for (id, w, h) in &node_sizes { | ||
| mutated |= self.apply(&MutationCommand::Resize { | ||
| id: *id, | ||
| width: if affects_w { | ||
| Some((w * scale_x).max(1.0)) | ||
| } else { | ||
| None | ||
| }, | ||
| height: if affects_h { | ||
| Some((h * scale_y).max(1.0)) | ||
| } else { | ||
| None | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This resize math only works for single-node selections inside the unclamped range.
tx/ty are applied uniformly to every selected node while only each node's own width/height is scaled, so multi-select resize preserves inter-node gaps instead of scaling positions from the union bounds. Also, new_w/new_h are clamped after tx/ty is computed, so north/west drags can overshoot once the pointer crosses the opposite edge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/editor/document.rs` around lines 149 - 189, The resize
currently computes a single tx/ty and applies it to all nodes and clamps
new_w/new_h after computing tx/ty, which causes incorrect position scaling for
multi-selection and overshoot when clamping; update the logic in document.rs so
that after calling compute_resize_geometry you clamp new_w/new_h and recompute
tx/ty against the clamped sizes (use compute_resize_geometry or the same math)
and, instead of a single MutationCommand::Translate for all ids, compute
per-node translated offsets from the selection union origin scaled by
scale_x/scale_y (use each node's original bounds from node_sizes and union
origin/width/height) and apply per-node Translate/Resize mutations (or a
combined per-node MutationCommand) so positions are scaled relative to union
bounds and respect clamping; reference compute_resize_geometry,
resize_affected_axes, resizable_ids, node_sizes, MutationCommand::Translate, and
MutationCommand::Resize to locate changes.
| /// Whether a node type supports resize. | ||
| pub fn node_supports_resize(node: &Node) -> bool { | ||
| !matches!(node, Node::Vector(_)) | ||
| } |
There was a problem hiding this comment.
node_supports_resize() is broader than the actual writer.
This returns true for every non-vector node, but resize_node() only mutates containers/trays, text, and variants backed by a real size field. Group, BooleanOperation, Path, Polygon, InitialContainer, etc. will advertise resize support, participate in union math, and then no-op on apply.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/editor/mutation.rs` around lines 59 - 62,
node_supports_resize currently returns true for every non-Vector node, causing
many node kinds (Group, BooleanOperation, Path, Polygon, InitialContainer, etc.)
to advertise resize support even though resize_node only mutates
containers/trays, text, and variants that actually have a size field. Update
node_supports_resize to return true only for the concrete Node variants that
resize_node can change (e.g., Node::Container, Node::Tray, Node::Text, and the
specific Node::Variant variant(s) that contain a real size field) and false for
all others; reference the node_supports_resize function and resize_node to
ensure the predicate matches the exact variants handled by resize_node.
| ``` | ||
| frame_cost ≈ total_pixels_touched / memory_bandwidth | ||
| ``` |
There was a problem hiding this comment.
Add fence languages to satisfy markdown lint.
Several fenced code blocks are missing language identifiers, which triggers MD040 and hurts readability/tooling.
Suggested doc patch
-```
+```text
frame_cost ≈ total_pixels_touched / memory_bandwidth- +text
large sigma → downsample 2× → downsample 2× → ... → blur at reduced size → upsample
-```
+```text
save_layer_cost = layer_bounds_area × zoom² × 2 (write to offscreen + read back)
- +text
save_layer ← offscreen A (full group bounds)
save_layer ← offscreen B (child bounds)
save_layer ← offscreen C (grandchild bounds)
draw rect
restore → composite C into B
restore → composite B into A
restore → composite A into target
-```
+```text
pixels_per_ms = (screen_width × screen_height) / render_time_ms
</details>
Also applies to: 78-80, 91-93, 97-105, 193-195
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/wg/feat-2d/render-cost-prediction.md around lines 35 - 37, Several
fenced code blocks in docs/wg/feat-2d/render-cost-prediction.md are missing a
language identifier and trigger MD040; update each triple-backtick fence for the
shown snippets (the blocks containing "frame_cost ≈ total_pixels_touched /
memory_bandwidth", "large sigma → downsample 2× → ... → blur at reduced size →
upsample", "save_layer_cost = layer_bounds_area × zoom² × 2 (write to offscreen
- read back)", the save/restore offscreen sequence block beginning with
"save_layer ← offscreen A", and "pixels_per_ms = (screen_width × screen_height)
/ render_time_ms") to use a language tag (use text) so they become ```text
fences, and apply the same change to the other mentioned ranges (78-80, 91-93,
97-105, 193-195).
</details>
<!-- fingerprinting:phantom:triton:hawk:bd7856ef-c7df-47b5-a17e-6a39db6c13d5 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| | ----------------------------------------- | --------------------------------------------------------------- | | ||
| | Non-normal blend mode on a group | Isolated offscreen to blend against dst | | ||
| | Group opacity (alpha < 1.0 with children) | Children must composite together first, then alpha applied once | | ||
| | Blur / backdrop filter | Reads from dst, needs snapshot | |
There was a problem hiding this comment.
Tighten wording: “needs snapshot” → “needs a snapshot”.
This reads nonstandard in technical docs; a small grammar fix improves clarity.
🧰 Tools
🪛 LanguageTool
[style] ~119-~119: The double modal “needs snapshot” is nonstandard (only accepted in certain dialects). Consider “to be snapshot”.
Context: ... | Reads from dst, needs snapshot | | Cl...
(NEEDS_FIXED)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-2d/render-cost-prediction.md` at line 119, Update the table row
that currently reads "Blur / backdrop filter | Reads from dst, needs snapshot"
to use correct grammar by changing "needs snapshot" to "needs a snapshot" so the
entry becomes "Blur / backdrop filter | Reads from dst, needs a snapshot";
locate the row text in the document and replace only that phrase to preserve
formatting and spacing.
| single-threaded with a hard per-frame deadline, requiring predictive | ||
| budgeting. | ||
|
|
||
| Local source: `/Users/softmarshmallow/Documents/Github/chromium/cc/` |
There was a problem hiding this comment.
Replace local absolute path with a portable repo reference.
/Users/... is machine-specific and won’t help other readers. Prefer a repo-relative pointer (or documented upstream path) in inline code.
Suggested doc patch
-Local source: `/Users/softmarshmallow/Documents/Github/chromium/cc/`
+Local source (Chromium tree): `cc/`As per coding guidelines, "docs/**/*.{md,mdx}: Never link outside /docs from docs markdown files; instead reference external paths like crates/ or packages/ as inline code."
📝 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.
| Local source: `/Users/softmarshmallow/Documents/Github/chromium/cc/` | |
| Local source (Chromium tree): `cc/` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-2d/render-cost-prediction.md` at line 229, Replace the
machine-specific absolute path string
"/Users/softmarshmallow/Documents/Github/chromium/cc/" in
render-cost-prediction.md with a portable, repo-relative reference (for example
use inline code like `crates/...` or `packages/...` or `../path/to/cc` as
appropriate) so the docs do not link outside the docs directory; ensure the
replacement follows the docs rule for docs/**/*.{md,mdx} by using an inline
repo-relative pointer or documented upstream path instead of the local absolute
path.
…ring Changed the image sampling options to use Nearest-neighbor filtering for upscaling, ensuring crispness for pixel art and patterns, while still utilizing mipmaps for downscaling. This adjustment enhances the visual quality of images rendered larger than their natural size.
…rm updates and UI interactions - Updated NODE_TRANSFORM comment to clarify its reserved status for future incremental updates. - Added handling to clear the translate anchor on double-click to prevent ghost translations during text editing. - Enhanced comments in hit_region.rs to specify behavior in editing mode for resize and rotate handles. - Added a check in render.rs to only register hit regions for visible handles, improving gesture handling efficiency. - Refined node_supports_resize function to explicitly list supported node types for resizing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/grida-canvas/src/surface/state.rs (1)
248-273:⚠️ Potential issue | 🟠 MajorThe first translate move delta is still lost.
The promotion logic at lines 260-262 sets
prev_canvas = pending.anchor_canvas, but then immediately falls through to theTranslatematch arm (lines 339-341) which overwritesprev_canvaswithcanvas_point.After dispatch completes, the application layer sees:
gesture_before = Idlegesture_after = Translate { prev_canvas: canvas_point }Since
gesture_beforehas noprev_canvas, the application cannot compute the initial delta from the anchor to the first move point. The distance the user dragged from click to first move is discarded.To preserve the anchor for the application layer's delta computation, don't fall through on the same event that promotes the gesture:
Suggested fix
self.gesture = SurfaceGesture::Translate { prev_canvas: pending.anchor_canvas, }; let new_cursor = CursorIcon::Move; if new_cursor != self.cursor { self.cursor = new_cursor; response.cursor_changed = true; } - // Don't return — fall through to the Translate arm so - // prev_canvas gets updated to canvas_point on the same - // event that promoted the gesture. + // Return here so the anchor is preserved. The application + // layer will see (Idle → Translate{anchor}) and compute + // the first delta on the next move event. + response.needs_redraw = true; + return response; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/surface/state.rs` around lines 248 - 273, The promotion code that converts a pending_pointer_down into SurfaceGesture::Translate currently sets self.gesture = SurfaceGesture::Translate { prev_canvas: pending.anchor_canvas } but then falls through and the Translate match arm overwrites prev_canvas with canvas_point, losing the anchor; change the logic in the block handling pending_pointer_down so that after setting self.gesture, updating self.cursor and response.cursor_changed you do not continue into the Translate handling for the same event (e.g., return/continue out of the function/loop or set a local flag and skip the Translate arm for this dispatch), thereby preserving prev_canvas == pending.anchor_canvas for the application to compute the initial delta (refer to pending_pointer_down, SurfaceGesture::Translate, prev_canvas, self.gesture, response.cursor_changed).
🤖 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-dev/src/editor/mutation.rs`:
- Around line 155-217: The resize_node function is currently applying raw
computed sizes (from compute_resize_geometry) which can become negative when
dragging past the opposite edge; before mutating Container/Tray
layout_dimensions, TextSpan/AttributedText width/height, or the generic Size via
node_size_mut, clamp computed width and height to a sensible minimum (e.g., >=
1.0 or a defined MIN_SIZE) or flip handle orientation and normalize to positive
values, then assign those clamped/normalized values and set changed; apply the
same clamping logic to the other resize code path mentioned (the second
occurrence that mirrors resize_node) so neither branch writes
negative/degenerate dimensions into the scene.
- Around line 35-45: The Translate arm of apply currently returns true for any
non-zero dx/dy even if no nodes are changed; modify the implementation to only
return true when at least one node was actually mutated: have translate_node (or
node_transform_mut) return a bool indicating whether it changed a node, then in
apply's MutationCommand::Translate arm iterate ids, call translate_node(&mut
scene.graph, id, *dx, *dy) and OR the results into a local changed flag, and
finally return that flag (instead of unconditionally true); apply the same
pattern to the other translate-like arms referenced (around lines 126-152).
---
Duplicate comments:
In `@crates/grida-canvas/src/surface/state.rs`:
- Around line 248-273: The promotion code that converts a pending_pointer_down
into SurfaceGesture::Translate currently sets self.gesture =
SurfaceGesture::Translate { prev_canvas: pending.anchor_canvas } but then falls
through and the Translate match arm overwrites prev_canvas with canvas_point,
losing the anchor; change the logic in the block handling pending_pointer_down
so that after setting self.gesture, updating self.cursor and
response.cursor_changed you do not continue into the Translate handling for the
same event (e.g., return/continue out of the function/loop or set a local flag
and skip the Translate arm for this dispatch), thereby preserving prev_canvas ==
pending.anchor_canvas for the application to compute the initial delta (refer to
pending_pointer_down, SurfaceGesture::Translate, prev_canvas, self.gesture,
response.cursor_changed).
🪄 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: 179dd60c-3962-4d60-99be-22289ba175ea
📒 Files selected for processing (5)
crates/grida-canvas/src/runtime/changes.rscrates/grida-canvas/src/surface/state.rscrates/grida-canvas/src/surface/ui/hit_region.rscrates/grida-canvas/src/surface/ui/render.rscrates/grida-dev/src/editor/mutation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/grida-canvas/src/runtime/changes.rs
- crates/grida-canvas/src/surface/ui/hit_region.rs
| pub fn apply(scene: &mut Scene, cmd: &MutationCommand) -> bool { | ||
| match cmd { | ||
| MutationCommand::Translate { ids, dx, dy } => { | ||
| if dx.abs() < 0.0001 && dy.abs() < 0.0001 { | ||
| return false; | ||
| } | ||
| for id in ids { | ||
| translate_node(&mut scene.graph, id, *dx, *dy); | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
Track whether any translate target actually moved.
Line 34 says apply() returns true only when the scene was modified, but the translate arm returns true for any non-zero delta even when ids is empty, a NodeId lookup fails, or the target falls through node_transform_mut() without mutating. That can trigger unnecessary full-scene flushes from a no-op gesture path.
💡 Proposed fix
pub fn apply(scene: &mut Scene, cmd: &MutationCommand) -> bool {
match cmd {
MutationCommand::Translate { ids, dx, dy } => {
if dx.abs() < 0.0001 && dy.abs() < 0.0001 {
return false;
}
+ let mut changed = false;
for id in ids {
- translate_node(&mut scene.graph, id, *dx, *dy);
+ changed |= translate_node(&mut scene.graph, id, *dx, *dy);
}
- true
+ changed
}
MutationCommand::Resize { id, width, height } => {
if let Ok(node) = scene.graph.get_node(id) {
if !node_supports_resize(node) {
return false;
@@
-fn translate_node(graph: &mut SceneGraph, id: &NodeId, dx: f32, dy: f32) {
+fn translate_node(graph: &mut SceneGraph, id: &NodeId, dx: f32, dy: f32) -> bool {
if let Ok(node) = graph.get_node_mut(id) {
+ let mut changed = false;
match node {
Node::Container(n) => {
let x = n.position.x().unwrap_or(0.0);
let y = n.position.y().unwrap_or(0.0);
n.position = LayoutPositioningBasis::Cartesian(CGPoint {
x: x + dx,
y: y + dy,
});
+ changed = true;
}
Node::Tray(n) => {
let x = n.position.x().unwrap_or(0.0);
let y = n.position.y().unwrap_or(0.0);
n.position = LayoutPositioningBasis::Cartesian(CGPoint {
x: x + dx,
y: y + dy,
});
+ changed = true;
}
_ => {
if let Some(t) = node_transform_mut(node) {
t.translate(dx, dy);
+ changed = true;
}
}
}
- graph.refresh_node_geo_data(id);
+ if changed {
+ graph.refresh_node_geo_data(id);
+ }
+ return changed;
}
+
+ false
}Also applies to: 126-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/editor/mutation.rs` around lines 35 - 45, The Translate
arm of apply currently returns true for any non-zero dx/dy even if no nodes are
changed; modify the implementation to only return true when at least one node
was actually mutated: have translate_node (or node_transform_mut) return a bool
indicating whether it changed a node, then in apply's MutationCommand::Translate
arm iterate ids, call translate_node(&mut scene.graph, id, *dx, *dy) and OR the
results into a local changed flag, and finally return that flag (instead of
unconditionally true); apply the same pattern to the other translate-like arms
referenced (around lines 126-152).
| fn resize_node(scene: &mut Scene, id: &NodeId, width: Option<f32>, height: Option<f32>) -> bool { | ||
| let mut changed = false; | ||
| if let Ok(node) = scene.graph.get_node_mut(id) { | ||
| match node { | ||
| Node::Container(n) => { | ||
| if let Some(w) = width { | ||
| n.layout_dimensions.layout_target_width = Some(w); | ||
| changed = true; | ||
| } | ||
| if let Some(h) = height { | ||
| n.layout_dimensions.layout_target_height = Some(h); | ||
| changed = true; | ||
| } | ||
| } | ||
| Node::Tray(n) => { | ||
| if let Some(w) = width { | ||
| n.layout_dimensions.layout_target_width = Some(w); | ||
| changed = true; | ||
| } | ||
| if let Some(h) = height { | ||
| n.layout_dimensions.layout_target_height = Some(h); | ||
| changed = true; | ||
| } | ||
| } | ||
| Node::TextSpan(n) => { | ||
| if let Some(w) = width { | ||
| n.width = Some(w); | ||
| changed = true; | ||
| } | ||
| if let Some(h) = height { | ||
| n.height = Some(h); | ||
| changed = true; | ||
| } | ||
| } | ||
| Node::AttributedText(n) => { | ||
| if let Some(w) = width { | ||
| n.width = Some(w); | ||
| changed = true; | ||
| } | ||
| if let Some(h) = height { | ||
| n.height = Some(h); | ||
| changed = true; | ||
| } | ||
| } | ||
| Node::Vector(_) => {} // not supported | ||
| _ => { | ||
| if let Some(s) = node_size_mut(node) { | ||
| if let Some(w) = width { | ||
| s.width = w; | ||
| changed = true; | ||
| } | ||
| if let Some(h) = height { | ||
| s.height = h; | ||
| changed = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if changed { | ||
| scene.graph.refresh_node_geo_data(id); | ||
| } | ||
| } | ||
| changed |
There was a problem hiding this comment.
Clamp crossed-over resizes before mutating the scene.
compute_resize_geometry() returns old_w - dx / old_h - dy directly, so dragging past the opposite edge produces negative dimensions. resize_node() then writes those raw values into Size, text width/height, and container/tray layout targets. Once that happens, bounds and hit-testing will flip or go degenerate. Clamp to a minimum size, or explicitly switch handle orientation, before these values are applied.
Also applies to: 248-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/editor/mutation.rs` around lines 155 - 217, The
resize_node function is currently applying raw computed sizes (from
compute_resize_geometry) which can become negative when dragging past the
opposite edge; before mutating Container/Tray layout_dimensions,
TextSpan/AttributedText width/height, or the generic Size via node_size_mut,
clamp computed width and height to a sensible minimum (e.g., >= 1.0 or a defined
MIN_SIZE) or flip handle orientation and normalize to positive values, then
assign those clamped/normalized values and set changed; apply the same clamping
logic to the other resize code path mentioned (the second occurrence that
mirrors resize_node) so neither branch writes negative/degenerate dimensions
into the scene.
Summary
Adds interactive selection surface UI to the Rust canvas engine and a dev-only editor module in grida-dev that performs real scene mutations (translate, resize) with full scene flush.
Architecture
UnknownTargetApplication.load_scene(). Self-contained, dev-only.Surface UI (cg)
surface/ui/handles.rs): 8 resize handles (4 visible corner knobs + 4 invisible side hit zones) + 4 rotation hit zones. DPR-aware, visibility-gated.surface/cursor.rs):Resize(ResizeDirection)andRotate(RotationCorner)cursor variants with native winit mapping.surface/gesture.rs):Translate,Resize,Rotatevariants with incrementalprev_*tracking model.surface/state.rs): pointer-down on already-selected node defers re-selection — drag preserves multi-selection, click applies.readonlyflag: defaultstrue(web unaffected). Native setsfalseto enable editing gestures.show_selection_handlesconfig flag onSurfaceOverlayConfig.Query API (cg, on UTA)
hit_test_point(canvas_point) -> Option<NodeId>hit_test_rect(rect) -> Vec<NodeId>get_node_bounds(id) -> Option<Rectangle>get_union_bounds(ids) -> Option<Rectangle>point_in_node_bounds(point, ids) -> boolview_matrix() -> AffineTransformEditor (grida-dev)
editor/document.rs:EditorDocument— owns aScene, appliesMutationCommands, flushes to renderer viaload_scene().editor/mutation.rs:MutationCommandenum (Translate, Resize), scene graph writers, node accessors, resize geometry helpers.Option<f32>per axis).NativeApplicationsnapshots gesture before surface dispatch, computes incremental delta, applies mutation, flushes.Cleanup
hit_overlay.rs(replaced by surface overlay hover feedback).hit_test_result,devtools_selection,perform_hit_test_hostfields/methods.TryCopyAsPNGnow reads from surface selection instead of deleted devtools selection.Node::transform_mut()/size_mut()moved from cg schema to grida-dev editor.Tests
tests/surface_interaction.rs): selection, deferred selection, translate gesture, readonly mode, marquee, schema accessors.surface/ui/handles.rs: handle geometry, hit priority, DPR scaling.surface/cursor.rs: direction constants, cursor variants.Summary by CodeRabbit
New Features
Improvements