G2: preserve keyframe selection during attribute edits

Co-authored-by: Codex <[email protected]>

crates/effects/src/ocio.rs

@@ -372,7 +372,7 @@ pub fn parse_spimtx(text: &str) -> Option<FileXform> {
     }
     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;
     }

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<KeyRef, KeyRef> = Default::default();
         let mut groups: std::collections::BTreeMap<(LayerId, Uid), Vec<Tick>> = 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<Keyframe> = 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<Tick, Tick> = 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<Tick> = 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()))
     })
 }

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;

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));
+}