Merge PR #2888

crates/ui-egui/src/icons.rs

@@ -745,6 +745,12 @@ pub fn keyframe(
     painter.extend(keyframe_shapes(center, size, left, right, fill));
 }
 
+/// The screen rect a [`keyframe`] glyph covers, read off the shapes it draws.
+pub fn keyframe_bounds(center: Pos2, size: f32, left: effectcraft_engine::keyframe::KeyHalf, right: effectcraft_engine::keyframe::KeyHalf) -> Rect {
+    // Any opaque fill: a transparent shape would report no bounds.
+    egui::Shape::Vec(keyframe_shapes(center, size, left, right, Color32::WHITE)).visual_bounding_rect()
+}
+
 /// The halves of a [`keyframe`] glyph. Flat, without a dark outline (as in After Effects), so
 /// keys that overlap read as one shape instead of a tangle of borders (#284); the hairline in
 /// the fill colour closes the seam between the halves.
@@ -812,6 +818,23 @@ mod tests {
         }
     }
 
+    /// Each half reaches as far as it is drawn, plus the hairline around it, and the halves are
+    /// independent (a key can come in linear and go out on hold).
+    #[test]
+    fn keyframe_bounds_follow_each_half() {
+        let c = egui::pos2(100.0, 50.0);
+        let r = 5.5;
+        // (half, how far it reaches sideways, how far up and down)
+        let extents = [(H::Linear, r, r), (H::Hold, r * 0.8, r * 0.8), (H::Bezier, r * 0.75, r), (H::Auto, r * 0.8, r * 0.8)];
+        for (l, lx, ly) in extents {
+            for (rt, rx, ry) in extents {
+                let b = super::keyframe_bounds(c, 11.0, l, rt);
+                let want = egui::Rect::from_min_max(c - egui::vec2(lx, ly.max(ry)), c + egui::vec2(rx, ly.max(ry))).expand(0.25);
+                assert!(b.min.distance(want.min) < 1e-3 && b.max.distance(want.max) < 1e-3, "{l:?}/{rt:?}: {b:?}, want {want:?}");
+            }
+        }
+    }
+
     /// #284: keyframes have no dark border (overlapping keys looked messy): each half is filled
     /// and edged in the key's own colour.
     #[test]

crates/ui-egui/src/panels/timeline.rs

@@ -13,7 +13,7 @@ use effectcraft_engine::project::{
 };
 use effectcraft_engine::render::EvalCtx;
 use effectcraft_engine::time::Tick;
-use egui::{Align2, Color32, Pos2, Rect, Sense, Stroke, StrokeKind, pos2, vec2};
+use egui::{Align2, Color32, Pos2, Rect, Sense, Stroke, StrokeKind, Vec2, pos2, vec2};
 use serde_json::json;
 use std::sync::Arc;
 
@@ -4232,6 +4232,20 @@ fn key_drag_begin(app: &mut EffectcraftApp, g: &Rw, kh: &KeyHit<'_>) {
     }
 }
 
+/// The size of a keyframe glyph and the radius of a roving key's dot.
+const KEY_GLYPH_SIZE: f32 = 11.0;
+const ROVING_KEY_RADIUS: f32 = 2.5;
+
+/// The rect a key's marker covers on screen (its hit rect is larger).
+fn key_glyph_rect(k: &effectcraft_engine::keyframe::Keyframe, center: Pos2) -> Rect {
+    if k.roving {
+        Rect::from_center_size(center, Vec2::splat(ROVING_KEY_RADIUS * 2.0))
+    } else {
+        let icon = k.icon();
+        icons::keyframe_bounds(center, KEY_GLYPH_SIZE, icon.left, icon.right)
+    }
+}
+
 /// Paints one keyframe marker; returns its hit rect.
 fn key_paint(g: &Rw, rc: &RowCtx<'_>, k: &effectcraft_engine::keyframe::Keyframe, x: f32, ks: bool) -> Rect {
     let t = g.tl.t;
@@ -4252,9 +4266,9 @@ fn key_paint(g: &Rw, rc: &RowCtx<'_>, k: &effectcraft_engine::keyframe::Keyframe
     let kcol = if k.muted { kcol.gamma_multiply(0.35) } else { kcol };
     if k.roving {
         // Roving keys: a small dot (their time follows their neighbours).
-        gp.circle_filled(c, 2.5, kcol);
+        gp.circle_filled(c, ROVING_KEY_RADIUS, kcol);
     } else {
-        icons::keyframe(&gp, c, 11.0, icon.left, icon.right, kcol);
+        icons::keyframe(&gp, c, KEY_GLYPH_SIZE, icon.left, icon.right, kcol);
     }
     Rect::from_center_size(c, vec2(12.0, 14.0))
 }
@@ -4678,7 +4692,33 @@ fn key_drag_follow(app: &mut EffectcraftApp, g: &mut Rw) {
     }
 }
 
