From f20dd38f94454deaae0b7921c9822932c4a21d3e Mon Sep 17 00:00:00 2001 From: Bar Borer Date: Mon, 2 Nov 2026 20:39:17 +0200 Subject: [PATCH 1/3] Puppet: a click on deformed-away space creates nothing (#2865) Co-Authored-By: Claude Opus 5.5 --- crates/effects/src/puppet.rs | 21 +++++++++ crates/effects/src/puppet/mesh.rs | 26 +++++++++++- crates/effects/src/puppet/tests.rs | 37 ++++++++++++++++ crates/engine/src/cmd/puppet.rs | 38 +++++++++++------ crates/engine/src/tests/paint.rs | 54 ++++++++++++++++++++++++ crates/ui-egui/src/panels/puppet_tool.rs | 12 +++++- crates/ui-egui/src/panels/viewer.rs | 8 +++- crates/ui-egui/tests/it/ui_viewer.rs | 36 ++++++++++++++++ 8 files changed, 215 insertions(+), 17 deletions(-) diff --git a/crates/effects/src/puppet.rs b/crates/effects/src/puppet.rs index 6ec2ce5ba..4db76c1f5 100644 --- a/crates/effects/src/puppet.rs +++ b/crates/effects/src/puppet.rs @@ -395,6 +395,27 @@ pub fn contains(mesh: &Mesh, deformed: &[[f64; 2]], p: [f64; 2]) -> bool { }) } +/// Whether `at` (effect space) is art a new mesh with this `expansion` could take: on the expanded +/// alpha itself, and not what a mesh of `params` already covers at rest, even if its deformation +/// has carried that art away. A mesh there would only rebuild the same outline: a full copy (#2865). +pub fn unmeshed_art_at(buf: &Buf, params: &Params, at: [f64; 2], expansion: f64) -> bool { + let (w, h) = (buf.img.width as usize, buf.img.height as usize); + let alpha: Vec = buf.img.data.iter().map(|p| p[3]).collect(); + let mut outline = mesh::outline_under(&alpha, w, h, buf.scale.max(1e-9), buf.offset, at, expansion); + if outline.is_empty() { + return false; + } + // As `triangulate` winds it. The simplified outline can cut a corner past `at`, so a mesh can + // miss the very art it was built from: the same outline is the same art. + if mesh::signed_area(&outline) < 0.0 { + outline.reverse(); + } + !parse(params).iter().any(|m| { + let mesh = mesh_for(buf, m); + mesh.outline == outline || contains(&mesh, &mesh.verts, at) + }) +} + #[inline] fn bary(a: [f64; 2], b: [f64; 2], c: [f64; 2], p: [f64; 2]) -> Option<[f64; 3]> { let d = (b[1] - c[1]) * (a[0] - c[0]) + (c[0] - b[0]) * (a[1] - c[1]); diff --git a/crates/effects/src/puppet/mesh.rs b/crates/effects/src/puppet/mesh.rs index f01ee8f78..1046dfa6e 100644 --- a/crates/effects/src/puppet/mesh.rs +++ b/crates/effects/src/puppet/mesh.rs @@ -194,6 +194,11 @@ pub fn region(g: &Grid, seed: [f64; 2]) -> Option { Some(Grid { w, h, on: outside.iter().map(|o| !o).collect() }) } +/// The region under `seed` only when `seed` is on a cell: `region` without its nearest fallback. +pub fn region_under(g: &Grid, seed: [f64; 2]) -> Option { + if g.at(seed[0].floor() as i64, seed[1].floor() as i64) { region(g, seed) } else { None } +} + /// Trace the outer boundary of a hole-free region along pixel cracks: a closed polygon through /// pixel corners, region on the left when walking (counter-clockwise in y-down coordinates). pub fn trace(g: &Grid) -> Vec<[f64; 2]> { @@ -617,6 +622,25 @@ pub fn triangulate(outline: &[[f64; 2]], opts: &MeshOpts) -> Mesh { /// The outline of the alpha region under `seed` (buffer pixels → layer space via `scale` and /// `offset`), expanded by `expansion` layer pixels. pub fn outline(alpha: &[f32], w: usize, h: usize, scale: f64, offset: [f64; 2], seed: [f64; 2], expansion: f64) -> Vec<[f64; 2]> { + outline_of(alpha, w, h, scale, offset, seed, expansion, region) +} + +/// As `outline`, but empty unless `seed` is on the expanded alpha itself. +pub fn outline_under(alpha: &[f32], w: usize, h: usize, scale: f64, offset: [f64; 2], seed: [f64; 2], expansion: f64) -> Vec<[f64; 2]> { + outline_of(alpha, w, h, scale, offset, seed, expansion, region_under) +} + +#[allow(clippy::too_many_arguments)] +fn outline_of( + alpha: &[f32], + w: usize, + h: usize, + scale: f64, + offset: [f64; 2], + seed: [f64; 2], + expansion: f64, + pick: fn(&Grid, [f64; 2]) -> Option, +) -> Vec<[f64; 2]> { // Pad so expansion can grow past the buffer edge. let pad = (expansion.max(0.0) * scale).ceil() as usize + 2; let (pw, ph) = (w + 2 * pad, h + 2 * pad); @@ -628,7 +652,7 @@ pub fn outline(alpha: &[f32], w: usize, h: usize, scale: f64, offset: [f64; 2], } let g = expand(&Grid { w: pw, h: ph, on }, expansion * scale); let sp = [seed[0] * scale + offset[0] + pad as f64, seed[1] * scale + offset[1] + pad as f64]; - let Some(r) = region(&g, sp) else { return vec![] }; + let Some(r) = pick(&g, sp) else { return vec![] }; let c = trace(&r); let c = simplify(&c, 0.75); c.iter().map(|p| [(p[0] - pad as f64 - offset[0]) / scale, (p[1] - pad as f64 - offset[1]) / scale]).collect() diff --git a/crates/effects/src/puppet/tests.rs b/crates/effects/src/puppet/tests.rs index 10426d46c..56dbbf982 100644 --- a/crates/effects/src/puppet/tests.rs +++ b/crates/effects/src/puppet/tests.rs @@ -323,3 +323,40 @@ fn single_pin_on_a_traced_solid_does_not_rotate_it() { } } } + +/// #2865: only art no mesh has takes a new mesh, however close the rest is. +#[test] +fn unmeshed_art_is_strictly_under_the_point_and_not_already_meshed() { + // Two discs; the mesh is on the left one, at rest. + let (w, h) = (96usize, 48usize); + let (l, r) = (disc_alpha(w, h, [24.0, 24.0], 18.0), disc_alpha(w, h, [72.0, 24.0], 12.0)); + let mut img = Image::new(w as u32, h as u32); + for (i, p) in img.data.iter_mut().enumerate() { + *p = [1.0, 1.0, 1.0, l[i].max(r[i])]; + } + let buf = Buf { img, offset: [0.0; 2], scale: 1.0 }; + let params = flatten_params(&puppet_fx(&[(PinKind::Position, [24.0, 24.0], [24.0, 24.0])]), &mut |p| p.value.clone()); + let e = 3.0; + assert!(unmeshed_art_at(&buf, ¶ms, [72.0, 24.0], e), "the other disc"); + assert!(!unmeshed_art_at(&buf, ¶ms, [24.0, 24.0], e), "the meshed disc"); + assert!(!unmeshed_art_at(&buf, ¶ms, [48.0, 24.0], e), "between them: no nearest fallback"); + assert!(!unmeshed_art_at(&buf, ¶ms, [500.0, 24.0], e), "off the buffer"); + // Pixel corners of the meshed disc that simplifying the outline left just outside the triangles: + // on its art, off its mesh, and a mesh there would be the same mesh again. + let m = parse(¶ms).remove(0); + let mesh = mesh_for(&buf, &m); + let mut slivers = 0; + for y in 0..h { + for x in 0..48 { + for (dx, dy) in [(0.02, 0.02), (0.98, 0.02), (0.02, 0.98), (0.98, 0.98)] { + let p = [x as f64 + dx, y as f64 + dy]; + if contains(&mesh, &mesh.verts, p) || mesh::outline_under(&l, w, h, 1.0, [0.0; 2], p, e).is_empty() { + continue; + } + slivers += 1; + assert!(!unmeshed_art_at(&buf, ¶ms, p, e), "{p:?}"); + } + } + } + assert!(slivers > 0, "no point of the art falls outside the mesh: this test proves nothing"); +} diff --git a/crates/engine/src/cmd/puppet.rs b/crates/engine/src/cmd/puppet.rs index 1d080e773..a11a073b9 100644 --- a/crates/engine/src/cmd/puppet.rs +++ b/crates/engine/src/cmd/puppet.rs @@ -7,7 +7,8 @@ //! //! Pin positions are in effect space. A new pin is attached to the mesh under it: the click is //! mapped back through the current deformation to the rest mesh (its hidden rest position), and -//! Position / Advanced pins get a Position keyframe at the current time, as in After Effects. +//! Position / Advanced pins get a Position keyframe at the current time, as in After Effects. Off +//! every mesh a pin starts a new mesh only on art no mesh has yet; elsewhere nothing happens (#2865). use effectcraft_effects::puppet::{self, MeshOpts, PinKind}; use effectcraft_effects::{Buf, Params}; @@ -62,6 +63,19 @@ fn layer_time(s: &Session, cid: ItemId, lid: LayerId, t: Tick) -> Result { Ok(l.layer_time(t)) } +/// Whether a pin at `at` (effect space) goes on this mesh as `def` deforms it: on it or beside it. +fn on_mesh(mesh: &puppet::Mesh, def: &[[f64; 2]], at: [f64; 2]) -> bool { + !mesh.tris.is_empty() && (puppet::contains(mesh, def, at) || def.iter().map(|v| (v[0] - at[0]).hypot(v[1] - at[1])).fold(f64::MAX, f64::min) < 12.0) +} + +/// Whether `puppet.addPin` at `at` (effect space) places a pin on a layer with a Puppet effect: on +/// a mesh, on art no mesh has, or anywhere when there is no mesh yet (the first pin). +pub fn places_pin(s: &Session, cid: ItemId, lid: LayerId, at: [f64; 2], t: Tick) -> bool { + let Some((buf, params)) = find_puppet(s, cid, lid).and_then(|fu| puppet_eval(s, cid, lid, fu, t)) else { return true }; + let meshes = puppet::overlay(&buf, ¶ms); + meshes.is_empty() || meshes.iter().any(|(_, mesh, def, _)| on_mesh(mesh, def, at)) || puppet::unmeshed_art_at(&buf, ¶ms, at, s.state.puppet.expansion) +} + fn add_pin(s: &mut Session, p: &Value) -> Result { let cmd = "puppet.addPin"; let (cid, lid) = layer_p(s, p, cmd)?; @@ -78,14 +92,17 @@ fn add_pin(s: &mut Session, p: &Value) -> Result { return Err(bad(cmd, "the Puppet tools work on footage, solid, text, shape and precomp layers")); } } + let opts = MeshOpts { + density: f_p(p, "density").unwrap_or(s.state.puppet.density), + expansion: f_p(p, "expansion").unwrap_or(s.state.puppet.expansion), + triangles: f_p(p, "triangles").unwrap_or(s.state.puppet.triangles), + }; // Which mesh: the one under the click (through the current deformation), else a new one. let fx_uid = find_puppet(s, cid, lid); let want_mesh = p.get("mesh").and_then(Value::as_u64); - let new_mesh = p.get("newMesh").and_then(Value::as_bool).unwrap_or(false); // (mesh, the pin's rest point, where it is now) let mut target: Option<(Uid, [f64; 2], [f64; 2])> = None; if let Some(fu) = fx_uid - && !new_mesh && let Some((buf, params)) = puppet_eval(s, cid, lid, fu, t) { let meshes = puppet::overlay(&buf, ¶ms); @@ -93,9 +110,7 @@ fn add_pin(s: &mut Session, p: &Value) -> Result { if want_mesh.is_some_and(|w| w != *uid) || mesh.tris.is_empty() { continue; } - let inside = puppet::contains(mesh, def, at); - let near = def.iter().map(|v| (v[0] - at[0]).hypot(v[1] - at[1])).fold(f64::MAX, f64::min) < 12.0; - if inside || near || want_mesh.is_some() { + if on_mesh(mesh, def, at) || want_mesh.is_some() { let rest = puppet::unmap(mesh, def, at); // A moving pin holds its nearest mesh vertex at `position + (vertex - rest)` // (puppet::solve). Choose the position that keeps that vertex where it is now, so @@ -120,12 +135,11 @@ fn add_pin(s: &mut Session, p: &Value) -> Result { { return Err(bad(cmd, format!("no mesh {w}"))); } + // Nothing to pin here: empty space, or art a mesh has already carried away (#2865). + if target.is_none() && !meshes.is_empty() && !puppet::unmeshed_art_at(&buf, ¶ms, at, opts.expansion) { + return Ok(json!({"pin": null, "changed": false})); + } } - let opts = MeshOpts { - density: f_p(p, "density").unwrap_or(s.state.puppet.density), - expansion: f_p(p, "expansion").unwrap_or(s.state.puppet.expansion), - triangles: f_p(p, "triangles").unwrap_or(s.state.puppet.triangles), - }; let size = { let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?; let l = comp.layer(lid).ok_or(EngineError::NoComp)?; @@ -652,7 +666,7 @@ pub fn specs() -> Vec { "Add Puppet Pin", [], None, - "{layer?, kind?: position|advanced|bend|starch|overlap, position: [x,y] (effect space), time? (s) | frame?, mesh?: uid, newMesh?, density?, expansion?, triangles?}", + "{layer?, kind?: position|advanced|bend|starch|overlap, position: [x,y] (effect space), time? (s) | frame?, mesh?: uid, density?, expansion?, triangles?}", has_comp, add_pin ), diff --git a/crates/engine/src/tests/paint.rs b/crates/engine/src/tests/paint.rs index 3659ce0c4..ccb5b3dfe 100644 --- a/crates/engine/src/tests/paint.rs +++ b/crates/engine/src/tests/paint.rs @@ -225,6 +225,60 @@ fn a_pin_added_to_a_bent_mesh_moves_nothing() { } } +fn mesh_count(s: &mut Session, id: u64) -> usize { + s.execute("puppet.info", json!({"layer": id})).unwrap()["meshes"].as_array().unwrap().len() +} + +/// #2865: where the deformed layer is empty a click makes nothing: no copy of the art it has moved +/// off, no undo step, no error. +#[test] +fn a_pin_where_the_deformed_layer_is_empty_creates_nothing() { + let (mut s, id) = setup(); + s.execute("puppet.addPin", json!({"layer": id, "position": [10, 30]})).unwrap(); + s.execute("puppet.addPin", json!({"layer": id, "position": [90, 30]})).unwrap(); + s.set_time(Tick::from_seconds_f64(1.0)); + s.execute("puppet.movePin", json!({"layer": id, "pin": "Puppet Pin 1", "position": [10, 70]})).unwrap(); + s.execute("puppet.movePin", json!({"layer": id, "pin": "Puppet Pin 2", "position": [90, 70]})).unwrap(); + let (cid, t) = (s.active_comp_id().unwrap(), Tick::from_seconds_f64(1.0)); + let before = s.render(cid, t, RenderOpts::default()); + let revision = s.revision; + let lid = effectcraft_project::LayerId(id); + // The solid's top, which the pins have carried 40 px down; and well off it. + for at in [[50, 10], [300, 300]] { + assert!(!crate::commands::puppet::places_pin(&s, cid, lid, [at[0] as f64, at[1] as f64], t), "{at:?}"); + let r = s.execute("puppet.addPin", json!({"layer": id, "position": at})).unwrap(); + assert_eq!(r["pin"], Value::Null, "{at:?}"); + assert_eq!(s.revision, revision, "{at:?} changed the project"); + } + assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (1, 2)); + assert_eq!(s.render(cid, t, RenderOpts::default()), before); + // On the deformed shape a pin still goes on the mesh. + assert!(crate::commands::puppet::places_pin(&s, cid, lid, [50.0, 70.0], t)); + assert!(s.execute("puppet.addPin", json!({"layer": id, "position": [50, 70]})).unwrap()["pin"].is_u64()); + assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (1, 3)); +} + +/// #2865: art no mesh has still takes its own mesh; the gap beside it takes none. The first pin of +/// a layer is as ever: it finds the art nearest the click. +#[test] +fn only_unmeshed_art_takes_a_new_mesh() { + let (mut s, id) = setup(); + for x in [[0, 30], [60, 100]] { + s.execute("mask.new", json!({"layer": id, "vertices": [[x[0], 0], [x[1], 0], [x[1], 60], [x[0], 60]], "closed": true})).unwrap(); + } + s.execute("puppet.addPin", json!({"layer": id, "position": [46, 30]})).unwrap(); + assert_eq!(mesh_count(&mut s, id), 1, "the first pin"); + s.undo(); + s.execute("puppet.addPin", json!({"layer": id, "position": [10, 30]})).unwrap(); + // In the gap, past reach of the left piece's mesh (Expansion takes it to x 33). + assert_eq!(s.execute("puppet.addPin", json!({"layer": id, "position": [46, 30]})).unwrap()["pin"], Value::Null); + assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (1, 1)); + s.execute("puppet.addPin", json!({"layer": id, "position": [80, 30]})).unwrap(); + assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (2, 2), "the right piece gets its own mesh"); + s.execute("puppet.addPin", json!({"layer": id, "position": [80, 50]})).unwrap(); + assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (2, 3), "and the next pin goes on it"); +} + #[test] fn puppet_pin_kinds_and_mesh_options() { let (mut s, id) = setup(); diff --git a/crates/ui-egui/src/panels/puppet_tool.rs b/crates/ui-egui/src/panels/puppet_tool.rs index 968fafaf2..ac2186bf8 100644 --- a/crates/ui-egui/src/panels/puppet_tool.rs +++ b/crates/ui-egui/src/panels/puppet_tool.rs @@ -84,8 +84,8 @@ pub fn overlay(app: &EffectcraftApp, ctx: &egui::Context, cid: ItemId, layer: &L /// The layer a Puppet tool press at comp point `cpt` works on, of the selected `layers` with a /// Puppet effect (stack order): the one whose mesh, as deformed now, is under it (pins go on the -/// deformed shape, also where it reaches past the layer's own bounds), else the one whose art is -/// (a new mesh there). Never a layer without a Puppet effect: the tools stay on the layers being +/// deformed shape, also where it reaches past the layer's own bounds), else the one `on_art` takes +/// it (see `takes_pin`). Never a layer without a Puppet effect: the tools stay on the layers being /// rigged, as in After Effects (#284). pub fn target(ectx: &EvalCtx, layers: &[(&Layer, Overlay)], cpt: [f64; 2], on_art: impl Fn(&Layer) -> bool) -> Option { let on_mesh = |l: &Layer, ov: &Overlay| { @@ -96,6 +96,14 @@ pub fn target(ectx: &EvalCtx, layers: &[(&Layer, Overlay)], cpt: [f64; 2], on_ar layers.iter().find(|(l, ov)| on_mesh(l, ov)).or_else(|| layers.iter().find(|(l, _)| on_art(l))).map(|(l, _)| l.id) } +/// Whether a press at comp point `cpt` places a pin on `layer`. Where it would not (the deformed +/// layer is empty there) the press is on nothing: a click does nothing, a drag is a marquee (#2865). +pub fn takes_pin(app: &EffectcraftApp, ectx: &EvalCtx, layer: &Layer, cpt: [f64; 2]) -> bool { + let Some(inv) = fx2c(ectx, layer).inverse() else { return false }; + let p = inv.apply(gv2(cpt[0], cpt[1])); + effectcraft_engine::commands::puppet::places_pin(&app.session, ectx.comp_id, layer.id, [p.x, p.y], ectx.time) +} + /// Keep a pin's keyframed Position in view in the Timeline after placing, moving or recording the /// pin in the viewer (#273). pub fn reveal_pin(app: &mut EffectcraftApp, layer: LayerId, pin: u64) { diff --git a/crates/ui-egui/src/panels/viewer.rs b/crates/ui-egui/src/panels/viewer.rs index bb7b2d075..5b42488cb 100644 --- a/crates/ui-egui/src/panels/viewer.rs +++ b/crates/ui-egui/src/panels/viewer.rs @@ -1092,7 +1092,9 @@ fn phase_dragstart(app: &mut EffectcraftApp, ui: &mut egui::Ui, ectx: &EvalCtx, if puppet_layers.is_empty() { return pick(app, ectx, cpt, false); } - super::puppet_tool::target(ectx, &puppet_layers, cpt, |l| layers_at(ectx, cpt).any(|h| h.id == l.id)) + super::puppet_tool::target(ectx, &puppet_layers, cpt, |l| { + layers_at(ectx, cpt).any(|h| h.id == l.id) && super::puppet_tool::takes_pin(app, ectx, l, cpt) + }) }; if resp.drag_started() && !text_owns @@ -1680,7 +1682,9 @@ fn phase_clicked(app: &mut EffectcraftApp, ectx: &EvalCtx, resp: &egui::Response if puppet_layers.is_empty() { return pick(app, ectx, cpt, false); } - super::puppet_tool::target(ectx, &puppet_layers, cpt, |l| layers_at(ectx, cpt).any(|h| h.id == l.id)) + super::puppet_tool::target(ectx, &puppet_layers, cpt, |l| { + layers_at(ectx, cpt).any(|h| h.id == l.id) && super::puppet_tool::takes_pin(app, ectx, l, cpt) + }) }; if resp.clicked() && !text_owns diff --git a/crates/ui-egui/tests/it/ui_viewer.rs b/crates/ui-egui/tests/it/ui_viewer.rs index f0086d28a..4a712dbe5 100644 --- a/crates/ui-egui/tests/it/ui_viewer.rs +++ b/crates/ui-egui/tests/it/ui_viewer.rs @@ -1657,6 +1657,42 @@ fn puppet_pins_go_on_the_deformed_mesh_and_the_tool_stays_on_the_rigged_layer() assert!(!rigged(&h, plate)); } +/// #2865: a press where the deformed layer is empty, though on its original bounds, adds nothing: +/// a click leaves the rig alone and a drag from there is a pin marquee. +#[test] +fn puppet_press_where_the_deformed_layer_is_empty_is_a_marquee_not_a_copy() { + let mut h = harness(); + let s = &mut h.state_mut().session; + s.execute("edit.clear", json!({"layers": ["Plate"]})).unwrap(); + s.execute("layer.select", json!({"layers": ["Box"]})).unwrap(); + let id = s.state.selected_layers[0].0; + let a = s.execute("puppet.addPin", json!({"layer": id, "position": [10, 40]})).unwrap()["pin"].as_u64().unwrap(); + let b = s.execute("puppet.addPin", json!({"layer": id, "position": [70, 40]})).unwrap()["pin"].as_u64().unwrap(); + // Carry the Box 60 px right: its left part (comp x 280..340) is now empty. + s.execute("puppet.movePin", json!({"layer": id, "pin": a, "position": [70, 40]})).unwrap(); + s.execute("puppet.movePin", json!({"layer": id, "pin": b, "position": [130, 40]})).unwrap(); + h.state_mut().ui.tool = Tool::Puppet; + h.run_steps(3); + let rig = |h: &mut Harness<'_, EffectcraftApp>| { + let info = h.state_mut().session.execute("puppet.info", json!({"layer": id})).unwrap(); + let meshes = info["meshes"].as_array().unwrap().clone(); + (meshes.len(), meshes.iter().map(|m| m["pins"].as_array().unwrap().len()).sum::()) + }; + let at = screen(&h, [285.0, 180.0]); + click(&mut h, at); + assert_eq!(rig(&mut h), (1, 2), "the click made nothing"); + let comp = rect(&h, "viewer.comp"); + drag(&mut h, at, comp.max - vec2(4.0, 4.0)); + assert_eq!(rig(&mut h), (1, 2), "nor did the drag"); + let mut sel: Vec = h.state().session.state.selected_props.iter().map(|(_, u)| *u).collect(); + sel.sort(); + assert_eq!(sel, [a, b], "the drag's marquee selected the pins"); + // On the deformed shape a click still pins it. + let at = screen(&h, [380.0, 180.0]); + click(&mut h, at); + assert_eq!(rig(&mut h), (1, 3)); +} + /// Filled circles painted this frame (flattening nested shape lists). fn circles(h: &Harness<'_, EffectcraftApp>) -> Vec { fn walk(s: &egui::Shape, out: &mut Vec) { From 94310894934f55a838d5d3b4a445a8814996dbe5 Mon Sep 17 00:00:00 2001 From: Bar Borer Date: Tue, 3 Nov 2026 03:37:31 +0200 Subject: [PATCH 2/3] Puppet: decide a marquee from where the button went down (#2865) The drag branch asked what was under the pointer when the drag was recognised, by which time a quick drag has already reached the art, so no gesture started at all. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/panels/viewer.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/panels/viewer.rs b/crates/ui-egui/src/panels/viewer.rs index 5b42488cb..570c0ebca 100644 --- a/crates/ui-egui/src/panels/viewer.rs +++ b/crates/ui-egui/src/panels/viewer.rs @@ -1168,8 +1168,9 @@ fn phase_dragstart(app: &mut EffectcraftApp, ui: &mut egui::Ui, ectx: &EvalCtx, t if t.is_shape() => Some(Gesture::Create { tool: t, start: map.to_comp(press) }), Tool::ObjectMatte => Some(Gesture::ObjectMatteBox { start: press }), t if t.puppet_kind().is_some() && super::puppet_tool::hit(&pin_hits, press).is_none() => { - // Off the pins: Alt or a press outside the art drags a marquee that selects pins. - (mods.alt || puppet_layer(app, cpt).is_none()).then_some(Gesture::PinMarquee { start: press }) + // Off the pins: Alt or a press outside the art drags a marquee that selects pins. Asked + // of where the button went down: the pointer may be over the art by the time this is a drag. + (mods.alt || puppet_layer(app, map.to_comp(press)).is_none()).then_some(Gesture::PinMarquee { start: press }) } t if t.puppet_kind().is_some() => super::puppet_tool::hit(&pin_hits, press).and_then(|(h, part)| { use super::puppet_tool::PinPart; From fa0c260630c26a41d16220bd1eca44791f66c04c Mon Sep 17 00:00:00 2001 From: Bar Borer Date: Tue, 3 Nov 2026 04:00:19 +0200 Subject: [PATCH 3/3] Puppet: refuse only a mesh that repeats one already there (#2865) Cut the fix down: a click off every mesh makes nothing only when the art it resolves to already has a mesh with that outline. The nearest-art fallback in `region` stays, so a near miss beside separate art still meshes it. `mesh::outline` is as it was and the `newMesh` option of puppet.addPin is back. Co-Authored-By: Claude Opus 5.5 --- crates/effects/src/puppet.rs | 21 ---------------- crates/effects/src/puppet/mesh.rs | 26 +------------------ crates/effects/src/puppet/tests.rs | 37 --------------------------- crates/engine/src/cmd/puppet.rs | 40 ++++++++++++++++++++---------- crates/engine/src/tests/paint.rs | 27 ++++++++++++++------ 5 files changed, 47 insertions(+), 104 deletions(-) diff --git a/crates/effects/src/puppet.rs b/crates/effects/src/puppet.rs index 4db76c1f5..6ec2ce5ba 100644 --- a/crates/effects/src/puppet.rs +++ b/crates/effects/src/puppet.rs @@ -395,27 +395,6 @@ pub fn contains(mesh: &Mesh, deformed: &[[f64; 2]], p: [f64; 2]) -> bool { }) } -/// Whether `at` (effect space) is art a new mesh with this `expansion` could take: on the expanded -/// alpha itself, and not what a mesh of `params` already covers at rest, even if its deformation -/// has carried that art away. A mesh there would only rebuild the same outline: a full copy (#2865). -pub fn unmeshed_art_at(buf: &Buf, params: &Params, at: [f64; 2], expansion: f64) -> bool { - let (w, h) = (buf.img.width as usize, buf.img.height as usize); - let alpha: Vec = buf.img.data.iter().map(|p| p[3]).collect(); - let mut outline = mesh::outline_under(&alpha, w, h, buf.scale.max(1e-9), buf.offset, at, expansion); - if outline.is_empty() { - return false; - } - // As `triangulate` winds it. The simplified outline can cut a corner past `at`, so a mesh can - // miss the very art it was built from: the same outline is the same art. - if mesh::signed_area(&outline) < 0.0 { - outline.reverse(); - } - !parse(params).iter().any(|m| { - let mesh = mesh_for(buf, m); - mesh.outline == outline || contains(&mesh, &mesh.verts, at) - }) -} - #[inline] fn bary(a: [f64; 2], b: [f64; 2], c: [f64; 2], p: [f64; 2]) -> Option<[f64; 3]> { let d = (b[1] - c[1]) * (a[0] - c[0]) + (c[0] - b[0]) * (a[1] - c[1]); diff --git a/crates/effects/src/puppet/mesh.rs b/crates/effects/src/puppet/mesh.rs index 1046dfa6e..f01ee8f78 100644 --- a/crates/effects/src/puppet/mesh.rs +++ b/crates/effects/src/puppet/mesh.rs @@ -194,11 +194,6 @@ pub fn region(g: &Grid, seed: [f64; 2]) -> Option { Some(Grid { w, h, on: outside.iter().map(|o| !o).collect() }) } -/// The region under `seed` only when `seed` is on a cell: `region` without its nearest fallback. -pub fn region_under(g: &Grid, seed: [f64; 2]) -> Option { - if g.at(seed[0].floor() as i64, seed[1].floor() as i64) { region(g, seed) } else { None } -} - /// Trace the outer boundary of a hole-free region along pixel cracks: a closed polygon through /// pixel corners, region on the left when walking (counter-clockwise in y-down coordinates). pub fn trace(g: &Grid) -> Vec<[f64; 2]> { @@ -622,25 +617,6 @@ pub fn triangulate(outline: &[[f64; 2]], opts: &MeshOpts) -> Mesh { /// The outline of the alpha region under `seed` (buffer pixels → layer space via `scale` and /// `offset`), expanded by `expansion` layer pixels. pub fn outline(alpha: &[f32], w: usize, h: usize, scale: f64, offset: [f64; 2], seed: [f64; 2], expansion: f64) -> Vec<[f64; 2]> { - outline_of(alpha, w, h, scale, offset, seed, expansion, region) -} - -/// As `outline`, but empty unless `seed` is on the expanded alpha itself. -pub fn outline_under(alpha: &[f32], w: usize, h: usize, scale: f64, offset: [f64; 2], seed: [f64; 2], expansion: f64) -> Vec<[f64; 2]> { - outline_of(alpha, w, h, scale, offset, seed, expansion, region_under) -} - -#[allow(clippy::too_many_arguments)] -fn outline_of( - alpha: &[f32], - w: usize, - h: usize, - scale: f64, - offset: [f64; 2], - seed: [f64; 2], - expansion: f64, - pick: fn(&Grid, [f64; 2]) -> Option, -) -> Vec<[f64; 2]> { // Pad so expansion can grow past the buffer edge. let pad = (expansion.max(0.0) * scale).ceil() as usize + 2; let (pw, ph) = (w + 2 * pad, h + 2 * pad); @@ -652,7 +628,7 @@ fn outline_of( } let g = expand(&Grid { w: pw, h: ph, on }, expansion * scale); let sp = [seed[0] * scale + offset[0] + pad as f64, seed[1] * scale + offset[1] + pad as f64]; - let Some(r) = pick(&g, sp) else { return vec![] }; + let Some(r) = region(&g, sp) else { return vec![] }; let c = trace(&r); let c = simplify(&c, 0.75); c.iter().map(|p| [(p[0] - pad as f64 - offset[0]) / scale, (p[1] - pad as f64 - offset[1]) / scale]).collect() diff --git a/crates/effects/src/puppet/tests.rs b/crates/effects/src/puppet/tests.rs index 56dbbf982..10426d46c 100644 --- a/crates/effects/src/puppet/tests.rs +++ b/crates/effects/src/puppet/tests.rs @@ -323,40 +323,3 @@ fn single_pin_on_a_traced_solid_does_not_rotate_it() { } } } - -/// #2865: only art no mesh has takes a new mesh, however close the rest is. -#[test] -fn unmeshed_art_is_strictly_under_the_point_and_not_already_meshed() { - // Two discs; the mesh is on the left one, at rest. - let (w, h) = (96usize, 48usize); - let (l, r) = (disc_alpha(w, h, [24.0, 24.0], 18.0), disc_alpha(w, h, [72.0, 24.0], 12.0)); - let mut img = Image::new(w as u32, h as u32); - for (i, p) in img.data.iter_mut().enumerate() { - *p = [1.0, 1.0, 1.0, l[i].max(r[i])]; - } - let buf = Buf { img, offset: [0.0; 2], scale: 1.0 }; - let params = flatten_params(&puppet_fx(&[(PinKind::Position, [24.0, 24.0], [24.0, 24.0])]), &mut |p| p.value.clone()); - let e = 3.0; - assert!(unmeshed_art_at(&buf, ¶ms, [72.0, 24.0], e), "the other disc"); - assert!(!unmeshed_art_at(&buf, ¶ms, [24.0, 24.0], e), "the meshed disc"); - assert!(!unmeshed_art_at(&buf, ¶ms, [48.0, 24.0], e), "between them: no nearest fallback"); - assert!(!unmeshed_art_at(&buf, ¶ms, [500.0, 24.0], e), "off the buffer"); - // Pixel corners of the meshed disc that simplifying the outline left just outside the triangles: - // on its art, off its mesh, and a mesh there would be the same mesh again. - let m = parse(¶ms).remove(0); - let mesh = mesh_for(&buf, &m); - let mut slivers = 0; - for y in 0..h { - for x in 0..48 { - for (dx, dy) in [(0.02, 0.02), (0.98, 0.02), (0.02, 0.98), (0.98, 0.98)] { - let p = [x as f64 + dx, y as f64 + dy]; - if contains(&mesh, &mesh.verts, p) || mesh::outline_under(&l, w, h, 1.0, [0.0; 2], p, e).is_empty() { - continue; - } - slivers += 1; - assert!(!unmeshed_art_at(&buf, ¶ms, p, e), "{p:?}"); - } - } - } - assert!(slivers > 0, "no point of the art falls outside the mesh: this test proves nothing"); -} diff --git a/crates/engine/src/cmd/puppet.rs b/crates/engine/src/cmd/puppet.rs index a11a073b9..d894516c2 100644 --- a/crates/engine/src/cmd/puppet.rs +++ b/crates/engine/src/cmd/puppet.rs @@ -8,7 +8,7 @@ //! Pin positions are in effect space. A new pin is attached to the mesh under it: the click is //! mapped back through the current deformation to the rest mesh (its hidden rest position), and //! Position / Advanced pins get a Position keyframe at the current time, as in After Effects. Off -//! every mesh a pin starts a new mesh only on art no mesh has yet; elsewhere nothing happens (#2865). +//! every mesh a pin starts a new mesh, unless its art already has one (#2865). use effectcraft_effects::puppet::{self, MeshOpts, PinKind}; use effectcraft_effects::{Buf, Params}; @@ -68,12 +68,25 @@ fn on_mesh(mesh: &puppet::Mesh, def: &[[f64; 2]], at: [f64; 2]) -> bool { !mesh.tris.is_empty() && (puppet::contains(mesh, def, at) || def.iter().map(|v| (v[0] - at[0]).hypot(v[1] - at[1])).fold(f64::MAX, f64::min) < 12.0) } -/// Whether `puppet.addPin` at `at` (effect space) places a pin on a layer with a Puppet effect: on -/// a mesh, on art no mesh has, or anywhere when there is no mesh yet (the first pin). +/// Whether a new mesh at `at` (effect space) would repeat a mesh of `params`: the art it would +/// outline is that mesh's own, even if a deformation has carried the mesh off it (#2865). +fn repeats_mesh(buf: &Buf, params: &Params, at: [f64; 2]) -> bool { + let (w, h) = (buf.img.width as usize, buf.img.height as usize); + let alpha: Vec = buf.img.data.iter().map(|p| p[3]).collect(); + puppet::parse(params).iter().any(|m| { + let mut outline = puppet::mesh::outline(&alpha, w, h, buf.scale.max(1e-9), buf.offset, at, m.opts.expansion); + // As `triangulate` winds it. + if puppet::mesh::signed_area(&outline) < 0.0 { + outline.reverse(); + } + puppet::mesh_for(buf, m).outline == outline + }) +} + +/// Whether `puppet.addPin` at `at` (effect space) places a pin on a layer with a Puppet effect. pub fn places_pin(s: &Session, cid: ItemId, lid: LayerId, at: [f64; 2], t: Tick) -> bool { let Some((buf, params)) = find_puppet(s, cid, lid).and_then(|fu| puppet_eval(s, cid, lid, fu, t)) else { return true }; - let meshes = puppet::overlay(&buf, ¶ms); - meshes.is_empty() || meshes.iter().any(|(_, mesh, def, _)| on_mesh(mesh, def, at)) || puppet::unmeshed_art_at(&buf, ¶ms, at, s.state.puppet.expansion) + !repeats_mesh(&buf, ¶ms, at) || puppet::overlay(&buf, ¶ms).iter().any(|(_, mesh, def, _)| on_mesh(mesh, def, at)) } fn add_pin(s: &mut Session, p: &Value) -> Result { @@ -92,17 +105,14 @@ fn add_pin(s: &mut Session, p: &Value) -> Result { return Err(bad(cmd, "the Puppet tools work on footage, solid, text, shape and precomp layers")); } } - let opts = MeshOpts { - density: f_p(p, "density").unwrap_or(s.state.puppet.density), - expansion: f_p(p, "expansion").unwrap_or(s.state.puppet.expansion), - triangles: f_p(p, "triangles").unwrap_or(s.state.puppet.triangles), - }; // Which mesh: the one under the click (through the current deformation), else a new one. let fx_uid = find_puppet(s, cid, lid); let want_mesh = p.get("mesh").and_then(Value::as_u64); + let new_mesh = p.get("newMesh").and_then(Value::as_bool).unwrap_or(false); // (mesh, the pin's rest point, where it is now) let mut target: Option<(Uid, [f64; 2], [f64; 2])> = None; if let Some(fu) = fx_uid + && !new_mesh && let Some((buf, params)) = puppet_eval(s, cid, lid, fu, t) { let meshes = puppet::overlay(&buf, ¶ms); @@ -135,11 +145,15 @@ fn add_pin(s: &mut Session, p: &Value) -> Result { { return Err(bad(cmd, format!("no mesh {w}"))); } - // Nothing to pin here: empty space, or art a mesh has already carried away (#2865). - if target.is_none() && !meshes.is_empty() && !puppet::unmeshed_art_at(&buf, ¶ms, at, opts.expansion) { + if target.is_none() && repeats_mesh(&buf, ¶ms, at) { return Ok(json!({"pin": null, "changed": false})); } } + let opts = MeshOpts { + density: f_p(p, "density").unwrap_or(s.state.puppet.density), + expansion: f_p(p, "expansion").unwrap_or(s.state.puppet.expansion), + triangles: f_p(p, "triangles").unwrap_or(s.state.puppet.triangles), + }; let size = { let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?; let l = comp.layer(lid).ok_or(EngineError::NoComp)?; @@ -666,7 +680,7 @@ pub fn specs() -> Vec { "Add Puppet Pin", [], None, - "{layer?, kind?: position|advanced|bend|starch|overlap, position: [x,y] (effect space), time? (s) | frame?, mesh?: uid, density?, expansion?, triangles?}", + "{layer?, kind?: position|advanced|bend|starch|overlap, position: [x,y] (effect space), time? (s) | frame?, mesh?: uid, newMesh?, density?, expansion?, triangles?}", has_comp, add_pin ), diff --git a/crates/engine/src/tests/paint.rs b/crates/engine/src/tests/paint.rs index ccb5b3dfe..2d2e540a5 100644 --- a/crates/engine/src/tests/paint.rs +++ b/crates/engine/src/tests/paint.rs @@ -252,33 +252,44 @@ fn a_pin_where_the_deformed_layer_is_empty_creates_nothing() { } assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (1, 2)); assert_eq!(s.render(cid, t, RenderOpts::default()), before); + // Whatever Expansion new meshes would now get, the art is the mesh's own. + s.state.puppet.expansion = 9.0; + assert_eq!(s.execute("puppet.addPin", json!({"layer": id, "position": [50, 10]})).unwrap()["pin"], Value::Null); // On the deformed shape a pin still goes on the mesh. assert!(crate::commands::puppet::places_pin(&s, cid, lid, [50.0, 70.0], t)); assert!(s.execute("puppet.addPin", json!({"layer": id, "position": [50, 70]})).unwrap()["pin"].is_u64()); assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (1, 3)); } -/// #2865: art no mesh has still takes its own mesh; the gap beside it takes none. The first pin of -/// a layer is as ever: it finds the art nearest the click. +/// #2865: separate art still gets its own mesh, also from a near miss in the gap beside it (a click +/// off the art finds the art nearest it): only a repeat of a mesh is refused. #[test] -fn only_unmeshed_art_takes_a_new_mesh() { +fn separate_art_still_gets_its_own_mesh() { let (mut s, id) = setup(); for x in [[0, 30], [60, 100]] { s.execute("mask.new", json!({"layer": id, "vertices": [[x[0], 0], [x[1], 0], [x[1], 60], [x[0], 60]], "closed": true})).unwrap(); } - s.execute("puppet.addPin", json!({"layer": id, "position": [46, 30]})).unwrap(); - assert_eq!(mesh_count(&mut s, id), 1, "the first pin"); - s.undo(); s.execute("puppet.addPin", json!({"layer": id, "position": [10, 30]})).unwrap(); - // In the gap, past reach of the left piece's mesh (Expansion takes it to x 33). - assert_eq!(s.execute("puppet.addPin", json!({"layer": id, "position": [46, 30]})).unwrap()["pin"], Value::Null); assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (1, 1)); + // In the gap, nearer the right piece and past reach of the left piece's mesh. + s.execute("puppet.addPin", json!({"layer": id, "position": [46, 30]})).unwrap(); + assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (2, 2), "the near miss meshes the right piece"); + s.undo(); s.execute("puppet.addPin", json!({"layer": id, "position": [80, 30]})).unwrap(); assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (2, 2), "the right piece gets its own mesh"); s.execute("puppet.addPin", json!({"layer": id, "position": [80, 50]})).unwrap(); assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (2, 3), "and the next pin goes on it"); } +/// `newMesh` asks for a new mesh wherever the click is, the art's own mesh or not. +#[test] +fn new_mesh_option_starts_a_mesh_on_meshed_art() { + let (mut s, id) = setup(); + s.execute("puppet.addPin", json!({"layer": id, "position": [10, 30]})).unwrap(); + s.execute("puppet.addPin", json!({"layer": id, "position": [50, 30], "newMesh": true})).unwrap(); + assert_eq!((mesh_count(&mut s, id), pin_count(&s, id)), (2, 2)); +} + #[test] fn puppet_pin_kinds_and_mesh_options() { let (mut s, id) = setup();