collective
Browse / github.com/storytold/filmcraft / patch
engine: effects.addKeyframe keeps an explicit time at the out point and says what it did (#3386)
`effects.addKeyframe` at the clip's out point creates a keyframe that `effects.deleteKeyframe` reports removed but keeps Measured on FilmCraft 0.12.0 (Windows portable) through the MCP server. On an image clip with a scale keyframe at 0 s, `effects.addKeyframe` with `time` equal to the clip's out point creates a keyframe that `effects.setKeyframe` does not find; `effects.deleteKeyframe` on it returns `null` and the keyframe count does not decrease. A second `effects.addKeyframe` at the same time removes it. In a longer edit the stray key showed as a jump in the last fraction of a second of the render; workaround: keep the last keyframe one frame before the out point. Suggestion: clamp or reject a keyframe tim
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- x-labb (imported from pull request #3409, open)
- Base
8fda2f232198c4c79ff9cfe5b6a347bb1375fa9e- Size
- +102 −7 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
b0169ef38a5257ff95bba628
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/commands.rs crates/engine/src/tests.rs docs/agents.md
@@ -3650,11 +3650,17 @@ fn reset_params(e: &mut filmcraft_project::EffectInstance, only: Option<&str>, f any } +/// `effects.addKeyframe` / `deleteKeyframe` / `moveKeyframe` / `setKeyframe` / `setKeyframeInterpolation`.+/// An explicit `time` keeps its place, outside the clip too (#1055, as in `effects.setParam`); only+/// the playhead fallback is pinned to the clip's last tick. `add` replies `{added, mediaTime}`:+/// whether it added the keyframe or (it is a toggle) removed the one there, and the media time of+/// that keyframe. `delete` replies `{removed}`: whether a keyframe sat exactly at `mediaTime`. fn keyframe_op(s: &mut Session, p: &Value, op: &str) -> Result<Value> { let c = clip_p(p, "clip").ok_or_else(|| bad("keyframe", "need `clip`"))?; let pid = str_p(p, "param").ok_or_else(|| bad("keyframe", "need `param`"))?.to_string(); let eff = p.get("effect").cloned().unwrap_or(json!("motion"));- let ph = time_p(s, p, "").unwrap_or(s.playhead());+ let at = time_p(s, p, "");+ let playhead = s.playhead(); let mtime = p.get("mediaTime").and_then(Value::as_i64).map(Tick); let to = p.get("to").and_then(Value::as_i64).map(Tick); let interp = str_p(p, "interpolation").map(str::to_string);@@ -3671,8 +3677,9 @@ fn keyframe_op(s: &mut Session, p: &Value, op: &str) -> Result<Value> { s.history.merge_key = None; } s.edit_sequence_as(label, merge.as_deref(), |q, _, _| {+ let mut reply = Value::Null; let (_, it) = q.find_item_mut(c).ok_or(filmcraft_edit::EditError::NoItem(c))?;- let mt_now = it.source_time_at(ph.clamp(it.start, (it.end() - Tick(1)).max(it.start)));+ let mt_now = it.source_time_at(at.unwrap_or_else(|| playhead.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),@@ -3683,12 +3690,15 @@ fn keyframe_op(s: &mut Session, p: &Value, op: &str) -> Result<Value> { match op { "add" => { // toggle: remove if a keyframe sits at the playhead, else add- if !prm.remove_keyframe_at(mt_now) {+ let added = !prm.remove_keyframe_at(mt_now);+ if added { prm.add_keyframe(mt_now); }+ reply = json!({"added": added, "mediaTime": mt_now.0}); } "delete" => {- prm.remove_keyframe_at(mtime.ok_or_else(|| bad("keyframe", "need `mediaTime`"))?);+ let removed = prm.remove_keyframe_at(mtime.ok_or_else(|| bad("keyframe", "need `mediaTime`"))?);+ reply = json!({"removed": removed}); } "move" => { let from = mtime.ok_or_else(|| bad("keyframe", "need `mediaTime`"))?;@@ -3732,9 +3742,8 @@ fn keyframe_op(s: &mut Session, p: &Value, op: &str) -> Result<Value> { k.interp = kind; } }- Ok(())- })?;- Ok(Value::Null)+ Ok(reply)+ }) } fn in_out_range(s: &Session) -> Result<TimeRange> {@@ -326,6 +326,86 @@ fn add_keyframe_toggles_the_keyframe_at_the_playhead() { assert_eq!(scale(&s), (0, 100.0)); } +/// #3386: `effects.addKeyframe` with a `time` at the clip's out point used to put the keyframe a+/// tick earlier, so a script that then looked for it at the out point found nothing (and+/// `effects.deleteKeyframe` answered `null` either way). An explicit `time` now keeps its place, as+/// in `effects.setParam` (#1055); only the playhead fallback is pinned to the clip. The replies say+/// where the keyframe is (`mediaTime`), whether it was added or toggled off (`added`), and whether a+/// delete removed anything (`removed`).+#[test]+fn add_keyframe_at_the_out_point_stays_there_and_the_replies_say_what_happened() {+ let mut s = demo();+ let (c, start, end) = {+ let it = &s.active_sequence().unwrap().video_tracks[0].items[0];+ (it.id.0, it.start, it.end())+ };+ let clip = |s: &Session| s.active_sequence().unwrap().find_item(filmcraft_project::ClipId(c)).unwrap().1.clone();+ let media_at = |s: &Session, t: Tick| clip(s).source_time_at(t);+ let keys = |s: &Session| clip(s).effect("motion").unwrap().params["scale"].keyframes.iter().map(|k| k.time).collect::<Vec<_>>();+ let add = |s: &mut Session, time: Tick| s.execute("effects.addKeyframe", json!({"clip": c, "effect": "motion", "param": "scale", "time": time.0})).unwrap();+ let delete = |s: &mut Session, media: Tick| {+ s.execute("effects.deleteKeyframe", json!({"clip": c, "effect": "motion", "param": "scale", "mediaTime": media.0})).unwrap()+ };+ let reply = |r: &Value| (r["added"].clone(), r["mediaTime"].clone());++ let r = add(&mut s, start);+ assert_eq!(reply(&r), (json!(true), json!(media_at(&s, start).0)), "{r}");++ // at the out point: the keyframe is at the out point, where a script looks for it+ let out = media_at(&s, end);+ let r = add(&mut s, end);+ assert_eq!(reply(&r), (json!(true), json!(out.0)), "{r}");+ assert_eq!(keys(&s), vec![media_at(&s, start), out]);++ // a delete at a time with no keyframe says so and changes nothing+ let r = delete(&mut s, out - Tick(1));+ assert_eq!(r["removed"], json!(false), "{r}");+ assert_eq!(keys(&s).len(), 2);+ let r = delete(&mut s, out);+ assert_eq!(r["removed"], json!(true), "{r}");+ assert_eq!(keys(&s), vec![media_at(&s, start)]);++ // adding twice at the same time toggles: the second reply says the keyframe was removed+ assert_eq!(add(&mut s, end)["added"], json!(true));+ let r = add(&mut s, end);+ assert_eq!(reply(&r), (json!(false), json!(out.0)), "{r}");+ assert_eq!(keys(&s), vec![media_at(&s, start)]);++ // past the end it keeps its time too+ let past = end + (end - start);+ let r = add(&mut s, past);+ assert_eq!(reply(&r), (json!(true), json!(media_at(&s, past).0)), "{r}");+ let past_media = media_at(&s, past);+ delete(&mut s, past_media);++ // only the playhead fallback is pinned to the clip: a playhead past the end lands on its last tick+ s.execute("playhead.set", json!({"seconds": past.seconds()})).unwrap();+ let r = s.execute("effects.addKeyframe", json!({"clip": c, "effect": "motion", "param": "scale"})).unwrap();+ assert_eq!(reply(&r), (json!(true), json!(media_at(&s, end - Tick(1)).0)), "{r}");++ // hostile times neither panic nor leave a half-made keyframe; a delete that names no time is an error+ for t in [i64::MIN, i64::MAX] {+ let _ = s.execute("effects.addKeyframe", json!({"clip": c, "effect": "motion", "param": "scale", "time": t}));+ }+ s.active_sequence().unwrap().check().unwrap();+ assert!(s.execute("effects.deleteKeyframe", json!({"clip": c, "effect": "motion", "param": "scale"})).is_err());+}++/// The position half of #3386, read from the project and not from a reply: a keyframe asked for at+/// the clip's out point is at the out point.+#[test]+fn add_keyframe_with_a_time_at_the_out_point_is_not_moved() {+ let mut s = demo();+ let (c, end) = {+ let it = &s.active_sequence().unwrap().video_tracks[0].items[0];+ (it.id.0, it.end())+ };+ s.execute("effects.addKeyframe", json!({"clip": c, "effect": "motion", "param": "scale", "time": end.0})).unwrap();+ let it = s.active_sequence().unwrap().find_item(filmcraft_project::ClipId(c)).unwrap().1.clone();+ let at: Vec<Tick> = it.effect("motion").unwrap().params["scale"].keyframes.iter().map(|k| k.time).collect();+ assert_eq!(at, vec![it.source_time_at(end)], "the keyframe sits at the out point, not a tick before it");+}+ /// `effects.setParam` writes a keyframe only on an animated parameter. An agent that sends a value /// with a `time` to a parameter that is not animated gets a static value (the result says so: /// 0 keyframes); `"keyframe": true` is the stopwatch and the value in one step.@@ -48,6 +48,12 @@ edit reports the value written at the requested time, rather than the static fal For example, setting opacity `blend` to `"Screen"` returns `{"keyframes": 0, "param": "blend", "value": 8}`. +`effects.addKeyframe` toggles: it returns `{"added": true, "mediaTime": t}` when it added a keyframe and+`{"added": false, "mediaTime": t}` when it removed the one already there; `mediaTime` is where the keyframe+is, to find it again. An explicit `time` keeps its place, outside the clip too, as in `effects.setParam`;+only the playhead (when no `time` is given) is held to the clip's last tick. `effects.deleteKeyframe`+returns `{"removed": true}` or `{"removed": false}` (no keyframe sat exactly at `mediaTime`).+ ### From an installed release The release packages ship `filmcraft-cli` alongside the desktop app, so no build is needed: