Animation presets: effects saved by an earlier version are upgraded on apply, as in a project

Opening a project runs migrate::upgrade_instance on every effect, so an
instance saved before a parameter was renamed, moved into a twirl-down
group, added or had its popup reordered takes the current shape.
Apply Animation Preset added a preset's effects as saved, without that
upgrade. A Change to Color saved with its single Tolerance applied
with that old property and without the Tolerance group, so the effect
did not read the saved tolerance.

apply_preset now upgrades each preset effect before adding it, sized by
the target layer's effect bounds as upgrade_effects does. A lone effect
property saved at a renamed or moved parameter id
(effects/ec.color.changetocolor/tolerance) is rewritten to the current
id through migrate::current_param_id, so it is applied instead of being
listed in skipped.

crates/effects/src/migrate.rs

@@ -344,6 +344,23 @@ pub fn upgrade_instance(spec: &EffectSpec, g: &mut PropGroup, ids: &mut Ids, lay
     *g != before
 }
 
+/// The spec id of the parameter that an instance of `spec` saved at `id` (`a/b/param`) is now:
+/// `id` itself when the spec still has it, the new id of a renamed parameter
+/// ([`PARAM_ID_ALIASES`]), or the twirl-down path of a top-level parameter that moved into a
+/// group, as [`upgrade_instance`] finds them. Returns `None` when no parameter matches.
+pub fn current_param_id(spec: &EffectSpec, id: &str) -> Option<&'static str> {
+    if let Some(ps) = spec.params.iter().find(|ps| ps.id == id) {
+        return Some(ps.id);
+    }
+    if let Some((_, _, new)) = PARAM_ID_ALIASES.iter().find(|(e, old, _)| *e == spec.id && *old == id) {
+        return Some(new);
+    }
+    if id.contains('/') {
+        return None;
+    }
+    spec.params.iter().find(|ps| ps.id.rsplit('/').next() == Some(id)).map(|ps| ps.id)
+}
+
 /// Bring an effect instance saved before project schema 2 into effect space (#227).
 ///
 /// Layers without a source rectangle (shape, text) have comp-sized effect bounds centred on
@@ -559,6 +576,21 @@ mod tests {
         assert_eq!(prop_at(&mut g, "stabilization/smoothness").unwrap().value, Value::Scalar(12.0));
     }
 
+    /// A saved parameter id maps to the parameter `upgrade_instance` moves it to.
+    #[test]
+    fn current_param_id_follows_renames_and_twirl_down_moves() {
+        let stab = find(crate::warp_stab::ID).unwrap();
+        assert_eq!(current_param_id(stab, "stabilization/smoothness"), Some("stabilization/smoothness"));
+        assert_eq!(current_param_id(stab, "smoothness"), Some("stabilization/smoothness"));
+        let ctc = find("ec.color.changetocolor").unwrap();
+        assert_eq!(current_param_id(ctc, "tolerance"), Some("toleranceGroup/hue"));
+        // Radio Waves' old Velocity is the growth speed, not the new Velocity.
+        let waves = find("ec.generate.radiowaves").unwrap();
+        assert_eq!(current_param_id(waves, "velocity"), Some("waveMotion/expansion"));
+        assert_eq!(current_param_id(stab, "noSuchParam"), None);
+        assert_eq!(current_param_id(stab, "other/smoothness"), None);
+    }
+
     #[test]
     fn card_wipe_back_layer_popup_becomes_a_layer_parameter() {
         let spec = find("ec.transition.cardwipe").unwrap();

crates/engine/src/commands/animation.rs

@@ -88,6 +88,21 @@ fn save_preset(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({"path": path, "properties": props.len(), "effects": effects.len()}))
 }
 
