From 8eb77a7d1e22c32632281b44839dc97f01cc3c8d Mon Sep 17 00:00:00 2001 From: sanyamsingh001 <314051937+sanyamsingh001@users.noreply.github.com> Date: Sat, 10 Oct 2026 13:22:32 +1000 Subject: [PATCH] G2: preserve keyframe selection during attribute edits Co-authored-by: Codex --- crates/effects/src/ocio.rs | 2 +- crates/engine/src/commands/prop.rs | 52 ++++---- crates/engine/src/lib.rs | 2 + crates/engine/src/tests_key_metadata_order.rs | 113 ++++++++++++++++++ 4 files changed, 144 insertions(+), 25 deletions(-) create mode 100644 crates/engine/src/tests_key_metadata_order.rs diff --git a/crates/effects/src/ocio.rs b/crates/effects/src/ocio.rs index d00f6e9e..c8fcf848 100644 --- a/crates/effects/src/ocio.rs +++ b/crates/effects/src/ocio.rs @@ -372,7 +372,7 @@ pub fn parse_spimtx(text: &str) -> Option { } let mut m = [0.0; 16]; let mut offset = [0.0; 4]; - for (r, row) in v.chunks_exact(4).enumerate() { + for (r, row) in v.as_chunks::<4>().0.iter().enumerate() { m[r * 4..r * 4 + 3].copy_from_slice(&row[..3]); offset[r] = row[3] / 65535.0; } diff --git a/crates/engine/src/commands/prop.rs b/crates/engine/src/commands/prop.rs index 063b179f..e2b0afd1 100644 --- a/crates/engine/src/commands/prop.rs +++ b/crates/engine/src/commands/prop.rs @@ -441,49 +441,53 @@ pub(crate) fn edit_keys(s: &mut Session, label: &str, merge: Option<&str>, f: im let sel = s.state.selected_keys.clone(); s.edit(label, merge, |proj, st| { let comp = proj.comp_mut(cid).ok_or(EngineError::NoComp)?; - let mut new_sel = vec![]; + let mut remapped: std::collections::HashMap = Default::default(); let mut groups: std::collections::BTreeMap<(LayerId, Uid), Vec> = Default::default(); for k in &sel { groups.entry((k.layer, k.prop)).or_default().push(k.time); } for ((lid, uid), times) in groups { let Some(pr) = comp.layer_mut(lid).and_then(|l| l.props.find_mut(uid)) else { continue }; - // Process in an order that avoids collisions when moving. + // Process each stored key once, in an order that avoids collisions when moving. let mut idx: Vec<(usize, Tick)> = times.iter().filter_map(|t| key_at(&pr.keys, *t).map(|i| (i, *t))).collect(); idx.sort_by_key(|x| x.0); - let mut moved: Vec = vec![]; - let mut keep_times = vec![]; - for (i, _) in idx.iter().rev() { - let mut t = pr.keys[*i].time; + idx.dedup_by_key(|x| x.0); + let mut moved: Vec<(Tick, Keyframe)> = vec![]; + // Target time -> surviving source time. A moved key replaces an existing key at + // its destination, including its selection identity. + let mut targets: std::collections::HashMap = Default::default(); + for (i, old) in idx.iter().rev() { + let Some(key) = pr.keys.get(*i) else { continue }; + let mut t = key.time; if f(&mut pr.keys, *i, &mut t) { - let mut k = pr.keys.remove(*i); - k.time = t; - moved.push(k); - } else if *i < pr.keys.len() { - keep_times.push(pr.keys[*i].time); + if *i < pr.keys.len() { + let mut k = pr.keys.remove(*i); + k.time = t; + moved.push((*old, k)); + } + } else if key_at(&pr.keys, *old).is_some() { + targets.insert(*old, *old); } } - for k in moved { - new_sel.push(KeyRef { layer: lid, prop: uid, time: k.time }); + for (old, k) in moved { + targets.insert(k.time, old); match pr.keys.binary_search_by(|x| x.time.cmp(&k.time)) { Ok(j) => pr.keys[j] = k, Err(j) => pr.keys.insert(j, k), } } - for t in keep_times { - new_sel.push(KeyRef { layer: lid, prop: uid, time: t }); - } - // Roving keys follow their neighbours: re-time and keep the selection on them. - let before: Vec = pr.keys.iter().map(|k| k.time).collect(); - if effectcraft_keyframe::retime_roving(&mut pr.keys, pr.spatial) { - for r in new_sel.iter_mut().filter(|r| r.layer == lid && r.prop == uid) { - if let Some(i) = before.iter().position(|t| *t == r.time) { - r.time = pr.keys[i].time; - } + // Resolve exact stored indices before roving changes their times. Deleted or + // stale references cannot select a neighbouring key. + let tracked: Vec<(Tick, usize)> = targets.into_iter().filter_map(|(at, old)| key_at(&pr.keys, at).map(|i| (old, i))).collect(); + effectcraft_keyframe::retime_roving(&mut pr.keys, pr.spatial); + for (old, i) in tracked { + if let Some(key) = pr.keys.get(i) { + remapped.insert(KeyRef { layer: lid, prop: uid, time: old }, KeyRef { layer: lid, prop: uid, time: key.time }); } } } - st.selected_keys = new_sel; + let mut seen = std::collections::HashSet::new(); + st.selected_keys = sel.iter().filter_map(|k| remapped.get(k).copied()).filter(|k| seen.insert(*k)).collect(); Ok(json!(st.selected_keys.len())) }) } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 24ccf1af..2790ab2a 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1020,6 +1020,8 @@ mod tests_fidelity; #[cfg(test)] mod tests_frame_export; #[cfg(test)] +mod tests_key_metadata_order; +#[cfg(test)] mod tests_keylight; #[cfg(test)] mod tests_lottie; diff --git a/crates/engine/src/tests_key_metadata_order.rs b/crates/engine/src/tests_key_metadata_order.rs new file mode 100644 index 00000000..fe2337c2 --- /dev/null +++ b/crates/engine/src/tests_key_metadata_order.rs @@ -0,0 +1,113 @@ +//! Key attributes preserve the order used by the next stagger gesture. + +use std::sync::Arc; + +use effectcraft_keyframe::Interp; +use effectcraft_project::LayerId; +use serde_json::json; + +use crate::{KeyRef, Session, time::FrameRate}; + +fn setup() -> (Session, u64) { + let mut s = Session::default(); + s.execute_checked("comp.new", json!({"width": 64, "height": 64, "frameRate": 30, "duration": 4})).unwrap(); + let layer = s.execute_checked("layer.newSolid", json!({"name": "Box", "color": "#fff"})).unwrap()["layer"].as_u64().unwrap(); + (s, layer) +} + +fn add(s: &mut Session, layer: u64, path: &str, time: i64, value: serde_json::Value) { + s.execute_checked("prop.addKey", json!({"layer": layer, "path": path, "time": time, "value": value})).unwrap(); +} + +fn uid(s: &Session, layer: u64, path: &str) -> u64 { + s.active_comp().unwrap().layer(LayerId(layer)).unwrap().props.prop(path).unwrap().uid +} + +fn key(layer: u64, prop: u64, frame: i64) -> KeyRef { + KeyRef { layer: LayerId(layer), prop, time: FrameRate::FPS_30.tick_of(frame) } +} + +#[test] +fn easing_preserves_interleaved_layer_and_property_order_with_one_undo() { + let (mut s, first) = setup(); + let second = s.execute_checked("layer.newSolid", json!({"name": "Other", "color": "#fff"})).unwrap()["layer"].as_u64().unwrap(); + for layer in [first, second] { + for time in [0, 1] { + add(&mut s, layer, "transform/opacity", time, json!(time * 25)); + } + } + add(&mut s, first, "transform/position", 1, json!([10, 20])); + let a = uid(&s, first, "transform/opacity"); + let b = uid(&s, second, "transform/opacity"); + let position = uid(&s, first, "transform/position"); + s.execute_checked( + "keys.select", + json!({"keys": [ + {"layer": second, "prop": b, "time": 1}, + {"layer": first, "prop": position, "time": 1}, + {"layer": first, "prop": a, "time": 0}, + {"layer": second, "prop": b, "time": 0} + ]}), + ) + .unwrap(); + let order = s.state.selected_keys.clone(); + let before = s.project.clone(); + s.history.clear(); + s.execute_checked("keys.easyEase", json!({})).unwrap(); + assert_eq!(s.state.selected_keys, order); + assert_eq!(s.history.undo.len(), 1); + for selected in &order { + let keys = &s.active_comp().unwrap().layer(selected.layer).unwrap().props.find(selected.prop).unwrap().keys; + assert_eq!(keys.iter().find(|k| k.time == selected.time).unwrap().out_interp, Interp::Bezier); + } + s.execute_checked("edit.undo", json!({})).unwrap(); + assert!(Arc::ptr_eq(&s.project, &before)); + assert_eq!(s.state.selected_keys, order); +} + +#[test] +fn roving_keeps_the_selected_key_identity_and_arbitrary_order() { + let (mut s, layer) = setup(); + for (time, x) in [(0, 0), (1, 25), (2, 100)] { + add(&mut s, layer, "transform/position", time, json!([x, 0])); + } + let prop = uid(&s, layer, "transform/position"); + s.execute_checked( + "keys.select", + json!({"keys": [ + {"layer": layer, "prop": prop, "time": 1}, + {"layer": layer, "prop": prop, "time": 2}, + {"layer": layer, "prop": prop, "time": 0} + ]}), + ) + .unwrap(); + s.execute_checked("keys.interpolation", json!({"roving": true})).unwrap(); + assert_eq!(s.state.selected_keys, vec![key(layer, prop, 15), key(layer, prop, 60), key(layer, prop, 0)]); + let position = s.active_comp().unwrap().layer(LayerId(layer)).unwrap().props.find(prop).unwrap(); + assert_eq!(position.keys[1].value.as_vec2(), [25.0, 0.0]); + assert_eq!(position.keys[1].time, FrameRate::FPS_30.tick_of(15)); + assert!(position.keys[1].roving); + assert!(!position.keys[0].roving && !position.keys[2].roving); +} + +#[test] +fn attribute_edit_deduplicates_legacy_refs_and_drops_stale_refs_without_snapping() { + let (mut s, layer) = setup(); + for time in [0, 1] { + add(&mut s, layer, "transform/opacity", time, json!(20)); + } + let prop = uid(&s, layer, "transform/opacity"); + let first = key(layer, prop, 0); + let second = key(layer, prop, 30); + s.state.selected_keys = vec![second, key(layer, prop, 10), first, second, key(layer, u64::MAX, 0), first, key(u64::MAX, prop, 0)]; + s.history.clear(); + let result = s.execute_checked("keys.toggleHold", json!({})).unwrap(); + assert_eq!(result, json!(2)); + assert_eq!(s.state.selected_keys, vec![second, first]); + assert_eq!(s.history.undo.len(), 1); + let opacity = s.active_comp().unwrap().layer(LayerId(layer)).unwrap().props.find(prop).unwrap(); + assert!(opacity.keys.iter().all(|k| k.out_interp == Interp::Hold)); + s.execute_checked("edit.undo", json!({})).unwrap(); + let opacity = s.active_comp().unwrap().layer(LayerId(layer)).unwrap().props.find(prop).unwrap(); + assert!(opacity.keys.iter().all(|k| k.out_interp == Interp::Linear)); +}