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

Essential Graphics: Make a property drag one undo step

This commit is contained in:
gyro 2026-10-09 21:32:42 -07:00
parent 7bd7621262
commit 81268330e9
6 changed files with 143 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
crates/engine/src/graphic_templates.rs
View File

@ -291,7 +291,7 @@ fn set_controls(s: &mut Session, clip: ClipId, values: &serde_json::Map<String,
props.insert(c.param.clone(), v.clone());
}
}
set_props(s, clip, ei, &props, tl, "Change Template Property")?;
set_props(s, clip, ei, &props, tl, "Change Template Property", None)?;
n += 1;
}
Ok(n)

22
crates/engine/src/graphics.rs
View File

@ -14,7 +14,7 @@ use filmcraft_render::graphic_clip::{item_layer_specs, layer_local_bounds, layer
use filmcraft_time::{Tick, TimeRange};
use serde_json::{Value, json};
use crate::commands::{CommandSpec, always, bad, f64_p, has_seq, str_p, time_p, u64_p};
use crate::commands::{CommandSpec, always, bad, bool_p, f64_p, has_seq, str_p, time_p, u64_p};
use crate::{EngineError, Result, Session};
type Run = fn(&mut Session, &Value) -> Result<Value>;
@ -255,10 +255,11 @@ fn apply_props(e: &mut EffectInstance, props: &serde_json::Map<String, Value>, m
}
/// Set properties `props` on a layer (keyframe-aware at time `tl`). Changing the text keeps
/// per-character styles on their characters.
pub(crate) fn set_props(s: &mut Session, clip: ClipId, eidx: usize, props: &serde_json::Map<String, Value>, tl: Tick, label: &str) -> Result<()> {
/// per-character styles on their characters. `merge` folds consecutive calls with the same key
/// into one undo step (a drag is one undoable change, like `effects.setParam`).
pub(crate) fn set_props(s: &mut Session, clip: ClipId, eidx: usize, props: &serde_json::Map<String, Value>, tl: Tick, label: &str, merge: Option<&str>) -> Result<()> {
let props = props.clone();
s.edit_sequence(label, |q, _, _| {
s.edit_sequence_as(label, merge, |q, _, _| {
let (_, it) = q.find_item_mut(clip).ok_or(filmcraft_edit::EditError::NoItem(clip))?;
let mt = it.source_time_at(tl.clamp(it.start, it.end() - Tick(1)));
let e = it.effects.get_mut(eidx).ok_or_else(|| bad("graphics.set", "no such layer"))?;
@ -737,7 +738,7 @@ fn menu_commands() -> Vec<CommandSpec> {
props.insert("sides".into(), json!(n));
let ph = s.playhead();
let before = s.history.undo.len();
set_props(s, clip, ei, &props, ph, "Change Graphic Property")?;
set_props(s, clip, ei, &props, ph, "Change Graphic Property", None)?;
if s.history.undo.len() > before {
s.history.undo.pop();
}
@ -956,7 +957,7 @@ pub fn commands() -> Vec<CommandSpec> {
let mut props = serde_json::Map::new();
props.insert("text".into(), Value::String(text));
let ph = s.playhead();
set_props(s, clip, ei, &props, ph, "Edit Text")?;
set_props(s, clip, ei, &props, ph, "Edit Text", None)?;
if merge && s.history.undo.len() >= 2 {
// keep the snapshot from before the typing session
s.history.undo.pop();
@ -969,14 +970,19 @@ pub fn commands() -> Vec<CommandSpec> {
"Set Graphic Properties",
&[],
None,
r##"{"clip":id?,"layer":n|name?,"props":{"font":"Inter","font_style":"Bold","size":120,"align":"center","tracking":50,"leading":0,"fill_color":"#ffcc00","stroke":true,"stroke_width":6,"background":true,"shadow":true,"position":[x,y],"scale":100,"rotation":0,"opacity":100,…},"time":ticks?}"##,
r##"{"clip":id?,"layer":n|name?,"props":{"font":"Inter","font_style":"Bold","size":120,"align":"center","tracking":50,"leading":0,"fill_color":"#ffcc00","stroke":true,"stroke_width":6,"background":true,"shadow":true,"position":[x,y],"scale":100,"rotation":0,"opacity":100,…},"time":ticks?,"merge":bool?,"begin":bool?}"##,
has_graphic,
|s, p| {
let clip = target_clip(s, p).ok_or_else(|| bad("graphics.set", "no graphic clip"))?;
let (_, ei) = layer_effect_index(s, clip, p)?;
let props = p.get("props").and_then(Value::as_object).ok_or_else(|| bad("graphics.set", "need `props`"))?.clone();
let tl = time_p(s, p, "").unwrap_or_else(|| s.playhead());
set_props(s, clip, ei, &props, tl, "Change Graphic Property")?;
// `merge`: a drag of a property is one undo step; `begin` starts a new one (#201)
let merge = bool_p(p, "merge").unwrap_or(false).then(|| format!("graphics.set:{}:{ei}", clip.0));
if bool_p(p, "begin").unwrap_or(false) {
s.history.merge_key = None;
}
set_props(s, clip, ei, &props, tl, "Change Graphic Property", merge.as_deref())?;
Ok(Value::Null)
},
),

32
crates/engine/src/graphics_tests.rs
View File

@ -123,6 +123,38 @@ fn typing_coalesces_into_one_undo_step_and_props_set() {
assert_eq!(text_of(&layers(&s, clip)[0]), "");
}
/// A drag of a property is one undo step: `graphics.set` with `merge` folds consecutive calls
/// into the step the first change of the press began (`begin`), so one undo restores the
/// original value (the color picker in Essential Graphics was committing one step per frame).
#[test]
fn a_property_drag_is_one_undo_step() {
let mut s = demo();
let r = s.execute("graphics.newText", json!({"text": "Title"})).unwrap();
let clip = ClipId(r["clip"].as_u64().unwrap());
s.execute("graphics.set", json!({"clip": clip.0, "layer": 0, "props": {"shadow_color": [0.0, 0.0, 0.0, 1.0]}})).unwrap();
let color = |s: &Session| -> [f32; 4] {
let q = s.active_sequence().unwrap();
let (_, it) = q.find_item(clip).unwrap();
let e = it.effects.iter().find(|e| e.effect == "graphic_text").unwrap();
e.param("shadow_color").unwrap().value_at(Tick::ZERO).as_color().unwrap()
};
let before = s.history.undo.len();
// the drag: the first change of the press begins the step, the rest fold into it
for (i, c) in [[0.1, 0.0, 0.0, 1.0], [0.2, 0.0, 0.0, 1.0], [0.3, 0.0, 0.0, 1.0], [0.4, 0.0, 0.0, 1.0]].iter().enumerate() {
s.execute("graphics.set", json!({"clip": clip.0, "layer": 0, "props": {"shadow_color": c}, "merge": true, "begin": i == 0})).unwrap();
}
assert_eq!(s.history.undo.len(), before + 1, "one undo step for the whole drag");
assert_eq!(color(&s)[0], 0.4);
// a second drag (a new press) begins its own step
s.execute("graphics.set", json!({"clip": clip.0, "layer": 0, "props": {"shadow_color": [1.0, 0.0, 0.0, 1.0]}, "merge": true, "begin": true})).unwrap();
s.execute("graphics.set", json!({"clip": clip.0, "layer": 0, "props": {"shadow_color": [0.0, 1.0, 0.0, 1.0]}, "merge": true})).unwrap();
assert_eq!(s.history.undo.len(), before + 2, "a new press begins a new step");
s.execute("edit.undo", json!({})).unwrap();
assert_eq!(color(&s)[0], 0.4, "undo takes back only the second drag");
s.execute("edit.undo", json!({})).unwrap();
assert_eq!(color(&s)[0], 0.0, "and then the first");
}
#[test]
fn align_and_distribute_layers() {
let mut s = demo();

12
crates/ui-egui/src/panels/graphics.rs
View File

@ -1575,7 +1575,17 @@ pub fn properties(app: &mut FilmcraftApp, ui: &mut egui::Ui, rect: Rect) {
for (id, r, label) in autos {
app.auto.add(&id, r, &label);
}
for (cmd, p) in actions {
for (cmd, mut p) in actions {
// a drag of a property is one undo step: the first change of each press begins a new one,
// the rest fold into it (like Effect Controls, #201)
if cmd == "graphics.set" && ui.ctx().input(|i| i.pointer.any_down()) {
let key = egui::Id::new("gfx-props-drag-step");
let press = ui.ctx().input(|i| i.pointer.press_start_time());
let begun = ui.ctx().data(|d| d.get_temp::<Option<f64>>(key)).flatten();
p["merge"] = json!(true);
p["begin"] = json!(begun != press);
ui.ctx().data_mut(|d| d.insert_temp(key, press));
}
if let Err(e) = app.session.execute(&cmd, p) {
app.ui.status = e.to_string();
}

84
crates/ui-egui/tests/graphics_ui.rs
View File

@ -152,3 +152,87 @@ fn type_tool_edit_style_and_move() {
d.ok("ui.set", json!({"tool": "Selection"}));
d.shot("shape-layer");
}
/// A drag of a property in the Properties panel is one undo step, so one Cmd+Z restores the
/// original value (a drag was committing one undo step per frame, so undo only stepped back one
/// frame's worth; the shadow color picker showed it as "sometimes undo works, sometimes not").
#[test]
fn a_property_drag_is_one_undo_step() {
let mut d = Driver::demo();
d.ok("ui.set", json!({"workspace": "Captions and Graphics"}));
let r = d.exec("graphics.newText", json!({"text": "Title"}));
let clip = r["clip"].as_u64().unwrap();
d.exec("graphics.selectLayer", json!({"clip": clip, "layers": [0]}));
d.frames(4);
// the layer's Opacity number (visible in the Align and Transform section)
d.exec("graphics.set", json!({"clip": clip, "layer": 0, "props": {"opacity": 50}}));
d.frames(2);
let opacity = |d: &mut Driver| -> f64 {
let seq = d.exec("sequence.inspect", json!({}));
let item = seq["video"].as_array().unwrap().iter().flat_map(|t| t["items"].as_array().unwrap().iter()).find(|i| i["clip"].as_u64() == Some(clip)).unwrap();
let e = item["effects"].as_array().unwrap().iter().find(|e| e["effect"] == "graphic_text").unwrap();
let v = e["params"]["opacity"]["value"].as_str().unwrap();
v.trim_start_matches("Float(").trim_end_matches(')').parse::<f64>().unwrap()
};
let before = opacity(&mut d);
let undo0 = d.exec("history.list", json!({}))["undo"].as_array().unwrap().len();
// drag the Opacity number to the left: several move frames, one undo step
let n = d.rect("graphics.prop.opacity");
let (cx, cy) = (n[0] + n[2] / 2.0, n[1] + n[3] / 2.0);
d.ok("ui.drag", json!({"from": {"x": cx, "y": cy}, "to": {"x": cx - 60.0, "y": cy}, "steps": 6}));
d.frames(4);
let after = opacity(&mut d);
assert!(after < before - 5.0, "the drag changed the opacity: {before} -> {after}");
let undo1 = d.exec("history.list", json!({}))["undo"].as_array().unwrap().len();
assert_eq!(undo1, undo0 + 1, "a property drag is one undo step");
d.exec("edit.undo", json!({}));
assert_eq!(opacity(&mut d), before, "one undo restores the original value");
}
/// The shadow color picker popup: a drag in the 2D picker is one undo step, so one Cmd+Z
/// restores the original color (a drag was committing one undo step per frame, so undo only
/// stepped back one frame's worth — the reported "sometimes undo works, sometimes not").
#[test]
fn shadow_color_drag_is_one_undo_step() {
let mut d = Driver::demo();
d.ok("ui.set", json!({"workspace": "Captions and Graphics"}));
let r = d.exec("graphics.newText", json!({"text": "Title"}));
let clip = r["clip"].as_u64().unwrap();
d.exec("graphics.selectLayer", json!({"clip": clip, "layers": [0]}));
d.frames(4);
let color = |d: &mut Driver| -> String {
let seq = d.exec("sequence.inspect", json!({}));
let item = seq["video"].as_array().unwrap().iter().flat_map(|t| t["items"].as_array().unwrap().iter()).find(|i| i["clip"].as_u64() == Some(clip)).unwrap();
let e = item["effects"].as_array().unwrap().iter().find(|e| e["effect"] == "graphic_text").unwrap();
e["params"].get("shadow_color").and_then(|p| p["value"].as_str()).unwrap_or("Color([0.0, 0.0, 0.0, 1.0])").to_string()
};
// scroll the properties panel so the shadow row is visible, then open the picker
d.ok("ui.scroll", json!({"x": 1400.0, "y": 500.0, "dx": 0.0, "dy": -400.0}));
d.frames(3);
let sw = d.rect("graphics.prop.shadow_color");
d.ok("ui.click", json!({"x": sw[0] + sw[2] / 2.0, "y": sw[1] + sw[3] / 2.0}));
d.frames(3);
// the popup flips above the swatch (it does not fit below); find a point in its 2D picker
let undo0 = d.exec("history.list", json!({}))["undo"].as_array().unwrap().len();
let mut hit = None;
for (dx, dy) in [(30.0, 30.0), (150.0, 30.0), (30.0, 150.0), (150.0, 150.0)] {
let (px, py) = (sw[0] + dx, sw[1] - dy);
let before = color(&mut d);
d.ok("ui.drag", json!({"from": {"x": px, "y": py}, "to": {"x": px + 60.0, "y": py + 40.0}, "steps": 4}));
d.frames(3);
let after = color(&mut d);
if after != before {
hit = Some((before, after));
break;
}
}
let (before, after) = hit.expect("no drag point hit the color picker");
assert_ne!(after, before, "the drag changed the shadow color");
let undo1 = d.exec("history.list", json!({}))["undo"].as_array().unwrap().len();
assert_eq!(undo1, undo0 + 1, "a color drag is one undo step");
d.exec("edit.undo", json!({}));
assert_eq!(color(&mut d), before, "one undo restores the original color");
}

2
docs/graphics.md
View File

@ -89,7 +89,7 @@ Timecode and Clip Name effects also draw with the text engine.
| `graphics.newShape` | (agents) | `shape` (rectangle/ellipse/polygon/path), `position`, `size`, `points`, `clip` |
| `graphics.setTextType` | Text Properties ▸ Text Layer Type | `clip`, `layer`, `type` (`point` / `paragraph`); the text stays where it is |
| `graphics.setText` | typing on the monitor | `clip`, `layer`, `text`, `merge` (coalesce one typing session into one undo step) |
| `graphics.set` | Properties panel | `clip`, `layer`, `props` {parameter id or camelCase alias: value; choices by index or name}, `time` |
| `graphics.set` | Properties panel | `clip`, `layer`, `props` {parameter id or camelCase alias: value; choices by index or name}, `time`, `merge` + `begin` (a drag is one undo step: send `merge: true` on every change and `begin: true` on the first change of each press) |
| `graphics.selectLayer` | layer list / monitor click | `clip`, `layers` |
| `graphics.deleteLayer`, `graphics.arrangeLayer` | layer list | `clip`, `layer`, `to` (front/back/forward/backward/index) |
| `graphics.align` | Align and Transform | `align` (left/hcenter/right/top/vcenter/bottom), `to` (frame/group/selection), `layers` |