-/// Releasing the key marquee selects the keys inside it.
+/// The keys whose markers touch box `br`, in row order (the comp's). `top` is the y of the first
+/// row, scrolled.
+fn keys_in_box(comp: &Comp, rows: &[Row], tm: &TMap, top: f32, rh: f32, br: Rect) -> Vec<effectcraft_engine::KeyRef> {
+    let mut keys = vec![];
+    let mut yy = top;
+    for row in rows {
+        let cy = yy + rh / 2.0;
+        yy += row_height(row, rh);
+        // No marker reaches a whole glyph from its row's center.
+        if let RowKind::Prop { uid } = row.kind
+            && cy >= br.min.y - KEY_GLYPH_SIZE
+            && cy <= br.max.y + KEY_GLYPH_SIZE
+            && let Some(l) = comp.layer(row.layer)
+            && let Some(pr) = l.props.find(uid)
+        {
+            for k in &pr.keys {
+                let x = tm.x(l.comp_time(k.time).seconds());
+                if key_glyph_rect(k, pos2(x, cy)).intersects(br) {
+                    keys.push(effectcraft_engine::KeyRef { layer: l.id, prop: uid, time: k.time });
+                }
+            }
+        }
+    }
+    keys
+}
+
+/// Releasing the key marquee selects the keys it touches.
 fn key_box_select(app: &mut EffectcraftApp, ui: &mut egui::Ui, g: &mut Rw) {
     let tm = g.tl.tm;
     let rows_rect = g.tl.rows_rect;
@@ -4692,25 +4732,8 @@ fn key_box_select(app: &mut EffectcraftApp, ui: &mut egui::Ui, g: &mut Rw) {
         && let Some(br) = ctx.data(|d| d.get_temp::<Rect>(egui::Id::unique("tl-box")))
     {
         ctx.data_mut(|d| d.remove::<Rect>(egui::Id::unique("tl-box")));
-        let mut keys = vec![];
-        let mut yy = rows_rect.min.y - app.ui.timeline.scroll_y;
-        for row in rows.iter() {
-            let cy = yy + rh / 2.0;
-            yy += row_height(row, rh);
-            if let RowKind::Prop { uid } = row.kind
-                && cy >= br.min.y
-                && cy <= br.max.y
-                && let Some(l) = comp.layer(row.layer)
-                && let Some(pr) = l.props.find(uid)
-            {
-                for k in &pr.keys {
-                    let x = tm.x(l.comp_time(k.time).seconds());
-                    if x >= br.min.x && x <= br.max.x {
-                        keys.push(json!({"layer": l.id.0, "prop": uid, "time": k.time.seconds()}));
-                    }
-                }
-            }
-        }
+        let touched = keys_in_box(&comp, rows, &tm, rows_rect.min.y - app.ui.timeline.scroll_y, rh, br);
+        let keys: Vec<serde_json::Value> = touched.iter().map(|k| json!({"layer": k.layer.0, "prop": k.prop, "time": k.time.seconds()})).collect();
         let mods = ui.input(|i| i.modifiers);
         if keys.is_empty() && !(mods.shift || mods.command) {
             // A marquee that catches nothing leaves nothing selected (layers included).

crates/ui-egui/src/panels/timeline/tests.rs

@@ -628,6 +628,52 @@ fn a_key_menu_edit_selects_the_clicked_key_before_applying_the_command() {
     assert_eq!(app.session.state.selected_keys[0].time, prop.keys[0].time);
 }
 
+/// A key's marquee target is the marker it draws: a roving key's is its smaller dot.
+#[test]
+fn a_keys_glyph_rect_is_what_it_draws() {
+    let mut s = effectcraft_engine::Session::default();
+    s.execute("comp.new", json!({"name": "C", "width": 64, "height": 64, "duration": 2})).unwrap();
+    let id = s.execute("layer.newSolid", json!({"name": "Box", "color": "#fff"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("prop.addKey", json!({"layer": id, "path": "transform/opacity", "time": 0.5, "value": 0})).unwrap();
+    let comp = s.active_comp().unwrap();
+    let mut k = comp.layer(LayerId(id)).unwrap().props.prop("transform/opacity").unwrap().keys[0].clone();
+    let c = pos2(100.0, 50.0);
+    k.roving = true;
+    assert_eq!(key_glyph_rect(&k, c), Rect::from_center_size(c, vec2(5.0, 5.0)));
+}
+
+/// Keys come back in row order, and only the rows and keys the box touches. Rows taller than one
+/// line (an expression editor) push the keys below them down, as they are drawn.
+#[test]
+fn keys_in_box_walks_the_rows_and_tests_each_glyph() {
+    let mut s = effectcraft_engine::Session::default();
+    s.execute("comp.new", json!({"name": "C", "width": 64, "height": 64, "duration": 4, "frameRate": 30})).unwrap();
+    let mut layers = vec![];
+    for name in ["A", "B"] {
+        let id = s.execute("layer.newSolid", json!({"name": name, "color": "#fff"})).unwrap()["layer"].as_u64().unwrap();
+        for t in [1.0, 2.0] {
+            s.execute("prop.addKey", json!({"layer": id, "path": "transform/opacity", "time": t, "value": 50})).unwrap();
+        }
+        layers.push(id);
+    }
+    let comp = s.active_comp().unwrap();
+    let uid = |id: u64| comp.layer(LayerId(id)).unwrap().props.prop("transform/opacity").unwrap().uid;
+    let row = |id: u64, kind: RowKind| Row { layer: LayerId(id), depth: 1, kind };
+    let rows = vec![
+        row(layers[1], RowKind::Prop { uid: uid(layers[1]) }),
+        row(layers[1], RowKind::Expr { uid: uid(layers[1]), lines: 3 }),
+        row(layers[0], RowKind::Prop { uid: uid(layers[0]) }),
+    ];
+    // 1 s = 100 pt. Rows are 20 pt: centers at y 10 and, past the 60 pt editor, 90.
+    let tm = TMap { x0: 0.0, start: 0.0, pps: 100.0 };
+    let hit = |br: Rect| -> Vec<(u64, i64)> { keys_in_box(comp, &rows, &tm, 0.0, 20.0, br).iter().map(|k| (k.layer.0, k.time.0)).collect() };
+    let (one, two) = (Tick::from_seconds_f64(1.0).0, Tick::from_seconds_f64(2.0).0);
+    let across = |x0: f32, x1: f32| Rect::from_min_max(pos2(x0, 0.0), pos2(x1, 200.0));
+    assert_eq!(hit(across(0.0, 300.0)), [(layers[1], one), (layers[1], two), (layers[0], one), (layers[0], two)], "row order");
+    assert_eq!(hit(Rect::from_min_max(pos2(0.0, 95.5), pos2(300.0, 200.0))), [(layers[0], one), (layers[0], two)], "the box reaches the glyph's bottom point");
+    assert!(hit(Rect::from_min_max(pos2(0.0, 97.0), pos2(300.0, 200.0))).is_empty(), "the box is below the glyph");
+}
+
 #[test]
 fn layer_drop_target_skips_moving_layers_beyond_the_viewport() {
     let mut rows: Vec<Row> = (1..=6).map(|id| Row { layer: LayerId(id), depth: 0, kind: RowKind::Layer }).collect();

crates/ui-egui/tests/it/main.rs

@@ -91,6 +91,7 @@ mod ui_time_zoom_controls;
 mod ui_timeline_audio_levels;
 mod ui_timeline_effect_label;
 mod ui_timeline_group_select;
+mod ui_timeline_marquee;
 mod ui_timeline_pan;
 mod ui_timeline_shapes;
 mod ui_timeline_switches;

crates/ui-egui/tests/it/ui_timeline_marquee.rs

@@ -0,0 +1,76 @@
+//! The key marquee in the Timeline (#2857): a box selects a key as soon as it touches any part of
+//! the key's glyph, not when it reaches the key's center.
+
+use effectcraft_engine::Session;
+use effectcraft_engine::project::LayerId;
+use effectcraft_engine::time::Tick;
+use effectcraft_ui_egui::EffectcraftApp;
+use egui::{Event, Modifiers, Pos2, Vec2, pos2, vec2};
+use egui_kittest::Harness;
+use serde_json::json;
+
+/// How far a linear key's diamond reaches from its center, without its hairline (0.25 pt).
+const REACH: f32 = 5.5;
+
+/// A comp with one solid, its Opacity keys at 1 s and 2 s (linear) revealed.
+fn harness() -> (Harness<'static, EffectcraftApp>, u64) {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "Box", "width": 320, "height": 180, "frameRate": 30, "duration": 4})).unwrap();
+    let id = s.execute("layer.newSolid", json!({"name": "L", "color": "#e04020", "width": 80, "height": 80})).unwrap()["layer"].as_u64().unwrap();
+    for (t, v) in [(1.0, 0.0), (2.0, 100.0)] {
+        s.execute("prop.addKey", json!({"layer": id, "path": "transform/opacity", "time": t, "value": v})).unwrap();
+    }
+    s.execute("layer.select", json!({"layers": [id]})).unwrap();
+    let mut h = Harness::builder().with_size(vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(s));
+    h.run_steps(3);
+    let ctx = h.ctx.clone();
+    effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, "timeline.reveal.opacity", json!({})).unwrap();
+    h.run_steps(4);
+    (h, id)
+}
+
+/// The center of the key at `secs`: the middle of its hit rect, which is centered on the glyph.
+fn key_center(h: &Harness<'_, EffectcraftApp>, id: u64, secs: f64) -> Pos2 {
+    let comp = h.state().session.active_comp().unwrap();
+    let uid = comp.layer(LayerId(id)).unwrap().props.prop("transform/opacity").unwrap().uid;
+    let key = format!("timeline.keyAt.{uid}.{}", Tick::from_seconds_f64(secs).0);
+    let e = h.state().auto.find(&key).unwrap_or_else(|| panic!("no {key}"));
+    pos2(e.rect[0] + e.rect[2] / 2.0, e.rect[1] + e.rect[3] / 2.0)
+}
+
+/// Draw a marquee from `from` to `to`, then report the selected keys' times in seconds.
+fn marquee(h: &mut Harness<'_, EffectcraftApp>, from: Pos2, to: Pos2) -> Vec<f64> {
+    let modifiers = Modifiers::default();
+    h.input_mut().events.push(Event::PointerMoved(from));
+    h.step();
+    h.input_mut().events.push(Event::PointerButton { pos: from, button: egui::PointerButton::Primary, pressed: true, modifiers });
+    h.step();
+    for i in 1..=24 {
+        h.input_mut().events.push(Event::PointerMoved(from + (to - from) * (i as f32 / 24.0)));
+        h.step();
+    }
+    h.input_mut().events.push(Event::PointerButton { pos: to, button: egui::PointerButton::Primary, pressed: false, modifiers });
+    h.run_steps(2);
+    h.state().session.state.selected_keys.iter().map(|k| k.time.seconds()).collect()
+}
+
+#[test]
+fn a_box_selects_a_key_it_grazes_from_any_side_and_not_one_it_stops_short_of() {
+    let (mut h, id) = harness();
+    let c = key_center(&h, id, 1.0);
+    // Boxes end `gap` from the key's center, coming from the right, the left and below (the row
+    // above holds the layer bar, where a press would drag the layer).
+    let sides: [(&str, fn(f32) -> (Vec2, Vec2)); 3] = [
+        ("right", |gap| (vec2(30.0, 5.0), vec2(gap, -5.0))),
+        ("left", |gap| (vec2(-30.0, 5.0), vec2(-gap, -5.0))),
+        ("below", |gap| (vec2(5.0, 40.0), vec2(-5.0, gap))),
+    ];
+    for (side, gap_to_box) in sides {
+        let (from, to) = gap_to_box(REACH);
+        assert_eq!(marquee(&mut h, c + from, c + to), [1.0], "a box touching the glyph's {side} point selects the key");
+        // Clear of the glyph and its hairline by 1 pt.
+        let (from, to) = gap_to_box(REACH + 0.25 + 1.0);
+        assert!(marquee(&mut h, c + from, c + to).is_empty(), "a box 1 pt short on the {side} misses it");
+        assert!(h.state().session.state.selected_layers.is_empty(), "a box that catches nothing deselects (layers included)");
+    }
+}