From b5306608cc727887e4010f932e05f77faac6603a Mon Sep 17 00:00:00 2001 From: Fabio Lussi <198260595+itrycodinganditsbad@users.noreply.github.com> Date: Sun, 11 Oct 2026 18:47:08 +0200 Subject: [PATCH] fix(effects): keep a setParam keyframe outside the clip at its time effects.setParam clamped an explicit `time` into the clip, so a keyframe past the end replaced one sitting on the last frame and a ramp across a cut was compressed to finish at the clip end. The same happened before the start. An explicit time now keeps its keyframe where it was asked, outside the clip if need be, as effects.moveKeyframe already allows; only the playhead fallback stays pinned to the clip. Fixes #1055 Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands.rs | 8 ++++-- crates/engine/src/tests.rs | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index 6609afdd..d7c7102a 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -2600,7 +2600,7 @@ fn build() -> Vec { "Set Effect Parameter", [], None, - r##"{"clip":id,"effect":"motion"|index,"param":str,"mask":n?,"value":num|[x,y]|"#rrggbb"|bool|path|"option name" (a choice takes its index or its name),"time":ticks? (where the keyframe goes when the parameter is animated; a parameter that is not animated gets a new static value whatever the time),"keyframe":bool? (true: write a keyframe at `time` and animate the parameter),"merge":bool?,"begin":bool?}"##, + r##"{"clip":id,"effect":"motion"|index,"param":str,"mask":n?,"value":num|[x,y]|"#rrggbb"|bool|path|"option name" (a choice takes its index or its name),"time":ticks? (where the keyframe goes when the parameter is animated, kept there even outside the clip; default: the playhead, pinned to the clip; a parameter that is not animated gets a new static value whatever the time),"keyframe":bool? (true: write a keyframe at `time` and animate the parameter),"merge":bool?,"begin":bool?}"##, has_seq, |s, p| { let c = clip_p(p, "clip").ok_or_else(|| bad("effects.setParam", "need `clip`"))?; @@ -2608,8 +2608,10 @@ fn build() -> Vec { let val = p.get("value").cloned().ok_or_else(|| bad("effects.setParam", "need `value`"))?; let eff = p.get("effect").cloned().unwrap_or(json!("motion")); let keyframe = bool_p(p, "keyframe").unwrap_or(false); + // an explicit `time` outside the clip keeps its keyframe there, before the start as after + // the end, so a ramp across a cut keeps its slope (#1055); the playhead is pinned to the clip + let at = time_p(s, p, ""); let ph = s.playhead(); - let tl = time_p(s, p, "").unwrap_or(ph); let pq = p.clone(); // `merge`: a drag of one parameter is one undo step (#201); `begin` starts a new one let merge = bool_p(p, "merge").unwrap_or(false).then(|| format!("setParam:{}:{eff}:{pid}:{}", c.0, p.get("mask").unwrap_or(&Value::Null))); @@ -2618,7 +2620,7 @@ fn build() -> Vec { } let keyframes = s.edit_sequence_as("Change Effect Parameter", merge.as_deref(), |q, _, _| { let (_, it) = q.find_item_mut(c).ok_or(filmcraft_edit::EditError::NoItem(c))?; - let mt = it.source_time_at(tl.clamp(it.start, (it.end() - Tick(1)).max(it.start))); + let mt = it.source_time_at(at.unwrap_or_else(|| ph.clamp(it.start, (it.end() - Tick(1)).max(it.start)))); let e = match &eff { Value::Number(n) => it.effects.get_mut(n.as_u64().unwrap_or(0) as usize), Value::String(sid) => it.effects.iter_mut().find(|e| &e.effect == sid), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 9852734d..5366a1dd 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -381,6 +381,59 @@ fn set_param_writes_a_keyframe_when_asked_or_animated() { assert_eq!(s.execute("effects.setParam", odd).unwrap()["keyframes"], 2); } +/// A keyframe set with a `time` outside the clip stays at that time (#1055), before the start as +/// after the end: a ramp that spans a cut shows its true slice, and a clip-end keyframe is not +/// overwritten by a later one. Only the playhead fallback is pinned to the clip. +#[test] +fn set_param_keeps_a_keyframe_outside_the_clip_at_its_time() { + let mut s = demo(); + let it = s.active_sequence().unwrap().video_tracks[0].items[1].clone(); + let (c, start, end) = (it.id.0, it.start, it.end()); + let opacity = |s: &Session| { + let it = s.active_sequence().unwrap().find_item(filmcraft_project::ClipId(c)).unwrap().1; + let p = &it.effect("opacity").unwrap().params["opacity"]; + (p.keyframes.iter().map(|k| (k.time, k.value.as_f64().unwrap())).collect::>(), it.clone()) + }; + let set = |value: f64, time: Tick| json!({"clip": c, "effect": "opacity", "param": "opacity", "value": value, "time": time.0, "keyframe": true}); + let len = end - start; + // Case A: a keyframe exactly at the clip end, then one past it, gives three keyframes + s.execute("effects.setParam", set(100.0, start)).unwrap(); + s.execute("effects.setParam", set(100.0, end)).unwrap(); + let r = s.execute("effects.setParam", set(20.0, end + len)).unwrap(); + assert_eq!(r["keyframes"], 3, "{r}"); + let (keys, it) = opacity(&s); + assert_eq!(keys, vec![(it.source_time_at(start), 100.0), (it.source_time_at(end), 100.0), (it.source_time_at(end + len), 20.0)]); + // Case B: a ramp past the end is not compressed; halfway through the clip is a quarter of the way down + s.undo(); + s.undo(); + let r = s.execute("effects.setParam", set(0.0, end + len)).unwrap(); + assert_eq!(r["keyframes"], 2, "{r}"); + let (_, it) = opacity(&s); + let p = it.effect("opacity").unwrap().params["opacity"].clone(); + let mid = start + Tick(len.0 / 2); + assert!((p.f64_at(it.source_time_at(mid)) - 75.0).abs() < 0.5, "{}", p.f64_at(it.source_time_at(mid))); + // before the start the same holds: the keyframe keeps its time and the ramp its slope + s.undo(); + s.undo(); + s.execute("effects.setParam", set(0.0, start - len)).unwrap(); + s.execute("effects.setParam", set(100.0, end)).unwrap(); + let (keys, it) = opacity(&s); + assert_eq!(keys, vec![(it.source_time_at(start - len), 0.0), (it.source_time_at(end), 100.0)]); + let p = it.effect("opacity").unwrap().params["opacity"].clone(); + assert!((p.f64_at(it.source_time_at(start)) - 50.0).abs() < 0.5, "{}", p.f64_at(it.source_time_at(start))); + // hostile times saturate instead of overflowing + for t in [i64::MIN, i64::MAX] { + s.execute("effects.setParam", json!({"clip": c, "effect": "opacity", "param": "opacity", "value": 50.0, "time": t})).unwrap(); + } + // with no `time`, a playhead past the clip still lands on its last frame + s.undo(); + s.undo(); + s.execute("playhead.set", json!({"time": (end + len).0})).unwrap(); + s.execute("effects.setParam", json!({"clip": c, "effect": "opacity", "param": "opacity", "value": 40.0})).unwrap(); + let (keys, it) = opacity(&s); + assert!(keys.contains(&(it.source_time_at(end - Tick(1)), 40.0)), "{keys:?}"); +} + /// A choice parameter takes its option name as well as its index, as transition parameters do /// (`effects.list {"detail": true}` lists the names). #[test]