From 1443ad0177214ec0967ed7bcc11d1bf4385ffc35 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Fri, 16 Oct 2026 13:53:12 +0530 Subject: [PATCH] fix(viewer): gizmo arrows take precedence over box handles under them Review on #1627: the X arrow tip can sit on a bounding-box handle (a 120 px wide layer at 100%). The arrow is painted on top, but the press and hover tests checked the handles first, so dragging it scaled the layer. Check the arrows first in both, and cover the overlap with a pointer-input test. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/panels/viewer.rs | 15 +++--- crates/ui-egui/tests/ui_3d.rs | 76 ++++++++++++++++++++--------- 2 files changed, 61 insertions(+), 30 deletions(-) diff --git a/crates/ui-egui/src/panels/viewer.rs b/crates/ui-egui/src/panels/viewer.rs index 008a29c7..93fcaa2f 100644 --- a/crates/ui-egui/src/panels/viewer.rs +++ b/crates/ui-egui/src/panels/viewer.rs @@ -1357,10 +1357,10 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { } else if vertex_at(hp, true).or_else(|| vertex_at(hp, false)).is_some() { // A path point under a box handle is grabbed first: no resize cursor. egui::CursorIcon::PointingHand - } else if let Some((.., cursor)) = handle_hits.iter().find(|(_, _, h, _)| h.distance(hp) < HANDLE) { - *cursor } else if axis_at(&axis_hits, hp).is_some() { egui::CursorIcon::Move + } else if let Some((.., cursor)) = handle_hits.iter().find(|(_, _, h, _)| h.distance(hp) < HANDLE) { + *cursor } else { egui::CursorIcon::Default } @@ -1544,7 +1544,12 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { // far enough to count as a drag it may have left a small handle. Tool::Selection => { let cpt = map.to_comp(press); - if let Some((lid, hi, ..)) = handle_hits.iter().find(|(_, _, h, _)| h.distance(press) < HANDLE + 2.0).cloned() { + // A gizmo arrow is drawn over the box handles, so it is grabbed first: move along + // that axis only, without snapping. + if let Some((lid, _, ax, ay)) = axis_at(&axis_hits, press) { + let p0 = comp.layer(lid).and_then(|l| Some(ectx.v3(l, l.transform()?, "position", [0.0; 3]))); + p0.map(|p0| Gesture::Move { layers: vec![(lid, p0, ax, ay)], start: cpt, snap_src: None }) + } else if let Some((lid, hi, ..)) = handle_hits.iter().find(|(_, _, h, _)| h.distance(press) < HANDLE + 2.0).cloned() { let layer = comp.layer(lid).cloned(); layer.and_then(|layer| { let tr = layer.transform()?; @@ -1568,10 +1573,6 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { .collect(), }) }) - } else if let Some((lid, _, ax, ay)) = axis_at(&axis_hits, press) { - // A gizmo arrow: move along that axis only, without snapping. - let p0 = comp.layer(lid).and_then(|l| Some(ectx.v3(l, l.transform()?, "position", [0.0; 3]))); - p0.map(|p0| Gesture::Move { layers: vec![(lid, p0, ax, ay)], start: cpt, snap_src: None }) } else if let Some(l) = pick(app, &ectx, cpt, mods.shift) { let layers: Vec<(LayerId, [f64; 3], [f64; 3], [f64; 3])> = app .session diff --git a/crates/ui-egui/tests/ui_3d.rs b/crates/ui-egui/tests/ui_3d.rs index 2ba5265a..223d9960 100644 --- a/crates/ui-egui/tests/ui_3d.rs +++ b/crates/ui-egui/tests/ui_3d.rs @@ -279,43 +279,73 @@ fn drag(h: &mut Harness<'_, EffectcraftApp>, from: egui::Pos2, to: egui::Pos2) { h.run_steps(3); } +/// A selected 3D solid (`w` x `h`) in an empty 640x360 comp seen by the default camera. +fn gizmo_harness(w: u32, h: u32) -> (Harness<'static, EffectcraftApp>, u64) { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "C", "width": 640, "height": 360, "duration": 2})).unwrap(); + let solid = s.execute("layer.newSolid", json!({"color": "#ff0000", "width": w, "height": h})).unwrap()["layer"].as_u64().unwrap(); + s.execute("layer.setSwitch", json!({"layer": solid, "switch": "threeD", "value": true})).unwrap(); + s.execute("layer.select", json!({"layers": [solid]})).unwrap(); + let mut harness = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(s)); + settle(&mut harness); + (harness, solid) +} + +fn transform(h: &mut Harness<'_, EffectcraftApp>, layer: u64, prop: &str) -> Vec { + let v = h.state_mut().session.execute("prop.get", json!({"layer": layer, "path": format!("transform/{prop}")})).unwrap()["value"].clone(); + serde_json::from_value(v).unwrap() +} + +/// The centre of the automation element `id`. +fn centre(h: &Harness<'_, EffectcraftApp>, id: &str) -> egui::Pos2 { + let [x, y, w, hh] = h.state().auto.find(id).unwrap_or_else(|| panic!("{id} is registered")).rect; + egui::pos2(x + w / 2.0, y + hh / 2.0) +} + /// #1618: the gizmo's arrows drag a 3D layer along one axis. Z, which points at the default /// camera, still has a grabbable arrow and pushes the layer in depth, as one undo step. #[test] fn gizmo_arrows_move_a_3d_layer_along_one_axis() { - let mut s = Session::default(); - s.execute("comp.new", json!({"name": "C", "width": 640, "height": 360, "duration": 2})).unwrap(); - let solid = s.execute("layer.newSolid", json!({"color": "#ff0000", "width": 200, "height": 200})).unwrap()["layer"].as_u64().unwrap(); - s.execute("layer.setSwitch", json!({"layer": solid, "switch": "threeD", "value": true})).unwrap(); - s.execute("layer.select", json!({"layers": [solid]})).unwrap(); - let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(s)); - settle(&mut h); - let pos = |h: &mut Harness<'_, EffectcraftApp>| -> Vec { - let v = h.state_mut().session.execute("prop.get", json!({"layer": solid, "path": "transform/position"})).unwrap()["value"].clone(); - serde_json::from_value(v).unwrap() - }; - let tip = |h: &Harness<'_, EffectcraftApp>, axis: &str| { - let id = format!("viewer.gizmo.{solid}.{axis}"); - let [x, y, w, hh] = h.state().auto.find(&id).unwrap_or_else(|| panic!("{id} is registered")).rect; - egui::pos2(x + w / 2.0, y + hh / 2.0) - }; - let p0 = pos(&mut h); + let (mut h, solid) = gizmo_harness(200, 200); + let p0 = transform(&mut h, solid, "position"); - let z = tip(&h, "z"); + let z = centre(&h, &format!("viewer.gizmo.{solid}.z")); drag(&mut h, z, z + egui::vec2(30.0, -30.0)); - let p1 = pos(&mut h); + let p1 = transform(&mut h, solid, "position"); assert_eq!(&p1[..2], &p0[..2], "x and y stay put"); assert!(p1[2] > p0[2] + 10.0, "pushed back in depth: {p0:?} -> {p1:?}"); settle(&mut h); - let x = tip(&h, "x"); + let x = centre(&h, &format!("viewer.gizmo.{solid}.x")); drag(&mut h, x, x + egui::vec2(40.0, 25.0)); - let p2 = pos(&mut h); + let p2 = transform(&mut h, solid, "position"); assert!(p2[0] > p1[0] + 10.0, "moved along x: {p1:?} -> {p2:?}"); assert_eq!(&p2[1..], &p1[1..], "only x changes"); h.state_mut().session.execute("edit.undo", json!({})).unwrap(); - assert_eq!(pos(&mut h), p1, "the x drag is one undo step"); + assert_eq!(transform(&mut h, solid, "position"), p1, "the x drag is one undo step"); h.state_mut().session.execute("edit.undo", json!({})).unwrap(); - assert_eq!(pos(&mut h), p0, "the z drag is one undo step"); + assert_eq!(transform(&mut h, solid, "position"), p0, "the z drag is one undo step"); +} + +/// #1627 review: an arrow drawn over a box handle is what the pointer grabs (it scaled the +/// layer). At 100% a 120 px wide layer has its right-edge handle under the X arrow's tip. +#[test] +fn a_gizmo_arrow_wins_over_the_box_handle_under_it() { + let (mut h, solid) = gizmo_harness(120, 200); + h.state_mut().ui.viewer.zoom = Some(1.0); + settle(&mut h); + let x = centre(&h, &format!("viewer.gizmo.{solid}.x")); + assert!(x.distance(centre(&h, &format!("viewer.handle.{solid}.5"))) < 2.0, "the right-edge handle is under the X tip"); + + h.event(egui::Event::PointerMoved(x)); + h.run_steps(2); + assert_eq!(h.output().platform_output.cursor_icon, egui::CursorIcon::Move, "hover shows the move cursor, not resize"); + + let p0 = transform(&mut h, solid, "position"); + drag(&mut h, x, x + egui::vec2(30.0, 0.0)); + let p1 = transform(&mut h, solid, "position"); + assert_eq!(transform(&mut h, solid, "scale"), vec![100.0, 100.0, 100.0], "not scaled"); + assert!((p1[0] - p0[0] - 30.0).abs() < 1e-6, "moved 30 px along x: {p0:?} -> {p1:?}"); + assert_eq!(&p1[1..], &p0[1..]); }