shifathosn
/
filmcraft
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #1120 from itrycodinganditsbad/fix-1055 

fix(effects): keep a setParam keyframe outside the clip at its time (#1055)
This commit is contained in:
Brandon Thomas 2026-10-11 18:21:40 -04:00
parent 218641ad01 b5306608cc
commit 45201e4d80
2 changed files with 58 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
crates/engine/src/commands.rs
View File

@ -2600,7 +2600,7 @@ fn build() -> Vec<CommandSpec> {
"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<CommandSpec> {
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<CommandSpec> {
}
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),

53
crates/engine/src/tests.rs
View File

@ -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::<Vec<_>>(), 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]