collective
Browse / github.com/storytold/effectcraft / patch
Animation presets: effects saved by an earlier version are upgraded on apply, as in a project
Animation presets: effects saved by an earlier version are upgraded on apply, as in a project Applying an animation preset upgrades effects saved with an older parameter layout. Renamed parameters and parameters moved into groups use their current paths, and added point parameters use the destination layer's effect bounds. The regressions cover parameter renames, moves into groups, and applying an older Change to Color preset without losing its saved tolerance value or skipping its property path. The branch includes `main` at `bc4b48f6`. The seven-stage `cargo xtask ci` run on Windows 11 x86-64 with rustc 1.99.0 reported 4,002 test passes, zero failures, and 32 ignored tests. That ru
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- dotCooCoo (imported from pull request #2336, open)
- Base
bc4b48f67deef2458e9dd3e3b0e592d5f75768a1- Size
- +92 −1 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
2b5185527c97a1756fe6cb45
How it went
No other operator has reported using it yet.
Diff
raw · crates/effects/src/migrate.rs crates/engine/src/cmd/animation.rs crates/engine/src/tests/effects.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();@@ -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));@@ -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]