From 0cb05a601bd95d3ad451b9e7887e9c6a7825982e Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 24 Oct 2026 13:24:17 -0700 Subject: [PATCH] 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 | 32 +++++++++++++++++++++++++ crates/engine/src/commands/animation.rs | 30 ++++++++++++++++++++++- crates/engine/src/tests_effects.rs | 31 ++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/crates/effects/src/migrate.rs b/crates/effects/src/migrate.rs index b5976b761..35d0960e0 100644 --- a/crates/effects/src/migrate.rs +++ b/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(); diff --git a/crates/engine/src/commands/animation.rs b/crates/engine/src/commands/animation.rs index 1bdf3fed7..ddf6a79e5 100644 --- a/crates/engine/src/commands/animation.rs +++ b/crates/engine/src/commands/animation.rs @@ -88,6 +88,21 @@ fn save_preset(s: &mut Session, p: &Value) -> Result { Ok(json!({"path": path, "properties": props.len(), "effects": effects.len()})) } +/// Rewrite an effect property path in a preset (`effects/[#n]/`) 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::().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 { 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 { .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 = 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 { }) .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 = + 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)); diff --git a/crates/engine/src/tests_effects.rs b/crates/engine/src/tests_effects.rs index 1ea57f4cf..28968b901 100644 --- a/crates/engine/src/tests_effects.rs +++ b/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/[#n]/` path that /// `essential.list` reports. #[test]