From c4c079d4f4ae5c344ac4b23d8dbb11625830fd5d Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Thu, 15 Oct 2026 20:57:50 +0530 Subject: [PATCH] fix(engine): copy only the selected keyframes across properties (#1552) Clicking keyframes on Position, Rotation and Scale highlights those properties, and Copy then added every key of a highlighted property that had none selected, so deselected keys came back on paste. The keyframe copy branch also left a stale value clipboard that Paste preferred over the new keys. Copy now takes whole animations only when no key is selected or the Transform group is selected, and every copy branch clears the clipboards. Also collapses a nested `if` in the STL importer into a match guard, which Clippy 1.95 flags under -D warnings (no behaviour change). Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands/edit.rs | 13 ++-- crates/engine/src/tests_menu_cmds.rs | 4 +- crates/engine/src/tests_timeline.rs | 79 +++++++++++++++++++++++ crates/model/src/stl.rs | 6 +- crates/ui-egui/tests/ui_keyframes.rs | 96 ++++++++++++++++++++++++++++ 5 files changed, 187 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/commands/edit.rs b/crates/engine/src/commands/edit.rs index 06ba43b1..bbdc43fe 100644 --- a/crates/engine/src/commands/edit.rs +++ b/crates/engine/src/commands/edit.rs @@ -228,9 +228,7 @@ fn copy(s: &mut Session, p: &Value) -> Result { } // Keyframes selected → copy keys (pasted at the CTI). if !s.state.selected_keys.is_empty() && p.get("layers").is_none() { - s.state.link_clipboard = None; - s.state.effect_clipboard.clear(); - s.state.contents_clipboard.clear(); + clear_clipboards(s); return s.execute("keys.copy", json!({})); } // Effects selected (Effect Controls / timeline) → copy the effects. @@ -256,12 +254,14 @@ fn copy(s: &mut Session, p: &Value) -> Result { } /// Edit ▸ Copy with properties selected (Position, or the Transform group), as in After -/// Effects: an animated property copies its keyframes (the selected ones, else all of them) and -/// an unanimated one its value, for Paste onto the same properties of other layers (#493). +/// Effects: an animated property copies its keyframes and an unanimated one its value, for Paste +/// onto the same properties of other layers (#493). With keyframes selected only those are copied, +/// even across several highlighted properties (#1552); a selected Transform group copies them all. /// `None` when no property (or Transform group) is selected. fn copy_properties(s: &mut Session) -> Result> { let Some(comp) = s.active_comp() else { return Ok(None) }; let mut leaves = vec![]; + let mut copy_all_keys = s.state.selected_keys.is_empty(); for (lid, uid) in &s.state.selected_props { let Some(l) = comp.layer(*lid) else { continue }; if l.props.find(*uid).is_some() { @@ -269,6 +269,7 @@ fn copy_properties(s: &mut Session) -> Result> { } else if let Some(g) = l.props.find_group(*uid) && l.props.match_path_of(*uid).as_deref() == Some("transform") { + copy_all_keys = true; g.walk("", &mut |_, pr| leaves.push((*lid, pr.uid))); } } @@ -286,7 +287,7 @@ fn copy_properties(s: &mut Session) -> Result> { { values.push((path, pr.value.clone())); } - } else if !keys.iter().any(|k| k.layer == *lid && k.prop == *uid) { + } else if copy_all_keys && !keys.iter().any(|k| k.layer == *lid && k.prop == *uid) { keys.extend(pr.keys.iter().map(|k| KeyRef { layer: *lid, prop: *uid, time: k.time })); } } diff --git a/crates/engine/src/tests_menu_cmds.rs b/crates/engine/src/tests_menu_cmds.rs index ec63651f..5203872a 100644 --- a/crates/engine/src/tests_menu_cmds.rs +++ b/crates/engine/src/tests_menu_cmds.rs @@ -433,8 +433,10 @@ fn copy_a_selected_property_pastes_onto_the_same_property() { s.execute("prop.toggleAnimation", json!({"layer": a, "path": "transform/opacity"})).unwrap(); s.execute("time.set", json!({"time": 1.0})).unwrap(); s.execute("prop.set", json!({"layer": a, "path": "transform/opacity", "value": 0})).unwrap(); + s.execute("prop.select", json!({"layer": a, "path": "transform/opacity"})).unwrap(); + assert_eq!(s.state.selected_keys.len(), 2); let transform = layer(&s, a).props.sub("transform").unwrap().uid; - s.execute("prop.select", json!({"layer": a, "prop": transform})).unwrap(); + s.execute("prop.select", json!({"layer": a, "prop": transform, "selectKeys": false})).unwrap(); s.execute("edit.copy", json!({})).unwrap(); s.execute("layer.select", json!({"layers": [b]})).unwrap(); s.execute("time.set", json!({"time": 2.0})).unwrap(); diff --git a/crates/engine/src/tests_timeline.rs b/crates/engine/src/tests_timeline.rs index 71a9943c..39a4d40b 100644 --- a/crates/engine/src/tests_timeline.rs +++ b/crates/engine/src/tests_timeline.rs @@ -108,6 +108,85 @@ fn keyframe_copy_paste_keeps_relative_timing_and_crosses_properties() { assert!(e.is_err() || prop(&s, l2, "transform/position").keys.is_empty()); } +#[test] +fn copy_selected_keys_excludes_deselected_properties_and_stale_values() { + let (mut s, source) = setup(); + s.execute("prop.set", json!({"layer": source, "path": "transform/opacity", "value": 25})).unwrap(); + select_prop_keys(&mut s, source, "transform/opacity"); + s.execute("edit.copy", json!({})).unwrap(); + assert_eq!(s.state.value_clipboard.len(), 1); + animate(&mut s, source, "transform/position", &[(0.0, json!([10, 20, 0])), (1.0, json!([30, 40, 0]))]); + animate(&mut s, source, "transform/rotation", &[(0.0, json!(15)), (1.0, json!(90))]); + animate(&mut s, source, "transform/scale", &[(0.0, json!([50, 60, 100])), (1.0, json!([150, 160, 100]))]); + select_prop_keys(&mut s, source, "transform/scale"); + s.execute( + "keys.select", + json!({"keys": [{"layer": source, "path": "transform/position", "time": 0}, {"layer": source, "path": "transform/rotation", "time": 1}], "add": true, "selectProperties": true}), + ) + .unwrap(); + s.execute( + "keys.select", + json!({"keys": [{"layer": source, "path": "transform/scale", "time": 0}, {"layer": source, "path": "transform/scale", "time": 1}], "toggle": true, "selectProperties": true}), + ) + .unwrap(); + s.execute("keys.easyEase", json!({})).unwrap(); + assert_eq!(s.state.selected_keys.len(), 2); + assert_eq!(s.state.selected_props.len(), 3); + assert_eq!(s.execute("edit.copy", json!({})).unwrap(), json!(2)); + assert!(s.state.value_clipboard.is_empty()); + let target = s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("time.set", json!({"time": 2})).unwrap(); + s.execute("layer.select", json!({"layers": [target]})).unwrap(); + s.execute("edit.paste", json!({})).unwrap(); + undo_redo_roundtrip( + &mut s, + |s| { + for (path, time, value) in [("transform/position", 2.0, KV::Vec3([10.0, 20.0, 0.0])), ("transform/rotation", 3.0, KV::Scalar(90.0))] { + let p = prop(s, target, path); + assert_eq!(p.keys.len(), 1); + let k = &p.keys[0]; + assert_eq!((k.time.seconds(), &k.value), (time, &value)); + assert_eq!((k.in_interp, k.out_interp), (Interp::Bezier, Interp::Bezier)); + assert_eq!(k.in_ease[0], effectcraft_keyframe::Ease::EASY); + assert_eq!(k.out_ease[0], effectcraft_keyframe::Ease::EASY); + } + assert_eq!(s.state.selected_keys.len(), 2); + assert_eq!(prop(s, target, "transform/scale").value, KV::Vec3([100.0; 3])); + assert!(prop(s, target, "transform/scale").keys.is_empty()); + assert_eq!(prop(s, target, "transform/opacity").value, KV::Scalar(100.0)); + }, + |s| { + assert!(prop(s, target, "transform/position").keys.is_empty()); + assert!(prop(s, target, "transform/rotation").keys.is_empty()); + }, + ); +} + +#[test] +fn copy_mixed_animated_and_static_properties_keeps_the_static_value() { + let (mut s, source) = setup(); + animate(&mut s, source, "transform/position", &[(0.0, json!([10, 20, 0])), (1.0, json!([30, 40, 0]))]); + s.execute("prop.set", json!({"layer": source, "path": "transform/rotation", "value": 45})).unwrap(); + select_prop_keys(&mut s, source, "transform/position"); + s.execute("prop.select", json!({"layer": source, "path": "transform/rotation", "add": true})).unwrap(); + assert_eq!(s.execute("edit.copy", json!({})).unwrap(), json!({"keys": 2, "values": 1})); + let target = s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("time.set", json!({"time": 2})).unwrap(); + s.execute("layer.select", json!({"layers": [target]})).unwrap(); + let undo = s.history.undo.len(); + s.execute("edit.paste", json!({})).unwrap(); + assert_eq!(s.history.undo.len(), undo + 1); + assert_eq!(prop(&s, target, "transform/rotation").value, KV::Scalar(45.0)); + let position = prop(&s, target, "transform/position"); + assert_eq!( + position.keys.iter().map(|k| (k.time.seconds(), k.value.clone())).collect::>(), + vec![(2.0, KV::Vec3([10.0, 20.0, 0.0])), (3.0, KV::Vec3([30.0, 40.0, 0.0]))] + ); + s.execute("edit.undo", json!({})).unwrap(); + assert!(prop(&s, target, "transform/position").keys.is_empty()); + assert_eq!(prop(&s, target, "transform/rotation").value, KV::Scalar(0.0)); +} + #[test] fn cut_removes_keys_and_paste_restores_them() { let (mut s, l) = setup(); diff --git a/crates/model/src/stl.rs b/crates/model/src/stl.rs index 6684858a..34e9a7f0 100644 --- a/crates/model/src/stl.rs +++ b/crates/model/src/stl.rs @@ -175,10 +175,8 @@ fn ascii(bytes: &[u8]) -> Result { let Some(key) = it.next() else { continue }; let bad = || perr(format!("STL line {}: expected three numbers", ln + 1)); match key.to_ascii_lowercase().as_str() { - "solid" => { - if name.is_none() { - name = Some(line.get(5..).unwrap_or("").trim().to_string()); - } + "solid" if name.is_none() => { + name = Some(line.get(5..).unwrap_or("").trim().to_string()); } "facet" => { // `facet normal nx ny nz` diff --git a/crates/ui-egui/tests/ui_keyframes.rs b/crates/ui-egui/tests/ui_keyframes.rs index 4b5d1338..6167fbf8 100644 --- a/crates/ui-egui/tests/ui_keyframes.rs +++ b/crates/ui-egui/tests/ui_keyframes.rs @@ -126,6 +126,102 @@ fn click_with(h: &mut Harness<'_, EffectcraftApp>, p: Pos2, modifiers: egui::Mod h.run_steps(2); } +#[test] +fn copy_and_paste_key_subsets_across_position_rotation_and_scale() { + for graph in [false, true] { + let (mut h, id, _) = harness(); + let paths = ["transform/position", "transform/rotation", "transform/scale"]; + for (path, times, values) in [ + (paths[0], [0.5, 1.0], [json!([100, 200, 0]), json!([300, 400, 0])]), + (paths[1], [0.5, 1.5], [json!(500), json!(900)]), + (paths[2], [0.0, 1.0], [json!([1500, 1600, 100]), json!([1700, 1800, 100])]), + ] { + for (time, value) in times.into_iter().zip(values) { + h.state_mut().session.execute("prop.addKey", json!({"layer": id.0, "path": path, "time": time, "value": value})).unwrap(); + } + } + let uids: Vec = paths.iter().map(|path| h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap().uid).collect(); + let ctx = h.ctx.clone(); + for command in ["timeline.reveal.position", "timeline.revealAdd.rotation", "timeline.revealAdd.scale"] { + effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, command, json!({})).unwrap(); + } + if graph { + h.state_mut().session.execute("edit.deselectAll", json!({})).unwrap(); + for uid in &uids { + h.state_mut().session.execute("prop.select", json!({"layer": id.0, "prop": uid, "add": true, "selectKeys": false})).unwrap(); + } + h.state_mut().ui.timeline.graph_editor = true; + h.state_mut().ui.timeline.graph_mode = "value".into(); + } + h.run_steps(4); + let key_positions = |h: &Harness<'_, EffectcraftApp>, uid: u64| { + if !graph { + return keys(h, uid); + } + let mut positions: Vec = h + .state() + .auto + .query(&format!("timeline.graph.key.{uid}.0.")) + .iter() + .map(|e| pos2(e.rect[0] + e.rect[2] / 2.0, e.rect[1] + e.rect[3] / 2.0)) + .collect(); + positions.sort_by(|a, b| a.x.total_cmp(&b.x)); + positions + }; + let shift = egui::Modifiers { shift: true, ..Default::default() }; + for include_scale in [true, false] { + h.state_mut().session.execute("keys.select", json!({"keys": []})).unwrap(); + h.run_steps(2); + let original: Vec<_> = + paths.iter().map(|path| h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap().keys.clone()).collect(); + let positions = key_positions(&h, uids[0]); + let rotations = key_positions(&h, uids[1]); + let scales = key_positions(&h, uids[2]); + click_with(&mut h, scales[0], Default::default()); + click_with(&mut h, positions[0], shift); + click_with(&mut h, rotations[1], shift); + if !include_scale { + click_with(&mut h, scales[1], shift); + click_with(&mut h, scales[0], shift); + click_with(&mut h, scales[1], shift); + } + let count = if include_scale { 3 } else { 2 }; + assert_eq!(h.state().session.state.selected_keys.len(), count); + assert_eq!(h.state().session.state.selected_props.len(), 3); + h.input_mut().events.push(Event::Copy); + h.step(); + let note = format!("EffectCraft: {count} keyframes"); + assert!(h.output().platform_output.commands.iter().any(|command| matches!(command, egui::OutputCommand::CopyText(text) if text == ¬e))); + h.state_mut().session.set_time(effectcraft_engine::time::Tick::from_seconds_f64(2.0)); + let undo_steps = h.state().session.history.undo.len(); + h.input_mut().events.push(Event::Paste(note)); + h.run_steps(2); + assert_eq!(h.state().session.history.undo.len(), undo_steps + 1); + for (path, time, value) in + [(paths[0], if include_scale { 2.5 } else { 2.0 }, json!([100.0, 200.0, 0.0])), (paths[1], if include_scale { 3.5 } else { 3.0 }, json!(900.0))] + { + let p = h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap(); + assert_eq!(p.keys.len(), 3); + assert_eq!(p.keys[2].time.seconds(), time); + assert_eq!(p.keys[2].value.to_json(), value); + } + let scale = h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(paths[2]).unwrap(); + if include_scale { + assert_eq!(scale.keys.len(), 3); + assert_eq!(scale.keys[2].time.seconds(), 2.0); + assert_eq!(scale.keys[2].value.to_json(), json!([1500.0, 1600.0, 100.0])); + } else { + assert_eq!(scale.keys, original[2]); + } + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + for (path, expected) in paths.iter().zip(&original) { + assert_eq!(&h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap().keys, expected); + } + h.run_steps(2); + } + } +} + #[test] fn shift_click_toggles_and_ctrl_click_switches_interpolation() { let (mut h, id, uid) = harness();