diff --git a/Cargo.lock b/Cargo.lock index cfab570d..2609335f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1148,6 +1148,7 @@ dependencies = [ "objc2 0.6.4", "objc2-app-kit 0.3.2", "objc2-foundation 0.3.2", + "pollster 0.4.0", "rfd", "serde_json", "winit", diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index a653b26b..fe523807 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -117,6 +117,9 @@ const USAGE: &str = "usage: effectcraft-cli : id or name, '-' = active comp; : id, '#n' or name; : JSON or bare string"; @@ -163,6 +166,9 @@ const VALUED: &[&str] = &[ "--webm-codec", "--audio-bitrate", "--opus-app", + // mcp + "--automation-read-root", + "--automation-write-root", ]; /// Options without a value. Any other `--option` is a usage error. @@ -232,6 +238,10 @@ impl Args { fn flag(&self, k: &str) -> bool { self.opts.iter().any(|(o, _)| o == k) } + /// Every value of a repeatable option, in order. + fn opts(&self, k: &str) -> Vec<&str> { + self.opts.iter().filter(|(o, _)| o == k).filter_map(|(_, v)| v.as_deref()).collect() + } fn opt(&self, k: &str) -> Option<&str> { self.opts.iter().rev().find(|(o, _)| o == k).and_then(|(_, v)| v.as_deref()) } @@ -595,7 +605,12 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { if args.flag("--autosave") && args.opt("--bridge").is_some() { return usage_err("--autosave is for headless MCP; the bridged app owns its auto-saves"); } - let b = backend(args, false)?; + let mut b = backend(args, false)?; + let (read, write) = (args.opts("--automation-read-root"), args.opts("--automation-write-root")); + if !read.is_empty() || !write.is_empty() { + let roots = |r: Vec<&str>| r.into_iter().map(std::path::PathBuf::from).collect::>(); + b.confine(effectcraft_automation::confine::Confinement::new(&roots(read), &roots(write))?); + } let server = McpServer::new(b); let mut server = if args.flag("--autosave") { let root = diff --git a/apps/effectcraft/Cargo.toml b/apps/effectcraft/Cargo.toml index ad3fc8b5..5fabc10c 100644 --- a/apps/effectcraft/Cargo.toml +++ b/apps/effectcraft/Cargo.toml @@ -18,6 +18,7 @@ egui = { workspace = true } eframe = { workspace = true, features = ["default_fonts", "wgpu", "accesskit", "links", "wayland", "x11"] } serde_json = { workspace = true } rfd = { workspace = true } +pollster = { workspace = true } cpal = { workspace = true } log = { workspace = true } diff --git a/apps/effectcraft/src/main.rs b/apps/effectcraft/src/main.rs index 70457a47..a5269789 100644 --- a/apps/effectcraft/src/main.rs +++ b/apps/effectcraft/src/main.rs @@ -21,6 +21,9 @@ mod control_server; mod launch_guard; #[cfg(target_os = "macos")] mod native_menu; +// Built everywhere so it stays checked; only the macOS app uses it. +#[cfg_attr(not(target_os = "macos"), allow(dead_code))] +mod sheet_dialogs; use effectcraft_ui_egui::EffectcraftApp; use serde_json::json; @@ -227,6 +230,9 @@ fn main() -> eframe::Result { })); #[cfg(target_os = "macos")] { + // The commands' file dialogs open as sheets instead of nested modal loops (#511). + let ctx = cc.egui_ctx.clone(); + app.hooks.file_dialog_async = Some(Box::new(move |dialog, done| sheet_dialogs::show(dialog, done, ctx.clone()))); app.hooks.app_action = Some(Box::new(native_menu::app_action)); app.hooks.clipboard_text = Some(Box::new(native_menu::clipboard_text)); } diff --git a/apps/effectcraft/src/sheet_dialogs.rs b/apps/effectcraft/src/sheet_dialogs.rs new file mode 100644 index 00000000..577eb0b6 --- /dev/null +++ b/apps/effectcraft/src/sheet_dialogs.rs @@ -0,0 +1,76 @@ +//! File dialogs that don't block the frame (macOS), for `Hooks::file_dialog_async`. +//! +//! A blocking `rfd::FileDialog` runs a nested modal event loop inside eframe's frame, that is +//! inside winit's event handler. On macOS a file dragged over the window while that loop ran +//! re-entered winit's handler, which panics across the Objective-C boundary and aborts the app +//! (#511). `rfd::AsyncFileDialog` shows the dialog as a sheet on the window and returns at once; +//! the answer arrives when the sheet closes, outside the frame, and the app runs the command then. + +use effectcraft_ui_egui::{DialogDone, FileDialog}; +use rfd::{AsyncFileDialog, FileHandle}; +use std::future::Future; +use std::pin::Pin; + +type Answer = Pin> + Send>>; + +/// Show `dialog` (on the main thread: the sheet opens here) and hand its paths to `done` once it +/// closes, then wake the UI. +pub fn show(dialog: FileDialog, done: DialogDone, ctx: egui::Context) { + let answer = open(dialog); + let spawned = std::thread::Builder::new().name("ec-file-dialog".into()).spawn(move || { + done(pollster::block_on(answer)); + ctx.request_repaint(); + }); + if let Err(e) = spawned { + log::error!("file dialog: {e}"); + } +} + +fn paths(files: Option>) -> Vec { + files.unwrap_or_default().into_iter().map(|f| f.path().to_string_lossy().to_string()).collect() +} + +fn path(file: Option) -> Vec { + paths(file.map(|f| vec![f])) +} + +/// The dialog, as the blocking hooks in `main.rs` show it. +fn open(dialog: FileDialog) -> Answer { + match dialog { + FileDialog::Open { exts, many } => { + let d = AsyncFileDialog::new().add_filter("Media", &exts); + if many { + let f = d.pick_files(); + Box::pin(async move { paths(f.await) }) + } else { + let f = d.pick_file(); + Box::pin(async move { path(f.await) }) + } + } + FileDialog::OpenProject => { + let f = AsyncFileDialog::new().add_filter("EffectCraft Project", &["ecproj", "ecprojx"]).pick_file(); + Box::pin(async move { path(f.await) }) + } + FileDialog::Folder => { + let f = AsyncFileDialog::new().pick_folder(); + Box::pin(async move { path(f.await) }) + } + FileDialog::Save { default, ext, project } => { + let at = std::path::Path::new(&default); + let name = at.file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); + let mut d = if project { + AsyncFileDialog::new().add_filter("EffectCraft Project", &["ecproj"]) + } else if ext.is_empty() { + AsyncFileDialog::new() + } else { + AsyncFileDialog::new().add_filter(&ext, &[ext.as_str()]) + }; + // A default with a folder that exists opens the dialog there. + if let Some(dir) = at.parent().filter(|d| d.is_dir()) { + d = d.set_directory(dir); + } + let f = d.set_file_name(name).save_file(); + Box::pin(async move { path(f.await) }) + } + } +} diff --git a/crates/automation/src/backend.rs b/crates/automation/src/backend.rs index 685c978f..c3947c87 100644 --- a/crates/automation/src/backend.rs +++ b/crates/automation/src/backend.rs @@ -4,9 +4,16 @@ use effectcraft_engine::Session; use effectcraft_engine::time::Tick; use serde_json::{Value, json}; +use crate::confine::Confinement; use crate::{BridgeClient, Error, Result, base64, encode_png, recompress_png}; -pub enum Backend { +pub struct Backend { + target: Target, + /// File access confined to folders (`--automation-read-root` / `--automation-write-root`). + confinement: Option, +} + +enum Target { /// An in-process session (no window): engine commands, inspection and rendering. Headless(Box), /// A running desktop app reached over its control channel: everything, plus the live UI. @@ -24,43 +31,66 @@ pub struct Frame { impl Backend { pub fn headless(session: Session) -> Self { - Backend::Headless(Box::new(session)) + Backend { target: Target::Headless(Box::new(session)), confinement: None } } /// Bridge to the app's control channel at `addr` (`9877` or `127.0.0.1:9877`). pub fn bridge(addr: &str) -> Result { - Ok(Backend::Bridge(BridgeClient::new(addr)?)) + Ok(Backend { target: Target::Bridge(BridgeClient::new(addr)?), confinement: None }) + } + + /// Confine the files the commands run here may read and write ([`Confinement`]). + pub fn confine(&mut self, confinement: Confinement) { + self.confinement = Some(confinement); + } + + pub fn confinement(&self) -> Option<&Confinement> { + self.confinement.as_ref() } pub fn is_bridge(&self) -> bool { - matches!(self, Backend::Bridge(_)) + matches!(self.target, Target::Bridge(_)) } pub fn session(&mut self) -> Option<&mut Session> { - match self { - Backend::Headless(s) => Some(s), - Backend::Bridge(_) => None, + match &mut self.target { + Target::Headless(s) => Some(s), + Target::Bridge(_) => None, } } - /// Run an engine command by id. + /// Run an engine command by id (checked against the [`Confinement`], if any). pub fn exec(&mut self, id: &str, params: Value) -> Result { let params = if params.is_null() { json!({}) } else { params }; - match self { - Backend::Headless(s) => { + if let Some(c) = self.confinement.clone() { + let lookup = match c.check_command(id, ¶ms)? { + Some(crate::confine::Lookup::ProjectFile) => Some((crate::confine::Lookup::ProjectFile, self.run("project.summary", json!({}))?)), + Some(crate::confine::Lookup::RenderOutputs) => Some((crate::confine::Lookup::RenderOutputs, self.run("renderQueue.list", json!({}))?)), + None => None, + }; + if let Some((lookup, found)) = lookup { + c.check_lookup(&lookup, &found)?; + } + } + self.run(id, params) + } + + fn run(&mut self, id: &str, params: Value) -> Result { + match &mut self.target { + Target::Headless(s) => { let r = s.execute_checked(id, params); s.drain_events(); Ok(r?) } - Backend::Bridge(b) => b.call("engine.execute", json!({"command": id, "params": params})), + Target::Bridge(b) => b.call("engine.execute", json!({"command": id, "params": params})), } } /// Call a raw control-channel method (bridge only). pub fn control(&mut self, method: &str, params: Value) -> Result { - match self { - Backend::Bridge(b) => b.call(method, if params.is_null() { json!({}) } else { params }), - Backend::Headless(_) => Err(Error::BadArgs(NEED_BRIDGE.into())), + match &mut self.target { + Target::Bridge(b) => b.call(method, if params.is_null() { json!({}) } else { params }), + Target::Headless(_) => Err(Error::BadArgs(NEED_BRIDGE.into())), } } @@ -73,15 +103,15 @@ impl Backend { /// [`Backend::render`]; `transparent` keeps the frame's alpha instead of compositing it over /// the comp's background colour. pub fn render_with(&mut self, comp: Option<&Value>, time: Option, max_side: u32, transparent: bool) -> Result { - match self { - Backend::Headless(s) => { + match &mut self.target { + Target::Headless(s) => { let cid = s.resolve_comp(comp).map_err(|_| no_comp(comp))?; let t = time.map(Tick::from_seconds_f64).unwrap_or_else(|| s.time()); let (w, h, rgba) = s.render_rgba8_alpha(cid, t, max_side, transparent)?; let png = encode_png(w, h, rgba, max_side)?; Ok(Frame { png, comp: json!(cid.0), time: t.seconds(), width: w, height: h }) } - Backend::Bridge(b) => { + Target::Bridge(b) => { let mut p = json!({"max_side": max_side, "base64": true}); if transparent { p["transparent"] = json!(true); diff --git a/crates/automation/src/confine.rs b/crates/automation/src/confine.rs new file mode 100644 index 00000000..f396b3da --- /dev/null +++ b/crates/automation/src/confine.rs @@ -0,0 +1,276 @@ +//! File access confinement for automation (`effectcraft-cli mcp --automation-read-root DIR +//! --automation-write-root DIR`, as in PhotoCraft): an agent may only read files under a read +//! root (or a write root) and only write under a write root. +//! +//! Every engine command an agent runs ([`crate::Backend::exec`], also each step of an +//! `engine.batch`) is checked before it runs: its file parameters must be absolute paths inside +//! the roots (symbolic links resolved, no `..`), any other absolute path in its parameters too, +//! and commands that write to a file they are not given (Save, Increment and Save, rendering the +//! Render Queue) are checked against where they would write. Commands that run code or open +//! other applications (scripts, Execute File, Reveal in Finder…) are refused while confined. + +use std::path::{Component, Path, PathBuf}; + +use serde_json::Value; + +use crate::{Error, Result}; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Access { + Read, + Write, +} + +/// The folders an agent may read and write. +#[derive(Clone, Debug)] +pub struct Confinement { + read: Vec, + write: Vec, +} + +/// Commands that run code, act outside the project or open other applications. +const REFUSED: &[&str] = &[ + "script.run", + "file.runScript", + "file.installScript", + "file.installScriptUIPanel", + "file.uninstallScript", + "file.executeFile", + "file.revealInFinder", + "layer.openSource", + "help.revealLogFile", + "file.autoSave", +]; + +/// Commands that read files named by these parameters. +const READS: &[(&str, &[&str])] = &[ + ("file.open", &["path"]), + ("file.openRecent", &["path"]), + ("file.import", &["path", "paths"]), + ("file.importMultiple", &["paths"]), + ("file.importRecent", &["path"]), + ("file.importLottie", &["path"]), + ("file.importTimeline", &["path"]), + ("file.replaceFootage", &["path"]), + ("file.relinkFootage", &["folder"]), + ("file.setProxy", &["path"]), + ("file.watchFolder", &["folder"]), + ("anim.applyPreset", &["path"]), + ("anim.applyRecentPreset", &["path"]), + ("keys.rpfCameraImport", &["path"]), + ("essential.importTemplate", &["path"]), + ("view.importGuides", &["path"]), + ("shortcuts.import", &["path"]), + ("effect.plugins.load", &["path", "folder"]), + ("mediaBrowser.go", &["path"]), + ("mediaBrowser.addFavorite", &["path"]), +]; + +/// Commands that write files named by these parameters. +const WRITES: &[(&str, &[&str])] = &[ + ("file.save", &["path"]), + ("file.saveAs", &["path"]), + ("file.saveCopy", &["path"]), + ("file.saveCopyAsXml", &["path"]), + ("file.collectFiles", &["folder"]), + ("file.exportLottie", &["path"]), + ("comp.saveFrameAs", &["path"]), + ("comp.saveFrameAsPsd", &["path"]), + ("comp.saveFrameAsExr", &["path"]), + ("anim.savePreset", &["path"]), + ("view.exportGuides", &["path"]), + ("render.saveCurrentPreview", &["path"]), + ("render.preRender", &["output"]), + ("essential.exportTemplate", &["path"]), + ("shortcuts.export", &["path"]), + ("renderQueue.add", &["output", "path"]), +]; + +/// Commands that write where they are not told: the project's file, the Render Queue's outputs. +const IMPLICIT_WRITES: &[&str] = &["file.save", "file.incrementAndSave", "renderQueue.render", "render.preRender"]; + +/// What must be looked up before an implicitly writing command may run. +#[derive(Debug, PartialEq, Eq)] +pub enum Lookup { + /// The open project's file (`project.summary` → `path`). + ProjectFile, + /// Every output of the queued Render Queue items (`renderQueue.list`). + RenderOutputs, +} + +impl Confinement { + /// Roots that must exist (each resolved to its real path). With no write root nothing may be + /// written; write roots may also be read. + pub fn new(read: &[PathBuf], write: &[PathBuf]) -> Result { + let real = |p: &PathBuf| p.canonicalize().map_err(|e| Error::BadArgs(format!("automation root {}: {e}", p.display()))); + let read = read.iter().map(real).collect::>>()?; + let write = write.iter().map(real).collect::>>()?; + Ok(Self { read, write }) + } + + fn roots(&self, access: Access) -> impl Iterator { + let read = if access == Access::Read { &self.read[..] } else { &[] }; + read.iter().chain(&self.write) + } + + /// `path` resolved, if it is inside the roots for `access`. + pub fn check(&self, path: &str, access: Access) -> Result { + let denied = |why: &str| { + let kind = if access == Access::Read { "read" } else { "write" }; + let roots: Vec = self.roots(access).map(|r| r.display().to_string()).collect(); + let roots = if roots.is_empty() { "none".to_string() } else { roots.join(", ") }; + Error::BadArgs(format!("cannot {kind} `{path}`: {why} (automation {kind} roots: {roots})")) + }; + let p = Path::new(path); + if !p.is_absolute() { + return Err(denied("give an absolute path")); + } + if p.components().any(|c| matches!(c, Component::ParentDir)) { + return Err(denied("`..` is not allowed")); + } + let real = resolve(p).ok_or_else(|| denied("it can't be resolved"))?; + if self.roots(access).any(|r| real.starts_with(r)) { Ok(real) } else { Err(denied("it is outside the automation roots")) } + } + + /// Check engine command `id` with `params` before it runs. Returns what must still be looked + /// up and passed to [`Confinement::check_lookup`] for commands that write implicitly. + pub fn check_command(&self, id: &str, params: &Value) -> Result> { + if REFUSED.contains(&id) { + return Err(Error::BadArgs(format!("`{id}` is not available while file access is confined (--automation-read-root/--automation-write-root)"))); + } + if id == "engine.batch" { + for step in params.get("steps").and_then(Value::as_array).into_iter().flatten() { + let Some(inner) = step.get("command").or_else(|| step.get("id")).and_then(Value::as_str) else { continue }; + let inner_params = step.get("params").cloned().unwrap_or(Value::Null); + let named = keys_of(READS, inner).or_else(|| keys_of(WRITES, inner)).unwrap_or(&[]); + // A step's `$N` reference is resolved only as the batch runs: never for a file. + if named.iter().filter_map(|k| inner_params.get(*k)).any(has_reference) { + return Err(Error::BadArgs(format!("`{inner}` in a batch: file paths can't be `$N` references while file access is confined"))); + } + if self.check_command(inner, &inner_params)?.is_some() { + return Err(Error::BadArgs(format!("`{inner}` writes to its project's or queue's files: run it on its own while file access is confined"))); + } + } + return Ok(None); + } + let (named, access) = match (keys_of(WRITES, id), keys_of(READS, id)) { + (Some(k), _) => (k, Access::Write), + (None, Some(k)) => (k, Access::Read), + (None, None) if id.starts_with("renderQueue.") || id == "prefs.set" => (&[][..], Access::Write), + (None, None) => (&[][..], Access::Read), + }; + let mut given = false; + for key in named { + let Some(v) = params.get(*key).filter(|v| !v.is_null()) else { continue }; + given = true; + let paths: Vec<&Value> = match v { + Value::Array(a) => a.iter().collect(), + v => vec![v], + }; + for p in paths { + // A Render Queue output may name a template instead of a file. + match p.as_str() { + Some(s) if id == "render.preRender" && !looks_like_path(s) => {} + Some(s) => { + self.check(s, access)?; + } + None => return Err(Error::BadArgs(format!("`{id}`: `{key}` must be a path"))), + } + } + } + // Any other absolute path the command is given. + let mut other = vec![]; + absolute_strings(params, named, &mut other); + for s in other { + self.check(&s, access)?; + } + // An entry of a recent list by index is a path the agent didn't give. + if matches!(id, "file.openRecent" | "file.importRecent" | "anim.applyRecentPreset") && !given { + return Err(Error::BadArgs(format!("`{id}`: give `path` while file access is confined"))); + } + if IMPLICIT_WRITES.contains(&id) && !(given && id == "file.save") { + return Ok(Some(if id == "file.save" || id == "file.incrementAndSave" { Lookup::ProjectFile } else { Lookup::RenderOutputs })); + } + Ok(None) + } + + /// Check what a [`Lookup`] found: `project.summary` or `renderQueue.list`'s result. + pub fn check_lookup(&self, lookup: &Lookup, found: &Value) -> Result<()> { + match lookup { + Lookup::ProjectFile => match found.get("path").and_then(Value::as_str) { + // Increment and Save writes beside it. + Some(p) => self.check(p, Access::Write).map(drop), + None => Err(Error::BadArgs("the project has no file yet: give `path` (Save As) inside an automation write root".into())), + }, + Lookup::RenderOutputs => { + let items = found.get("items").or(Some(found)).and_then(Value::as_array).cloned().unwrap_or_default(); + for it in items.iter().filter(|it| it.get("render").and_then(Value::as_bool) != Some(false)) { + let extra = it.get("extraOutputs").and_then(Value::as_array).into_iter().flatten().map(|o| o.get("outputPath")); + for p in std::iter::once(it.get("outputPath")).chain(extra).chain([it.get("logPath")]).flatten() { + if let Some(p) = p.as_str() { + self.check(p, Access::Write)?; + } + } + } + Ok(()) + } + } + } +} + +fn keys_of(table: &[(&str, &'static [&'static str])], id: &str) -> Option<&'static [&'static str]> { + table.iter().find(|(c, _)| *c == id).map(|(_, k)| *k) +} + +fn has_reference(v: &Value) -> bool { + match v { + Value::String(s) => s.starts_with('$'), + Value::Array(a) => a.iter().any(has_reference), + _ => false, + } +} + +/// A file path rather than a template name: has a separator or an extension. +fn looks_like_path(s: &str) -> bool { + s.contains(['/', '\\']) || Path::new(s).extension().is_some() +} + +/// Strings in `v` (recursively, except under the top-level `skip` keys) that are absolute paths, +/// `~` paths or `file:` URLs. +fn absolute_strings(v: &Value, skip: &[&str], out: &mut Vec) { + fn walk(v: &Value, out: &mut Vec, depth: usize) { + if depth > 64 { + return; + } + match v { + Value::String(s) => { + let t = s.trim(); + let drive = t.len() > 2 && t.as_bytes().get(1) == Some(&b':') && t.as_bytes().first().is_some_and(u8::is_ascii_alphabetic); + if t.starts_with('/') || t.starts_with('\\') || t.starts_with('~') || drive || t.to_ascii_lowercase().starts_with("file:") { + out.push(t.to_string()); + } + } + Value::Array(a) => a.iter().for_each(|x| walk(x, out, depth + 1)), + Value::Object(o) => o.values().for_each(|x| walk(x, out, depth + 1)), + _ => {} + } + } + match v { + Value::Object(o) => o.iter().filter(|(k, _)| !skip.contains(&k.as_str())).for_each(|(_, x)| walk(x, out, 0)), + v => walk(v, out, 0), + } +} + +/// `p` with symbolic links resolved: its longest existing ancestor's real path plus the rest +/// (which doesn't exist yet, so can't be a link). +fn resolve(p: &Path) -> Option { + let mut rest = vec![]; + let mut at = p; + loop { + if let Ok(real) = at.canonicalize() { + return Some(rest.iter().rev().fold(real, |acc: PathBuf, c| acc.join(c))); + } + rest.push(at.file_name()?.to_os_string()); + at = at.parent()?; + } +} diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 6a673309..a755e0dc 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -19,6 +19,7 @@ mod autosave; pub mod backend; pub mod base64; pub mod bridge; +pub mod confine; mod long_job; pub mod server; pub mod tools; diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index 51d15694..37de10a5 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -1181,3 +1181,74 @@ fn command_tools_accept_id_as_an_alias() { let (_, err) = call(&mut s, "execute_command", json!({})); assert!(err, "neither key is an error"); } + +/// #500: `--automation-read-root` / `--automation-write-root` confine the files an agent reads +/// and writes: inside the roots works, outside (or relative, `..`, through a symbolic link, by +/// a batch step, by Save to the project's own file, from a script) is refused. +#[test] +fn automation_roots_confine_file_access() { + use crate::confine::Confinement; + let base = std::env::temp_dir().join(format!("ec-mcp-confine-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let (read, write, outside) = (base.join("in"), base.join("out"), base.join("elsewhere")); + for d in [&read, &write, &outside] { + std::fs::create_dir_all(d).unwrap(); + } + let p = |d: &std::path::Path, f: &str| d.join(f).to_string_lossy().into_owned(); + // Unconfined (the default): anywhere. + let mut s = server(); + call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"name": "A", "width": 64, "height": 64}})); + call_json(&mut s, "save_project", json!({"path": p(&outside, "free.ecproj")})); + call_json(&mut s, "save_project", json!({"path": p(&read, "source.ecproj")})); + + let mut s = server(); + s.backend().confine(Confinement::new(std::slice::from_ref(&read), std::slice::from_ref(&write)).unwrap()); + let denied = |s: &mut McpServer, name: &str, args: Value| { + let (c, err) = call(s, name, args.clone()); + assert!(err, "{name} {args} should be refused: {c:?}"); + c[0]["text"].as_str().unwrap_or_default().to_string() + }; + // Reading: inside a read root, not outside. + call_json(&mut s, "open_project", json!({"path": p(&read, "source.ecproj")})); + let msg = denied(&mut s, "open_project", json!({"path": p(&outside, "free.ecproj")})); + assert!(msg.contains("outside the automation roots"), "{msg}"); + // Writing: inside a write root only; not into a read root, not relative, not with `..`. + call_json(&mut s, "save_project", json!({"path": p(&write, "saved.ecproj")})); + assert!(write.join("saved.ecproj").is_file()); + denied(&mut s, "save_project", json!({"path": p(&read, "copy.ecproj")})); + denied(&mut s, "save_project", json!({"path": "relative.ecproj"})); + denied(&mut s, "save_project", json!({"path": p(&write, "../elsewhere/up.ecproj")})); + denied(&mut s, "execute_command", json!({"command": "comp.saveFrameAs", "params": {"path": p(&outside, "f.png")}})); + // Any other absolute path a command is given. + denied(&mut s, "execute_command", json!({"command": "renderQueue.add", "params": {"output": p(&outside, "movie.mp4")}})); + // A batch step. + let steps = json!([{"command": "comp.new", "params": {"name": "B"}}, {"command": "file.saveAs", "params": {"path": p(&outside, "b.ecproj")}}]); + denied(&mut s, "batch", json!({"steps": steps})); + let steps = json!([{"command": "file.saveAs", "params": {"path": "$1.path"}}]); + denied(&mut s, "batch", json!({"steps": steps})); + // Save (and Increment and Save) write the open project's own file: it must be writable. + call_json(&mut s, "open_project", json!({"path": p(&read, "source.ecproj")})); + denied(&mut s, "save_project", json!({})); + denied(&mut s, "execute_command", json!({"command": "file.incrementAndSave"})); + // Rendering the queue writes its items' outputs (by default beside the project). + call_json(&mut s, "execute_command", json!({"command": "renderQueue.add", "params": {"comp": "A"}})); + let queue = call_json(&mut s, "execute_command", json!({"command": "renderQueue.list"})); + assert!(queue["items"][0]["outputPath"].as_str().is_some_and(|o| o.starts_with(&*read.to_string_lossy())), "{queue}"); + let msg = denied(&mut s, "execute_command", json!({"command": "renderQueue.render"})); + assert!(msg.contains("cannot write"), "{msg}"); + call_json(&mut s, "open_project", json!({"path": p(&write, "saved.ecproj")})); + call_json(&mut s, "save_project", json!({})); + // Through a symbolic link out of a root. + #[cfg(unix)] + { + std::os::unix::fs::symlink(&outside, write.join("link")).unwrap(); + denied(&mut s, "save_project", json!({"path": p(&write.join("link"), "escaped.ecproj")})); + assert!(!outside.join("escaped.ecproj").exists()); + } + // Scripts can read and write anything: refused. + denied(&mut s, "run_script", json!({"code": "1 + 1"})); + denied(&mut s, "execute_command", json!({"command": "file.runScript", "params": {"path": p(&read, "x.jsx")}})); + // Writes to the files the agent named: none outside. + assert_eq!(std::fs::read_dir(&outside).unwrap().count(), 1, "only the unconfined save"); + let _ = std::fs::remove_dir_all(&base); +} diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 12ea3167..fa86095b 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -103,12 +103,19 @@ pub fn available(bridge: bool) -> impl Iterator { TOOLS.iter().filter(move |t| bridge || !t.bridge_only) } +/// Tools refused while file access is confined: they drive the live app like a person (menus, +/// shortcuts, Open Recent…) or call control-channel methods directly, past the command checks. +const UNCONFINED_ONLY: &[&str] = &["control", "ui_click", "ui_drag", "ui_key", "ui_type", "ui_set", "script_ui"]; + /// Run a tool by name. pub fn run(b: &mut Backend, name: &str, args: &Value) -> Result { let t = find(name).ok_or_else(|| Error::BadArgs(format!("unknown tool `{name}`")))?; if t.bridge_only && !b.is_bridge() { return Err(Error::BadArgs(NEED_BRIDGE.into())); } + if b.confinement().is_some() && UNCONFINED_ONLY.contains(&name) { + return Err(Error::BadArgs(format!("`{name}` is not available while file access is confined (--automation-read-root/--automation-write-root)"))); + } let empty = json!({}); (t.run)(b, if args.is_null() { &empty } else { args }) } @@ -389,6 +396,9 @@ fn render_frame(b: &mut Backend, a: &Value) -> Result { let f = b.render_with(get(a, "comp"), get(a, "time").and_then(Value::as_f64), max_side, transparent)?; let mut info = json!({"comp": f.comp, "time": f.time, "width": f.width, "height": f.height}); if let Some(p) = get(a, "path").and_then(Value::as_str) { + if let Some(c) = b.confinement() { + c.check(p, crate::confine::Access::Write)?; + } std::fs::write(p, &f.png).map_err(|e| Error::Other(format!("cannot write {p}: {e}")))?; info["path"] = json!(p); } @@ -482,6 +492,9 @@ fn batch_tool(b: &mut Backend, a: &Value) -> Result { fn screenshot(b: &mut Backend, a: &Value) -> Result { let max_side = get(a, "max_side").and_then(Value::as_u64).unwrap_or(1600) as u32; let keep = get(a, "path").and_then(Value::as_str).map(str::to_string); + if let (Some(c), Some(p)) = (b.confinement(), &keep) { + c.check(p, crate::confine::Access::Write)?; + } let path = keep.clone().unwrap_or_else(|| std::env::temp_dir().join(format!("effectcraft-mcp-shot-{}.png", std::process::id())).to_string_lossy().into_owned()); b.control("ui.screenshot", obj(&[("path", Some(&json!(path))), ("panel", get(a, "panel")), ("id", get(a, "id"))]))?; diff --git a/crates/gpu/src/adv3d.rs b/crates/gpu/src/adv3d.rs index f3b9fee2..6ae11445 100644 --- a/crates/gpu/src/adv3d.rs +++ b/crates/gpu/src/adv3d.rs @@ -15,7 +15,6 @@ use effectcraft_render::three_d::Dof; use effectcraft_render::three_d::adv::{Prepared, Rendered, Scene, Target}; use effectcraft_render::three_d::bokeh; -use wgpu::util::DeviceExt; use crate::context::{Enc, GpuContext, GpuImage}; @@ -443,14 +442,11 @@ fn raster_into(e: &mut Enc, s: &Scene) -> Option<(wgpu::Texture, wgpu::Texture)> b }) .collect(); - let vbuf = dev.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: Some("adv3d vertices"), contents: &vb, usage: wgpu::BufferUsages::VERTEX }); + let vbuf = g.buffer_init(Some("adv3d vertices"), &vb, wgpu::BufferUsages::VERTEX); let ib: Vec = s.indices.iter().flat_map(|i| i.to_le_bytes()).collect(); - let ibuf = dev.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: Some("adv3d indices"), contents: &ib, usage: wgpu::BufferUsages::INDEX }); - let ub = dev.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: Some("adv3d globals"), contents: &bufs[0], usage: wgpu::BufferUsages::UNIFORM }); - let sbs: Vec = bufs[1..] - .iter() - .map(|b| dev.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: Some("adv3d data"), contents: b, usage: wgpu::BufferUsages::STORAGE })) - .collect(); + let ibuf = g.buffer_init(Some("adv3d indices"), &ib, wgpu::BufferUsages::INDEX); + let ub = g.buffer_init(Some("adv3d globals"), &bufs[0], wgpu::BufferUsages::UNIFORM); + let sbs: Vec = bufs[1..].iter().map(|b| g.buffer_init(Some("adv3d data"), b, wgpu::BufferUsages::STORAGE)).collect(); let mut entries = vec![wgpu::BindGroupEntry { binding: 0, resource: ub.as_entire_binding() }]; for (i, b) in sbs.iter().enumerate() { entries.push(wgpu::BindGroupEntry { binding: i as u32 + 1, resource: b.as_entire_binding() }); @@ -736,7 +732,7 @@ fn dispatch(e: &mut Enc, kernel: &str, u0: [u32; 4], f0: [f32; 4], b: Binds, gro let g = e.g; let post = &pipes(g).post; let Some((_, pipe)) = post.kernels.iter().find(|(k, _)| *k == kernel) else { return }; - let ub = g.device.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: None, contents: ¶ms(u0, f0), usage: wgpu::BufferUsages::UNIFORM }); + let ub = g.buffer_init(None, ¶ms(u0, f0), wgpu::BufferUsages::UNIFORM); let mut entries = vec![ wgpu::BindGroupEntry { binding: 0, resource: ub.as_entire_binding() }, wgpu::BindGroupEntry { binding: 1, resource: wgpu::BindingResource::TextureView(b.color.unwrap_or(&post.dummy_color)) }, @@ -859,11 +855,7 @@ fn depth_of_field(e: &mut Enc, acc: &wgpu::Buffer, depth: &wgpu::Buffer, (w, h): for v in [f32::INFINITY.to_bits(), 0, 0, 0] { init.extend_from_slice(&v.to_le_bytes()); } - let range = g.device.create_buffer_init(&wgpu::util::BufferInitDescriptor { - label: Some("adv3d radius range"), - contents: &init, - usage: wgpu::BufferUsages::STORAGE | wgpu::BufferUsages::COPY_SRC, - }); + let range = g.buffer_init(Some("adv3d radius range"), &init, wgpu::BufferUsages::STORAGE | wgpu::BufferUsages::COPY_SRC); let f0 = [dof.focus as f32, dof.aperture as f32, dof.blur_level as f32, scale as f32]; let b = Binds { bufs: [None, Some(depth), None, None, Some(&radius), Some(&range), None], ..Default::default() }; dispatch(e, "radius_of", [w, h, 0, 0], f0, b, pixel_groups(w, h)); diff --git a/crates/gpu/src/context.rs b/crates/gpu/src/context.rs index c5065eae..788757bf 100644 --- a/crates/gpu/src/context.rs +++ b/crates/gpu/src/context.rs @@ -5,7 +5,6 @@ use std::sync::{Arc, Mutex, Weak}; use effectcraft_effects::Buf; use effectcraft_raster::Image; -use wgpu::util::DeviceExt; /// Compute entry points in `kernels.wgsl` (one pipeline each). const ENTRIES: &[&str] = &[ @@ -325,7 +324,34 @@ fn init_resource(_device: &wgpu::Device, _label: &str, create: impl FnOnce() Ok(create()) } +/// A buffer holding `contents` (padded to 4 bytes), written through the queue. Unlike +/// `DeviceExt::create_buffer_init`, which maps the buffer at creation and panics when the +/// allocation failed (a driver reports it as an internal error, not out-of-memory: Adreno's +/// `ERROR_INITIALIZATION_FAILED`, #591), a failed allocation only yields an invalid buffer and +/// an error for the caller's error scope. +pub(crate) fn buffer_init(device: &wgpu::Device, queue: &wgpu::Queue, label: Option<&str>, contents: &[u8], usage: wgpu::BufferUsages) -> wgpu::Buffer { + let align = wgpu::COPY_BUFFER_ALIGNMENT as usize; + let padded = contents.len().div_ceil(align).saturating_mul(align); + let buffer = + device.create_buffer(&wgpu::BufferDescriptor { label, size: padded as u64, usage: usage | wgpu::BufferUsages::COPY_DST, mapped_at_creation: false }); + if padded > 0 { + if padded == contents.len() { + queue.write_buffer(&buffer, 0, contents); + } else { + let mut data = contents.to_vec(); + data.resize(padded, 0); + queue.write_buffer(&buffer, 0, &data); + } + } + buffer +} + impl GpuContext { + /// [`buffer_init`] on this context's device. + pub(crate) fn buffer_init(&self, label: Option<&str>, contents: &[u8], usage: wgpu::BufferUsages) -> wgpu::Buffer { + buffer_init(&self.device, &self.queue, label, contents, usage) + } + /// Build on an existing device (the desktop app shares egui-wgpu's). `Err` when the /// adapter cannot run the compositor (no compute shaders, e.g. WebGL2; no float storage /// textures) or the device was created with limits below what its pipelines bind. @@ -549,9 +575,8 @@ impl GpuContext { view_formats: &[], }) })?; - let dummy_buf = init_resource(&device, "dummy storage buffer", || { - device.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: Some("dummy"), contents: &[0u8; 16], usage: wgpu::BufferUsages::STORAGE }) - })?; + let dummy_buf = + init_resource(&device, "dummy storage buffer", || buffer_init(&device, &queue, Some("dummy"), &[0u8; 16], wgpu::BufferUsages::STORAGE))?; let dummy_tex = init_resource(&device, "dummy texture view", || dummy.create_view(&Default::default()))?; let max_dim = device.limits().max_texture_dimension_2d; Ok(GpuContext { @@ -996,7 +1021,7 @@ impl<'g> Enc<'g> { self.missing = true; return; }; - let ub = g.device.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: None, contents: &p.bytes(), usage: wgpu::BufferUsages::UNIFORM }); + let ub = g.buffer_init(None, &p.bytes(), wgpu::BufferUsages::UNIFORM); let sv = src.texture.create_view(&Default::default()); let av = aux.map(|a| a.texture.create_view(&Default::default())); let ov = out.texture.create_view(&Default::default()); @@ -1229,7 +1254,7 @@ impl<'g> Enc<'g> { while bytes.len() < 16 || !bytes.len().is_multiple_of(4) { bytes.push(0); } - self.g.device.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: Some("data"), contents: &bytes, usage: wgpu::BufferUsages::STORAGE }) + self.g.buffer_init(Some("data"), &bytes, wgpu::BufferUsages::STORAGE) } /// Convert to the viewer's premultiplied RGBA8 texture (egui-wgpu's native texture format), diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index 723da308..252145c1 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -264,17 +264,22 @@ impl Gpu { } } - /// Run work in a native thread-local out-of-memory scope. A failed result is discarded - /// and cached allocations are released; callers can retry with a fresh CPU renderer. + /// Run work in native thread-local out-of-memory and internal-error scopes. A failed + /// result is discarded and cached allocations are released; callers can retry with a fresh + /// CPU renderer. Some drivers report a failed allocation as an internal error rather than + /// out-of-memory (Adreno's `ERROR_INITIALIZATION_FAILED`, #591), so both count. /// Browser callers use the host/owned-device failure bridge and bounded readbacks instead /// of blocking their event loop on an error-scope future. pub fn within_memory(&self, f: impl FnOnce() -> T) -> Result { self.ctx.check_health()?; #[cfg(not(target_arch = "wasm32"))] { - let scope = self.ctx.device.push_error_scope(wgpu::ErrorFilter::OutOfMemory); + let oom = self.ctx.device.push_error_scope(wgpu::ErrorFilter::OutOfMemory); + let internal = self.ctx.device.push_error_scope(wgpu::ErrorFilter::Internal); let out = f(); - match pollster::block_on(scope.pop()) { + let internal = pollster::block_on(internal.pop()); + let oom = pollster::block_on(oom.pop()); + match internal.or(oom) { Some(e) => { self.ctx.clear_uploads(); Err(e.to_string()) diff --git a/crates/gpu/src/particles.rs b/crates/gpu/src/particles.rs index 015edc82..7934dee0 100644 --- a/crates/gpu/src/particles.rs +++ b/crates/gpu/src/particles.rs @@ -6,7 +6,6 @@ use std::sync::{Mutex, OnceLock}; use effectcraft_effects::psim::{ParticleSystem, SimParticle, SimRequest}; -use wgpu::util::DeviceExt; use crate::context::GpuContext; @@ -152,7 +151,7 @@ pub(crate) fn simulate(g: &GpuContext, req: &SimRequest) -> Option = req.births.iter().flat_map(|b| b.to_le_bytes()).collect(); - let births = d.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: Some("births"), contents: &births, usage: wgpu::BufferUsages::STORAGE }); + let births = g.buffer_init(Some("births"), &births, wgpu::BufferUsages::STORAGE); let prev = prev.unwrap_or_else(|| { d.create_buffer(&wgpu::BufferDescriptor { label: Some("particles"), size: STATE_BYTES, usage: wgpu::BufferUsages::STORAGE, mapped_at_creation: false }) }); @@ -162,11 +161,7 @@ pub(crate) fn simulate(g: &GpuContext, req: &SimRequest) -> Option Option>; +/// A file dialog a host shows without blocking the frame ([`Hooks::file_dialog_async`]). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum FileDialog { + /// Files to open or import with these extensions; several when `many`. + Open { exts: Vec, many: bool }, + /// An EffectCraft project to open. + OpenProject, + /// A folder. + Folder, + /// Where to save `default` (a file name or path) with extension `ext` (empty: no filter); + /// `project`: the project save dialog, given the file name. + Save { default: String, ext: String, project: bool }, +} + +impl FileDialog { + /// The save dialog for `default`, as [`Hooks::save_dialog`] picks it. + pub fn save(default: &str) -> Self { + let ext = std::path::Path::new(default).extension().map(|e| e.to_string_lossy().to_string()).unwrap_or_default(); + Self::Save { default: default.to_string(), project: ext.eq_ignore_ascii_case("ecproj"), ext } + } +} + +/// Called with the paths a [`FileDialog`] picked (empty: cancelled), from any thread. +pub type DialogDone = Box) + Send>; + +/// A non-blocking file dialog's answer for the command that asked (`menus::dialog_answered`). +pub(crate) struct DialogAnswer { + pub command: String, + pub key: &'static str, + pub params: serde_json::Value, + pub paths: Vec, +} + /// Host hooks provided by the native app (file pickers etc.). #[derive(Default)] pub struct Hooks { @@ -121,6 +154,11 @@ pub struct Hooks { /// Application actions the OS performs (`app.hide`, `app.hideOthers`, `app.showAll` on /// macOS). Returns false when the host doesn't handle the id. pub app_action: Option bool>>, + /// Shows a command's file dialog without blocking the frame and calls `done` with the + /// answer (the host then asks for a repaint). Used in place of the blocking pickers above for + /// the commands' dialogs (File ▸ Import, Open, Save As…) where a modal dialog's nested event + /// loop inside a frame is unsafe: on macOS a drag over the window then aborted the app (#511). + pub file_dialog_async: Option>, } impl Hooks { @@ -261,6 +299,11 @@ pub struct EffectcraftApp { gpu_checked: bool, gpu_failures: Option, gpu_failure_applied: Option, + /// The graphics device was lost and the window is closing (see `close_after_device_loss`); + /// `true` keeps crash recovery for the next launch. + device_lost_close: Option, + /// Answers of file dialogs shown without blocking ([`Hooks::file_dialog_async`]). + pub(crate) dialog_answers: (Sender, Receiver), /// Commands from the native menu bar. pub command_inbox: Option>, /// Pointer position in comp pixels (Info panel). @@ -347,6 +390,8 @@ impl EffectcraftApp { gpu_checked: false, gpu_failures: None, gpu_failure_applied: None, + device_lost_close: None, + dialog_answers: std::sync::mpsc::channel(), command_inbox: None, pointer_comp: None, dialog_state: Default::default(), @@ -643,6 +688,33 @@ impl EffectcraftApp { ctx.request_repaint(); } + /// After the graphics device is lost, eframe's renderer cannot draw again: any mesh makes + /// egui-wgpu panic on the main thread when it cannot stage its index buffer (#591). Draw + /// nothing from then on, auto-save a project with unsaved changes (keeping the crash + /// recovery marker, so the next launch offers it) and close the window. + fn close_after_device_loss(&mut self, ctx: &egui::Context) { + if self.device_lost_close.is_some() { + return; + } + let keep_recovery = self.session.is_dirty(); + if keep_recovery { + match self.session.autosave_now().and_then(|path| self.session.autosave_wait().map(|()| path)) { + Ok(path) => log::warn!("graphics device lost: auto-saved {path}; the next launch offers to recover it"), + Err(e) => log::error!("graphics device lost: {e}"), + } + } + self.device_lost_close = Some(keep_recovery); + log::warn!("graphics device lost: closing the window"); + ctx.send_viewport_cmd(egui::ViewportCommand::Close); + } + + /// Run the commands whose non-blocking file dialogs have been answered. + pub(crate) fn run_dialog_answers(&mut self) { + while let Ok(answer) = self.dialog_answers.1.try_recv() { + menus::dialog_answered(self, answer); + } + } + /// Set up the GPU compositor on egui-wgpu's device (once). Without an adapter that runs /// compute shaders (WebGL2, software GL) everything stays on the CPU. pub(crate) fn init_gpu(&mut self, frame: &eframe::Frame) { @@ -1483,6 +1555,7 @@ impl EffectcraftApp { fn frame(&mut self, ui: &mut egui::Ui) { let ctx = ui.ctx().clone(); self.apply_gpu_failure(&ctx); + self.run_dialog_answers(); self.auto.begin_frame(); self.frames.set_context(&ctx); if self.session.render_job.is_some() { @@ -1672,8 +1745,11 @@ impl EffectcraftApp { impl eframe::App for EffectcraftApp { fn on_exit(&mut self) { - // Clean exit: no crash recovery next launch. - self.session.end_recovery(); + // Clean exit: no crash recovery next launch (unless the graphics device was lost with + // unsaved changes, which are in the auto-save it offers). + if self.device_lost_close != Some(true) { + self.session.end_recovery(); + } } fn raw_input_hook(&mut self, ctx: &egui::Context, raw_input: &mut egui::RawInput) { @@ -1732,6 +1808,10 @@ impl eframe::App for EffectcraftApp { } fn ui(&mut self, ui: &mut egui::Ui, frame: &mut eframe::Frame) { + if self.gpu_failure_applied == Some(true) { + self.close_after_device_loss(ui.ctx()); + return; + } self.init_gpu(frame); if !self.fonts_ready { ui.ctx().request_repaint(); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 6e7ec9fc..52ae3b15 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1200,7 +1200,31 @@ fn file_dialog(app: &mut EffectcraftApp, id: &str, params: &Value) -> Option crate::FileDialog::Folder, + Ask::Import => crate::FileDialog::Open { exts: MEDIA.iter().map(|e| e.to_string()).collect(), many: key == "paths" }, + Ask::OpenProject => crate::FileDialog::OpenProject, + Ask::Open(exts) => crate::FileDialog::Open { exts: exts.iter().map(|e| e.to_string()).collect(), many: false }, + Ask::Save(default) => crate::FileDialog::save(default), + }; + let tx = app.dialog_answers.0.clone(); + let answer = crate::DialogAnswer { command: id.to_string(), key, params: params.clone(), paths: Vec::new() }; + spawn( + request, + Box::new(move |paths| { + let _ = tx.send(crate::DialogAnswer { paths, ..answer }); + }), + ); + return Some(Ok(json!({"dialog": id}))); + } let picked: Option = match ask { Ask::Folder => { let Some(f) = app.hooks.pick_folder.as_ref() else { return Some(Err("no folder dialog available (pass `folder`)".into())) }; @@ -1208,15 +1232,7 @@ fn file_dialog(app: &mut EffectcraftApp, id: &str, params: &Value) -> Option { let Some(f) = app.hooks.pick_files.as_ref() else { return Some(Err("no file dialog available (pass `paths`)".into())) }; - let paths = f(&[ - "mp4", "mov", "m4v", "mkv", "webm", "png", "jpg", "jpeg", "gif", "webp", "tif", "tiff", "bmp", "exr", "wav", "aif", "aiff", "mp3", "flac", - "ogg", "opus", "svg", "pdf", "ai", "eps", "psd", "psb", "gltf", "glb", "obj", "json", "csv", "tsv", - ]); - match (paths.is_empty(), key) { - (true, _) => None, - (false, "paths") => Some(json!(paths)), - (false, _) => paths.first().map(|s| json!(s)), - } + picked_value(key, f(MEDIA)) } Ask::OpenProject => { let Some(f) = app.hooks.pick_open_project.as_ref() else { return Some(Err("no file dialog available (pass `path`)".into())) }; @@ -1232,6 +1248,30 @@ fn file_dialog(app: &mut EffectcraftApp, id: &str, params: &Value) -> Option) -> Option { + match (paths.is_empty(), key) { + (true, _) => None, + (false, "paths") => Some(json!(paths)), + (false, _) => paths.into_iter().next().map(Value::from), + } +} + +/// A file dialog shown without blocking ([`crate::Hooks::file_dialog_async`]) answered: run its +/// command with the picked path (nothing when it was cancelled). +pub(crate) fn dialog_answered(app: &mut EffectcraftApp, answer: crate::DialogAnswer) { + let Some(v) = picked_value(answer.key, answer.paths) else { return }; + if let Err(e) = run_with_picked(app, &answer.command, answer.key, &answer.params, v) { + app.ui.status = e; + } +} + +/// Run command `id` with the path picked for it in parameter `key`. +fn run_with_picked(app: &mut EffectcraftApp, id: &str, key: &str, params: &Value, v: Value) -> Result { + let mut p = params.as_object().cloned().unwrap_or_default(); p.insert(key.to_string(), v); // Picked files import in the background, with the Importing card showing progress (#270). if matches!(id, "file.import" | "file.importMultiple") { @@ -1243,7 +1283,7 @@ fn file_dialog(app: &mut EffectcraftApp, id: &str, params: &Value) -> Option Option>> = Arc::default(); + let queue = pending.clone(); + app.hooks.file_dialog_async = Some(Box::new(move |dialog, done| queue.lock().unwrap().push((dialog, done)))); + assert_eq!(invoke(&mut app, &ctx, "file.saveAs", json!({})).unwrap(), json!({"dialog": "file.saveAs"})); + let (dialog, done) = pending.lock().unwrap().pop().expect("a dialog was shown"); + assert_eq!(dialog, crate::FileDialog::Save { default: "Untitled Project.ecproj".into(), ext: "ecproj".into(), project: true }); + assert!(app.session.path.is_none(), "nothing saved until the dialog answers"); + let dir = std::env::temp_dir().join(format!("ec-dialog-answer-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("Answered.ecproj"); + // Answered from another thread, as the host's dialog does. + std::thread::spawn(move || done(vec![path.to_string_lossy().to_string()])).join().unwrap(); + app.run_dialog_answers(); + assert_eq!(app.session.path.as_deref(), Some(dir.join("Answered.ecproj").to_string_lossy().as_ref())); + assert!(dir.join("Answered.ecproj").is_file()); + // Cancelled: nothing runs. + invoke(&mut app, &ctx, "comp.saveFrameAs", json!({})).unwrap(); + let (dialog, done) = pending.lock().unwrap().pop().expect("a dialog was shown"); + assert_eq!(dialog, crate::FileDialog::Save { default: "Frame.png".into(), ext: "png".into(), project: false }); + done(Vec::new()); + app.run_dialog_answers(); + assert!(!dir.join("Frame.png").exists()); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn menu_items_cover_the_tree() { assert_eq!(menus().last(), Some(&"Help")); diff --git a/crates/ui-egui/src/panels/viewer.rs b/crates/ui-egui/src/panels/viewer.rs index a532cdeb..248f9bcb 100644 --- a/crates/ui-egui/src/panels/viewer.rs +++ b/crates/ui-egui/src/panels/viewer.rs @@ -872,6 +872,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let mut handle_hits: Vec<(LayerId, usize, Pos2, egui::CursorIcon)> = Vec::new(); let mut vertex_hits: Vec = Vec::new(); let mut key_hits: Vec = Vec::new(); + let mut motion_frames: Vec = Vec::new(); let mut paths: Vec = Vec::new(); let sel_vertices = app.session.state.selected_vertices.clone(); // Pen path in progress ends when the tool changes or its mask is gone; Esc / Enter also end @@ -891,7 +892,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { for l in comp.layers.iter().filter(|l| selected.contains(&l.id) && l.is_active_at(time)) { let col = if label_handles { t.label(l.label) } else { t.accent }; // Motion path of animated position (keyframes and spatial tangents are draggable). - ov::motion_path(app, &painter, &map, &ectx, l, col, &mut key_hits); + ov::motion_path(app, &painter, &map, &ectx, l, col, &mut key_hits, &mut motion_frames); // Masks and shape paths. if app.ui.viewer.show_masks { let pen = ui.data(|d| d.get_temp::<(LayerId, u64)>(pen_id())); @@ -1153,7 +1154,15 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { Tool::Orbit => egui::CursorIcon::Alias, Tool::PanCamera => egui::CursorIcon::Move, Tool::Dolly => egui::CursorIcon::ResizeVertical, - t if t.is_shape() || t.is_pen() => egui::CursorIcon::Crosshair, + t if t.is_pen() => { + pen_cursor( + &ctx, + hp, + pen_cursor_icon(t, ov::segment_at(&paths, &map, hp, 5.0).is_some() || ov::motion_frame_at(&motion_frames, &key_hits, hp).is_some()), + ); + egui::CursorIcon::None + } + t if t.is_shape() => egui::CursorIcon::Crosshair, _ if app.ui.viewer.roi_draw => egui::CursorIcon::Crosshair, t if t.puppet_kind().is_some() => match super::puppet_tool::hit(&pin_hits, hp) { Some((_, super::puppet_tool::PinPart::Ring)) => egui::CursorIcon::Alias, @@ -1201,7 +1210,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { && ui.input(|i| i.pointer.primary_pressed()) && let Some(pos) = ui.input(|i| i.pointer.press_origin()) { - pen_press(app, ui, &ectx, &map, &paths, pos, mods); + pen_press(app, ui, &ectx, &map, &paths, ov::motion_frame_at(&motion_frames, &key_hits, pos), pos, mods); } // A pen click (press + release without a drag) ends its tangent gesture on release. if ui.input(|i| i.pointer.primary_released()) @@ -1703,19 +1712,35 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { } Tool::Type | Tool::TypeVertical => {} Tool::Pen | Tool::MaskFeather => {} + // On a motion path (After Effects): Add Vertex adds a keyframe where it is clicked, + // Delete Vertex removes the keyframe, Convert Vertex switches it between Linear and + // Auto Bezier spatial interpolation. Tool::PenAdd => { if let Some((l, m, seg, tt)) = ov::segment_at(&paths, &map, pos, 6.0) { let _ = app.session.execute("mask.insertVertex", json!({"layer": l.0, "mask": m, "segment": seg, "t": tt})); + } else if let Some(f) = ov::motion_frame_at(&motion_frames, &key_hits, pos) { + add_motion_key(app, f); } } Tool::PenDelete => { if let Some(h) = vertex_at(pos, false) { let _ = app.session.execute("mask.deleteVertices", json!({"vertices": [{"layer": h.layer.0, "mask": h.mask, "index": h.index}]})); + } else if let Some(k) = ov::motion_key_at(&key_hits, pos) + && select_motion_key(app, &k) + { + let _ = app.session.execute("keys.delete", json!({})); } } Tool::PenConvert => { if let Some(h) = vertex_at(pos, false) { let _ = app.session.execute("mask.convertVertex", json!({"layer": h.layer.0, "mask": h.mask, "index": h.index})); + } else if let Some(k) = ov::motion_key_at(&key_hits, pos) { + // A key showing direction handles becomes Linear; one without, Auto Bezier. + let curved = key_hits.iter().any(|h| h.tangent.is_some() && h.layer == k.layer && h.prop == k.prop && h.time == k.time); + if select_motion_key(app, &k) { + let spatial = if curved { "linear" } else { "autoBezier" }; + let _ = app.session.execute("keys.interpolation", json!({"spatial": spatial})); + } } } Tool::Selection if mods.alt && vertex_at(pos, false).is_some() => { @@ -1907,7 +1932,60 @@ fn draw_rigs(app: &mut EffectcraftApp, painter: &egui::Painter, map: &ViewerMap, /// footage/solid layer (or shape layer, with Tool Creates Mask), a shape path (Shape n group with /// the Tools bar's Fill and Stroke) on the selected shape layer, or a new shape layer when /// nothing is selected. The placed point snaps. -fn pen_press(app: &mut EffectcraftApp, ui: &mut egui::Ui, ectx: &EvalCtx, map: &ViewerMap, paths: &[ov::PathInfo], pos: Pos2, mods: egui::Modifiers) { +/// Add a Position keyframe at a motion path's frame, keeping the path where it is. +fn add_motion_key(app: &mut EffectcraftApp, f: ov::PathFrame) { + if let Err(e) = app.session.execute("prop.addKey", json!({"layer": f.layer.0, "prop": f.prop, "time": f.time.seconds()})) { + app.ui.status = e.to_string(); + } +} + +/// Select only the motion path's keyframe `k` (what the vertex tools act on). +fn select_motion_key(app: &mut EffectcraftApp, k: &ov::KeyHit) -> bool { + app.session.execute("keys.select", json!({"keys": [{"layer": k.layer.0, "prop": k.prop, "time": k.time.seconds()}]})).is_ok() +} + +/// The pointer for a Pen slot tool: the tool's own pen, the Pen showing Add Vertex's over a path +/// it would add a point to (After Effects). +fn pen_cursor_icon(tool: Tool, over_path: bool) -> crate::icons::Icon { + use crate::icons::Icon; + match tool { + Tool::Pen if over_path => Icon::PenAdd, + Tool::PenAdd => Icon::PenAdd, + Tool::PenDelete => Icon::PenDelete, + Tool::PenConvert => Icon::PenConvert, + Tool::MaskFeather => Icon::MaskFeather, + _ => Icon::Pen, + } +} + +/// Paint pen pointer `icon` with its tip at `at`, outlined for any background. +fn pen_cursor(ctx: &egui::Context, at: Pos2, icon: crate::icons::Icon) { + use crate::icons::Icon; + // Where each glyph's tip sits in its 16 × 16 box. + let tip = match icon { + Icon::PenAdd | Icon::PenDelete | Icon::MaskFeather => vec2(6.0, 2.5), + Icon::PenConvert => vec2(8.0, 3.5), + _ => vec2(8.0, 1.8), + }; + let painter = ctx.layer_painter(egui::LayerId::new(egui::Order::Tooltip, egui::Id::new("viewer-pen-cursor"))); + let rect = Rect::from_min_size(at - tip, vec2(16.0, 16.0)); + for d in [vec2(-1.0, 0.0), vec2(1.0, 0.0), vec2(0.0, -1.0), vec2(0.0, 1.0)] { + crate::icons::paint(&painter, rect.translate(d), icon, Color32::BLACK); + } + crate::icons::paint(&painter, rect, icon, Color32::WHITE); +} + +#[allow(clippy::too_many_arguments)] +fn pen_press( + app: &mut EffectcraftApp, + ui: &mut egui::Ui, + ectx: &EvalCtx, + map: &ViewerMap, + paths: &[ov::PathInfo], + motion_frame: Option, + pos: Pos2, + mods: egui::Modifiers, +) { use effectcraft_engine::project::LayerSource; let gid = egui::Id::new("viewer-gesture"); let current: Option<(LayerId, u64)> = ui.data(|d| d.get_temp(pen_id())); @@ -1939,7 +2017,11 @@ fn pen_press(app: &mut EffectcraftApp, ui: &mut egui::Ui, ectx: &EvalCtx, map: & } return; } - // On a visible path's segment the Pen adds a vertex there. + // On a visible path's segment the Pen adds a vertex there; on a motion path, a keyframe. + if let Some(f) = motion_frame { + add_motion_key(app, f); + return; + } if let Some((l, m, seg, t)) = ov::segment_at(paths, map, pos, 5.0) { let _ = app.session.execute("mask.insertVertex", json!({"layer": l.0, "mask": m, "segment": seg, "t": t})); return; diff --git a/crates/ui-egui/src/panels/viewer_overlays.rs b/crates/ui-egui/src/panels/viewer_overlays.rs index 0038f192..54218e9f 100644 --- a/crates/ui-egui/src/panels/viewer_overlays.rs +++ b/crates/ui-egui/src/panels/viewer_overlays.rs @@ -248,6 +248,32 @@ pub(crate) struct KeyHit { pub p2c: Mat3, } +/// A frame on a motion path (one of its dots): where the Pen and Add Vertex tools add a +/// keyframe. +#[derive(Clone, Copy)] +pub(crate) struct PathFrame { + pub layer: LayerId, + pub prop: u64, + /// Layer time of the frame. + pub time: Tick, + pub pos: Pos2, +} + +/// The keyframe dot on a motion path under `pos`. +pub(crate) fn motion_key_at(hits: &[KeyHit], pos: Pos2) -> Option { + hits.iter().rev().find(|h| h.tangent.is_none() && h.pos.distance(pos) < 6.0).copied() +} + +/// The motion-path frame nearest `pos` (within a few points) where a keyframe can be added: not +/// on a keyframe dot already there. +pub(crate) fn motion_frame_at(frames: &[PathFrame], hits: &[KeyHit], pos: Pos2) -> Option { + if motion_key_at(hits, pos).is_some() { + return None; + } + let near = frames.iter().filter(|f| f.pos.distance(pos) < 5.0).min_by(|a, b| a.pos.distance(pos).total_cmp(&b.pos.distance(pos)))?; + (!hits.iter().any(|h| h.tangent.is_none() && h.layer == near.layer && h.prop == near.prop && h.time == near.time)).then_some(*near) +} + /// Parent space → comp matrix of a 2D layer at the context time. pub(crate) fn parent_to_comp(ectx: &EvalCtx, l: &Layer) -> Mat3 { match l.parent.and_then(|p| ectx.comp.layer(p)) { @@ -261,7 +287,17 @@ pub(crate) fn parent_to_comp(ectx: &EvalCtx, l: &Layer) -> Mat3 { /// Draw the position motion path of a selected 2D layer: frame dots along the path, keyframe /// squares (filled when selected) and spatial Bezier tangent handles. Works through parents. -pub(crate) fn motion_path(app: &mut EffectcraftApp, painter: &egui::Painter, map: &ViewerMap, ectx: &EvalCtx, l: &Layer, col: Color32, hits: &mut Vec) { +#[allow(clippy::too_many_arguments)] +pub(crate) fn motion_path( + app: &mut EffectcraftApp, + painter: &egui::Painter, + map: &ViewerMap, + ectx: &EvalCtx, + l: &Layer, + col: Color32, + hits: &mut Vec, + frames: &mut Vec, +) { let Some(pos) = l.transform().and_then(|tr| tr.get("position")) else { return }; if pos.keys.len() < 2 || l.is_3d() { return; @@ -278,7 +314,9 @@ pub(crate) fn motion_path(app: &mut EffectcraftApp, painter: &egui::Painter, map let mut pts = vec![]; let mut lt = lt0; while lt <= lt1 && pts.len() < 20_000 { - pts.push(to_scr(pos.value_at(lt).as_vec3())); + let q = to_scr(pos.value_at(lt).as_vec3()); + frames.push(PathFrame { layer: l.id, prop: pos.uid, time: lt, pos: q }); + pts.push(q); lt += fd; } pts.push(to_scr(pos.value_at(lt1).as_vec3())); diff --git a/crates/ui-egui/src/widgets.rs b/crates/ui-egui/src/widgets.rs index 364a2472..b4fa41f3 100644 --- a/crates/ui-egui/src/widgets.rs +++ b/crates/ui-egui/src/widgets.rs @@ -165,12 +165,24 @@ pub fn drag_value(value: egui::DragValue<'_>) -> impl egui::Widget { } /// Keep the keyboard on an inline editor that is open until it loses the focus. The frame it -/// opens (it has no focus yet) its whole `text` is selected, so typing replaces the value. -/// (Only then is the focus requested: a request resets what keys the field keeps for itself, -/// such as Tab with [`egui::TextEdit::lock_focus`].) +/// opens its whole `text` is selected, so typing replaces the value; after that the caret is the +/// user's (arrow keys move it), even if the field is without the focus for a frame (#488). +/// (The focus is only requested while the field doesn't have it: a request resets what keys the +/// field keeps for itself, such as Tab with [`egui::TextEdit::lock_focus`].) pub fn keep_focus(ctx: &egui::Context, field: &Response, text: &str) { + // The editor is open in consecutive passes; a gap means it was closed and opened again. + let shown = field.id.with("keep-focus-pass"); + let pass = ctx.cumulative_pass_nr(); + let previous = ctx.data_mut(|d| { + let previous = d.get_temp::(shown); + d.insert_temp(shown, pass); + previous + }); + let opening = previous.is_none_or(|p| p.saturating_add(1) < pass); if !ctx.memory(|m| m.has_focus(field.id)) { - select_all(ctx, field.id, text); + if opening { + select_all(ctx, field.id, text); + } field.request_focus(); } } @@ -592,6 +604,47 @@ pub(crate) fn constrained(c: &[f64], d: usize, v: f64) -> Vec { mod tests { use super::*; + /// #488: an inline editor selects its whole text only as it opens. Should it be without the + /// keyboard for a frame later (an input method, another handler), it takes the keyboard back + /// with the caret where the user put it instead of selecting everything again, which made + /// the arrows seem dead and Backspace erase the whole name. + #[test] + fn inline_editors_select_all_only_as_they_open() { + let ctx = egui::Context::default(); + let mut text = String::from("Comp 1"); + let mut id = egui::Id::NULL; + let frame = |ctx: &egui::Context, text: &mut String, id: &mut egui::Id| { + let mut out = ctx.run_ui(egui::RawInput::default(), |ui| { + let r = ui.add(egui::TextEdit::singleline(text)); + keep_focus(ui.ctx(), &r, text); + *id = r.id; + }); + out.textures_delta.clear(); + }; + let range = |ctx: &egui::Context, id: egui::Id| egui::TextEdit::load_state(ctx, id).and_then(|s| s.cursor.char_range()); + let ix = |n: usize| egui::text::CCursor::new(n).index; + frame(&ctx, &mut text, &mut id); + frame(&ctx, &mut text, &mut id); + let all = range(&ctx, id).unwrap(); + assert_eq!((all.primary.index.min(all.secondary.index), all.primary.index.max(all.secondary.index)), (ix(0), ix(6)), "opened with all selected"); + // The caret after "Co", then a frame without the keyboard. + let mut state = egui::TextEdit::load_state(&ctx, id).unwrap(); + state.cursor.set_char_range(Some(egui::text::CCursorRange::one(egui::text::CCursor::new(2)))); + state.store(&ctx, id); + ctx.memory_mut(|m| m.surrender_focus(id)); + frame(&ctx, &mut text, &mut id); + frame(&ctx, &mut text, &mut id); + assert!(ctx.memory(|m| m.has_focus(id)), "the editor keeps the keyboard"); + let r = range(&ctx, id).unwrap(); + assert_eq!((r.primary.index, r.secondary.index), (ix(2), ix(2)), "the caret stays where it was"); + // Closed for a frame and opened again: all selected again. + ctx.run_ui(egui::RawInput::default(), |_| {}).textures_delta.clear(); + frame(&ctx, &mut text, &mut id); + frame(&ctx, &mut text, &mut id); + let r = range(&ctx, id).unwrap(); + assert_eq!((r.primary.index.min(r.secondary.index), r.primary.index.max(r.secondary.index)), (ix(0), ix(6))); + } + /// #581: a field's text edit fills it, so its text and hint sit on the field's middle line /// (it was a row at the top of the field, its text high). #[test] diff --git a/crates/ui-egui/tests/ui_gpu.rs b/crates/ui-egui/tests/ui_gpu.rs index 92d18a44..58536f16 100644 --- a/crates/ui-egui/tests/ui_gpu.rs +++ b/crates/ui-egui/tests/ui_gpu.rs @@ -86,3 +86,30 @@ fn viewer_draws_gpu_frames_that_match_the_cpu() { cpu_shot.save(format!("{d}/ui_cpu_viewer.png")).unwrap(); } } + +/// Once the graphics device is lost, eframe's renderer cannot stage buffers and egui-wgpu +/// panicked on the main thread drawing the next frame (#591). The app now draws nothing, +/// auto-saves unsaved changes and closes its window instead. +#[test] +fn a_lost_device_draws_nothing_autosaves_and_closes() { + let dir = std::env::temp_dir().join(format!("ec-device-lost-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let mut s = Session { autosave_folder_override: Some(dir.clone()), ..Default::default() }; + s.execute("comp.new", json!({"name": "Unsaved", "width": 64, "height": 64, "duration": 1})).unwrap(); + assert!(s.is_dirty()); + let mut h = Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_eframe(|_| EffectcraftApp::new(s)); + h.run_steps(3); + assert!(!h.output().shapes.is_empty(), "the UI draws before the loss"); + let bridge = effectcraft_ui_egui::gpu_failure::GpuFailureBridge::new(&h.ctx); + h.state_mut().set_gpu_failure_bridge(bridge.clone()); + bridge.report("GPU device lost (Unknown): Device is lost", true); + h.step(); + let closing = h.output().viewport_output.get(&egui::ViewportId::ROOT).is_some_and(|v| v.commands.contains(&egui::ViewportCommand::Close)); + assert!(closing, "the window closes"); + assert!(h.output().shapes.is_empty(), "nothing for the lost renderer to stage"); + h.step(); + assert!(h.output().shapes.is_empty()); + let saved = h.state().session.autosave.last_path.clone().expect("unsaved changes auto-saved"); + assert!(std::path::Path::new(&saved).is_file(), "{saved}"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/ui-egui/tests/ui_inline_edit.rs b/crates/ui-egui/tests/ui_inline_edit.rs index fc5c522e..4119d79b 100644 --- a/crates/ui-egui/tests/ui_inline_edit.rs +++ b/crates/ui-egui/tests/ui_inline_edit.rs @@ -474,3 +474,44 @@ fn a_layout_switch_keeps_the_name_field_and_arabic_names_are_kept() { assert_eq!(focused.trim(), "File", "Alt focused the menu bar"); } } + +/// #488: once a rename field is open, the arrow keys move its caret (the whole name is selected +/// only as it opens), so part of a name can be edited. +#[test] +fn arrow_keys_move_the_caret_in_rename_fields() { + let (mut h, comp) = harness(); + let name = |h: &Harness<'_, EffectcraftApp>| h.state().session.project.items.values().find(|i| i.id.0 == comp).unwrap().name.clone(); + let p = center(&h, &format!("project.item.{comp}.name")); + click(&mut h, p, 1); + key(&mut h, Key::Enter); + // The arrows keep the field (Up / Down too); in "Main", two to the left of its end. + key(&mut h, Key::ArrowRight); + key(&mut h, Key::ArrowLeft); + key(&mut h, Key::ArrowUp); + key(&mut h, Key::ArrowDown); + key(&mut h, Key::End); + key(&mut h, Key::ArrowLeft); + key(&mut h, Key::ArrowLeft); + type_text(&mut h, "X"); + key(&mut h, Key::Backspace); + key(&mut h, Key::Backspace); + type_text(&mut h, "Y"); + key(&mut h, Key::Enter); + assert_eq!(name(&h), "MYin"); + // The Timeline's layer rename. + let layer = h.state_mut().session.execute("layer.newSolid", json!({"name": "Plate", "color": "#406080"})).unwrap()["layer"].as_u64().unwrap(); + h.state_mut().session.execute("layer.select", json!({"layers": [layer]})).unwrap(); + h.run_steps(3); + let p = center(&h, &format!("timeline.layer.{layer}.row")); + click(&mut h, p, 1); + key(&mut h, Key::Enter); + key(&mut h, Key::ArrowRight); + key(&mut h, Key::ArrowUp); + key(&mut h, Key::End); + key(&mut h, Key::ArrowLeft); + key(&mut h, Key::ArrowLeft); + type_text(&mut h, "-"); + key(&mut h, Key::Enter); + let layer_name = h.state().session.active_comp().unwrap().layer(effectcraft_engine::project::LayerId(layer)).unwrap().name.clone(); + assert_eq!(layer_name, "Pla-te"); +} diff --git a/crates/ui-egui/tests/ui_viewer.rs b/crates/ui-egui/tests/ui_viewer.rs index ee13761b..bdb4189d 100644 --- a/crates/ui-egui/tests/ui_viewer.rs +++ b/crates/ui-egui/tests/ui_viewer.rs @@ -1901,3 +1901,50 @@ fn roto_brush_ctrl_drag_resizes_the_brush() { assert_eq!(s.len(), 1); assert!((s[0].radius - dia / 2.0).abs() < 1e-6, "{} vs {dia}", s[0].radius); } + +/// #553: the Pen slot's tools work on a motion path as in After Effects: Add Vertex (and the +/// Pen) add a keyframe where the path is clicked, Convert Vertex switches a keyframe between +/// Linear and Auto Bezier, Delete Vertex removes it; the pointer is the tool's pen. +#[test] +fn vertex_tools_edit_motion_path_keyframes() { + let mut h = harness(); + let box_id: LayerId = h.state().session.active_comp().unwrap().layers[0].id; + let s = &mut h.state_mut().session; + s.execute("layer.select", json!({"layers": [box_id.0]})).unwrap(); + s.execute("prop.addKey", json!({"layer": box_id.0, "path": "transform/position", "time": 0.0, "value": [100, 100, 0]})).unwrap(); + s.execute("prop.addKey", json!({"layer": box_id.0, "path": "transform/position", "time": 2.0, "value": [500, 100, 0]})).unwrap(); + s.execute("view.snapping", json!({"value": false})).unwrap(); + s.set_time(effectcraft_engine::time::Tick::from_seconds_f64(3.0)); + let keys = |h: &Harness<'_, EffectcraftApp>| { + h.state().session.active_comp().unwrap().layer(box_id).unwrap().props.prop("transform/position").unwrap().keys.clone() + }; + h.state_mut().ui.tool = Tool::PenAdd; + h.run_steps(3); + let mid = screen(&h, [300.0, 100.0]); + h.input_mut().events.push(Event::PointerMoved(mid)); + h.step(); + assert_eq!(h.output().platform_output.cursor_icon, egui::CursorIcon::None, "the pen is painted in place of the system pointer"); + click(&mut h, mid); + let k = keys(&h); + assert_eq!(k.len(), 3, "Add Vertex adds a keyframe on the path"); + let v = k[1].value.as_vec3(); + assert!((v[0] - 300.0).abs() < 10.0 && (v[1] - 100.0).abs() < 1.0, "{v:?}"); + assert!((k[1].time.seconds() - 1.0).abs() < 0.1, "{:?}", k[1].time); + // Convert Vertex: the new key's handles go (Linear), then come back (Auto Bezier). + h.state_mut().ui.tool = Tool::PenConvert; + let at = screen(&h, [v[0] as f32, v[1] as f32]); + click(&mut h, at); + let k = keys(&h); + assert!(!k[1].spatial_auto && k[1].spatial_in == [0.0; 3] && k[1].spatial_out == [0.0; 3], "Linear"); + click(&mut h, at); + assert!(keys(&h)[1].spatial_auto, "Auto Bezier"); + // Delete Vertex removes it. + h.state_mut().ui.tool = Tool::PenDelete; + click(&mut h, at); + assert_eq!(keys(&h).len(), 2); + // The Pen adds one too. + h.state_mut().ui.tool = Tool::Pen; + let quarter = screen(&h, [200.0, 100.0]); + click(&mut h, quarter); + assert_eq!(keys(&h).len(), 3); +} diff --git a/docs/mcp.md b/docs/mcp.md index 0d7f83e9..1104927a 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -3,6 +3,19 @@ Run `effectcraft-cli mcp` for a headless session, or `effectcraft-cli mcp --bridge 9877` to use the desktop app's control channel. Existing ports are unchanged. +## Confining file access + +`--automation-read-root DIR` and `--automation-write-root DIR` (each repeatable, as in +PhotoCraft) confine what the agent can reach: files are read only under a read or write root and +written only under a write root; with no write root nothing is written. Every engine command +(`execute_command`, `command_run`, each `batch` step) is checked before it runs: its file +parameters must be absolute paths inside the roots (symbolic links resolved, no `..`), and so must +any other absolute path in its parameters. Save, Increment and Save and rendering the Render +Queue are checked against the files they write (the open project's, the queued outputs). Scripts +(`run_script`, `file.runScript`…), Execute File, Reveal in Finder, `$N` references to file paths +in a batch, and the bridge's UI-driving tools (`ui_click`, `ui_key`, `ui_type`, `ui_drag`, +`ui_set`, `script_ui`, `control`) are refused while confined. Without the flags nothing changes. + The server provides the same core conventions as FilmCraft #28: | Tool | Arguments | Result | diff --git a/packaging/windows/effectcraft.wxs b/packaging/windows/effectcraft.wxs index 6757c446..00b0c5d6 100644 --- a/packaging/windows/effectcraft.wxs +++ b/packaging/windows/effectcraft.wxs @@ -4,7 +4,8 @@ wix build effectcraft.wxs installer-ui.wxs -arch x64 -d Version=1.2.3 -d BinDir=... -d IconPath=... -o out.msi - Per-machine install into Program Files (or a folder the user picks) and a Start Menu shortcut. + Per-machine install into Program Files (or a folder the user picks), a Start Menu shortcut and, + unless DESKTOPSHORTCUT=0 (the wizard's "Create a desktop shortcut" box), a desktop shortcut. --> + + + + + + + + @@ -70,11 +82,20 @@ + + + + + + + + diff --git a/packaging/windows/installer-ui.wxs b/packaging/windows/installer-ui.wxs index 14989932..16ab58d7 100644 --- a/packaging/windows/installer-ui.wxs +++ b/packaging/windows/installer-ui.wxs @@ -3,7 +3,7 @@ EffectCraft installer wizard: original dialogs built from Windows Installer's own controls and events (no WixUI library), included by effectcraft.wxs with . - fresh install: DestDlg (folder, Change... opens BrowseDlg) -> ReadyDlg -> ProgressDlg -> ExitDlg + fresh install: DestDlg (folder, Change... opens BrowseDlg, desktop shortcut) -> ReadyDlg -> ProgressDlg -> ExitDlg upgrade: ReadyDlg (the installed copy's folder, not editable) -> ProgressDlg -> ExitDlg already there: MaintDlg (Repair or Remove) -> ProgressDlg -> ExitDlg @@ -29,6 +29,7 @@ + diff --git a/packaging/windows/test-msi.ps1 b/packaging/windows/test-msi.ps1 index 7cdaef86..5868c77b 100644 --- a/packaging/windows/test-msi.ps1 +++ b/packaging/windows/test-msi.ps1 @@ -10,8 +10,10 @@ installed effectcraft.exe, as there is no Path value yet), 3. upgrades to 0.0.3 the same way (found from the Path value 0.0.2 wrote), 4. repairs a damaged (unversioned) CLI there with the wizard's Repair mode, - 5. uninstalls: files and the App Paths registration go, - 6. installs 0.0.3 with no folder: it must land in Program Files, then uninstalls. + 5. uninstalls: files, the App Paths registration and the desktop shortcut go, + 6. installs 0.0.3 with no folder and DESKTOPSHORTCUT=0: it must land in Program Files with no + desktop shortcut, then uninstalls. + The installs before step 6 take the default desktop shortcut, which upgrades keep. The wizard itself needs a person (or the snapshot steps in the PR); this covers what msiexec /qn and upgrades do with the folder. Needs an elevated shell and WiX v5 (`wix`) on PATH. Run by .github/workflows/packaging-lint.yml. @@ -24,6 +26,8 @@ $ErrorActionPreference = 'Stop' trap { Write-Output "FAILED: $_"; exit 1 } $Root = (Resolve-Path (Join-Path $PSScriptRoot '..\..')).Path $AppPaths = 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\effectcraft.exe' +# A per-machine install's desktop is the Public one. +$DesktopLink = Join-Path ([Environment]::GetFolderPath('CommonDesktopDirectory')) 'EffectCraft.lnk' function Invoke-Native([string] $What, [scriptblock] $Block) { Write-Output "==> $What" @@ -50,6 +54,10 @@ function Assert-InstalledIn([string] $Dir, [switch] $NoPathValue) { } Write-Output "ok installed in $Dir" } +function Assert-DesktopShortcut([bool] $Present) { + if ((Test-Path -LiteralPath $DesktopLink) -ne $Present) { throw "desktop shortcut present: $(-not $Present), expected $Present" } + Write-Output "ok desktop shortcut present: $Present" +} Remove-Item -Recurse -Force $Work -ErrorAction SilentlyContinue $Bin = Join-Path $Work 'bin' @@ -87,6 +95,7 @@ if (Test-Path -LiteralPath $AppPaths) { throw 'EffectCraft is already installed Invoke-Msiexec 'install the legacy package to a custom folder' @('/i', "`"$($Msi['0.0.1'])`"", "INSTALLFOLDER=`"$Custom`"") Assert-InstalledIn $Custom -NoPathValue +Assert-DesktopShortcut $true if (Test-Path -LiteralPath $Default) { throw "a custom install also wrote $Default" } Invoke-Msiexec 'upgrade the legacy install without a folder' @('/i', "`"$($Msi['0.0.2'])`"") @@ -95,6 +104,7 @@ if (Test-Path -LiteralPath $Default) { throw "the upgrade moved the install to $ Invoke-Msiexec 'upgrade again without a folder' @('/i', "`"$($Msi['0.0.3'])`"") Assert-InstalledIn $Custom +Assert-DesktopShortcut $true # Read single values: Get-ItemProperty throws on some machines' malformed Uninstall entries. $arp = @(Get-ChildItem 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall' | ForEach-Object { if ($_.GetValue('DisplayName') -eq 'EffectCraft') { $_.GetValue('DisplayVersion') } }) @@ -117,9 +127,11 @@ Write-Output "ok repair ($mode) restored effectcraft-cli.exe" Invoke-Msiexec 'uninstall the custom install' @('/x', "`"$($Msi['0.0.3'])`"") if (Test-Path -LiteralPath (Join-Path $Custom 'effectcraft.exe')) { throw 'uninstall left effectcraft.exe behind' } if (Test-Path -LiteralPath $AppPaths) { throw 'uninstall left the App Paths registration behind' } +Assert-DesktopShortcut $false -Invoke-Msiexec 'install to the default folder' @('/i', "`"$($Msi['0.0.3'])`"") +Invoke-Msiexec 'install to the default folder without a desktop shortcut' @('/i', "`"$($Msi['0.0.3'])`"", 'DESKTOPSHORTCUT=0') Assert-InstalledIn $Default +Assert-DesktopShortcut $false Invoke-Msiexec 'uninstall the default install' @('/x', "`"$($Msi['0.0.3'])`"") if (Test-Path -LiteralPath (Join-Path $Default 'effectcraft.exe')) { throw 'uninstall left effectcraft.exe behind' } Write-Output 'MSI folder handling ok'