diff --git a/crates/engine/src/commands/link.rs b/crates/engine/src/commands/link.rs index 2eeb7c5a0..66145f17e 100644 --- a/crates/engine/src/commands/link.rs +++ b/crates/engine/src/commands/link.rs @@ -102,11 +102,39 @@ fn quote(s: &str) -> String { /// The AE expression reference to property `uid` of `target` as seen from a property of `from`. pub(crate) fn reference(comp: &Comp, from: &Layer, target: &Layer, uid: Uid, compact: bool) -> Option { + reference_with(comp, from, target, uid, compact, false) +} + +/// [`reference`], with the properties inside an effect selected by their index (`by_index`, an +/// Alt pick whip in After Effects): `effect("Color Control")(1)` rather than `("Color")`. The +/// effect keeps its name. An index is the property's place among its group's children, as an +/// expression's `(1)` reads it: it follows a renamed parameter, not one moved to another place. +pub(crate) fn reference_with(comp: &Comp, from: &Layer, target: &Layer, uid: Uid, compact: bool, by_index: bool) -> Option { let chain = target.props.node_chain(uid)?; + // Each node's one-based index among its group's children, and whether it lies inside an + // effect (below the effect itself): those are selected by index with `by_index`. + let mut parent = &target.props; + let mut depth_in_effects: Option = None; + let mut by_number: Vec> = vec![]; + for n in &chain { + let at = parent.children.iter().position(|c| c.uid() == n.uid()).map(|i| i + 1); + by_number.push(at.filter(|_| by_index && depth_in_effects.is_some_and(|d| d >= 1))); + depth_in_effects = match depth_in_effects { + Some(d) => Some(d + 1), + None => (n.match_id() == "effects").then_some(0), + }; + if let Node::Group(g) = n { + parent = g; + } + } + let selector = |i: usize, label: &str| match by_number.get(i).copied().flatten() { + Some(at) => format!("({at})"), + None => format!("({})", quote(label)), + }; let mut parts: Vec = vec![]; let mut pending: Option<&str> = None; let mut in_effect = false; - for n in chain { + for (i, n) in chain.iter().copied().enumerate() { if let Some(f) = pending.take() { parts.push(format!("{f}({})", quote(n.name()))); in_effect |= f == "effect"; @@ -119,7 +147,7 @@ pub(crate) fn reference(comp: &Comp, from: &Layer, target: &Layer, uid: Uid, com (Node::Group(_), "transform") if !in_effect => parts.push("transform".into()), (Node::Group(_), "text") if !in_effect => parts.push("text".into()), (Node::Prop(_), "sourceText") => parts.push("sourceText".into()), - _ if in_effect => parts.push(format!("({})", quote(n.name()))), + _ if in_effect => parts.push(selector(i, n.name())), _ => parts.push(camel(n.name())), } } @@ -127,10 +155,9 @@ pub(crate) fn reference(comp: &Comp, from: &Layer, target: &Layer, uid: Uid, com let _ = comp; if !compact { // Match names: language-independent, like After Effects with Compact English off. - let chain = target.props.node_chain(uid)?; let mut out = if from.id == target.id { "thisLayer".to_string() } else { format!("thisComp.layer({})", quote(&target.name)) }; - for n in chain { - out.push_str(&format!("({})", quote(n.match_id()))); + for (i, n) in chain.iter().copied().enumerate() { + out.push_str(&selector(i, n.match_id())); } return Some(out); } @@ -161,7 +188,9 @@ fn pick_whip(s: &mut Session, p: &Value) -> Result { let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?; let from = comp.layer(lid).ok_or(EngineError::NoComp)?; let target = comp.layer(tl).ok_or(EngineError::NoComp)?; - let r = reference(comp, from, target, tu, s.prefs.general.expression_pick_whip_compact).ok_or_else(|| bad("prop.pickWhip", "no such target property"))?; + let by_index = b_p(p, "byIndex").unwrap_or(false); + let r = reference_with(comp, from, target, tu, s.prefs.general.expression_pick_whip_compact, by_index) + .ok_or_else(|| bad("prop.pickWhip", "no such target property"))?; let (me, them) = (from.props.find(uid).ok_or(EngineError::NoComp)?, target.props.find(tu).ok_or(EngineError::NoComp)?); let (dm, dt) = (dims(from, me), dims(target, them)); // Dropped on one of the target's values: that dimension (`position[0]`), as in After Effects. @@ -206,7 +235,7 @@ pub fn specs() -> Vec { "Pick Whip (Link Property)", [], None, - "{layer?, path|prop, target: {layer, path|prop, dimension?}, expression?, range?: [start, end]} → sets an AE reference expression (`dimension`: one of the target's values, `position[0]`; with `expression`: inserts it there, replacing the characters in `range`, default the end)", + "{layer?, path|prop, target: {layer, path|prop, dimension?}, expression?, range?: [start, end], byIndex?: bool} → sets an AE reference expression (`dimension`: one of the target's values, `position[0]`; with `expression`: inserts it there, replacing the characters in `range`, default the end; `byIndex`: an effect's properties by their index, `effect(\"Color Control\")(1)`, as an Alt pick whip does)", has_layers, pick_whip ), diff --git a/crates/engine/src/tests_timeline.rs b/crates/engine/src/tests_timeline.rs index 71a9943c0..949351b57 100644 --- a/crates/engine/src/tests_timeline.rs +++ b/crates/engine/src/tests_timeline.rs @@ -570,6 +570,59 @@ fn pick_whip_generates_ae_reference_expressions() { assert!(s.execute("prop.pickWhip", json!({"layer": l, "path": "transform/opacity", "target": {"layer": l, "path": "transform/opacity"}})).is_err()); } +/// #1072: an Alt pick whip (`byIndex`) selects an effect's properties by their index, +/// `effect("Color Control")(1)`, its place among its group's children (nested groups too). The +/// effect keeps its name; other properties, dimensions, insertion into an expression and match +/// names (Compact English off) work as before. (`effectcraft-host`'s `pick_whip` tests check the +/// references read the right values.) +#[test] +fn pick_whip_by_index_selects_effect_properties_by_their_index() { + let (mut s, l) = setup(); + let o = s.execute("layer.newSolid", json!({"name": "Other", "color": "#00ff00"})).unwrap()["layer"].as_u64().unwrap(); + for fx in ["Color Control", "Fractal Noise", "Point Control"] { + s.execute("effect.apply", json!({"layer": o, "effect": fx})).unwrap(); + } + // Fractal Noise ▸ Transform ▸ Scale, by its places in the property tree. + let fx = s.active_comp().unwrap().layer(effectcraft_project::LayerId(o)).unwrap().props.sub("effects").unwrap().groups().nth(1).unwrap().clone(); + let group = fx.children.iter().position(|c| c.match_id() == "transform").unwrap() + 1; + let scale = fx.sub("transform").unwrap().children.iter().position(|c| c.match_id() == "scale").unwrap() + 1; + let whip = |s: &mut Session, path: &str, target: Value, extra: Value| { + let mut p = json!({"layer": l, "path": path, "target": target, "byIndex": true}); + p.as_object_mut().unwrap().extend(extra.as_object().unwrap().clone()); + s.execute("prop.pickWhip", p).unwrap() + }; + let other = "thisComp.layer(\"Other\")"; + let undo = s.history.undo.len(); + assert_eq!( + whip(&mut s, "transform/rotation", json!({"layer": o, "path": "effects/#1/color"}), json!({})), + json!(format!("{other}.effect(\"Color Control\")(1)[0]")), + "a colour into one value: its first" + ); + assert_eq!(s.history.undo.len(), undo + 1, "one undo step"); + let nested = format!("{other}.effect(\"Fractal Noise\")({group})({scale})"); + assert_eq!(whip(&mut s, "transform/rotation", json!({"layer": o, "path": "effects/#2/transform/scale"}), json!({})), json!(nested)); + assert_eq!( + whip(&mut s, "transform/opacity", json!({"layer": o, "path": "effects/#3/point", "dimension": 1}), json!({})), + json!(format!("{other}.effect(\"Point Control\")(1)[1]")) + ); + assert_eq!( + whip(&mut s, "transform/rotation", json!({"layer": o, "path": "effects/#2/transform/scale"}), json!({"expression": "1 + ", "range": [4, 4]})), + json!(format!("1 + {nested}")), + "inserted at the cursor" + ); + assert_eq!( + whip(&mut s, "transform/opacity", json!({"layer": o, "path": "transform/opacity"}), json!({})), + json!(format!("{other}.transform.opacity")), + "only an effect's properties go by index" + ); + // Match names (Compact English off): the effect by its match name, its properties by index. + s.execute("prefs.set", json!({"key": "general.expressionPickWhipCompact", "value": false})).unwrap(); + let e = whip(&mut s, "transform/rotation", json!({"layer": o, "path": "effects/#2/transform/scale"}), json!({})); + assert!(e.as_str().unwrap().ends_with(&format!("({group})({scale})")), "{e}"); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(prop(&s, l, "transform/rotation").expr.unwrap().text, format!("1 + {nested}"), "one undo per pick"); +} + /// #407: Reset without a value gives a property its own default (a Transform property's, an /// effect parameter's) besides removing its keyframes and expression. #[test] diff --git a/crates/host/tests/pick_whip.rs b/crates/host/tests/pick_whip.rs new file mode 100644 index 000000000..340cda053 --- /dev/null +++ b/crates/host/tests/pick_whip.rs @@ -0,0 +1,38 @@ +//! The pick whip's references read the property they were made from (#1072): an Alt pick whip's +//! (`byIndex`) select an effect's properties by index, `effect("Fractal Noise")(i)(j)`, and must +//! read the same value as the reference by name. +//! +//! (With Compact English off, a reference to another layer, `thisComp.layer("Other")("effects")…`, +//! doesn't evaluate yet, by name or by index: "not a callable function".) + +use serde_json::{Value, json}; + +#[test] +fn index_references_read_the_same_values_as_names() { + let mut s = effectcraft_host::session(); + s.execute("comp.new", json!({"name": "T", "width": 64, "height": 64, "duration": 1})).unwrap(); + let l = s.execute("layer.newSolid", json!({"name": "Reader", "color": "#ffffff"})).unwrap()["layer"].as_u64().unwrap(); + let o = s.execute("layer.newSolid", json!({"name": "Other", "color": "#00ff00"})).unwrap()["layer"].as_u64().unwrap(); + for fx in ["Fractal Noise", "Point Control"] { + s.execute("effect.apply", json!({"layer": o, "effect": fx})).unwrap(); + } + s.execute("prop.set", json!({"layer": o, "path": "effects/#1/transform/scale", "value": 77})).unwrap(); + s.execute("prop.set", json!({"layer": o, "path": "effects/#2/point", "value": [12, 34]})).unwrap(); + let evaluated = |s: &mut effectcraft_engine::Session, path: &str| -> Value { + let v = s.execute("prop.get", json!({"layer": l, "path": path})).unwrap(); + assert!(v.get("expressionError").is_none(), "{v}"); + v["evaluated"].clone() + }; + for (path, target, dimension, want) in + [("transform/rotation", "effects/#1/transform/scale", None, json!(77.0)), ("transform/opacity", "effects/#2/point", Some(1), json!(34.0))] + { + let mut t = json!({"layer": o, "path": target}); + if let Some(d) = dimension { + t["dimension"] = json!(d); + } + for by_index in [false, true] { + let e = s.execute("prop.pickWhip", json!({"layer": l, "path": path, "target": t, "byIndex": by_index})).unwrap(); + assert_eq!(evaluated(&mut s, path), want, "by index {by_index}: {e}"); + } + } +} diff --git a/docs/agents.md b/docs/agents.md index 1bec71d51..d7d9142ee 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -604,7 +604,9 @@ Extension points and the bundled extensions: [plugins.md](plugins.md). Display Style between Timecode and Frames: `file.projectSettings {"timeDisplay": "frames"}`. * `prop.pickWhip {layer, prop, target: {layer, prop, dimension?}}` links a property as its expression pick whip does; `dimension` picks one of the target's values (`position[0]`), as a - drop on that value field does. + drop on that value field does. `byIndex: true` (an Alt pick whip: Alt held as it drops) refers + to an effect's properties by their index, `effect("Color Control")(1)`: it still reads a + renamed property, but another one if properties are reordered. * Every property row has a property pick whip in the Parent & Link column (`timeline.prop..propertyPickWhip`; the expression row's is `timeline.prop..pickWhip`). Dragged onto another property's name or value, in the Timeline or Effect Controls