+/// Rewrite an effect property path in a preset (`effects/<effect id>[#n]/<param>`) to the current
+/// parameter id when it names a parameter that the effect has since renamed or moved into a
+/// twirl-down group. Any other path comes back unchanged.
+fn upgrade_effect_path(path: &str) -> String {
+    let Some((seg, tail)) = path.strip_prefix("effects/").and_then(|r| r.split_once('/')) else { return path.to_string() };
+    let effect = match seg.rsplit_once('#') {
+        Some((m, n)) if !m.is_empty() && n.parse::<usize>().is_ok() => m,
+        _ => seg,
+    };
+    match effectcraft_effects::find(effect).and_then(|spec| effectcraft_effects::migrate::current_param_id(spec, tail)) {
+        Some(id) if id != tail => format!("effects/{seg}/{id}"),
+        _ => path.to_string(),
+    }
+}
+
 fn apply_preset(s: &mut Session, p: &Value) -> Result<Value> {
     let doc: Value = match (p.get("preset"), str_p(p, "path")) {
         (Some(v), _) => v.clone(),
@@ -117,7 +132,7 @@ fn apply_preset(s: &mut Session, p: &Value) -> Result<Value> {
         .map(|a| {
             a.iter()
                 .filter_map(|v| {
-                    let path = v["path"].as_str()?.to_string();
+                    let path = upgrade_effect_path(v["path"].as_str()?);
                     let value: KV = serde_json::from_value(v["value"].clone()).ok()?;
                     let keys: Vec<Keyframe> = serde_json::from_value(v["keys"].clone()).unwrap_or_default();
                     let expr = serde_json::from_value(v["expression"].clone()).ok().flatten();
@@ -130,16 +145,29 @@ fn apply_preset(s: &mut Session, p: &Value) -> Result<Value> {
         })
         .unwrap_or_default();
     let t = s.time();
+    // Effect bounds per layer size the point defaults of parameters added since the preset was saved.
+    let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?;
+    let sizes: BTreeMap<LayerId, [f64; 2]> =
+        ids.iter().filter_map(|lid| comp.layer(*lid).map(|l| (*lid, effectcraft_render::effect_bounds(&s.project, comp, l).0))).collect();
     let mut applied = 0;
     let mut skipped = vec![];
     s.edit("Apply Animation Preset", None, |proj, _| {
         let mut next = proj.next_id;
         for lid in &ids {
+            let size = sizes.get(lid).copied().unwrap_or([100.0, 100.0]);
             let l = layer_mut(proj, cid, *lid)?;
             let lt = l.layer_time(t);
             if let Some(fx) = l.props.sub_mut("effects") {
                 for g in &effects {
                     let mut g = g.clone();
+                    // Effects saved by an earlier version get the upgrade a project gets on open.
+                    let spec = match &g.kind {
+                        GroupKind::Effect { effect } => effectcraft_effects::find(effect),
+                        _ => None,
+                    };
+                    if let Some(spec) = spec {
+                        effectcraft_effects::migrate::upgrade_instance(spec, &mut g, &mut Ids(&mut next), size);
+                    }
                     g.reassign_uids(&mut next);
                     next += 1;
                     fx.children.push(Node::Group(g));

crates/engine/src/tests_effects.rs

@@ -772,6 +772,37 @@ fn preset_round_trips_effect_properties_by_match_name() {
     }
 }
 
+/// An animation preset saved by an earlier version gets the upgrade a project gets on open: a
+/// Change to Color saved with its single Tolerance applies with the Tolerance group, that value
+/// as its Hue and the added parameters at their legacy defaults, and a lone property saved at the
+/// old id applies to the new one.
+#[test]
+fn applying_an_old_preset_upgrades_its_effects_and_property_paths() {
+    let (mut s, a, b) = comp_with_two_solids();
+    apply(&mut s, a, "Change to Color");
+    let mut old = layer(&s, a).effects().unwrap().groups().next().unwrap().clone();
+    old.children.retain(|c| c.match_id() != "toleranceGroup");
+    old.children.push(effectcraft_project::Property::new(0, "tolerance", "Tolerance", KV::Scalar(12.0)).into());
+    let preset = json!({"effectcraftAnimationPreset": 1, "name": "old", "properties": [], "effects": [old]});
+    let r = s.execute("anim.applyPreset", json!({"preset": preset, "layers": [b]})).unwrap();
+    assert_eq!(r["applied"], 1, "{r}");
+    let l = layer(&s, b);
+    let g = l.effects().unwrap().groups().next().unwrap();
+    assert!(g.get("tolerance").is_none(), "the old Tolerance is still there");
+    let tol = g.sub("toleranceGroup").expect("no Tolerance group");
+    assert_eq!(tol.get("hue").unwrap().value, KV::Scalar(12.0));
+    assert_eq!(tol.get("lightness").unwrap().value, KV::Scalar(100.0));
+    assert_eq!(tol.get("saturation").unwrap().value, KV::Scalar(100.0));
+    let path = "effects/ec.color.changetocolor/tolerance";
+    let preset = json!({"effectcraftAnimationPreset": 1, "name": "old", "properties": [{"path": path, "value": KV::Scalar(30.0), "keys": []}], "effects": []});
+    let r = s.execute("anim.applyPreset", json!({"preset": preset, "layers": [a]})).unwrap();
+    assert_eq!(r["applied"], 1, "{r}");
+    assert_eq!(r["skipped"], json!([]), "{r}");
+    let l = layer(&s, a);
+    let hue = l.effects().unwrap().groups().next().unwrap().sub("toleranceGroup").unwrap().get("hue").unwrap().value.clone();
+    assert_eq!(hue, KV::Scalar(30.0));
+}
+
 /// #2064: `prop.addKey` accepts the `effects/<match name>[#n]/<property>` path that
 /// `essential.list` reports.
 #[test]