From be926adb927a25c3b599320df4b62c06183c136f Mon Sep 17 00:00:00 2001 From: bflatastic Date: Fri, 9 Oct 2026 16:45:46 +0700 Subject: [PATCH] Render Queue: output files end in their format's extension once (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renders on macOS came out as `Comp 1.mov.ecproj`, and an image sequence's every frame as `Comp 1_00001.png.ecproj`; another output was `Comp 2.mov.mov`. Cause: the save dialogs for anything but a project went through the project dialog hook, which filters to `.ecproj`; macOS's save panel appends the allowed extension to any other name. 0.4.0's Render Queue Choose File… used it (#192 moved Output To to its own dialog since), and Save Frame As, Save Current Preview, Export Lottie, Save a Copy As XML, Collect Files, Watch Folder and the Premiere export form's Browse still did. Nothing then fixed the name: Output To kept `.ecproj` (no format claims it), and changing the format replaced only the last extension (`Comp 2.mov.ecproj` → `Comp 2.mov.mov`). `[compName]` of a comp named `Comp 2.mov` also doubled it. Fix: `Hooks::save_dialog` asks the dialog for the default name's own extension (`pick_save_file`), the project dialog only for projects or on hosts without another (the web); the desktop dialog has no filter for a name without an extension (folders). `OutputFormat::with_extension` makes an output path end in the format's extension exactly once (project extensions and repeats of the format's dropped, a missing one added; `logo.png.mov` kept), used by Output To, format changes and `resolve_output`, so existing projects and templates render to the right name too. Output To picks the file type from the extension before a stray `.ecproj`. Test: output_names_end_in_the_format_extension_once (project), output_to_ends_in_the_format_extension_once (engine) and save_dialogs_offer_the_file_extension (ui); the engine and ui tests fail with the fix reverted. Co-Authored-By: Claude Opus 5.5 --- apps/effectcraft/src/main.rs | 3 +- crates/engine/src/commands/render_queue.rs | 7 +- crates/engine/src/render_queue.rs | 6 +- crates/engine/src/tests_codec_options.rs | 28 ++++++++ crates/project/src/render_queue.rs | 84 +++++++++++++++++++++- crates/ui-egui/src/lib.rs | 22 +++++- crates/ui-egui/src/menus.rs | 45 +++++++++++- crates/ui-egui/src/panels/forms.rs | 10 +-- crates/ui-egui/src/panels/render_queue.rs | 16 ++--- 9 files changed, 191 insertions(+), 30 deletions(-) diff --git a/apps/effectcraft/src/main.rs b/apps/effectcraft/src/main.rs index 1d62289c..113e38c5 100644 --- a/apps/effectcraft/src/main.rs +++ b/apps/effectcraft/src/main.rs @@ -182,7 +182,8 @@ fn main() -> eframe::Result { app.hooks.pick_save_file = Some(Box::new(|name: &str, ext: &str| { // A default with a folder that exists opens the dialog there. let path = std::path::Path::new(name); - let mut d = rfd::FileDialog::new().add_filter(ext, &[ext]); + // (No filter for a name without an extension, such as a folder to create.) + let mut d = if ext.is_empty() { rfd::FileDialog::new() } else { rfd::FileDialog::new().add_filter(ext, &[ext]) }; if let Some(dir) = path.parent().filter(|d| d.is_dir()) { d = d.set_directory(dir); } diff --git a/crates/engine/src/commands/render_queue.rs b/crates/engine/src/commands/render_queue.rs index 8abc855c..ab9a785a 100644 --- a/crates/engine/src/commands/render_queue.rs +++ b/crates/engine/src/commands/render_queue.rs @@ -8,7 +8,7 @@ use effectcraft_project::render_queue::{ AlphaMode, AudioFormat, AudioOutput, Channels, ColorDepth, CurrentOrOff, DEFAULT_SEQUENCE_TEMPLATE, DEFAULT_TEMPLATE, EffectsMode, FieldRender, OutputFormat, OutputModule, PostRenderAction, ProResProfile, Pulldown, RESIZE_PRESETS, RenderLog, RenderQuality, RenderQueueItem, RenderSettings, - RenderStatus, ResizeQuality, SwitchOverride, TimeSpan, post_render_parse, + RenderStatus, ResizeQuality, SwitchOverride, TimeSpan, post_render_parse, without_project_extension, }; use effectcraft_project::render_templates::{RenderTemplates, TemplateKind, TemplateSlot}; use effectcraft_time::{FrameRate, Tick}; @@ -364,7 +364,8 @@ fn apply_output(om: &mut OutputModule, templates: &RenderTemplates, roi: Option< if let Some(l) = b_p(p, "includeProjectLink") { om.include_project_link = l; } - if let Some(o) = str_p(p, "output").or(str_p(p, "path")) { + // (Without the `.ecproj` a save dialog filtered to projects appended, #293.) + if let Some(o) = str_p(p, "output").or(str_p(p, "path")).map(without_project_extension) { om.output = o.to_string(); // Only an extension the format doesn't write changes it: `.mp4` is H.264's, HEVC's and AV1's. if let Some(f) = OutputFormat::from_path(o).filter(|f| f.extension() != om.format.extension() && !o.contains("[fileExtension]")) { @@ -379,6 +380,8 @@ fn apply_output(om: &mut OutputModule, templates: &RenderTemplates, roi: Option< om.output = keep; } } + // The format's extension exactly once (`Comp 2.mov.mov` → `Comp 2.mov`). + om.output = om.format.with_extension(&om.output); } if *om != before { // Output To alone doesn't make the module custom. diff --git a/crates/engine/src/render_queue.rs b/crates/engine/src/render_queue.rs index 0ea4aac9..e1efe9c4 100644 --- a/crates/engine/src/render_queue.rs +++ b/crates/engine/src/render_queue.rs @@ -321,7 +321,8 @@ impl Session { .unwrap_or_else(|| "Untitled Project".into()) } - /// The output path of a queue item: template tokens expanded, relative paths resolved against + /// The output path of a queue item: template tokens expanded, ending in the format's + /// extension exactly once ([`OutputFormat::with_extension`]), relative paths resolved against /// the project's folder (or the working directory). `None` when the comp is gone or the /// output is empty ("Needs Output"). pub fn resolve_output(&self, item: &RenderQueueItem) -> Option { @@ -338,7 +339,8 @@ impl Session { let pname = self.project_name(); let vars = TemplateVars { comp_name: &name, project_name: &pname, width: w, height: h, frame_rate: rate.as_f64(), start_frame: f0, end_frame: f0 + n - 1 }; - let p = expand_template(tpl, item.output.format, &vars); + // The format's extension exactly once, whatever the name or a save dialog added (#293). + let p = item.output.format.with_extension(&expand_template(tpl, item.output.format, &vars)); let path = std::path::Path::new(&p); if path.is_absolute() { return Some(p); diff --git a/crates/engine/src/tests_codec_options.rs b/crates/engine/src/tests_codec_options.rs index 8adabba5..af724932 100644 --- a/crates/engine/src/tests_codec_options.rs +++ b/crates/engine/src/tests_codec_options.rs @@ -110,6 +110,34 @@ fn output_to_keeps_a_format_that_writes_the_extension() { assert_eq!((&r["output"]["format"], &r["outputModuleSummary"]), (&json!("Hevc"), &a["outputModuleSummary"]), "{r}"); } +/// Output To ends in the format's extension exactly once (#293). A name from a save dialog +/// filtered to projects (`Comp 1.mov.ecproj`; for a PNG sequence every frame got `.png.ecproj`) +/// loses the `.ecproj` and picks the file type from the extension before it; a comp named +/// `Comp 2.mov` renders to `Comp 2.mov`, not `Comp 2.mov.mov`. +#[test] +fn output_to_ends_in_the_format_extension_once() { + let mut s = session(); + let cases = [ + ("h264", "/out/Comp 1.mov.ecproj", OutputFormat::ProRes, "/out/Comp 1.mov"), + ("png", "/out/Comp 1_[#####].png.ecproj", OutputFormat::PngSequence, "/out/Comp 1_[#####].png"), + ("prores", "/out/Comp 2.mov.mov", OutputFormat::ProRes, "/out/Comp 2.mov"), + ("prores", "/out/render", OutputFormat::ProRes, "/out/render.mov"), + ]; + for (format, out, want, path) in cases { + let a = s.execute("renderQueue.add", json!({"format": format})).unwrap(); + let r = s.execute("renderQueue.setOutput", json!({"item": a["item"], "path": out})).unwrap(); + let om = &s.project.render_queue.last().unwrap().output; + assert_eq!((om.format, om.output.as_str()), (want, path), "{format} → {out}"); + assert!(r["outputPath"].as_str().unwrap().ends_with(&path[5..]), "{r}"); + } + s.execute("comp.new", json!({"name": "Comp 2.mov", "width": 64, "height": 36, "frameRate": 30, "duration": 1})).unwrap(); + for (format, name) in [("prores", "Comp 2.mov"), ("h264", "Comp 2.mov.mp4"), ("png", "Comp 2.mov_[#####].png")] { + let a = s.execute("renderQueue.add", json!({"format": format})).unwrap(); + let path = a["outputPath"].as_str().unwrap(); + assert_eq!(std::path::Path::new(path).file_name().unwrap().to_string_lossy(), name, "{format}: {path}"); + } +} + /// An explicit RGB + Alpha request with the AV1 WebM codec (which has no alpha) is refused, not /// silently rendered opaque (#166). Without `channels`, AV1 WebM renders RGB as before. #[test] diff --git a/crates/project/src/render_queue.rs b/crates/project/src/render_queue.rs index 62ee10ff..d74e183d 100644 --- a/crates/project/src/render_queue.rs +++ b/crates/project/src/render_queue.rs @@ -622,6 +622,51 @@ impl OutputFormat { let ext = std::path::Path::new(path).extension()?.to_string_lossy().to_ascii_lowercase(); OutputFormat::from_name(&ext) } + /// An output `path` without its extensions, for this format to add its own: the project + /// extensions a save dialog filtered to projects appended (`Comp 1.png.ecproj`, #293), one + /// output format's extension (the one being replaced), and any more of this format's + /// (`Comp 2.mov.mov`, from a format change or a comp named `Comp 2.mov`). Another extension + /// stays part of the name (`logo.png.mov`). + pub fn output_stem(self, path: &str) -> &str { + let stem = without_project_extension(path); + let mut stem = strip_extension(stem, |e| OutputFormat::from_name(e).is_some()).unwrap_or(stem); + while let Some(s) = strip_extension(stem, |e| e.eq_ignore_ascii_case(self.extension())) { + stem = s; + } + stem + } + /// `path` ending in this format's extension exactly once (see [`OutputFormat::output_stem`]); + /// a path that already does is kept as it is. Templates still to expand `[fileExtension]` are + /// kept too. + pub fn with_extension(self, path: &str) -> String { + if path.contains("[fileExtension]") { + return path.to_string(); + } + let fixed = format!("{}.{}", self.output_stem(path), self.extension()); + if fixed.eq_ignore_ascii_case(path) { path.to_string() } else { fixed } + } +} + +/// Project file extensions: never a render's output. +const PROJECT_EXTENSIONS: [&str; 2] = ["ecproj", "ecprojx"]; + +/// An output `path` without the project extensions a save dialog filtered to projects appended +/// to it (`Comp 1.mov.ecproj` → `Comp 1.mov`, #293). +pub fn without_project_extension(path: &str) -> &str { + let mut p = path; + while let Some(s) = strip_extension(p, |e| PROJECT_EXTENSIONS.iter().any(|x| e.eq_ignore_ascii_case(x))) { + p = s; + } + p +} + +/// `path` without its last extension when `strip` accepts that extension. +fn strip_extension(path: &str, strip: impl Fn(&str) -> bool) -> Option<&str> { + let ext = std::path::Path::new(path).extension()?.to_str()?; + if !strip(ext) { + return None; + } + path.strip_suffix(ext)?.strip_suffix('.') } /// Output Module ▸ Channels. @@ -1130,8 +1175,7 @@ impl OutputModule { } return; } - let p = std::path::Path::new(&self.output); - let mut stem = p.with_extension("").to_string_lossy().to_string(); + let mut stem = format.output_stem(&self.output).to_string(); if format.is_sequence() && !stem.contains('#') { stem.push_str("_[#####]"); } else if !format.is_sequence() { @@ -1451,6 +1495,42 @@ mod tests { assert_eq!(ProResProfile::from_name("hq"), Some(ProResProfile::Hq)); } + /// Output names end in the format's extension exactly once (#293): a save dialog filtered to + /// projects appended `.ecproj` (`Comp 1.mov.ecproj`, `Comp 1_00001.png.ecproj` for every + /// frame), and a format change or a comp named `Comp 2.mov` doubled it (`Comp 2.mov.mov`). + #[test] + fn output_names_end_in_the_format_extension_once() { + use OutputFormat::*; + for (path, format, want) in [ + ("/out/Comp 1.mov.ecproj", ProRes, "/out/Comp 1.mov"), + ("/out/Comp 1_[#####].png.ecproj", PngSequence, "/out/Comp 1_[#####].png"), + ("/out/Comp 1.mov.ecprojx.ecproj", ProRes, "/out/Comp 1.mov"), + ("/out/Comp 2.mov.mov", ProRes, "/out/Comp 2.mov"), + ("/out/Comp 2.mov.mov.mov", ProRes, "/out/Comp 2.mov"), + ("/out/render", ProRes, "/out/render.mov"), + ("/out/frames_[#####]", PngSequence, "/out/frames_[#####].png"), + ("/out/logo.png.mov", ProRes, "/out/logo.png.mov"), + ("/out/v1.2", H264, "/out/v1.2.mp4"), + ("/my.folder/Clip.MOV", ProRes, "/my.folder/Clip.MOV"), + ("/out/x.mp4", Hevc, "/out/x.mp4"), + ("[compName].[fileExtension]", ProRes, "[compName].[fileExtension]"), + ] { + assert_eq!(format.with_extension(path), want, "{path} as {format:?}"); + } + assert_eq!(without_project_extension("C:/out/Comp 1.mov.ecproj"), "C:/out/Comp 1.mov"); + assert_eq!(without_project_extension("Comp 1.mov"), "Comp 1.mov"); + // Changing the format replaces one extension and drops the project one. + let mut m = OutputModule { output: "/out/Comp 1.mov.ecproj".into(), ..OutputModule::for_format(ProRes) }; + m.set_format(H264); + assert_eq!(m.output, "/out/Comp 1.mp4"); + m.output = "/out/Comp 2.mov.mov".into(); + m.set_format(ProRes); + assert_eq!(m.output, "/out/Comp 2.mov"); + m.output = "/out/Comp 2.mov.ecproj".into(); + m.set_format(PngSequence); + assert_eq!(m.output, "/out/Comp 2_[#####].png"); + } + #[test] fn pulldown_cadence() { assert_eq!(Pulldown::Wwssw.pattern(), "WWSSW"); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 691a3385..2d659543 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -103,8 +103,8 @@ pub struct Hooks { pub audio_devices: Option Vec>>, /// Picks a folder (Settings paths). pub pick_folder: Option Option>>, - /// Save dialog for other file kinds: (default name or path, extension). A default path opens - /// the dialog in its folder. + /// Save dialog for other file kinds: (default name or path, extension; empty: no filter). A + /// default path opens the dialog in its folder. pub pick_save_file: Option Option>>, /// The system clipboard's text (native menu Edit ▸ Paste into a text field). pub clipboard_text: Option Option>>, @@ -113,6 +113,24 @@ pub struct Hooks { pub app_action: Option bool>>, } +impl Hooks { + /// The host's save dialog for `default` (a file name or path), offering its extension: the + /// project dialog for project files, else [`Hooks::pick_save_file`]. The project dialog is + /// the fallback for hosts without that (the web, whose dialog only names the file): on macOS + /// a dialog filtered to projects appends `.ecproj` to any other name (#293). `None` when the + /// host has no save dialog; `Some(None)` when it was cancelled. + pub fn save_dialog(&self, default: &str) -> Option> { + let path = std::path::Path::new(default); + let ext = path.extension().map(|e| e.to_string_lossy().to_string()).unwrap_or_default(); + match (&self.pick_save_file, &self.pick_save) { + (Some(pick), _) if !ext.eq_ignore_ascii_case("ecproj") => Some(pick(default, &ext)), + (_, Some(pick)) => Some(pick(&path.file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default())), + (Some(pick), None) => Some(pick(default, &ext)), + (None, None) => None, + } + } +} + #[derive(Default)] pub struct Playback { pub playing: bool, diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 388658aa..4132f50e 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1152,8 +1152,8 @@ fn file_dialog(app: &mut EffectcraftApp, id: &str, params: &Value) -> Option { - let Some(f) = app.hooks.pick_save.as_ref() else { return Some(Err("no file dialog available (pass `path`)".into())) }; - f(default).map(|s| json!(s)) + let Some(picked) = app.hooks.save_dialog(default) else { return Some(Err("no file dialog available (pass `path`)".into())) }; + picked.map(|s| json!(s)) } }; let Some(v) = picked else { return Some(Ok(Value::Null)) }; @@ -1709,6 +1709,47 @@ mod tests { assert!(!labels.contains(&"Yellow".to_string())); } + /// Saving anything but a project asks the save dialog for that file's extension (#293): the + /// project dialog, filtered to `.ecproj`, made macOS append `.ecproj` (`Frame.png.ecproj`). + /// Hosts with only the project dialog (the web) still get it, given the file name. + #[test] + fn save_dialogs_offer_the_file_extension() { + use std::cell::RefCell; + use std::rc::Rc; + let mut app = EffectcraftApp::new(effectcraft_engine::Session::default()); + app.session.execute("comp.new", json!({"name": "A", "width": 64, "height": 36, "frameRate": 30, "duration": 1})).unwrap(); + let ctx = egui::Context::default(); + let asked: Rc>> = Rc::default(); + let (a, b) = (asked.clone(), asked.clone()); + app.hooks.pick_save = Some(Box::new(move |name: &str| { + a.borrow_mut().push(format!("project {name}")); + None + })); + app.hooks.pick_save_file = Some(Box::new(move |name: &str, ext: &str| { + b.borrow_mut().push(format!("file {name} [{ext}]")); + None + })); + for id in ["comp.saveFrameAs", "render.saveCurrentPreview", "file.exportLottie", "file.saveCopyAsXml", "file.collectFiles", "file.saveAs"] { + invoke(&mut app, &ctx, id, json!({})).unwrap(); + } + assert_eq!( + *asked.borrow(), + [ + "file Frame.png [png]", + "file Preview.mp4 [mp4]", + "file Animation.json [json]", + "file Untitled Project.ecprojx [ecprojx]", + "file Collected Files []", + "project Untitled Project.ecproj", + ] + ); + // Without the other dialog, the project one names any file. + app.hooks.pick_save_file = None; + asked.borrow_mut().clear(); + invoke(&mut app, &ctx, "comp.saveFrameAs", json!({})).unwrap(); + assert_eq!(*asked.borrow(), ["project Frame.png"]); + } + #[test] fn menu_items_cover_the_tree() { assert_eq!(menus().last(), Some(&"Help")); diff --git a/crates/ui-egui/src/panels/forms.rs b/crates/ui-egui/src/panels/forms.rs index 4df53e3a..f4de5f21 100644 --- a/crates/ui-egui/src/panels/forms.rs +++ b/crates/ui-egui/src/panels/forms.rs @@ -758,13 +758,9 @@ pub fn show_form(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { if let Some(i) = browse && let Some(FieldKind::SavePath(value)) = f.fields.get_mut(i).map(|f| &mut f.kind) { - let default = std::path::Path::new(value.as_str()).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); - match app.hooks.pick_save.as_ref() { - Some(pick) => { - if let Some(path) = pick(&default) { - *value = path; - } - } + match app.hooks.save_dialog(value) { + Some(Some(path)) => *value = path, + Some(None) => {} None => app.ui.status = "no file dialog available: type the path".into(), } } diff --git a/crates/ui-egui/src/panels/render_queue.rs b/crates/ui-egui/src/panels/render_queue.rs index c791d214..f0fb2941 100644 --- a/crates/ui-egui/src/panels/render_queue.rs +++ b/crates/ui-egui/src/panels/render_queue.rs @@ -396,12 +396,10 @@ fn output_to_menu(it: &RenderQueueItem) -> Vec<(String, Value)> { } /// Output To's save dialog, as After Effects' Output Movie To: it opens at the current output -/// (folder and name) and offers the output format's extension. `None` when the host has no -/// save dialog for other file kinds; `Some(None)` when the dialog was cancelled. +/// (folder and name, which ends in the format's extension) and offers that extension. `None` +/// when the host has no save dialog; `Some(None)` when the dialog was cancelled. fn pick_output(app: &EffectcraftApp, it: &RenderQueueItem) -> Option> { - let pick = app.hooks.pick_save_file.as_ref()?; - let default = app.session.resolve_output(it).unwrap_or_default(); - Some(pick(&default, it.output.format.extension())) + app.hooks.save_dialog(&app.session.resolve_output(it).unwrap_or_default()) } pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { @@ -697,13 +695,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let olabels: Vec = omenu.iter().map(|(l, _)| l.clone()).collect(); if let Some(i) = widgets::popup_menu(ui, oid, odd.left_bottom(), &olabels, None) { if omenu[i].1.get("choose").is_some() { - // The web has no folders to choose from: its save "dialog" only names the file. - let picked = pick_output(app, it).or_else(|| { - let default = app.session.resolve_output(it).unwrap_or_default(); - let name = std::path::Path::new(&default).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); - app.hooks.pick_save.as_ref().map(|f| f(&name)) - }); - match picked { + match pick_output(app, it) { Some(Some(path)) => actions.push(("renderQueue.setOutput", json!({"item": it.id, "path": path}))), Some(None) => {} None => editing = Some((it.id, it.output.output.clone())),