From b63efeb0d3719f7cf21f9d641a5eb011f989c728 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Tue, 13 Oct 2026 15:12:32 +0530 Subject: [PATCH 01/16] perf(project): read file sizes once per sort --- crates/ui-egui/src/i18n/ui.rs | 14 +++ crates/ui-egui/src/panels/project.rs | 158 +++++++++++++++++++++++- crates/ui-egui/tests/ui_project_rows.rs | 102 +++++++++++++++ scripts/bench-project-size-sort.sh | 4 + 4 files changed, 276 insertions(+), 2 deletions(-) create mode 100755 scripts/bench-project-size-sort.sh diff --git a/crates/ui-egui/src/i18n/ui.rs b/crates/ui-egui/src/i18n/ui.rs index 3a4dd4aff..d61eff44d 100644 --- a/crates/ui-egui/src/i18n/ui.rs +++ b/crates/ui-egui/src/i18n/ui.rs @@ -252,6 +252,8 @@ const JAPANESE: &[(&str, &str)] = &[ ("Focus Distance", "フォーカス距離"), ("Follow Feature Rotation and Scale", "特徴の回転とスケールに追従"), ("Footage: {}", "フッテージ：{}"), + ("File size: {} bytes", "ファイルサイズ: {} バイト"), + ("File size unavailable", "ファイルサイズを取得できません"), ("Format: {}", "形式：{}"), ("Frame Aspect Ratio: {}", "フレームアスペクト比：{}"), ("Frame Blending: {}", "フレームブレンド：{}"), @@ -796,6 +798,8 @@ const SIMPLIFIED_CHINESE: &[(&str, &str)] = &[ ("Focus Distance", "焦距"), ("Follow Feature Rotation and Scale", "跟随特征的旋转和缩放"), ("Footage: {}", "素材：{}"), + ("File size: {} bytes", "文件大小：{} 字节"), + ("File size unavailable", "无法获取文件大小"), ("Format: {}", "格式：{}"), ("Frame Aspect Ratio: {}", "帧长宽比：{}"), ("Frame Blending: {}", "帧混合：{}"), @@ -1528,6 +1532,8 @@ const TRADITIONAL_CHINESE: &[(&str, &str)] = &[ ("Focus Distance", "焦距"), ("Follow Feature Rotation and Scale", "跟隨特徵的旋轉和縮放"), ("Footage: {}", "素材：{}"), + ("File size: {} bytes", "檔案大小：{} 位元組"), + ("File size unavailable", "無法取得檔案大小"), ("Format: {}", "格式：{}"), ("Frame Aspect Ratio: {}", "影格長寬比：{}"), ("Frame Blending: {}", "影格混合：{}"), @@ -2263,6 +2269,8 @@ const UKRAINIAN: &[(&str, &str)] = &[ ("Focus Distance", "Відстань фокусування"), ("Follow Feature Rotation and Scale", "Враховувати обертання та масштаб ознаки"), ("Footage: {}", "Матеріал: {}"), + ("File size: {} bytes", "Розмір файлу: {} байт"), + ("File size unavailable", "Розмір файлу недоступний"), ("Format: {}", "Формат: {}"), ("Frame Aspect Ratio: {}", "Пропорції кадру: {}"), ("Frame Blending: {}", "Змішування кадрів: {}"), @@ -2815,6 +2823,8 @@ const PORTUGUESE: &[(&str, &str)] = &[ ("Focus Distance", "Distância de foco"), ("Follow Feature Rotation and Scale", "Acompanhar rotação e escala do recurso"), ("Footage: {}", "Mídia: {}"), + ("File size: {} bytes", "Tamanho do arquivo: {} bytes"), + ("File size unavailable", "Tamanho do arquivo indisponível"), ("Format: {}", "Formato: {}"), ("Frame Aspect Ratio: {}", "Proporção do quadro: {}"), ("Frame Blending: {}", "Mesclagem de quadros: {}"), @@ -3265,6 +3275,8 @@ const CZECH: &[(&str, &str)] = &[ ("Focus Distance", "Vzdálenost zaostření"), ("Follow Feature Rotation and Scale", "Sledovat otočení a měřítko prvku"), ("Footage: {}", "Záznam: {}"), + ("File size: {} bytes", "Velikost souboru: {} bajtů"), + ("File size unavailable", "Velikost souboru není k dispozici"), ("Format: {}", "Formát: {}"), ("Frame Aspect Ratio: {}", "Poměr stran snímku: {}"), ("Frame Blending: {}", "Prolnutí snímků: {}"), @@ -3820,6 +3832,8 @@ const SPANISH: &[(&str, &str)] = &[ ("Focus Distance", "Distancia de enfoque"), ("Follow Feature Rotation and Scale", "Seguir la rotación y la escala de la característica"), ("Footage: {}", "Material de archivo: {}"), + ("File size: {} bytes", "Tamaño del archivo: {} bytes"), + ("File size unavailable", "Tamaño del archivo no disponible"), ("Format: {}", "Formato: {}"), ("Frame Aspect Ratio: {}", "Relación de aspecto del fotograma: {}"), ("Frame Blending: {}", "Fusión de fotogramas: {}"), diff --git a/crates/ui-egui/src/panels/project.rs b/crates/ui-egui/src/panels/project.rs index 46187fa8c..2cb6b8c73 100644 --- a/crates/ui-egui/src/panels/project.rs +++ b/crates/ui-egui/src/panels/project.rs @@ -84,11 +84,14 @@ pub const COLUMNS: [(&str, &str, f32); 6] = [ /// Sort siblings by the Project panel's sort column; ties fall back to the name. pub fn sort_items(items: &mut [&Item], col: &str, desc: bool) { + if col == "size" { + sort_by_size(items, desc, file_size); + return; + } let name = |i: &Item| i.name.to_lowercase(); items.sort_by(|a, b| { let o = match col { "type" => a.type_name().cmp(b.type_name()), - "size" => file_size(a).cmp(&file_size(b)), "fps" => a.frame_rate().map(|r| r.as_f64()).partial_cmp(&b.frame_rate().map(|r| r.as_f64())).unwrap_or(std::cmp::Ordering::Equal), "duration" => a.duration().cmp(&b.duration()), "path" => file_path(a).cmp(file_path(b)), @@ -101,6 +104,15 @@ pub fn sort_items(items: &mut [&Item], col: &str, desc: bool) { }); } +fn sort_by_size(items: &mut [&Item], desc: bool, mut size_of: impl FnMut(&Item) -> Option) { + let mut key = |it: &&Item| (size_of(it), it.name.to_lowercase()); + if desc { + items.sort_by_cached_key(|it| std::cmp::Reverse(key(it))); + } else { + items.sort_by_cached_key(key); + } +} + /// Where a drop at a row lands: into a folder row, else into the row's folder; `None` row = root. pub fn drop_folder(project: &effectcraft_engine::project::Project, row: Option) -> Option { let it = project.item(row?)?; @@ -754,7 +766,19 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let cp = lp.with_clip_rect(cell); let txt = match *key { "type" => it.type_name().to_string(), - "size" => file_size(it).map(fmt_size).unwrap_or_default(), + "size" => { + let size = file_size(it); + let text = size.map(fmt_size).unwrap_or_else(|| if matches!(it.kind, ItemKind::Footage(_)) { "—".into() } else { String::new() }); + if matches!(it.kind, ItemKind::Footage(_)) { + let detail = match size { + Some(bytes) => crate::i18n::tr_args("File size: {} bytes", &[&bytes]), + None => crate::i18n::tr("File size unavailable").to_string(), + }; + let resp = ui.interact(cell, egui::Id::new(("psize", id.0)), Sense::hover()).on_hover_text(detail.as_str()); + app.auto.add_response(&format!("project.item.{}.size", id.0), &resp, &detail); + } + text + } "duration" => it.duration().map(|d| fmt_dur(d.seconds(), it.frame_rate().map(|r| r.as_f64()).unwrap_or(30.0))).unwrap_or_default(), "fps" => it.frame_rate().map(|f| format!("{:.2}", f.as_f64())).unwrap_or_default(), "path" => file_path(it).to_string(), @@ -1113,6 +1137,136 @@ pub fn set_column(cols: &mut Vec, key: &str, on: bool) { #[cfg(test)] mod tests { use super::*; + use effectcraft_engine::project::{Footage, Project}; + + struct SizeFiles(std::path::PathBuf); + + impl SizeFiles { + fn new() -> Self { + static NEXT: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + let n = NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let path = std::env::temp_dir().join(format!("effectcraft-size-{}-{n}", std::process::id())); + std::fs::create_dir(&path).unwrap(); + Self(path) + } + + fn add(&self, project: &mut Project, name: &str, bytes: Option) { + let path = self.0.join(name); + if let Some(bytes) = bytes { + std::fs::File::create(&path).unwrap().set_len(bytes).unwrap(); + } + project.add_item(name, Label::Aqua, None, ItemKind::Footage(Footage { path: path.to_string_lossy().into_owned(), ..Default::default() })); + } + } + + impl Drop for SizeFiles { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + #[test] + fn size_keys_are_sampled_once_with_stable_ties() { + let mut project = Project::default(); + for name in ["Zeta", "alpha", "ALPHA", "Missing", "Zero"] { + project.add_item(name, Label::Aqua, None, ItemKind::Folder); + } + for (desc, expected) in [(false, ["Missing", "Zero", "alpha", "ALPHA", "Zeta"]), (true, ["Zeta", "alpha", "ALPHA", "Zero", "Missing"])] { + let mut reads = std::collections::BTreeMap::new(); + let mut rows = project.children(None); + sort_by_size(&mut rows, desc, |item| { + *reads.entry(item.id).or_insert(0) += 1; + match item.name.as_str() { + "Zeta" => Some(30), + "alpha" | "ALPHA" => Some(10), + "Zero" => Some(0), + _ => None, + } + }); + assert_eq!(rows.iter().map(|item| item.name.as_str()).collect::>(), expected); + assert_eq!(reads.len(), 5); + assert!(reads.values().all(|&count| count == 1)); + } + } + + #[test] + fn size_sort_reads_files_again_after_they_change() { + let files = SizeFiles::new(); + let mut project = Project::default(); + project.add_item("Folder", Label::Aqua, None, ItemKind::Folder); + files.add(&mut project, "Missing", None); + files.add(&mut project, "Zero", Some(0)); + files.add(&mut project, "Small", Some(10)); + files.add(&mut project, "Large", Some(20)); + let mut rows = project.children(None); + sort_items(&mut rows, "size", false); + assert_eq!(rows.iter().map(|item| item.name.as_str()).collect::>(), ["Folder", "Missing", "Zero", "Small", "Large"]); + sort_items(&mut rows, "size", true); + assert_eq!(rows.iter().map(|item| item.name.as_str()).collect::>(), ["Large", "Small", "Zero", "Missing", "Folder"]); + std::fs::OpenOptions::new().write(true).open(files.0.join("Small")).unwrap().set_len(30).unwrap(); + sort_items(&mut rows, "size", false); + assert_eq!(rows.iter().map(|item| item.name.as_str()).collect::>(), ["Folder", "Missing", "Zero", "Large", "Small"]); + std::fs::remove_file(files.0.join("Large")).unwrap(); + sort_items(&mut rows, "size", false); + assert_eq!(rows.iter().map(|item| item.name.as_str()).collect::>(), ["Folder", "Large", "Missing", "Zero", "Small"]); + } + + #[test] + fn size_sort_accepts_empty_and_singleton_lists() { + let mut project = Project::default(); + let id = project.add_item("Only", Label::Aqua, None, ItemKind::Folder); + for desc in [false, true] { + sort_items(&mut [], "size", desc); + let mut rows = project.children(None); + sort_items(&mut rows, "size", desc); + assert_eq!(rows.iter().map(|item| item.id).collect::>(), [id]); + } + } + + #[test] + #[ignore = "filesystem benchmark; run scripts/bench-project-size-sort.sh"] + fn size_sort_benchmark() { + let files = SizeFiles::new(); + let mut project = Project::default(); + for i in 0..1000 { + files.add(&mut project, &format!("Clip {i:04}"), Some((i * 137) % 1000 + 1)); + } + let original = project.children(None); + for item in &original { + assert!(file_size(item).is_some()); + } + let mut before_times = Vec::new(); + let mut after_times = Vec::new(); + let mut before_reads = 0; + let mut after_reads = 0; + for _ in 0..5 { + let mut before = original.clone(); + before_reads = 0; + let start = std::time::Instant::now(); + before.sort_by(|a, b| { + before_reads += 2; + file_size(a).cmp(&file_size(b)).then_with(|| a.name.to_lowercase().cmp(&b.name.to_lowercase())) + }); + before_times.push(start.elapsed()); + let mut after = original.clone(); + after_reads = 0; + let start = std::time::Instant::now(); + sort_by_size(&mut after, false, |item| { + after_reads += 1; + file_size(item) + }); + after_times.push(start.elapsed()); + assert_eq!(before.iter().map(|item| item.id).collect::>(), after.iter().map(|item| item.id).collect::>()); + assert_eq!(after_reads, 1000); + assert!(before_reads > after_reads); + } + before_times.sort(); + after_times.sort(); + println!( + "1,000 footage files; warmed metadata; five paired sorts\nmetadata reads/sort: {before_reads} -> {after_reads}\nmedian sort time: {:?} -> {:?}", + before_times[2], after_times[2] + ); + } #[test] fn ae_duration_and_size_formats() { diff --git a/crates/ui-egui/tests/ui_project_rows.rs b/crates/ui-egui/tests/ui_project_rows.rs index 27ce5e693..949310f5c 100644 --- a/crates/ui-egui/tests/ui_project_rows.rs +++ b/crates/ui-egui/tests/ui_project_rows.rs @@ -84,6 +84,108 @@ fn command_clicking_a_folder_twirl_opens_the_folders_inside_it() { assert!(h.state().ui.project_open_folders.is_empty(), "and close again together"); } +#[test] +fn size_column_sorts_and_explains_sizes_without_swallowing_row_clicks() { + use effectcraft_ui_egui::theme::ThemeKind; + for (name, size, theme) in [ + ("wide-dark", egui::vec2(1600.0, 1000.0), ThemeKind::Dark), + ("compact-dark", egui::vec2(1024.0, 768.0), ThemeKind::Dark), + ("compact-light", egui::vec2(1024.0, 768.0), ThemeKind::Light), + ] { + check_size_column(name, size, theme); + } +} + +fn check_size_column(name: &str, size: egui::Vec2, theme: effectcraft_ui_egui::theme::ThemeKind) { + struct Files(std::path::PathBuf); + impl Drop for Files { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + let files = Files(std::env::temp_dir().join(format!( + "effectcraft-size-ui-{}-{}", + std::process::id(), + std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos() + ))); + std::fs::create_dir(&files.0).unwrap(); + let mut s = Session::default(); + s.prefs.general.language = "en".into(); + let project = std::sync::Arc::make_mut(&mut s.project); + let folder = project.add_item("Folder", Label::Aqua, None, ItemKind::Folder); + let mut ids = Vec::new(); + for (name, bytes) in [("Small", Some(10)), ("Large", Some(20)), ("Unavailable", None), ("Empty", Some(0))] { + let path = files.0.join(name); + if let Some(bytes) = bytes { + std::fs::File::create(&path).unwrap().set_len(bytes).unwrap(); + } + ids.push(project.add_item(name, Label::Aqua, None, ItemKind::Footage(Footage { path: path.to_string_lossy().into_owned(), ..Default::default() }))); + } + let [small, large, unavailable, empty] = ids.as_slice() else { panic!("fixture items") }; + let (small, large, unavailable, empty) = (*small, *large, *unavailable, *empty); + let mut app = EffectcraftApp::new(s); + app.ui.start_screen = false; + app.ui.project_columns = vec!["size".into()]; + let mut h = Harness::builder().with_size(size).build_eframe(|_| app); + let ctx = h.ctx.clone(); + h.state_mut().set_theme(&ctx, theme); + h.run_steps(3); + let size_id = |id: ItemId| format!("project.item.{}.size", id.0); + let assert_order = |h: &Harness<'_, EffectcraftApp>, expected: &[ItemId]| { + let mut visible = expected.to_vec(); + visible.sort_by(|a, b| center(h, &format!("project.item.{}", a.0)).y.total_cmp(¢er(h, &format!("project.item.{}", b.0)).y)); + assert_eq!(visible, expected); + }; + let undo = h.state().session.history.undo.len(); + click(&mut h, "project.sort.size"); + assert_order(&h, &[folder, unavailable, empty, small, large]); + click(&mut h, "project.sort.size"); + assert_order(&h, &[large, small, empty, unavailable, folder]); + assert_eq!(h.state().auto.find(&size_id(empty)).unwrap().label, "File size: 0 bytes"); + assert!(h.state().auto.find(&size_id(folder)).is_none()); + let at = center(&h, &size_id(small)); + h.input_mut().events.push(Event::PointerMoved(at)); + h.run_steps(5); + assert!(h.query_by_label("File size: 10 bytes").is_some(), "hover exposes exact bytes when both files display 1 KB"); + if let Ok(dir) = std::env::var("PROJECT_SIZE_SNAPSHOTS") { + h.render().unwrap().save(format!("{dir}/project-size-{name}-hover.png")).unwrap(); + } + click_at(&mut h, at); + assert_eq!(h.state().session.state.project_selection, [small]); + let at = center(&h, &size_id(unavailable)); + h.input_mut().events.push(Event::PointerMoved(at)); + h.run_steps(5); + assert!(h.query_by_label("File size unavailable").is_some()); + let has_dash = |shape: &egui::Shape| matches!(shape, egui::Shape::Text(text) if text.galley.text() == "—"); + assert!(h.output().shapes.iter().any(|shape| has_dash(&shape.shape)), "unavailable footage has a visible marker"); + if let Ok(dir) = std::env::var("PROJECT_SIZE_SNAPSHOTS") { + h.render().unwrap().save(format!("{dir}/project-size-{name}-unavailable.png")).unwrap(); + } + std::fs::OpenOptions::new().write(true).open(files.0.join("Small")).unwrap().set_len(30).unwrap(); + h.run_steps(3); + assert_order(&h, &[small, large, empty, unavailable, folder]); + assert_eq!(h.state().auto.find(&size_id(small)).unwrap().label, "File size: 30 bytes"); + assert_eq!(h.state().session.history.undo.len(), undo, "sorting and size details don't edit the project"); + let from = center(&h, &size_id(small)); + let to = center(&h, &format!("project.item.{}.name", folder.0)); + h.input_mut().events.push(Event::PointerMoved(from)); + h.step(); + button(&mut h, from, true); + h.step(); + for k in 1..=6 { + h.input_mut().events.push(Event::PointerMoved(from + (to - from) * (k as f32 / 6.0))); + h.step(); + } + button(&mut h, to, false); + h.run_steps(3); + assert_eq!(h.state().session.project.item(small).unwrap().parent, Some(folder), "dragging from Size still moves the item"); + assert_eq!(h.state().session.history.undo.len(), undo + 1); + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + h.run_steps(3); + assert_eq!(h.state().session.project.item(small).unwrap().parent, None); + assert_order(&h, &[small, large, empty, unavailable, folder]); +} + #[test] fn folder_twirl_opens_and_closes_the_folder_on_one_click() { let (mut h, folder, comp, _) = harness(); diff --git a/scripts/bench-project-size-sort.sh b/scripts/bench-project-size-sort.sh new file mode 100755 index 000000000..b85e495d2 --- /dev/null +++ b/scripts/bench-project-size-sort.sh @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +set -euo pipefail +cd "$(dirname "${BASH_SOURCE[0]}")/.." +CARGO_PROFILE_RELEASE_LTO=false cargo test -p effectcraft-ui-egui --release --lib panels::project::tests::size_sort_benchmark -- --ignored --nocapture From da06216475cf3e7ea5aaedf40169220f6e7aa824 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Tue, 13 Oct 2026 16:48:19 +0530 Subject: [PATCH 02/16] perf(presets): reuse cached text and user preset data --- crates/engine/src/commands/text_anim.rs | 9 ++- crates/engine/src/tests_text.rs | 58 ++++++++++++++ crates/ui-egui/src/panels/effects_presets.rs | 83 +++++++++++++++++--- crates/ui-egui/tests/ui_presets_folder.rs | 75 +++++++++++++++++- 4 files changed, 209 insertions(+), 16 deletions(-) diff --git a/crates/engine/src/commands/text_anim.rs b/crates/engine/src/commands/text_anim.rs index 228d48424..92ab23f17 100644 --- a/crates/engine/src/commands/text_anim.rs +++ b/crates/engine/src/commands/text_anim.rs @@ -6,6 +6,7 @@ use effectcraft_project::build::{self, Ids}; use effectcraft_project::{Layer, LayerSource, Node, PropGroup}; use effectcraft_time::Tick; use serde_json::{Value, json}; +use std::sync::OnceLock; use super::{CommandSpec, b_p, bad, has_comp, layer_mut, layer_p, str_p}; use crate::{Result, Session, cmd, query}; @@ -13,8 +14,10 @@ use crate::{Result, Session, cmd, query}; /// The original text animator presets (`presets/text_animators.json`). pub const PRESETS_JSON: &str = include_str!("../../presets/text_animators.json"); -fn presets() -> Vec { - serde_json::from_str::(PRESETS_JSON).ok().and_then(|v| v.get("presets").and_then(Value::as_array).cloned()).unwrap_or_default() +fn presets() -> &'static [Value] { + static PRESETS: OnceLock> = OnceLock::new(); + PRESETS + .get_or_init(|| serde_json::from_str::(PRESETS_JSON).ok().and_then(|v| v.get("presets").and_then(Value::as_array).cloned()).unwrap_or_default()) } /// (id, name) of every preset. @@ -303,7 +306,7 @@ fn apply_preset(s: &mut Session, p: &Value) -> Result { let (cid, lid) = layer_p(s, p, C)?; let want = str_p(p, "preset").ok_or_else(|| bad(C, "`preset` id or name required"))?.to_string(); let preset = presets() - .into_iter() + .iter() .find(|x| x.get("id").and_then(Value::as_str) == Some(&want) || x.get("name").and_then(Value::as_str).is_some_and(|n| n.eq_ignore_ascii_case(&want))) .ok_or_else(|| bad(C, format!("no preset {want}")))?; let name = preset.get("name").and_then(Value::as_str).unwrap_or("Preset").to_string(); diff --git a/crates/engine/src/tests_text.rs b/crates/engine/src/tests_text.rs index 7b9650062..a57f87a70 100644 --- a/crates/engine/src/tests_text.rs +++ b/crates/engine/src/tests_text.rs @@ -164,6 +164,64 @@ fn presets_apply_render_and_round_trip() { assert!(matches(tg.sub("pathOptions").unwrap()).contains(&"forceAlignment".to_string())); } +#[test] +fn cached_text_presets_keep_the_catalog_and_application_independent() { + let expected = [ + ("typewriter", "Typewriter"), + ("fadeUpCharacters", "Fade Up Characters"), + ("bounceInWords", "Bounce In Words"), + ("trackingIn", "Tracking In"), + ("scramble", "Scramble"), + ("blurIn", "Blur In"), + ("jitter", "Jitter"), + ("dropInLines", "Drop In Lines"), + ]; + let (mut s, layer) = session(); + let catalog = s.execute("text.presets", json!({})).unwrap(); + let mut returned = catalog.clone(); + returned[0]["name"] = json!("Edited copy"); + let project = s.project.clone(); + for _ in 0..2 { + let names = crate::text_presets(); + assert_eq!(names.iter().map(|(id, name)| (id.as_str(), name.as_str())).collect::>(), expected); + for (id, _) in expected { + let result = s.execute("layer.applyTextPreset", json!({"layer": layer, "preset": id})).unwrap(); + assert!(!result["animators"].as_array().unwrap().is_empty(), "{id}"); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(s.project, project, "{id}: undo restores the document"); + } + assert_eq!(s.execute("text.presets", json!({})).unwrap(), catalog); + } +} + +#[test] +#[ignore = "warm text-preset listing benchmark"] +fn text_preset_listing_benchmark() { + let old = || { + let v: serde_json::Value = serde_json::from_str(include_str!("../presets/text_animators.json")).unwrap(); + let presets = v["presets"].as_array().unwrap().clone(); + presets.iter().map(|p| (p["id"].as_str().unwrap().to_string(), p["name"].as_str().unwrap().to_string())).collect::>() + }; + assert_eq!(old(), crate::text_presets()); + let mut before = Vec::new(); + let mut after = Vec::new(); + for _ in 0..5 { + let start = std::time::Instant::now(); + for _ in 0..1000 { + std::hint::black_box(old()); + } + before.push(start.elapsed()); + let start = std::time::Instant::now(); + for _ in 0..1000 { + std::hint::black_box(crate::text_presets()); + } + after.push(start.elapsed()); + } + before.sort(); + after.sort(); + println!("Text preset names, median per warm lookup: {:?} -> {:?}", before[2] / 1000, after[2] / 1000); +} + #[test] fn menus_have_text_animation_entries() { let (s, _) = session(); diff --git a/crates/ui-egui/src/panels/effects_presets.rs b/crates/ui-egui/src/panels/effects_presets.rs index d3921cd85..9ed7e0df9 100644 --- a/crates/ui-egui/src/panels/effects_presets.rs +++ b/crates/ui-egui/src/panels/effects_presets.rs @@ -8,6 +8,7 @@ use crate::i18n::tr; use egui::{Align2, Color32, Rect, Sense, Stroke, pos2, vec2}; use serde_json::json; +use std::sync::Arc; use crate::icons::{self, Icon}; use crate::panels::DragPayload; @@ -228,7 +229,7 @@ fn preset_row( name: &str, auto_id: &str, query: &str, - on_apply: Apply, + on_apply: impl FnOnce() -> Apply, apply: &mut Option, ) { let t = app.tokens; @@ -245,22 +246,22 @@ fn preset_row( highlighted(lp, pos2(r.min.x + 70.0, r.center().y), name, query, &t); app.auto.add(auto_id, r, name); if resp.double_clicked() { - *apply = Some(on_apply); + *apply = Some(on_apply()); } } /// User preset files, rescanned every few seconds (and on Refresh List). -fn user_presets(ctx: &egui::Context, force: bool) -> Vec { +fn user_presets(ctx: &egui::Context, force: bool, scan: impl FnOnce() -> Vec) -> Arc> { let id = egui::Id::new("fx-user-presets"); let now = ctx.input(|i| i.time); if !force - && let Some((at, v)) = ctx.data(|d| d.get_temp::<(f64, std::sync::Arc>)>(id)) + && let Some((at, v)) = ctx.data(|d| d.get_temp::<(f64, Arc>)>(id)) && now - at < 5.0 { - return (*v).clone(); + return v; } - let v = presets_dir().map(|d| scan_presets(&d)).unwrap_or_default(); - ctx.data_mut(|d| d.insert_temp(id, (now, std::sync::Arc::new(v.clone())))); + let v = Arc::new(scan()); + ctx.data_mut(|d| d.insert_temp(id, (now, v.clone()))); v } @@ -389,13 +390,13 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { categories.push(c); } } - let files: Vec = - user_presets(&ctx, force_scan).into_iter().filter(|f| query.is_empty() || f.name.to_lowercase().contains(&query)).collect(); + let user = user_presets(&ctx, force_scan, || presets_dir().map(|d| scan_presets(&d)).unwrap_or_default()); + let files: Vec<&PresetFile> = user.iter().filter(|f| query.is_empty() || f.name.to_lowercase().contains(&query)).collect(); let any = !text.is_empty() || !builtin.is_empty() || !files.is_empty(); if any && folder_row(app, ui, &lp, list, &mut y, "presets", "* Animation Presets", 0.0, searching, None) { if !text.is_empty() && folder_row(app, ui, &lp, list, &mut y, "presets/Text", "Text", 14.0, searching, None) { for (id, name) in text { - preset_row(app, ui, &lp, list, &mut y, &name, &format!("effects.preset.text.{id}"), &query, Apply::TextPreset(id.clone()), &mut apply); + preset_row(app, ui, &lp, list, &mut y, &name, &format!("effects.preset.text.{id}"), &query, || Apply::TextPreset(id.clone()), &mut apply); } } for cat in &categories { @@ -403,7 +404,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { continue; } for (id, name, _) in builtin.iter().filter(|(_, _, c)| c == cat) { - preset_row(app, ui, &lp, list, &mut y, name, &format!("effects.preset.builtin.{id}"), &query, Apply::Builtin(id.clone()), &mut apply); + preset_row(app, ui, &lp, list, &mut y, name, &format!("effects.preset.builtin.{id}"), &query, || Apply::Builtin(id.clone()), &mut apply); } } if !files.is_empty() && folder_row(app, ui, &lp, list, &mut y, "presets/User", "User Presets", 14.0, searching, None) { @@ -417,7 +418,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { &f.name, &format!("effects.preset.file.{}", f.name), &query, - Apply::PresetFile(f.path.clone()), + || Apply::PresetFile(f.path.clone()), &mut apply, ); } @@ -536,6 +537,64 @@ mod tests { let v = scan_presets(&d); assert_eq!(v.iter().map(|p| p.name.as_str()).collect::>(), vec!["bounce", "Zoom In"]); assert!(scan_presets(&d.join("missing")).is_empty()); + let ctx = egui::Context::default(); + let set_time = |time| { + let mut output = ctx.run_ui(egui::RawInput { time: Some(time), ..Default::default() }, |_| {}); + output.textures_delta.clear(); + }; + let scans = std::cell::Cell::new(0); + let scan = || { + scans.set(scans.get() + 1); + scan_presets(&d) + }; + set_time(1.0); + let first = user_presets(&ctx, false, scan); + assert_eq!(first.iter().map(|p| p.name.as_str()).collect::>(), ["bounce", "Zoom In"]); + std::fs::remove_file(d.join("bounce.ECPRESET")).unwrap(); + std::fs::write(d.join("Another.ecpreset"), "{}").unwrap(); + set_time(5.999); + let hit = user_presets(&ctx, false, scan); + assert!(Arc::ptr_eq(&first, &hit)); + assert_eq!(hit.iter().map(|p| p.name.as_str()).collect::>(), ["bounce", "Zoom In"]); + assert_eq!(scans.get(), 1); + set_time(6.0); + let expired = user_presets(&ctx, false, scan); + assert_eq!(expired.iter().map(|p| p.name.as_str()).collect::>(), ["Another", "Zoom In"]); + std::fs::remove_file(d.join("Another.ecpreset")).unwrap(); + let refreshed = user_presets(&ctx, true, scan); + assert_eq!(refreshed.iter().map(|p| p.name.as_str()).collect::>(), ["Zoom In"]); + assert_eq!(scans.get(), 3); + assert_eq!(first.iter().map(|p| p.name.as_str()).collect::>(), ["bounce", "Zoom In"]); let _ = std::fs::remove_dir_all(&d); } + + #[test] + #[ignore = "warm custom-preset cache benchmark"] + fn user_preset_cache_benchmark() { + let ctx = egui::Context::default(); + let files = user_presets(&ctx, false, || { + (0..1000).map(|i| PresetFile { name: format!("Preset {i:04}"), path: format!("/presets/Preset {i:04}.ecpreset") }).collect() + }); + assert_eq!(files.first().unwrap().name, "Preset 0000"); + assert_eq!(files.last().unwrap().name, "Preset 0999"); + let mut before = Vec::new(); + let mut after = Vec::new(); + for _ in 0..5 { + let start = std::time::Instant::now(); + for _ in 0..1000 { + let (_, shared) = ctx.data(|d| d.get_temp::<(f64, Arc>)>(egui::Id::new("fx-user-presets"))).unwrap(); + std::hint::black_box((*shared).clone()); + } + before.push(start.elapsed()); + let start = std::time::Instant::now(); + for _ in 0..1000 { + std::hint::black_box(user_presets(&ctx, false, Vec::new)); + } + after.push(start.elapsed()); + } + assert!(Arc::ptr_eq(&files, &user_presets(&ctx, false, Vec::new))); + before.sort(); + after.sort(); + println!("1,000 custom presets, median per warm cache lookup: {:?} -> {:?}", before[2] / 1000, after[2] / 1000); + } } diff --git a/crates/ui-egui/tests/ui_presets_folder.rs b/crates/ui-egui/tests/ui_presets_folder.rs index 7ba632a6f..92d723564 100644 --- a/crates/ui-egui/tests/ui_presets_folder.rs +++ b/crates/ui-egui/tests/ui_presets_folder.rs @@ -1,6 +1,7 @@ //! Effects & Presets: copy the configured folder path through the contents menu. use effectcraft_engine::Session; +use effectcraft_ui_egui::panels::effects_presets::PresetFile; use effectcraft_ui_egui::{ControlRequest, EffectcraftApp, control, dock::PanelKind}; use egui::{Event, Modifiers, OutputCommand, PointerButton, pos2}; use egui_kittest::Harness; @@ -18,8 +19,9 @@ fn click(h: &mut Harness<'_, EffectcraftApp>, id: &str) -> Vec { let e = h.state().auto.find(id).unwrap_or_else(|| panic!("missing {id}")); let pos = pos2(e.rect[0] + e.rect[2] / 2.0, e.rect[1] + e.rect[3] / 2.0); let mut copied = Vec::new(); + h.input_mut().events.push(Event::PointerMoved(pos)); + h.step(); for pressed in [true, false] { - h.input_mut().events.push(Event::PointerMoved(pos)); h.input_mut().events.push(Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }); h.step(); for command in &h.output().platform_output.commands { @@ -88,3 +90,74 @@ fn presets_folder_control_command_returns_the_path_or_a_configuration_error() { assert_eq!(copied, [expected.as_str()]); } } + +#[test] +fn shared_preset_rows_search_apply_and_undo_the_selected_definition() { + struct Files(std::path::PathBuf); + impl Drop for Files { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + let files = Files(std::env::temp_dir().join(format!( + "ec-preset-rows-{}-{}", + std::process::id(), + std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos() + ))); + std::fs::create_dir(&files.0).unwrap(); + let mut entries = Vec::new(); + for (name, value) in [("Custom Cache A", 35.0), ("Custom Cache B", 70.0)] { + let path = files.0.join(format!("{name}.ecpreset")); + let preset = json!({"effectcraftAnimationPreset": 1, "name": name, "properties": [{"path": "transform/opacity", "value": {"t": "Scalar", "v": value}}], "effects": []}); + std::fs::write(&path, serde_json::to_vec(&preset).unwrap()).unwrap(); + entries.push(PresetFile { name: name.into(), path: path.to_string_lossy().into_owned() }); + } + let mut session = Session::default(); + session.prefs.general.language = "en".into(); + session.execute("comp.new", json!({"width": 64, "height": 36, "duration": 2})).unwrap(); + session.execute("layer.newText", json!({"text": "Cache"})).unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).with_step_dt(1.0 / 60.0).build_eframe(|_| { + let mut app = EffectcraftApp::new(session); + app.ui.start_screen = false; + app.show_panel(PanelKind::EffectsPresets); + app + }); + h.run_steps(3); + let shared = std::sync::Arc::new(entries); + let now = h.ctx.input(|i| i.time); + h.ctx.data_mut(|d| d.insert_temp(egui::Id::new("fx-user-presets"), (now, shared.clone()))); + let search = |h: &mut Harness<'_, EffectcraftApp>, text: &str| { + click(h, "effects.search"); + h.input_mut().events.push(Event::Key { key: egui::Key::A, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::COMMAND }); + h.input_mut().events.push(Event::Text(text.into())); + h.run_steps(40); + }; + search(&mut h, "Typewriter"); + assert!(h.state().auto.find("effects.preset.text.typewriter").is_some()); + assert!(h.state().auto.find("effects.preset.file.Custom Cache A").is_none()); + click(&mut h, "effects.preset.text.typewriter"); + click(&mut h, "effects.preset.text.typewriter"); + h.run_steps(3); + assert_eq!(h.state().session.active_comp().unwrap().layers[0].props.group("text/animators").unwrap().groups().count(), 1); + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + for (name, expected) in [("Custom Cache A", 35.0), ("Custom Cache B", 70.0)] { + search(&mut h, name); + let id = format!("effects.preset.file.{name}"); + let history = h.state().session.history.undo.len(); + assert_eq!(h.state().session.active_comp().unwrap().layers[0].props.prop("transform/opacity").unwrap().value.as_f64(), 100.0); + click(&mut h, &id); + click(&mut h, &id); + h.run_steps(3); + assert_eq!(h.state().session.active_comp().unwrap().layers[0].props.prop("transform/opacity").unwrap().value.as_f64(), expected); + assert_eq!(h.state().session.history.undo.len(), history + 1); + if let Ok(dir) = std::env::var("PRESET_CACHE_SNAPSHOTS") { + h.render().unwrap().save(format!("{dir}/{name}.png")).unwrap(); + } + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + h.run_steps(2); + assert_eq!(h.state().session.active_comp().unwrap().layers[0].props.prop("transform/opacity").unwrap().value.as_f64(), 100.0); + } + let (_, cached) = h.ctx.data(|d| d.get_temp::<(f64, std::sync::Arc>)>(egui::Id::new("fx-user-presets"))).unwrap(); + assert!(std::sync::Arc::ptr_eq(&shared, &cached)); + assert_eq!(shared.iter().map(|p| p.name.as_str()).collect::>(), ["Custom Cache A", "Custom Cache B"]); +} From bb1a0e5209c847fc0650394593d7463c5dcebedb Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Tue, 13 Oct 2026 17:42:07 +0530 Subject: [PATCH 03/16] perf(media-browser): reuse listings and cache filename sort keys --- crates/engine/src/media_browser.rs | 67 +++++++- crates/ui-egui/src/panels/media_panels.rs | 59 ++++++- .../ui-egui/tests/ui_media_browser_cache.rs | 145 ++++++++++++++++++ 3 files changed, 265 insertions(+), 6 deletions(-) create mode 100644 crates/ui-egui/tests/ui_media_browser_cache.rs diff --git a/crates/engine/src/media_browser.rs b/crates/engine/src/media_browser.rs index 5edd67ca3..a77859d55 100644 --- a/crates/engine/src/media_browser.rs +++ b/crates/engine/src/media_browser.rs @@ -173,7 +173,7 @@ impl VirtualTree { }) }) .collect(); - out.sort_by(|a, b| b.is_dir.cmp(&a.is_dir).then_with(|| a.name.to_lowercase().cmp(&b.name.to_lowercase()))); + sort_entries(&mut out); Ok(out) } } @@ -193,6 +193,10 @@ pub struct Entry { pub kind: Option<&'static str>, } +fn sort_entries(entries: &mut [Entry]) { + entries.sort_by_cached_key(|e| (!e.is_dir, e.name.to_lowercase())); +} + const VIDEO: &[&str] = &["mp4", "m4v", "mov", "webm", "mkv", "avi", "mxf", "gif"]; const IMAGE: &[&str] = &["png", "jpg", "jpeg", "tif", "tiff", "exr", "bmp", "webp", "psd", "psb", "svg", "tga", "dpx", "cin", "hdr"]; const AUDIO: &[&str] = &["wav", "aif", "aiff", "mp3", "m4a", "aac", "flac", "ogg", "opus"]; @@ -251,7 +255,7 @@ pub fn list(dir: &str, importable_only: bool) -> Result, String> { Some(Entry { name, path, is_dir: md.is_dir(), size: if md.is_dir() { 0 } else { md.len() }, modified: unix(md.modified()), kind }) }) .collect(); - out.sort_by(|a, b| b.is_dir.cmp(&a.is_dir).then_with(|| a.name.to_lowercase().cmp(&b.name.to_lowercase()))); + sort_entries(&mut out); Ok(out) } @@ -407,6 +411,10 @@ mod tests { assert_eq!(media.len(), 3); assert_eq!(parent(&dir.join("Shots").to_string_lossy()).as_deref(), Some(d.as_str())); assert!(list(&dir.join("nope").to_string_lossy(), false).is_err()); + std::fs::write(dir.join("Ω.csv"), b"x").unwrap(); + std::fs::write(dir.join("作品.csv"), b"x").unwrap(); + let all = list(&d, false).unwrap(); + assert_eq!(all.iter().map(|e| e.name.as_str()).collect::>(), ["Shots", "A.mov", "b.png", "notes.txt", "Ω.csv", "作品.csv"]); std::fs::remove_dir_all(&dir).unwrap(); } @@ -436,6 +444,61 @@ mod tests { assert_eq!(vparent("/"), None); } + #[test] + fn listings_keep_folders_first_and_stable_unicode_order() { + let mut tree = VirtualTree::default(); + for name in ["作品.csv", "ä.csv", "Ä.csv", "İ.csv", "B.csv", "a.csv", "A.csv"] { + tree.add_file(&format!("/Shoot/{name}"), &format!("/files/{name}"), 1, None); + } + for name in ["zFolder", "Alpha"] { + tree.add_dir(&format!("/Shoot/{name}")); + } + let entries = tree.list("/Shoot", true).unwrap(); + assert_eq!( + entries.iter().map(|e| e.name.as_str()).collect::>(), + ["Alpha", "zFolder", "A.csv", "a.csv", "B.csv", "İ.csv", "Ä.csv", "ä.csv", "作品.csv"] + ); + assert_eq!(entries[2].path, "/files/A.csv"); + let mut reversed = entries.iter().rev().cloned().collect::>(); + sort_entries(&mut reversed); + assert_eq!( + reversed.iter().map(|e| e.name.as_str()).collect::>(), + ["Alpha", "zFolder", "a.csv", "A.csv", "B.csv", "İ.csv", "ä.csv", "Ä.csv", "作品.csv"] + ); + } + + #[test] + #[ignore = "Media Browser filename sorting benchmark"] + fn directory_sort_benchmark() { + let entries: Vec<_> = (0..1000) + .map(|i| Entry { + name: format!("作品 {} {:04}.mov", if i % 2 == 0 { "Clip" } else { "clip" }, i * 7919 % 1000), + path: format!("/Shoot/{i}.mov"), + is_dir: i % 7 == 0, + size: 1, + modified: None, + kind: Some("video"), + }) + .collect(); + let mut before = Vec::new(); + let mut after = Vec::new(); + for _ in 0..5 { + let mut old = entries.clone(); + let start = std::time::Instant::now(); + old.sort_by(|a, b| b.is_dir.cmp(&a.is_dir).then_with(|| a.name.to_lowercase().cmp(&b.name.to_lowercase()))); + before.push(start.elapsed()); + let mut new = entries.clone(); + let start = std::time::Instant::now(); + sort_entries(&mut new); + after.push(start.elapsed()); + assert_eq!(old, new); + std::hint::black_box(new); + } + before.sort(); + after.sort(); + println!("1,000 filename sort, median: {:?} -> {:?}", before[2], after[2]); + } + #[test] fn iso_dates() { assert_eq!(iso_date(0), "1970-01-01T00:00:00Z"); diff --git a/crates/ui-egui/src/panels/media_panels.rs b/crates/ui-egui/src/panels/media_panels.rs index 4a0ab6ae7..2023fcd5d 100644 --- a/crates/ui-egui/src/panels/media_panels.rs +++ b/crates/ui-egui/src/panels/media_panels.rs @@ -15,6 +15,7 @@ use effectcraft_engine::media_browser as mb; use effectcraft_engine::project::ItemId; use egui::{Align2, Color32, Rect, Sense, Stroke, pos2, vec2}; use serde_json::{Value, json}; +use std::sync::Arc; use super::DragPayload; use super::panel_kit as kit; @@ -24,6 +25,7 @@ use crate::{EffectcraftApp, widgets}; // ---------------------------------------------------------------- Media Browser type Thumb = Option<(egui::TextureHandle, [u32; 2])>; +type Listing = (f64, Arc, String>>); /// Thumbnail of a file (cached per path; `None` while not decoded or not decodable). fn thumbnail(app: &EffectcraftApp, ctx: &egui::Context, path: &str, budget: &mut u32) -> Thumb { @@ -76,11 +78,11 @@ pub fn media_browser(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { // Listing cached per folder for a second. let lid = egui::Id::new(("mb-list", dir.clone(), st.importable_only)); let now = ctx.input(|i| i.time); - let listing: Option<(f64, Result, String>)> = ctx.data(|d| d.get_temp(lid)); + let listing: Option = ctx.data(|d| d.get_temp(lid)); let entries = match listing { Some((at, l)) if now - at < 1.0 => l, _ => { - let l = b.list(&dir, st.importable_only); + let l = Arc::new(b.list(&dir, st.importable_only)); ctx.data_mut(|d| d.insert_temp(lid, (now, l.clone()))); l } @@ -115,7 +117,7 @@ pub fn media_browser(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { } if kit::button(app, ui, Rect::from_min_size(pos2(ax, y), vec2(w, 20.0)), &format!("mediaBrowser.action.{id}"), label, false) { kit::exec(app, "mediaBrowser.action", json!({"action": id})); - ctx.data_mut(|d| d.remove::<(f64, Result, String>)>(lid)); + ctx.data_mut(|d| d.remove::(lid)); } ax -= 6.0; } @@ -150,7 +152,7 @@ pub fn media_browser(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { } // Files. let list = Rect::from_min_max(pos2(side.max.x + 1.0, side.min.y), rect.max); - let entries = match entries { + let entries = match entries.as_ref() { Ok(e) => e, Err(e) => { p.text(list.center(), Align2::CENTER_CENTER, e, Tokens::ui(12.0), t.danger); @@ -409,3 +411,52 @@ pub fn import_card(app: &mut EffectcraftApp, ui: &mut egui::Ui, full: Rect) -> f } full.max.y - 16.0 - bottom } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[ignore = "Media Browser warm listing cache benchmark"] + fn listing_cache_benchmark() { + let ctx = egui::Context::default(); + let entries: Vec<_> = (0..1000) + .map(|i| mb::Entry { + name: format!("Clip {i:04}.mov"), + path: format!("/Shoot/作品 Clip {i:04}.mov"), + is_dir: false, + size: 1, + modified: None, + kind: Some("video"), + }) + .collect(); + let old_id = egui::Id::new("old-list"); + let new_id = egui::Id::new("new-list"); + ctx.data_mut(|d| { + d.insert_temp(old_id, (0.0, Ok::<_, String>(entries.clone()))); + d.insert_temp(new_id, (0.0, Arc::new(Ok::<_, String>(entries)))); + }); + let (_, original) = ctx.data(|d| d.get_temp::(new_id)).unwrap(); + assert_eq!(original.as_ref().as_ref().unwrap()[0].name, "Clip 0000.mov"); + let mut before = Vec::new(); + let mut after = Vec::new(); + for _ in 0..5 { + let start = std::time::Instant::now(); + for _ in 0..1000 { + let _ = std::hint::black_box(ctx.data(|d| d.get_temp::<(f64, Result, String>)>(old_id)).unwrap()); + } + before.push(start.elapsed()); + let start = std::time::Instant::now(); + for _ in 0..1000 { + std::hint::black_box(ctx.data(|d| d.get_temp::(new_id)).unwrap()); + } + after.push(start.elapsed()); + } + let (_, cached) = ctx.data(|d| d.get_temp::(new_id)).unwrap(); + assert!(Arc::ptr_eq(&original, &cached)); + assert_eq!(cached.as_ref().as_ref().unwrap().last().unwrap().name, "Clip 0999.mov"); + before.sort(); + after.sort(); + println!("1,000 Media Browser entries, median per warm cache lookup: {:?} -> {:?}", before[2] / 1000, after[2] / 1000); + } +} diff --git a/crates/ui-egui/tests/ui_media_browser_cache.rs b/crates/ui-egui/tests/ui_media_browser_cache.rs new file mode 100644 index 000000000..feade97bf --- /dev/null +++ b/crates/ui-egui/tests/ui_media_browser_cache.rs @@ -0,0 +1,145 @@ +use std::sync::{ + Arc, Mutex, + atomic::{AtomicBool, Ordering}, +}; + +use effectcraft_engine::{ + Session, + media_browser::{Browser, Entry, VirtualTree}, +}; +use effectcraft_ui_egui::{EffectcraftApp, dock::PanelKind}; +use egui::{Event, Modifiers, PointerButton, pos2}; +use egui_kittest::Harness; +use serde_json::{Value, json}; + +#[derive(Default)] +struct MutableBrowser { + tree: Mutex, + offline: AtomicBool, +} + +impl Browser for MutableBrowser { + fn home(&self) -> String { + "/Shoot".into() + } + fn list(&self, dir: &str, only: bool) -> Result, String> { + if self.offline.load(Ordering::Relaxed) { + return Err("Folder temporarily unavailable".into()); + } + self.tree.lock().unwrap().list(dir, only) + } + fn actions(&self) -> Vec<(String, String)> { + vec![("refresh".into(), "Refresh".into())] + } + fn action(&self, id: &str) -> Result { + assert_eq!(id, "refresh"); + self.tree.lock().unwrap().add_file("/Shoot/From action.csv", "/files/action.csv", 1, None); + Ok(json!({})) + } +} + +type Listing = (f64, Arc, String>>); + +fn listing(h: &Harness<'_, EffectcraftApp>, only: bool) -> Listing { + let dir = h.state().session.state.media_browser.folder.clone().unwrap_or_else(|| "/Shoot".into()); + h.ctx.data(|d| d.get_temp(egui::Id::new(("mb-list", dir, only)))).unwrap() +} + +fn names(list: &Arc, String>>) -> Vec<&str> { + list.as_ref().as_ref().unwrap().iter().map(|e| e.name.as_str()).collect() +} + +fn click(h: &mut Harness<'_, EffectcraftApp>, id: &str) { + let e = h.state().auto.find(id).unwrap_or_else(|| panic!("missing {id}")); + let pos = pos2(e.rect[0] + 8.0, e.rect[1] + e.rect[3] / 2.0); + h.input_mut().events.push(Event::PointerMoved(pos)); + h.step(); + for pressed in [true, false] { + h.input_mut().events.push(Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }); + h.step(); + } + h.run_steps(2); +} + +fn at(h: &mut Harness<'_, EffectcraftApp>, time: f64) { + h.input_mut().time = Some(time); + h.step(); + h.input_mut().time = Some(time); + h.step(); + h.input_mut().time = None; +} + +#[test] +fn browser_cache_refreshes_filters_navigates_and_recovers_from_errors() { + let browser = Arc::new(MutableBrowser::default()); + { + let mut tree = browser.tree.lock().unwrap(); + tree.add_file("/Shoot/Clip.csv", "/files/clip.csv", 1, None); + tree.add_file("/Shoot/notes.txt", "/files/notes.txt", 1, None); + tree.add_file("/Shoot/Sub/Child.csv", "/files/child.csv", 1, None); + } + let mut session = Session { browser: Some(browser.clone()), ..Default::default() }; + session.prefs.general.language = "en".into(); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).with_step_dt(1.0 / 60.0).build_eframe(|_| { + let mut app = EffectcraftApp::new(session); + app.ui.start_screen = false; + app.show_panel(PanelKind::MediaBrowser); + app.ui.maximized = Some(PanelKind::MediaBrowser); + app + }); + h.run_steps(3); + let (time, first) = listing(&h, false); + assert_eq!(names(&first), ["Sub", "Clip.csv", "notes.txt"]); + assert!(h.state().auto.find("mediaBrowser.entry.Clip.csv").is_some()); + browser.tree.lock().unwrap().add_file("/Shoot/Added.csv", "/files/added.csv", 2, Some(5)); + at(&mut h, time + 0.999); + assert!(Arc::ptr_eq(&first, &listing(&h, false).1)); + assert!(h.state().auto.find("mediaBrowser.entry.Added.csv").is_none()); + at(&mut h, time + 1.0); + let refreshed = listing(&h, false).1; + assert_eq!(names(&refreshed), ["Sub", "Added.csv", "Clip.csv", "notes.txt"]); + assert!(h.state().auto.find("mediaBrowser.entry.Added.csv").is_some()); + assert_eq!(names(&first), ["Sub", "Clip.csv", "notes.txt"]); + click(&mut h, "mediaBrowser.importableOnly"); + assert!(h.state().session.state.media_browser.importable_only); + assert_eq!(names(&listing(&h, true).1), ["Sub", "Added.csv", "Clip.csv"]); + assert!(h.state().auto.find("mediaBrowser.entry.notes.txt").is_none()); + click(&mut h, "mediaBrowser.importableOnly"); + assert!(!h.state().session.state.media_browser.importable_only); + assert!(Arc::ptr_eq(&refreshed, &listing(&h, false).1)); + click(&mut h, "mediaBrowser.action.refresh"); + assert_eq!(names(&listing(&h, false).1), ["Sub", "Added.csv", "Clip.csv", "From action.csv", "notes.txt"]); + assert!(h.state().auto.find("mediaBrowser.entry.From action.csv").is_some()); + assert_eq!(names(&refreshed), ["Sub", "Added.csv", "Clip.csv", "notes.txt"]); + h.run_steps(40); + click(&mut h, "mediaBrowser.entry.Sub"); + click(&mut h, "mediaBrowser.entry.Sub"); + assert_eq!(h.state().session.state.media_browser.folder.as_deref(), Some("/Shoot/Sub")); + assert_eq!(names(&listing(&h, false).1), ["Child.csv"]); + assert!(h.state().auto.find("mediaBrowser.entry.Child.csv").is_some()); + browser.tree.lock().unwrap().remove("/Shoot/Sub/Child.csv"); + let time = listing(&h, false).0; + at(&mut h, time + 1.0); + assert_eq!(names(&listing(&h, false).1), Vec::<&str>::new()); + assert!(h.state().auto.find("mediaBrowser.entry.Child.csv").is_none()); + click(&mut h, "mediaBrowser.up"); + assert_eq!(h.state().session.state.media_browser.folder.as_deref(), Some("/Shoot")); + assert!(h.state().auto.find("mediaBrowser.entry.Clip.csv").is_some()); + browser.offline.store(true, Ordering::Relaxed); + let time = listing(&h, false).0; + at(&mut h, time + 1.0); + let (time, error) = listing(&h, false); + assert_eq!(error.as_ref(), &Err("Folder temporarily unavailable".into())); + assert!(h.output().shapes.iter().any(|s| matches!(&s.shape, egui::Shape::Text(t) if t.galley.text() == "Folder temporarily unavailable"))); + assert!(h.state().auto.find("mediaBrowser.entry.Clip.csv").is_none()); + browser.offline.store(false, Ordering::Relaxed); + browser.tree.lock().unwrap().remove("/Shoot/notes.txt"); + at(&mut h, time + 0.999); + assert!(Arc::ptr_eq(&error, &listing(&h, false).1)); + at(&mut h, time + 1.0); + assert_eq!(names(&listing(&h, false).1), ["Sub", "Added.csv", "Clip.csv", "From action.csv"]); + assert!(h.state().auto.find("mediaBrowser.entry.Clip.csv").is_some()); + if let Ok(path) = std::env::var("MEDIA_BROWSER_SNAPSHOT") { + h.render().unwrap().save(path).unwrap(); + } +} From 8041329df8b145a6c38191fc6a620bbc05b38280 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Wed, 14 Oct 2026 11:42:13 +0530 Subject: [PATCH 04/16] feat(media-browser): add Copy Source Path --- crates/ui-egui/src/i18n/ui.rs | 7 + crates/ui-egui/src/menus.rs | 7 +- crates/ui-egui/src/panels/media_panels.rs | 8 ++ .../tests/ui_media_browser_copy_path.rs | 125 ++++++++++++++++++ docs/control-protocol.md | 1 + 5 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 crates/ui-egui/tests/ui_media_browser_copy_path.rs diff --git a/crates/ui-egui/src/i18n/ui.rs b/crates/ui-egui/src/i18n/ui.rs index 3a4dd4aff..bca5f66cd 100644 --- a/crates/ui-egui/src/i18n/ui.rs +++ b/crates/ui-egui/src/i18n/ui.rs @@ -228,6 +228,7 @@ const JAPANESE: &[(&str, &str)] = &[ ("EffectCraft didn't quit normally last time.", "EffectCraftは前回正常に終了しませんでした。"), ("Effects & Presets options", "エフェクト＆プリセットのオプション"), ("Copy Presets Folder Path", "プリセットフォルダーのパスをコピー"), + ("Copy Source Path", "ソースパスをコピー"), ("Effects: {}", "エフェクト：{}"), ("Empty folder", "空のフォルダー"), ("Enable Depth of Field", "被写界深度を有効化"), @@ -772,6 +773,7 @@ const SIMPLIFIED_CHINESE: &[(&str, &str)] = &[ ("EffectCraft didn't quit normally last time.", "EffectCraft 上次未正常退出。"), ("Effects & Presets options", "效果和预设选项"), ("Copy Presets Folder Path", "复制预设文件夹路径"), + ("Copy Source Path", "复制源路径"), ("Effects: {}", "效果：{}"), ("Empty folder", "空文件夹"), ("Enable Depth of Field", "启用景深"), @@ -1504,6 +1506,7 @@ const TRADITIONAL_CHINESE: &[(&str, &str)] = &[ ("EffectCraft didn't quit normally last time.", "EffectCraft 上次未正常結束。"), ("Effects & Presets options", "效果和預設集選項"), ("Copy Presets Folder Path", "複製預設集資料夾路徑"), + ("Copy Source Path", "複製來源路徑"), ("Effects: {}", "效果：{}"), ("Empty folder", "空資料夾"), ("Enable Depth of Field", "啟用景深"), @@ -2239,6 +2242,7 @@ const UKRAINIAN: &[(&str, &str)] = &[ ("EffectCraft didn't quit normally last time.", "Минулого разу EffectCraft завершив роботу некоректно."), ("Effects & Presets options", "Параметри ефектів і наборів"), ("Copy Presets Folder Path", "Копіювати шлях до папки наборів"), + ("Copy Source Path", "Копіювати шлях до джерела"), ("Effects: {}", "Ефекти: {}"), ("Empty folder", "Порожня тека"), ("Enable Depth of Field", "Увімкнути глибину різкості"), @@ -2791,6 +2795,7 @@ const PORTUGUESE: &[(&str, &str)] = &[ ("EffectCraft didn't quit normally last time.", "O EffectCraft não foi encerrado normalmente da última vez."), ("Effects & Presets options", "Opções de Efeitos e predefinições"), ("Copy Presets Folder Path", "Copiar caminho da pasta de predefinições"), + ("Copy Source Path", "Copiar caminho da origem"), ("Effects: {}", "Efeitos: {}"), ("Empty folder", "Pasta vazia"), ("Enable Depth of Field", "Ativar profundidade de campo"), @@ -3241,6 +3246,7 @@ const CZECH: &[(&str, &str)] = &[ ("EffectCraft didn't quit normally last time.", "EffectCraft nebyl při posledním použití ukončen běžným způsobem."), ("Effects & Presets options", "Možnosti efektů a přednastavení"), ("Copy Presets Folder Path", "Kopírovat cestu ke složce přednastavení"), + ("Copy Source Path", "Kopírovat cestu ke zdroji"), ("Effects: {}", "Efekty: {}"), ("Empty folder", "Prázdná složka"), ("Enable Depth of Field", "Povolit hloubku ostrosti"), @@ -3653,6 +3659,7 @@ const CZECH: &[(&str, &str)] = &[ /// order. Terms follow the Spanish menu catalog (composición, material de archivo, cola de procesamiento, /// ajuste preestablecido, método abreviado). Written from the English source. const SPANISH: &[(&str, &str)] = &[ + ("Copy Source Path", "Copiar ruta de origen"), ("Appearance Settings…", "Ajustes de apariencia…"), ("Add to favorites", "Añadir a favoritos"), ("Remove from favorites", "Quitar de favoritos"), diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 8b5fd1c30..b1eba4f0c 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -291,7 +291,7 @@ fn only_comp(p: &Value) -> Option { /// Execute a UI or engine command by id. pub fn invoke(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, params: Value) -> Result { - if id == "effects.copyPresetsFolderPath" { + if matches!(id, "effects.copyPresetsFolderPath" | "mediaBrowser.copySourcePath") { return frontend(app, ctx, id, params); } // Layer ▸ New ▸ Text (menu, shortcut, context menus) puts an insertion point in the centre of @@ -880,6 +880,11 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu let now = ctx.input(|i| i.time); let v = &mut app.ui.viewer; Ok(match id { + "mediaBrowser.copySourcePath" => { + let path = p.get("path").and_then(Value::as_str).filter(|path| !path.is_empty()).ok_or("A source path is required")?; + ctx.copy_text(path.to_string()); + json!({"path": path}) + } "effects.copyPresetsFolderPath" => { if cfg!(target_arch = "wasm32") { return Err("The web app has no local presets folder".into()); diff --git a/crates/ui-egui/src/panels/media_panels.rs b/crates/ui-egui/src/panels/media_panels.rs index 4a0ab6ae7..75c966912 100644 --- a/crates/ui-egui/src/panels/media_panels.rs +++ b/crates/ui-egui/src/panels/media_panels.rs @@ -209,6 +209,14 @@ pub fn media_browser(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { egui::DragAndDrop::set_payload(&ctx, DragPayload::Files(vec![e.path.clone()])); } crate::widgets::context_menu(&resp, |ui| { + let copy = ui.button(tr("Copy Source Path")); + app.auto.add(&format!("mediaBrowser.entry.{}.copySourcePath", e.name), copy.rect, "Copy Source Path"); + if copy.clicked() { + if let Err(e) = crate::menus::invoke(app, &ctx, "mediaBrowser.copySourcePath", json!({"path": e.path})) { + app.ui.status = e; + } + ui.close(); + } if !e.is_dir && ui.button(tr("Import")).clicked() { actions.push(("mediaBrowser.import", json!({"paths": [e.path]}))); ui.close(); diff --git a/crates/ui-egui/tests/ui_media_browser_copy_path.rs b/crates/ui-egui/tests/ui_media_browser_copy_path.rs new file mode 100644 index 000000000..8da34c5bd --- /dev/null +++ b/crates/ui-egui/tests/ui_media_browser_copy_path.rs @@ -0,0 +1,125 @@ +use std::sync::Arc; + +use effectcraft_engine::{ + Session, + media_browser::{Browser, Entry, VirtualTree}, +}; +use effectcraft_ui_egui::{ControlRequest, EffectcraftApp, control, dock::PanelKind}; +use egui::{Event, Modifiers, OutputCommand, PointerButton, pos2}; +use egui_kittest::Harness; +use serde_json::{Value, json}; + +struct TestBrowser(VirtualTree); +impl Browser for TestBrowser { + fn home(&self) -> String { + "/Media".into() + } + fn list(&self, dir: &str, only: bool) -> Result, String> { + self.0.list(dir, only) + } +} + +fn click(h: &mut Harness<'_, EffectcraftApp>, id: &str, button: PointerButton) -> Vec { + let e = h.state().auto.find(id).unwrap_or_else(|| panic!("missing {id}")); + let pos = pos2(e.rect[0] + e.rect[2] / 2.0, e.rect[1] + e.rect[3] / 2.0); + h.input_mut().events.push(Event::PointerMoved(pos)); + h.step(); + let mut copied = Vec::new(); + for pressed in [true, false] { + h.input_mut().events.push(Event::PointerButton { pos, button, pressed, modifiers: Modifiers::NONE }); + h.step(); + copied.extend(h.output().platform_output.commands.iter().filter_map(|c| match c { + OutputCommand::CopyText(text) => Some(text.clone()), + _ => None, + })); + } + copied +} + +#[test] +fn file_folder_and_virtual_paths_copy_from_the_context_menu_without_document_changes() { + let cases = [ + ("作品 clip.csv", "/Users/example/Shoot 01/作品 clip.csv"), + ("Notes.txt", "./relative notes.txt"), + ("Virtual.csv", "browser://assets/opaque%20作品.csv?version=2"), + ("Folder 作品", "/Media/Folder 作品"), + ]; + let mut tree = VirtualTree::default(); + for (name, path) in cases.iter().take(3) { + tree.add_file(&format!("/Media/{name}"), path, 1, None); + } + tree.add_dir("/Media/Folder 作品"); + let mut session = Session { browser: Some(Arc::new(TestBrowser(tree))), ..Default::default() }; + session.prefs.general.language = "en".into(); + session.execute("comp.new", json!({"width": 64, "height": 36, "duration": 2})).unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).with_step_dt(1.0 / 60.0).build_eframe(|_| { + let mut app = EffectcraftApp::new(session); + app.ui.start_screen = false; + app.show_panel(PanelKind::MediaBrowser); + app.ui.maximized = Some(PanelKind::MediaBrowser); + app + }); + h.run_steps(3); + let project = h.state().session.project.clone(); + let revision = h.state().session.revision; + let history = h.state().session.history.undo.len(); + let browser = serde_json::to_value(&h.state().session.state.media_browser).unwrap(); + for (name, path) in cases { + let id = format!("mediaBrowser.entry.{name}.copySourcePath"); + assert!(h.state().auto.find(&id).is_none()); + assert!(click(&mut h, &format!("mediaBrowser.entry.{name}"), PointerButton::Secondary).is_empty()); + h.run_steps(3); + assert!(h.state().auto.find(&id).is_some()); + if name == "作品 clip.csv" + && let Ok(file) = std::env::var("MEDIA_BROWSER_COPY_SNAPSHOT") + { + h.render().unwrap().save(file).unwrap(); + } + assert_eq!(click(&mut h, &id, PointerButton::Primary), [path]); + h.run_steps(3); + assert!(h.state().auto.find(&id).is_none()); + assert_eq!(h.state().session.project, project); + assert_eq!(h.state().session.revision, revision); + assert_eq!(h.state().session.history.undo.len(), history); + assert_eq!(serde_json::to_value(&h.state().session.state.media_browser).unwrap(), browser); + } +} + +#[test] +fn copy_path_control_command_preserves_the_path_and_rejects_bad_arguments() { + let mut app = EffectcraftApp::new(Session::default()); + let ctx = egui::Context::default(); + let project = app.session.project.clone(); + let revision = app.session.revision; + let history = app.session.history.undo.len(); + for params in [json!({"path": " ./作品 clip.csv "}), json!({}), json!({"path": ""}), json!({"path": 42}), json!({"path": null})] { + let mut reply = None; + let mut out = ctx.run_ui(Default::default(), |ui| { + let (request, _rx) = ControlRequest::new("ui.menu.invoke", json!({"id": "mediaBrowser.copySourcePath", "params": params})); + reply = match control::handle(&mut app, ui.ctx(), &request) { + control::Outcome::Done(value) => Some(value), + _ => panic!("expected immediate reply"), + }; + }); + out.textures_delta.clear(); + let copied: Vec<_> = out + .platform_output + .commands + .iter() + .filter_map(|c| match c { + OutputCommand::CopyText(text) => Some(text.as_str()), + _ => None, + }) + .collect(); + if params.get("path").and_then(Value::as_str).is_some_and(|s| !s.is_empty()) { + assert_eq!(reply, Some(json!({"ok": true, "result": {"path": " ./作品 clip.csv "}}))); + assert_eq!(copied, [" ./作品 clip.csv "]); + } else { + assert_eq!(reply, Some(json!({"ok": false, "error": "A source path is required"}))); + assert!(copied.is_empty()); + } + assert_eq!(app.session.project, project); + assert_eq!(app.session.revision, revision); + assert_eq!(app.session.history.undo.len(), history); + } +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 5f7a3b51f..4d01ef2e9 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -37,6 +37,7 @@ printf '{"id":1,"method":"engine.execute","params":{"command":"comp.new","params |---|---|---| | `engine.execute` | `{command, params?}` | The command's result. Every engine command (`effectcraft-cli commands`) runs exactly as it does headless, so it never opens a dialog: `comp.new {}` creates a default comp. UI-only ids (`tool.*`, `view.*`, `window.*`, `playback.*`, `timeline.*`, `app.*`) fall through to the menu dispatcher. Unknown top-level params are rejected, and the error lists the accepted keys. | | `ui.menu.invoke` | `{id, params?}` | Like a menu click: commands with empty params may open their dialog (New Composition, Solid Settings, file pickers). | +| Copy a Media Browser source path | `ui.menu.invoke {id: "mediaBrowser.copySourcePath", params: {path}}` | Copies the exact non-empty path string to the clipboard and returns `{path}`. Supports file/folder paths and virtual source identifiers. Missing, empty or non-string paths return an error. | | UI commands (via either) | | `timeline.column {column, visible?}` (av, keys, label, num, comment, switches, modes, parent, in, out, duration, stretch), `timeline.sourceName {value?}`, `timeline.search {query}`, `timeline.reveal.` / `timeline.revealAdd.` (the property shortcuts and Shift+shortcut: position, scale, rotation, opacity, anchor, effects, masks, feather, levels, animated, and the double-press sets maskPath, maskOpacity, timeRemap, expressions, material, paint, selected, missingEffects, waveform), `timeline.keyAt.` (Alt+Shift+A/P/S/R/T), `timeline.twirlSelected` (Ctrl/Cmd+`), `timeline.collapseAll`, `flowchart.options {layers?, effects?, solids?, direction?: lr\|tb, comp?}`, `flowchart.graph {comp?}` → `{nodes, edges}`, `window.workspace {name}` (all After Effects workspaces). | | `engine.commands` | `{filter?, enabledOnly?}` | `[{id, label, menu, shortcut, params, enabled, why}]` | | `ui.menu.list` | `{}` | The menu tree with enablement. | From bed289e2a19b96c88d6ef6ea3778704cadc997c0 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Wed, 14 Oct 2026 12:09:48 +0530 Subject: [PATCH 05/16] fix(presets): identify copy folder action by label Follow up on #1210: remove the fixed menu index from automation and click handling, and move the preset-folder documentation into the Effects & Presets section. --- crates/ui-egui/src/panels/effects_presets.rs | 15 +++---- crates/ui-egui/tests/ui_presets_folder.rs | 41 +++++++++++--------- docs/agents.md | 18 ++++----- 3 files changed, 39 insertions(+), 35 deletions(-) diff --git a/crates/ui-egui/src/panels/effects_presets.rs b/crates/ui-egui/src/panels/effects_presets.rs index d3921cd85..bb81abf80 100644 --- a/crates/ui-egui/src/panels/effects_presets.rs +++ b/crates/ui-egui/src/panels/effects_presets.rs @@ -288,6 +288,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { app.auto.add("effects.menu", mr, "Effects & Presets options"); let view = app.ui.effects_view.clone(); let mark = |on: bool, l: &str| if on { format!("✓ {l}") } else { format!(" {l}") }; + let copy_label = format!(" {}", tr("Copy Presets Folder Path")); let menu = vec![ mark(view.depth == "all", "Show Effects for All Color Depths"), mark(view.depth == "32", "Show 32 bpc Effects Only"), @@ -301,7 +302,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { " Refresh List".into(), " Clear Recently Used".into(), "-".into(), - format!(" {}", tr("Copy Presets Folder Path")), + copy_label.clone(), ]; let mut force_scan = false; let separators = menu.iter().filter(|label| *label == "-").count(); @@ -314,7 +315,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { ui.separator(); } else { let resp = ui.selectable_label(false, label); - if i == 12 { + if label == ©_label { app.auto.add("effects.copyPresetsFolderPath", resp.rect, "Copy Presets Folder Path"); } if resp.clicked() { @@ -330,6 +331,11 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { if let Some(i) = choice { let v = &mut app.ui.effects_view; match i { + i if menu.get(i) == Some(©_label) => { + if let Err(e) = crate::menus::invoke(app, &ctx, "effects.copyPresetsFolderPath", json!({})) { + app.ui.status = e; + } + } 0 => v.depth = "all".into(), 1 => v.depth = "32".into(), 2 => v.depth = "gpu".into(), @@ -338,11 +344,6 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { 7 => v.alphabetical = true, 9 => force_scan = true, 10 => app.ui.effects_recent.clear(), - 12 => { - if let Err(e) = crate::menus::invoke(app, &ctx, "effects.copyPresetsFolderPath", json!({})) { - app.ui.status = e; - } - } _ => {} } } diff --git a/crates/ui-egui/tests/ui_presets_folder.rs b/crates/ui-egui/tests/ui_presets_folder.rs index 7ba632a6f..8d5491631 100644 --- a/crates/ui-egui/tests/ui_presets_folder.rs +++ b/crates/ui-egui/tests/ui_presets_folder.rs @@ -34,26 +34,29 @@ fn click(h: &mut Harness<'_, EffectcraftApp>, id: &str) -> Vec { #[test] fn presets_menu_copies_the_configured_folder_without_editing_the_project() { let expected = expected_folder(); - let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| { - let mut app = EffectcraftApp::new(Session::default()); - app.ui.start_screen = false; - app.show_panel(PanelKind::EffectsPresets); - app - }); - h.run_steps(3); - let before = serde_json::to_value(&h.state().session.project).unwrap(); - let revision = h.state().session.revision; - assert!(h.state().auto.find("effects.copyPresetsFolderPath").is_none()); - click(&mut h, "effects.menu"); - h.run_steps(3); - if let Ok(path) = std::env::var("PRESETS_FOLDER_SNAPSHOT") { - h.render().unwrap().save(path).unwrap(); + for language in ["en", "ja"] { + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| { + let mut app = EffectcraftApp::new(Session::default()); + app.session.prefs.general.language = language.into(); + app.ui.start_screen = false; + app.show_panel(PanelKind::EffectsPresets); + app + }); + h.run_steps(3); + let before = serde_json::to_value(&h.state().session.project).unwrap(); + let revision = h.state().session.revision; + assert!(h.state().auto.find("effects.copyPresetsFolderPath").is_none()); + click(&mut h, "effects.menu"); + h.run_steps(3); + if let Ok(path) = std::env::var("PRESETS_FOLDER_SNAPSHOT") { + h.render().unwrap().save(path).unwrap(); + } + assert_eq!(click(&mut h, "effects.copyPresetsFolderPath"), [expected.as_str()]); + h.run_steps(2); + assert!(h.state().auto.find("effects.copyPresetsFolderPath").is_none()); + assert_eq!(serde_json::to_value(&h.state().session.project).unwrap(), before); + assert_eq!(h.state().session.revision, revision); } - assert_eq!(click(&mut h, "effects.copyPresetsFolderPath"), [expected]); - h.run_steps(2); - assert!(h.state().auto.find("effects.copyPresetsFolderPath").is_none()); - assert_eq!(serde_json::to_value(&h.state().session.project).unwrap(), before); - assert_eq!(h.state().session.revision, revision); } #[test] diff --git a/docs/agents.md b/docs/agents.md index 1bec71d51..872065f64 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -10,15 +10,6 @@ user can do, an agent can do too. There are three ways in: | **CLI** (`effectcraft-cli exec/get/set/...`) | one-shot scripting and CI; JSON output with `--json` | no | | **Control channel** (`effectcraft --control 9877`) | driving and seeing the live UI; see [control-protocol.md](control-protocol.md) | yes | -## Find the presets folder - -The native Effects & Presets panel's options menu includes **Copy Presets Folder Path**. It copies -the folder the panel scans: `EC_PRESETS_DIR` when set, otherwise `~/Documents/EffectCraft/Presets` -using `HOME` or `USERPROFILE`. Relative paths are resolved to an absolute path. The folder may -not exist yet; this action does not create it. -Over the live UI control channel, `ui.menu.invoke {"id":"effects.copyPresetsFolderPath"}` copies -the same path and returns `{path}`. The web app reports that it has no local presets folder. - ## MCP setup Build once with `cargo build --release -p effectcraft-cli`, then register the server. For Claude @@ -714,3 +705,12 @@ open the demo project; without it the app starts with an empty project) and use mode or the raw control channel. A good loop is: `ui_elements` to find an id, `ui_click` or `ui_drag` to act, then `screenshot {"panel":"Timeline"}` to check the result. `render_frame` shows the composition itself at any zoom. + +### Effects & Presets + +The native Effects & Presets panel's options menu includes **Copy Presets Folder Path**. It copies +the folder the panel scans: `EC_PRESETS_DIR` when set, otherwise `~/Documents/EffectCraft/Presets` +using `HOME` or `USERPROFILE`. Relative paths are resolved to an absolute path. The folder may +not exist yet; this action does not create it. +Over the live UI control channel, `ui.menu.invoke {"id":"effects.copyPresetsFolderPath"}` copies +the same path and returns `{path}`. The web app reports that it has no local presets folder. From 68d928aff325c9af3682c840b4ef7e14a015b97a Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Wed, 14 Oct 2026 13:42:59 +0530 Subject: [PATCH 06/16] fix(render): preserve preceding effects in GPU Echo (#1120) --- crates/gpu/src/tests_fx_noise.rs | 22 ++++++++++++++++++++++ crates/render/src/lib.rs | 26 +++++++++++++++++--------- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/crates/gpu/src/tests_fx_noise.rs b/crates/gpu/src/tests_fx_noise.rs index 0fc384425..476f08642 100644 --- a/crates/gpu/src/tests_fx_noise.rs +++ b/crates/gpu/src/tests_fx_noise.rs @@ -203,6 +203,28 @@ fn echo_and_posterize_time() { } } +#[test] +fn echo_after_star_burst_preserves_the_stars() { + for depth in [BitDepth::Bpc8, BitDepth::Bpc32] { + for operator in 0..7 { + for posterize in [false, true] { + let mut s = Scene::new(depth); + let mut l = s.solid([1.0; 3], 97, 61); + s.effect(&mut l, "ec.sim.ccstarburst", &[("gridSpacing", n(8.0)), ("size", n(40.0))]); + s.effect(&mut l, "ec.time.echo", &[("numberOfEchoes", n(2.0)), ("echoTime", n(-0.1)), ("decay", n(0.6)), ("echoOperator", e(operator))]); + if posterize { + s.effect(&mut l, "ec.time.posterizetime", &[("frameRate", n(4.0))]); + } + s.push(l); + let t = Tick::from_seconds_f64(0.6); + let label = format!("Star Burst + Echo {operator} posterize={posterize} {depth:?}"); + check(&label, compare_at(&s, opts(), t), 0.0); + check(&format!("{label} half"), compare_at(&s, RenderOpts { scale: 0.5, ..opts() }, t), 0.0); + } + } + } +} + #[test] fn median_levels_match_cpu_division() { let Some(g) = crate::tests::gpu() else { return }; diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 41036fe6b..808356f12 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -216,7 +216,7 @@ struct FxHost<'r, 'a, 'c> { /// whose effects run on the comp below). origin: [f64; 2], /// Index of the effect being rendered (bounds `self_at` to effects before it). - index: std::sync::atomic::AtomicUsize, + index: usize, original: Option<&'c Buf>, } @@ -307,7 +307,7 @@ impl EffectHost for FxHost<'_, '_, '_> { if self.r.depth > MAX_FX_DEPTH || self.layer.switches.adjustment { return None; } - let n = effects.min(self.index.load(std::sync::atomic::Ordering::Relaxed)); + let n = effects.min(self.index); let t = self.layer.comp_time(Tick::from_seconds_f64(layer_time)); let sub = self.r.nested(); sub.layer_input(&self.ctx.at(t), self.layer, n).map(|b| (*b).clone()) @@ -337,8 +337,7 @@ impl EffectHost for FxHost<'_, '_, '_> { } fn params_at(&self, layer_time: f64) -> Option { - let i = self.index.load(std::sync::atomic::Ordering::Relaxed); - let g = self.layer.effects()?.groups().nth(i)?; + let g = self.layer.effects()?.groups().nth(self.index)?; let ctx = self.ctx.at(self.layer.comp_time(Tick::from_seconds_f64(layer_time))); Some(effectcraft_effects::flatten_params(g, &mut |pr| ctx.value(self.layer, pr))) } @@ -1158,7 +1157,7 @@ impl<'a> Renderer<'a> { buf }); } - let host = FxHost { r: self, ctx, layer, origin, index: Default::default(), original: original.as_ref() }; + let host = FxHost { r: self, ctx, layer, origin, index: 0, original: original.as_ref() }; let env = EffectEnv { masks: &mask_shapes, host: Some(&host), @@ -1192,12 +1191,21 @@ impl<'a> Renderer<'a> { { let run: Vec<_> = stack[k..].iter().take_while(|(_, _, s)| s.gpu && a.supports_effect(s.id)).collect(); let params: Vec = run.iter().map(|(_, g, _)| self.effect_params(ctx, layer, g)).collect(); + let hosts: Vec<_> = run.iter().map(|(i, _, _)| FxHost { index: *i, ..host }).collect(); let steps: Vec = run .iter() .zip(¶ms) - .map(|((i, g, s), params)| FxStep { + .zip(&hosts) + .map(|(((i, g, s), params), host)| FxStep { spec: s, - ctx: EffectCtx { params, time: lt.seconds(), layer_size, seed: g.uid as u32, adjustment, env: EffectEnv { effect_index: *i, ..env } }, + ctx: EffectCtx { + params, + time: lt.seconds(), + layer_size, + seed: g.uid as u32, + adjustment, + env: EffectEnv { host: Some(host), effect_index: *i, ..env }, + }, }) .collect(); let t0 = web_time::Instant::now(); @@ -1212,8 +1220,8 @@ impl<'a> Renderer<'a> { } k += 1; let params = self.effect_params(ctx, layer, g); - host.index.store(i, std::sync::atomic::Ordering::Relaxed); - let env = EffectEnv { effect_index: i, ..env }; + let host = FxHost { index: i, ..host }; + let env = EffectEnv { host: Some(&host), effect_index: i, ..env }; let ectx = EffectCtx { params: ¶ms, time: lt.seconds(), layer_size, seed: g.uid as u32, adjustment, env }; let t0 = web_time::Instant::now(); target.cpu(&mut |buf| { From c280506ba0261cc1c34656857907d18410f4cdcd Mon Sep 17 00:00:00 2001 From: DWAA1660 Date: Wed, 14 Oct 2026 20:51:09 -0600 Subject: [PATCH 07/16] fix(config): isolate atomic writes and preserve long filenames Create each atomic write through its own exclusively created temporary sibling and keep its open file handle until writing finishes. Concurrent writers publish complete files without truncating or following an existing temporary entry; failed writes or renames clean up their own staging file. Keep the temporary basename bounded independently of the destination name so previously supported long project filenames still save. Retain the concurrent-writer and symlink regressions and add a 251-byte filename overwrite regression that checks exact bytes and staging cleanup. Fixes #1233 Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/config.rs | 46 ++++++++++++++------ crates/engine/src/tests_settings.rs | 66 +++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/config.rs b/crates/engine/src/config.rs index c333c9734..00e7377b7 100644 --- a/crates/engine/src/config.rs +++ b/crates/engine/src/config.rs @@ -164,32 +164,50 @@ impl ConfigStore for DirConfig { } } -/// The temporary file an atomic write of `path` goes through. -pub fn temp_path(path: &Path) -> PathBuf { - let mut name = path.file_name().map(|n| n.to_os_string()).unwrap_or_default(); - name.push(".tmp"); - path.with_file_name(name) +/// A new, empty temporary file next to `path` for an atomic write of it, and its path. The name +/// is unique to this process and this write (`.effectcraft--.tmp`) and the file is created +/// exclusively, so two writers never share one and nothing already at that name (a file, a +/// symbolic link) is opened or followed (#1233). It ends in `.tmp`, so a file left by a hard +/// kill never counts as an auto-save. Its bounded name does not include the destination's name, +/// so a valid long destination name still fits the filesystem's component limit. +fn create_temp(path: &Path) -> std::io::Result<(std::fs::File, PathBuf)> { + use std::sync::atomic::{AtomicU64, Ordering}; + static NEXT: AtomicU64 = AtomicU64::new(0); + let mut last = None; + for _ in 0..16 { + let n = NEXT.fetch_add(1, Ordering::Relaxed); + let tmp = path.with_file_name(format!(".effectcraft-{}-{n}.tmp", std::process::id())); + match std::fs::OpenOptions::new().write(true).create_new(true).open(&tmp) { + Ok(f) => return Ok((f, tmp)), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last = Some(e), + Err(e) => return Err(e), + } + } + Err(last.unwrap_or_else(|| std::io::Error::other("no free temporary file name"))) } /// Write `data` to `path` atomically: write a temporary file next to it, flush it to disk, then /// rename it over `path`. A crash at any point leaves either the old file or the new one, never a -/// torn file. +/// torn file, and concurrent writers each publish a whole file of their own. pub fn atomic_write(path: &Path, data: &[u8]) -> std::io::Result<()> { - atomic_write_with(path, data, |tmp, data| { + atomic_write_with(path, data, |f, data| { use std::io::Write; - let mut f = std::fs::File::create(tmp)?; f.write_all(data)?; f.sync_all() }) } -/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). A -/// failed write removes the temporary file and leaves `path` untouched. -pub fn atomic_write_with(path: &Path, data: &[u8], write: impl FnOnce(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { - let tmp = temp_path(path); - if let Err(e) = write(&tmp, data) { +/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write); it is +/// given the temporary file, already created. A failed write or rename removes the temporary +/// file and leaves `path` untouched. +pub fn atomic_write_with(path: &Path, data: &[u8], write: impl FnOnce(&mut std::fs::File, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { + let (mut f, tmp) = create_temp(path)?; + let written = write(&mut f, data); + // Closed before the rename (Windows can't rename an open file). + drop(f); + if let Err(e) = written.and_then(|()| std::fs::rename(&tmp, path)) { let _ = std::fs::remove_file(&tmp); return Err(e); } - std::fs::rename(&tmp, path) + Ok(()) } diff --git a/crates/engine/src/tests_settings.rs b/crates/engine/src/tests_settings.rs index 458c886fc..65e39b467 100644 --- a/crates/engine/src/tests_settings.rs +++ b/crates/engine/src/tests_settings.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use serde_json::json; -use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with, temp_path}; +use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with}; use crate::prefs::{PREFS_FILE, Prefs}; use crate::shortcuts::{DEFAULT_PRESET, Keymaps, normalize}; use crate::{Session, autosave}; @@ -548,6 +548,66 @@ fn frontend_commands_are_bindable_with_panel_scope() { // ---------------------------------------------------------------- auto-save / recovery +/// The names in `d`, sorted. +fn names(d: &std::path::Path) -> Vec { + let mut v: Vec = std::fs::read_dir(d).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + v.sort(); + v +} + +#[cfg(unix)] +#[test] +fn atomic_write_overwrites_a_long_valid_filename() { + let d = tmp("atomic-long-name"); + // The old `.tmp` fits a 255-byte component limit for this 251-byte project name. + // Including the full name plus a process id and counter made an otherwise valid save fail. + let name = format!("{}.ecproj", "x".repeat(244)); + assert_eq!(name.len(), 251); + let f = d.join(&name); + std::fs::write(&f, b"original project").unwrap(); + let replacement = b"replacement project\0bytes\xff"; + atomic_write(&f, replacement).unwrap(); + assert_eq!(std::fs::read(&f).unwrap(), replacement); + assert_eq!(names(&d), [name], "no temporary file is left after overwriting the project"); + let _ = std::fs::remove_dir_all(d); +} + +#[test] +fn concurrent_atomic_writes_each_publish_a_whole_file() { + // Writers of the same file used to share one `.tmp`: they truncated each other's data and + // the later rename failed once the first had moved it away (#1233). + let d = tmp("atomic-concurrent"); + let f = d.join("shared.ecproj"); + for round in 0..10 { + let contents: Vec> = (0..8u8).map(|w| vec![b'a' + w; 256 * 1024 + usize::from(w) * 1000]).collect(); + std::thread::scope(|sc| { + for c in &contents { + let f = &f; + sc.spawn(move || atomic_write(f, c).expect("every writer succeeds")); + } + }); + let got = std::fs::read(&f).unwrap(); + assert!(contents.contains(&got), "round {round}: the file is one writer's whole data ({} bytes)", got.len()); + assert_eq!(names(&d), ["shared.ecproj"], "round {round}: no temporary file is left"); + } + let _ = std::fs::remove_dir_all(d); +} + +#[cfg(unix)] +#[test] +fn atomic_write_does_not_follow_a_link_at_the_old_temp_name() { + let d = tmp("atomic-link"); + let f = d.join("p.ecproj"); + let victim = d.join("victim.txt"); + std::fs::write(&victim, b"keep").unwrap(); + // The temporary name used to be `.tmp`, opened with File::create (following links). + std::os::unix::fs::symlink(&victim, d.join("p.ecproj.tmp")).unwrap(); + atomic_write(&f, b"new project").unwrap(); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep", "the link's target is untouched"); + assert_eq!(std::fs::read(&f).unwrap(), b"new project"); + let _ = std::fs::remove_dir_all(d); +} + #[test] fn atomic_write_survives_a_crash_mid_write() { let d = tmp("atomic"); @@ -555,12 +615,12 @@ fn atomic_write_survives_a_crash_mid_write() { atomic_write(&f, b"version 1").unwrap(); // The writer dies halfway through the temporary file. let r = atomic_write_with(&f, b"version 2 is much longer", |tmp, data| { - std::fs::write(tmp, &data[..5])?; + std::io::Write::write_all(tmp, &data[..5])?; Err(std::io::Error::other("power cut")) }); assert!(r.is_err()); assert_eq!(std::fs::read(&f).unwrap(), b"version 1"); - assert!(!temp_path(&f).exists(), "the partial temp file is cleaned up"); + assert_eq!(names(&d), ["p.ecproj"], "the partial temp file is cleaned up"); // A temp file left by a hard kill doesn't count as an auto-save. std::fs::write(d.join("p auto-save 1.ecproj.tmp"), b"junk").unwrap(); assert!(autosave::existing(&d, "p").is_empty()); From 2c08f675c22fc9fbf9a300a60deadee9944ce3c4 Mon Sep 17 00:00:00 2001 From: DWAA1660 Date: Wed, 14 Oct 2026 21:15:40 -0600 Subject: [PATCH 08/16] fix(engine): preserve edits when applying background analyses Merge each offloaded property-group result against the snapshot the job started from. Preserve disjoint live edits, group identity and child changes; refuse conflicting results without changing the document or undo/redo history. Record an apply rejection on its originating job so completion logs and waiting control commands report failure. Keep concurrent jobs' outcomes separate and retain the correct completion position when the bounded log rolls over. Add actual-worker regressions for conflicts, disjoint edits, concurrent analyses and undo/redo, plus control completion/error regressions. Fixes #1249 Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/lib.rs | 3 + crates/engine/src/offload.rs | 180 ++++++++++++++++++++++++- crates/engine/src/tests_mask_warp.rs | 156 +++++++++++++++++++++ crates/ui-egui/src/control.rs | 17 ++- crates/ui-egui/tests/ui_web_offload.rs | 42 ++++++ 5 files changed, 388 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 80956687a..96e09b57a 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -495,6 +495,8 @@ pub struct Session { /// Offloaded jobs that ended, newest last (kind, error): how a caller waiting for one /// (`wait: true` over the control channel in the browser) learns its outcome. pub offload_log: Vec<(offload::JobKind, Option)>, + /// Number of older completion entries evicted from the bounded offload log. + pub offload_log_start: usize, /// Generic background tasks (Content-Aware Fill, Scene Edit Detection), see [`jobs`]. pub tasks: Vec, /// Finished tasks, newest last (Progress panel, `jobs.list`). @@ -598,6 +600,7 @@ impl Default for Session { offload: None, offloaded: vec![], offload_log: vec![], + offload_log_start: 0, browser: None, storage: None, tasks: vec![], diff --git a/crates/engine/src/offload.rs b/crates/engine/src/offload.rs index 561ebe6c4..bd1fec8a3 100644 --- a/crates/engine/src/offload.rs +++ b/crates/engine/src/offload.rs @@ -171,6 +171,20 @@ pub enum JobKind { } impl JobKind { + /// The corresponding `jobs.list` id, also used by waiting control commands. + pub fn job_id(self) -> &'static str { + match self { + JobKind::Render => "render", + JobKind::Warp => "warp", + JobKind::WarpPlan => "warpPlan", + JobKind::Camera => "camera", + JobKind::RotoFreeze | JobKind::RotoPropagate => "roto", + JobKind::Track => "track", + JobKind::MaskTrack => "maskTrack", + JobKind::ContentFill => "contentFill", + } + } + /// The undo step an analysis result becomes. pub fn label(self) -> &'static str { match self { @@ -699,6 +713,9 @@ pub struct OffloadedJob { /// Segmentations received (Roto Brush propagation). pub segs: u64, started: web_time::Instant, + /// The analysed group as the job started: its result is merged with what changed since + /// ([`merge_group`]). + base: Option>, } /// A Render Queue job running in a worker (see [`crate::render_queue::RenderJob`]). @@ -730,6 +747,7 @@ impl Session { } let id = next_id(); let inbox = Arc::new(Inbox::default()); + let base = self.project.comp(comp).and_then(|c| c.layer(layer)).and_then(|l| l.props.find_group(uid)).cloned().map(Box::new); let req = WorkerRequest { id, project: self.project.to_json(), files: footage_files(&self.project), job }; off.start(req, inbox.clone())?; self.offloaded.push(OffloadedJob { @@ -742,6 +760,7 @@ impl Session { progress: OffloadProgress::default(), segs: 0, started: web_time::Instant::now(), + base, }); self.events.push(Event::ProjectChanged { revision: self.revision }); Ok(()) @@ -779,7 +798,7 @@ impl Session { j.progress.done = done; j.progress.total = total; } - WorkerReply::Group { comp, layer, group, message } => results.push((j.kind, comp, layer, group, message)), + WorkerReply::Group { comp, layer, group, message } => results.push((j.id, j.kind, comp, layer, group, message, j.base.clone())), WorkerReply::Fill { plan, files } => fills.push((plan, files)), WorkerReply::WarpPlan { key, summary } => effectcraft_effects::warp_stab::store_summary(key, *summary), WorkerReply::Segs { segs } => { @@ -802,7 +821,7 @@ impl Session { Err(e) => self.events.push(Event::Toast { message: format!("Content-Aware Fill: {e}"), error: true }), } } - for (kind, comp, layer, group, message) in results { + for (id, kind, comp, layer, group, message, base) in results { let uid = group.uid; let r = self.edit(kind.label(), None, |p, _| { let g = p @@ -810,12 +829,31 @@ impl Session { .and_then(|c| c.layer_mut(layer)) .and_then(|l| l.props.find_group_mut(uid)) .ok_or_else(|| crate::EngineError::Other("the analysed property was removed during the analysis".into()))?; - *g = *group; + // Edits made while the job ran stay (#1249): only what the analysis changed is + // taken from its result. + *g = match base { + Some(base) => merge_group(&base, g, &group).ok_or_else(|| { + crate::EngineError::Other(format!( + "“{}” was changed during the analysis where its result changes it too, so nothing was applied: analyse again", + g.name + )) + })?, + None => *group, + }; Ok(()) }); match r { Ok(()) => self.events.push(Event::Toast { message, error: false }), - Err(e) => self.events.push(Event::Toast { message: format!("{}: {e}", kind.label()), error: true }), + Err(e) => { + // Completion consumers read the job's error, not its toasts. Keep an apply + // rejection on the originating job; Done logs and reports it just like a + // WorkerReply::Failed, without failing another concurrent analysis. + if let Some(job) = self.offloaded.iter_mut().find(|j| j.id == id) { + job.progress.error = Some(e.to_string()); + } else { + self.events.push(Event::Toast { message: format!("{}: {e}", kind.label()), error: true }); + } + } } } let mut done = vec![]; @@ -830,6 +868,7 @@ impl Session { self.offload_log.push((kind, p.error.clone().or_else(|| p.cancelled.then(|| "stopped".to_string())))); if self.offload_log.len() > 32 { self.offload_log.remove(0); + self.offload_log_start = self.offload_log_start.saturating_add(1); } if let Some(e) = p.error { self.events.push(Event::Toast { message: format!("{}: {e}", kind.label()), error: true }); @@ -880,6 +919,76 @@ impl Session { } } +/// `live` if only it changed since `base`, `result` if only that did (or both the same way); +/// `None` when both changed it differently. +fn pick(base: &T, live: &T, result: &T) -> Option { + if live == base { + Some(result.clone()) + } else if result == base || result == live { + Some(live.clone()) + } else { + None + } +} + +fn node_uid(n: &effectcraft_project::Node) -> Uid { + match n { + effectcraft_project::Node::Prop(p) => p.uid, + effectcraft_project::Node::Group(g) => g.uid, + } +} + +/// Merge an analysis `result` into the `live` group (#1249). `base` is the group as the job +/// started. The group's own fields and each property are taken from whichever side changed +/// them, groups are merged child by child (matched by uid), and a child added or removed by one +/// side is added or removed. `None` when both sides changed the same field or property +/// differently, one side removed a child the other changed, or both reordered or added and +/// removed children differently: a conflict. +pub(crate) fn merge_group(base: &PropGroup, live: &PropGroup, result: &PropGroup) -> Option { + use effectcraft_project::Node; + if live == base { + return Some(result.clone()); + } + if result == base || result == live { + return Some(live.clone()); + } + let header = |g: &PropGroup| PropGroup { children: vec![], ..g.clone() }; + let mut out = pick(&header(base), &header(live), &header(result))?; + let ids = |g: &PropGroup| g.children.iter().map(node_uid).collect::>(); + let (bi, li, ri) = (ids(base), ids(live), ids(result)); + // The children's order and set: from the side that changed them. + let order = if li == bi { + ri + } else if ri == bi || ri == li { + li + } else { + return None; + }; + fn find(g: &PropGroup, u: Uid) -> Option<&effectcraft_project::Node> { + g.children.iter().find(|n| node_uid(n) == u) + } + // A child one side removed must be unchanged on the other, or that side's edit would be lost. + for u in bi.iter().filter(|u| !order.contains(u)) { + let b = find(base, *u); + if [find(live, *u), find(result, *u)].into_iter().flatten().any(|n| Some(n) != b) { + return None; + } + } + for u in order { + let node = match (find(base, u), find(live, u), find(result, u)) { + (Some(Node::Group(b)), Some(Node::Group(l)), Some(Node::Group(r))) => Node::Group(merge_group(b, l, r)?), + (Some(b), Some(l), Some(r)) => pick(b, l, r)?, + // Added by one side, or by both alike. + (None, Some(l), None) => l.clone(), + (None, None, Some(r)) => r.clone(), + (None, Some(l), Some(r)) if l == r => l.clone(), + _ => return None, + }; + out.children.push(node); + } + Some(out) +} + #[cfg(test)] mod tests { use super::*; @@ -892,6 +1001,69 @@ mod tests { a } + /// A group with properties `a` (uid 2) and `b` (uid 3) and a sub-group (uid 4) holding `c` + /// (uid 5). + fn group() -> PropGroup { + use effectcraft_keyframe::Value as KV; + use effectcraft_project::{Node, Property}; + let mut sub = PropGroup::new(4, "sub", "Sub"); + sub.children.push(Node::Prop(Property::new(5, "c", "C", KV::Scalar(0.0)))); + let mut g = PropGroup::new(1, "fx", "Effect"); + g.children.push(Node::Prop(Property::new(2, "a", "A", KV::Scalar(0.0)))); + g.children.push(Node::Prop(Property::new(3, "b", "B", KV::Scalar(0.0)))); + g.children.push(Node::Group(sub)); + g + } + + fn set(g: &mut PropGroup, uid: Uid, v: f64) { + g.find_mut(uid).unwrap().value = effectcraft_keyframe::Value::Scalar(v); + } + + fn get(g: &PropGroup, uid: Uid) -> f64 { + match g.find(uid).unwrap().value { + effectcraft_keyframe::Value::Scalar(v) => v, + ref v => panic!("{v:?}"), + } + } + + #[test] + fn analysis_results_merge_with_edits_made_meanwhile() { + use effectcraft_project::Node; + let base = group(); + // The user changes `a` and switches the effect off; the analysis writes `b` and `c`. + let mut live = base.clone(); + set(&mut live, 2, 1.0); + live.enabled = false; + let mut result = base.clone(); + set(&mut result, 3, 2.0); + set(&mut result, 5, 3.0); + let m = merge_group(&base, &live, &result).unwrap(); + assert_eq!((get(&m, 2), get(&m, 3), get(&m, 5), m.enabled), (1.0, 2.0, 3.0, false)); + // Unchanged on either side: the other side wins whole. + assert_eq!(merge_group(&base, &base, &result).unwrap(), result); + assert_eq!(merge_group(&base, &live, &base).unwrap(), live); + // Both changed `b`, differently: a conflict (nothing is applied). The same way: fine. + let mut both = live.clone(); + set(&mut both, 3, 9.0); + assert!(merge_group(&base, &both, &result).is_none()); + set(&mut both, 3, 2.0); + assert_eq!(get(&merge_group(&base, &both, &result).unwrap(), 3), 2.0); + // The analysis adds a child (a result group) while the user edits `a`: both kept. + let mut added = result.clone(); + added.children.push(Node::Group(PropGroup::new(6, "out", "Output"))); + let m = merge_group(&base, &live, &added).unwrap(); + assert!(m.find_group(6).is_some() && get(&m, 2) == 1.0); + // The analysis drops the sub-group the user didn't touch: dropped. + let mut dropped = result.clone(); + dropped.children.retain(|n| !matches!(n, Node::Group(g) if g.uid == 4)); + let m = merge_group(&base, &live, &dropped).unwrap(); + assert!(m.find_group(4).is_none() && get(&m, 2) == 1.0); + // …but if the user changed something in it, that's a conflict. + let mut touched = live.clone(); + set(&mut touched, 5, 7.0); + assert!(merge_group(&base, &touched, &dropped).is_none()); + } + #[test] fn protocol_round_trips() { let mut s = Session::default(); diff --git a/crates/engine/src/tests_mask_warp.rs b/crates/engine/src/tests_mask_warp.rs index e08ce2e66..137feb1cb 100644 --- a/crates/engine/src/tests_mask_warp.rs +++ b/crates/engine/src/tests_mask_warp.rs @@ -412,6 +412,162 @@ impl Deferred { } } +/// #1249: a setting changed while the analysis runs in a worker survives its result, which +/// used to replace the whole effect with the copy the job started from. +#[test] +fn an_edit_made_during_an_offloaded_analysis_survives_its_result() { + let (mut s, clip, solid) = setup(shaky); + s.execute("edit.clear", json!({"layers": [solid.0]})).unwrap(); + s.execute("layer.select", json!({"layers": [clip.0]})).unwrap(); + s.execute("effect.apply", json!({"effect": "Warp Stabilizer"})).unwrap(); + set_param(&mut s, clip, "stabilization/smoothness", json!(37.5)); + let off = std::sync::Arc::new(Deferred::default()); + s.offload = Some(off.clone()); + s.execute_checked("warp.analyze", json!({"layer": clip.0})).unwrap(); + // While the job runs: another setting of the same effect. + set_param(&mut s, clip, "stabilization/smoothness", json!(12.5)); + let fx = |s: &Session| s.active_comp().unwrap().layer(clip).unwrap().props.prop("effects/#1/stabilization/smoothness").unwrap().value.clone(); + off.run(s.footage.clone()); + assert!(s.poll_offload()); + assert!(!s.drain_events().iter().any(|e| matches!(e, crate::Event::Toast { error: true, .. })), "applied without a conflict"); + let st = s.execute("warp.status", json!({})).unwrap(); + assert_eq!(st["analyzed"], true, "the analysis landed: {st}"); + assert_eq!(fx(&s), KV::Scalar(12.5), "the edit made during the analysis stays"); + // One undo step takes the analysis back and keeps the edit. + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(fx(&s), KV::Scalar(12.5)); + assert_eq!(s.execute("warp.status", json!({})).unwrap()["analyzed"], false); +} + +fn point_track_pose(f: u32) -> ([f64; 2], f64, f64) { + ([150.0 + 2.0 * f as f64, 110.0 + f as f64], 0.0, 1.0) +} + +/// Hold an actual point-tracking worker request so commands can edit its target before the +/// serialized worker reply is delivered. +fn held_point_track() -> (Session, LayerId, Uid, std::sync::Arc) { + let (mut s, clip, solid) = setup(patch_frames(point_track_pose)); + s.execute("edit.clear", json!({"layers": [solid.0]})).unwrap(); + s.execute("layer.select", json!({"layers": [clip.0]})).unwrap(); + let tracker = s.execute("track.motion", json!({})).unwrap()["tracker"].as_u64().unwrap(); + s.execute("track.setPoint", json!({"point": 1, "center": point_track_pose(0).0, "featureSize": [36, 36], "searchSize": [80, 80]})).unwrap(); + let off = std::sync::Arc::new(Deferred::default()); + s.offload = Some(off.clone()); + s.execute_checked("track.analyze", json!({"direction": "forward"})).unwrap(); + assert_eq!(off.kinds(), [crate::offload::JobKind::Track]); + (s, clip, tracker, off) +} + +fn tracked_center(s: &Session, clip: LayerId, tracker: Uid) -> Uid { + s.active_comp().unwrap().layer(clip).unwrap().props.find_group(tracker).unwrap().track_points().next().unwrap().get("featureCenter").unwrap().uid +} + +#[test] +fn offloaded_conflict_preserves_state_and_reports_completion_failure() { + let (mut s, clip, tracker, off) = held_point_track(); + let center = tracked_center(&s, clip, tracker); + s.execute("prop.set", json!({"layer": clip.0, "prop": center, "value": [180.0, 140.0]})).unwrap(); + s.execute("prop.renameGroup", json!({"layer": clip.0, "prop": tracker, "name": "Redo survives"})).unwrap(); + assert!(s.undo()); + let before = s.project.clone(); + let state = serde_json::to_value(&s.state).unwrap(); + let steps = |s: &Session| { + let entries = + |v: &[(String, std::sync::Arc)]| v.iter().map(|(label, p)| (label.clone(), p.to_json())).collect::>(); + ( + entries(&s.history.undo), + entries(&s.history.redo), + s.history.branches.iter().map(|b| (b.parent.to_json(), entries(&b.steps))).collect::>(), + s.history.merge_key.clone(), + ) + }; + let history = steps(&s); + let inbox = s.offloaded(crate::offload::JobKind::Track).unwrap().inbox.clone(); + off.run(s.footage.clone()); + let (done, replies): (Vec<_>, Vec<_>) = inbox.take().into_iter().partition(|r| matches!(r, crate::offload::WorkerReply::Done)); + assert_eq!(done.len(), 1); + for reply in replies { + inbox.push(reply); + } + s.drain_events(); + assert!(s.poll_offload()); + let pending = s.offloaded(crate::offload::JobKind::Track).unwrap(); + assert!(pending.progress.error.as_ref().is_some_and(|e| e.contains("nothing was applied"))); + assert!(!pending.progress.finished); + assert!(s.offload_log.is_empty(), "the error is kept before completion is logged"); + // The current completion remains identifiable after the bounded log evicts an old entry. + s.offload_log = vec![(crate::offload::JobKind::Warp, None); 32]; + for reply in done { + inbox.push(reply); + } + assert!(s.poll_offload()); + assert!(std::sync::Arc::ptr_eq(&s.project, &before), "a rejected result changes no document content"); + assert_eq!(serde_json::to_value(&s.state).unwrap(), state); + assert_eq!(steps(&s), history); + let errors: Vec<_> = s + .drain_events() + .into_iter() + .filter_map(|e| match e { + crate::Event::Toast { error: true, message } => Some(message), + _ => None, + }) + .collect(); + assert_eq!(errors.len(), 1, "the apply rejection is reported once: {errors:?}"); + assert!(errors[0].contains("nothing was applied")); + assert_eq!(s.offload_log.len(), 32); + assert_eq!(s.offload_log_start, 1); + let (kind, error) = s.offload_log.last().unwrap(); + assert_eq!(*kind, crate::offload::JobKind::Track); + assert!(error.as_ref().is_some_and(|e| e.contains("nothing was applied")), "a rejected result must report failure to JobWaiter"); + assert!(s.redo()); + assert_eq!(s.active_comp().unwrap().layer(clip).unwrap().props.find_group(tracker).unwrap().name, "Redo survives"); +} + +#[test] +fn offloaded_disjoint_edit_reports_success_and_keeps_one_undo_step() { + let (mut s, clip, tracker, off) = held_point_track(); + s.execute("prop.renameGroup", json!({"layer": clip.0, "prop": tracker, "name": "Live track"})).unwrap(); + s.execute("prop.setGroupEnabled", json!({"layer": clip.0, "prop": tracker, "value": false})).unwrap(); + let undo = s.history.undo.len(); + off.run(s.footage.clone()); + s.drain_events(); + assert!(s.poll_offload()); + let layer = s.active_comp().unwrap().layer(clip).unwrap(); + let track = layer.props.find_group(tracker).unwrap(); + assert_eq!(track.name, "Live track"); + assert!(!track.enabled); + assert_eq!(track.track_points().next().unwrap().get("featureCenter").unwrap().keys.len(), 25); + assert_eq!(s.history.undo.len(), undo + 1); + assert_eq!(s.offload_log.last(), Some(&(crate::offload::JobKind::Track, None))); + assert!(!s.drain_events().iter().any(|e| matches!(e, crate::Event::Toast { error: true, .. }))); + let applied = s.project.to_json(); + assert!(s.undo()); + let track = s.active_comp().unwrap().layer(clip).unwrap().props.find_group(tracker).unwrap(); + assert_eq!(track.name, "Live track"); + assert!(!track.enabled); + assert!(track.track_points().next().unwrap().get("featureCenter").unwrap().keys.is_empty()); + assert!(s.redo()); + assert_eq!(s.project.to_json(), applied); +} + +#[test] +fn offloaded_conflict_does_not_fail_a_concurrent_analysis() { + let (mut s, clip, tracker, off) = held_point_track(); + let effect = s.execute("effect.apply", json!({"effect": "Warp Stabilizer", "layers": [clip.0]})).unwrap()["effects"][0].as_u64().unwrap(); + s.execute_checked("warp.analyze", json!({"layer": clip.0, "effect": effect})).unwrap(); + assert_eq!(off.kinds(), [crate::offload::JobKind::Track, crate::offload::JobKind::Warp]); + let center = tracked_center(&s, clip, tracker); + s.execute("prop.set", json!({"layer": clip.0, "prop": center, "value": [180.0, 140.0]})).unwrap(); + off.run(s.footage.clone()); + assert!(s.poll_offload()); + let track = s.offload_log.iter().find(|(kind, _)| *kind == crate::offload::JobKind::Track).unwrap(); + assert!(track.1.as_ref().is_some_and(|e| e.contains("nothing was applied"))); + let warp = s.offload_log.iter().find(|(kind, _)| *kind == crate::offload::JobKind::Warp).unwrap(); + assert!(warp.1.is_none(), "the other analysis completed successfully: {warp:?}"); + assert_eq!(s.execute("warp.status", json!({"layer": clip.0, "effect": effect})).unwrap()["analyzed"], true); + assert!(s.active_comp().unwrap().layer(clip).unwrap().props.find(center).unwrap().keys.is_empty()); +} + /// M13.32: where jobs run elsewhere (the browser), `warp.status` never solves the /// stabilization plan on the page's thread (seconds for a long Subspace Warp clip): the /// analysis job sends the plan's summary with its result, and a settings change has the plan diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index bb45d1108..15493ad79 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -68,8 +68,8 @@ pub struct JobWaiter { pub result: Value, /// Ids of the jobs it started (`jobs.list` ids: `render`, `track`, `roto`…). pub jobs: Vec, - /// Length of [`effectcraft_engine::Session::offload_log`] when it started. - pub log_len: usize, + /// Absolute completion-log position when it started (including evicted entries). + pub log_cursor: usize, } /// Commands that block with `wait` (and its default). @@ -86,7 +86,7 @@ fn execute_engine(app: &mut EffectcraftApp, id: &str, params: Value) -> Result = app.session.jobs().into_iter().map(|j| j.id).collect(); - let log_len = app.session.offload_log.len(); + let log_cursor = app.session.offload_log_start.saturating_add(app.session.offload_log.len()); let mut p = params.clone(); p["wait"] = json!(false); let result = app.session.execute_checked(id, p).map_err(|e| e.to_string())?; @@ -94,7 +94,7 @@ fn execute_engine(app: &mut EffectcraftApp, id: &str, params: Value) -> Result Value { let s = &mut app.session; s.poll_render(); - let errors: Vec = s.offload_log.iter().skip(self.log_len).filter_map(|(_, e)| e.clone()).collect(); - if let Some(e) = errors.first() { + let error = s + .offload_log + .iter() + .skip(self.log_cursor.saturating_sub(s.offload_log_start)) + .filter(|(kind, _)| self.jobs.iter().any(|id| id == kind.job_id())) + .find_map(|(_, e)| e.as_ref()); + if let Some(e) = error { return json!({"ok": false, "error": e}); } let mut r = self.result; diff --git a/crates/ui-egui/tests/ui_web_offload.rs b/crates/ui-egui/tests/ui_web_offload.rs index cc49325f1..67ec6a9f8 100644 --- a/crates/ui-egui/tests/ui_web_offload.rs +++ b/crates/ui-egui/tests/ui_web_offload.rs @@ -219,3 +219,45 @@ fn waiting_commands_reply_when_their_offloaded_job_ends() { let v = reply.try_recv().expect("immediate reply"); assert_eq!(v["result"]["rendering"], true, "{v}"); } + +/// Completion errors belong to the waiting command's jobs, including when several +/// different analyses complete in the same UI frame. +#[test] +fn waiting_commands_report_only_their_own_completion_errors() { + use effectcraft_engine::offload::JobKind; + use effectcraft_ui_egui::control::JobWaiter; + + for (kind, job_id) in [ + (JobKind::Render, "render"), + (JobKind::Warp, "warp"), + (JobKind::WarpPlan, "warpPlan"), + (JobKind::Camera, "camera"), + (JobKind::RotoFreeze, "roto"), + (JobKind::RotoPropagate, "roto"), + (JobKind::Track, "track"), + (JobKind::MaskTrack, "maskTrack"), + (JobKind::ContentFill, "contentFill"), + ] { + let other = if kind == JobKind::Track { JobKind::Warp } else { JobKind::Track }; + let mut app = EffectcraftApp::new(Session::default()); + app.session.offload_log = vec![(kind, Some("old error".into())), (other, Some("unrelated failure".into())), (kind, None)]; + let waiter = || JobWaiter { command: "analysis".into(), params: json!({}), result: json!({"running": true}), jobs: vec![job_id.into()], log_cursor: 1 }; + let result = waiter().finish(&mut app); + assert_eq!(result, json!({"ok": true, "result": {"running": false}}), "{kind:?}"); + app.session.offload_log.push((kind, Some("nothing was applied".into()))); + let result = waiter().finish(&mut app); + assert_eq!(result, json!({"ok": false, "error": "nothing was applied"}), "{kind:?}"); + } +} + +#[test] +fn waiting_command_sees_its_error_after_the_completion_log_rolls_over() { + use effectcraft_engine::offload::JobKind; + use effectcraft_ui_egui::control::JobWaiter; + let mut app = EffectcraftApp::new(Session::default()); + app.session.offload_log = vec![(JobKind::Warp, None); 31]; + app.session.offload_log.push((JobKind::Track, Some("nothing was applied".into()))); + app.session.offload_log_start = 1; + let waiter = JobWaiter { command: "track.analyze".into(), params: json!({}), result: json!({}), jobs: vec!["track".into()], log_cursor: 32 }; + assert_eq!(waiter.finish(&mut app), json!({"ok": false, "error": "nothing was applied"})); +} From 2d216151c688fb74558128e8bd7943c8a0ba0ad1 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Thu, 15 Oct 2026 13:33:50 +0530 Subject: [PATCH 09/16] Import: Composition - Retain Layer Sizes for PDF / Illustrator files (#1536) The PDF / Illustrator import dialog now offers Composition - Retain Layer Sizes. Each file layer's footage is cropped to its artwork bounds (fills, stroke outlines, images, clips) and positioned to keep its place on the page. Decoding, continuous rasterisation, shape conversion and reload share one crop (effectcraft_pdf::layer_doc_with_sizes), so they agree. Also collapses a match guard in the STL reader for current stable clippy. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands/file.rs | 6 +- crates/engine/src/footage_reload.rs | 7 ++ crates/engine/src/vector.rs | 26 +++++-- crates/host/tests/vector_import.rs | 108 ++++++++++++++++++++++----- crates/media/src/layered.rs | 2 +- crates/model/src/stl.rs | 6 +- crates/pdf/README.md | 4 +- crates/pdf/src/lib.rs | 21 ++++++ crates/pdf/src/tests.rs | 45 +++++++++++ crates/svg/src/bounds.rs | 64 ++++++++++++++++ crates/svg/src/lib.rs | 23 ++++++ crates/svg/src/raster.rs | 18 +---- crates/ui-egui/src/panels/forms.rs | 11 ++- crates/ui-egui/tests/ui_drag_drop.rs | 25 +++++++ 14 files changed, 314 insertions(+), 52 deletions(-) create mode 100644 crates/svg/src/bounds.rs diff --git a/crates/engine/src/commands/file.rs b/crates/engine/src/commands/file.rs index 3b43eb633..501d15482 100644 --- a/crates/engine/src/commands/file.rs +++ b/crates/engine/src/commands/file.rs @@ -391,7 +391,7 @@ fn prepare_import(s: &mut Session, p: &Value) -> Result { Ok(b) if effectcraft_psd::is_psd(&b) => b, Ok(b) if effectcraft_pdf::sniff(&b).is_some() && !path.to_ascii_lowercase().ends_with(".svg") => { // PDF / Illustrator / EPS: one layer per file layer. - match import_vector_comp(s, &path, &b, page) { + match import_vector_comp(s, &path, &b, page, retain) { Ok((comp, made)) => { comps.push(comp); items.extend(made); @@ -540,7 +540,7 @@ pub(crate) fn import_psd_comp(s: &mut Session, path: &str, bytes: Vec, retai /// Import page `page` of a PDF / Illustrator / EPS file as a composition (one undo step). /// Returns (comp, items). -fn import_vector_comp(s: &mut Session, path: &str, bytes: &[u8], page: u32) -> Result<(u64, Vec)> { +fn import_vector_comp(s: &mut Session, path: &str, bytes: &[u8], page: u32, retain: bool) -> Result<(u64, Vec)> { let mut name = std::path::Path::new(path).file_stem().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "Vector".into()); if page > 0 { name = format!("{name} (Page {})", page + 1); @@ -549,7 +549,7 @@ fn import_vector_comp(s: &mut Session, path: &str, bytes: &[u8], page: u32) -> R let secs = if s.prefs.import.still_footage == "seconds" { s.prefs.import.still_seconds } else { 10.0 }; let duration = rate.snap_nearest(effectcraft_time::Tick::from_seconds_f64(secs)); let (comp, folder, items) = s.edit("Import", None, |proj, st| { - let r = crate::vector::import_vector_comp(proj, path, bytes, &name, page, rate, duration).map_err(EngineError::Other)?; + let r = crate::vector::import_vector_comp(proj, path, bytes, &name, page, retain, (rate, duration)).map_err(EngineError::Other)?; st.project_selection = vec![r.0]; Ok(r) })?; diff --git a/crates/engine/src/footage_reload.rs b/crates/engine/src/footage_reload.rs index a8f32a379..ca51069a4 100644 --- a/crates/engine/src/footage_reload.rs +++ b/crates/engine/src/footage_reload.rs @@ -58,6 +58,12 @@ fn psd_layer(s: &Session, path: &str, l: &SourceLayer) -> Option<(u32, Option<(u Some((u32::try_from(found.index).ok()?, size)) } +fn vector_size(s: &Session, f: &Footage) -> Option<(u32, u32)> { + let bytes = s.services.read_file(&f.path).ok()?; + let doc = crate::vector::vector_doc(&f.path, &bytes, f.layer.as_ref(), f.page)?.ok()?; + (!doc.root.children.is_empty()).then(|| doc.pixel_size()) +} + /// Footage `f` as its file is now, given the importer's fresh look at the file (`probed`). fn refreshed(s: &Session, f: &Footage, probed: Footage) -> Footage { let mut nf = f.clone(); @@ -89,6 +95,7 @@ fn refreshed(s: &Session, f: &Footage, probed: Footage) -> Footage { match &f.layer { // A smart object's size is its embedded file's, recorded at import. Some(l) if l.embedded.is_some() => (w, h) = (f.width, f.height), + Some(_) if crate::vector::is_vector(f) => (w, h) = vector_size(s, f).unwrap_or((f.width, f.height)), Some(l) => match psd_layer(s, &f.path, l) { Some((index, size)) => { if let Some(nl) = nf.layer.as_mut() { diff --git a/crates/engine/src/vector.rs b/crates/engine/src/vector.rs index 23400953f..3c349a7df 100644 --- a/crates/engine/src/vector.rs +++ b/crates/engine/src/vector.rs @@ -271,7 +271,7 @@ pub fn vector_doc(path: &str, bytes: &[u8], layer: Option<&effectcraft_project:: Some( effectcraft_pdf::parse_page(bytes, page as usize) .map(|d| match layer { - Some(l) => effectcraft_pdf::layer_doc(&d, l.index as usize), + Some(l) => effectcraft_pdf::layer_doc_with_sizes(&d, l.index as usize, l.layer_size).0, None => d, }) .map_err(|e| format!("{path}: {e}")), @@ -279,7 +279,7 @@ pub fn vector_doc(path: &str, bytes: &[u8], layer: Option<&effectcraft_project:: } /// Import page `page` of a PDF / Illustrator / EPS document as a composition the size of the -/// page: one footage item per file layer (each showing only that layer, continuously +/// page: one footage item per file layer (optionally cropped to its artwork, continuously /// rasterisable), top layer first. Returns (comp, folder, items). pub fn import_vector_comp( proj: &mut Project, @@ -287,11 +287,12 @@ pub fn import_vector_comp( bytes: &[u8], name: &str, page: u32, - rate: effectcraft_time::FrameRate, - duration: effectcraft_time::Tick, + retain: bool, + timing: (effectcraft_time::FrameRate, effectcraft_time::Tick), ) -> Result<(ItemId, ItemId, Vec), String> { use effectcraft_color::Label; use effectcraft_project::{AlphaMode, Footage, FootageKind, ItemKind, SourceLayer}; + let (rate, duration) = timing; let doc = effectcraft_pdf::parse_page(bytes, page as usize).map_err(|e| format!("{path}: {e}"))?; let codec = effectcraft_pdf::codec(path, bytes).unwrap_or("PDF"); let (w, h) = doc.pixel_size(); @@ -301,24 +302,33 @@ pub fn import_vector_comp( let mut items = vec![]; let names = effectcraft_pdf::layer_names(&doc); for (i, lname) in names.iter().enumerate() { + let (size, offset) = if retain { + let (layer, offset) = effectcraft_pdf::layer_doc_with_sizes(&doc, i, true); + (layer.pixel_size(), offset) + } else { + ((w, h), [0.0, 0.0]) + }; let f = Footage { path: path.to_string(), kind: FootageKind::Still, - width: w, - height: h, + width: size.0, + height: size.1, pixel_aspect: 1.0, frame_rate: rate, has_video: true, alpha: AlphaMode::Straight, loop_count: 1, codec: codec.into(), - layer: Some(SourceLayer { index: i as u32, name: lname.clone(), layer_size: false, ..Default::default() }), + layer: Some(SourceLayer { index: i as u32, name: lname.clone(), layer_size: retain, ..Default::default() }), page, ..Default::default() }; let id = proj.add_item(&format!("{lname}/{file}"), Label::Lavender, Some(folder), ItemKind::Footage(f)); items.push(id); - let mut l = build::layer(proj, &comp, lname, LayerSource::Footage { item: id }, (w, h), None); + let mut l = build::layer(proj, &comp, lname, LayerSource::Footage { item: id }, size, None); + if retain && let Some(position) = l.props.prop_mut("transform/position") { + position.value = Value::Vec3([offset[0] + f64::from(size.0) / 2.0, offset[1] + f64::from(size.1) / 2.0, 0.0]); + } l.name = comp.unique_layer_name(lname); comp.layers.insert(0, l); } diff --git a/crates/host/tests/vector_import.rs b/crates/host/tests/vector_import.rs index 44d2073f9..49f8d950c 100644 --- a/crates/host/tests/vector_import.rs +++ b/crates/host/tests/vector_import.rs @@ -260,23 +260,25 @@ fn pdf_and_ai_footage_layers_and_shapes_from_vector_layer() { // Import As: Composition — one layer per file layer, top first, together like the page. let path = tmp("layers.ai"); std::fs::write(&path, &bytes).unwrap(); - let mut s = effectcraft_host::session(); - let r = s.execute_checked("file.import", json!({"paths": [path], "importAs": "composition"})).unwrap(); - let cid = ItemId(r["comps"][0].as_u64().unwrap()); - let comp = s.project.comp(cid).unwrap(); - let names: Vec<&str> = comp.layers.iter().map(|l| l.name.as_str()).collect(); - assert_eq!(names, vec!["Art", "Sky"]); - assert_eq!((comp.width, comp.height), (120, 80)); - let art = comp.layers[0].id.0; - let img = s.render(cid, effectcraft_time::Tick::ZERO, RenderOpts::default()); - let d = mean_diff(&img, &direct); - assert!(d < 1.0 / 255.0, "layered comp vs page {d}"); - // Each layer's footage shows only its layer. - s.execute("layer.setSwitch", json!({"layers": [art], "switch": "video", "value": false})).unwrap(); - let sky = s.render(cid, effectcraft_time::Tick::ZERO, RenderOpts::default()); - assert_eq!(sky.get(5, 5), img.get(5, 5)); - let p = sky.get(60, 30); - assert!(p[2] > p[0], "only sky under the art {p:?}"); + for import_as in ["composition", "compositionLayerSizes"] { + let mut s = effectcraft_host::session(); + let r = s.execute_checked("file.import", json!({"paths": [path], "importAs": import_as})).unwrap(); + let cid = ItemId(r["comps"][0].as_u64().unwrap()); + let comp = s.project.comp(cid).unwrap(); + let names: Vec<&str> = comp.layers.iter().map(|l| l.name.as_str()).collect(); + assert_eq!(names, vec!["Art", "Sky"]); + assert_eq!((comp.width, comp.height), (120, 80)); + let art = comp.layers[0].id.0; + let img = s.render(cid, effectcraft_time::Tick::ZERO, RenderOpts::default()); + let d = mean_diff(&img, &direct); + assert!(d < 1.0 / 255.0, "layered comp vs page {d}"); + // Each layer's footage shows only its layer. + s.execute("layer.setSwitch", json!({"layers": [art], "switch": "video", "value": false})).unwrap(); + let sky = s.render(cid, effectcraft_time::Tick::ZERO, RenderOpts::default()); + assert_eq!(sky.get(5, 5), img.get(5, 5)); + let p = sky.get(60, 30); + assert!(p[2] > p[0], "only sky under the art {p:?}"); + } } #[test] @@ -522,3 +524,75 @@ fn shapes_from_vector_layer_keeps_images_as_footage_layers() { assert!((0..3).all(|k| (a[k] - b[k]).abs() < 0.05), "({x}, {y}): {a:?} vs {b:?}"); } } + +fn retain_pdf(width: u32) -> Vec { + effectcraft_pdf::write::pdf( + &[ + (1, "<< /Type /Catalog /Pages 2 0 R >>".into(), None), + (2, "<< /Type /Pages /Kids [3 0 R] /Count 1 >>".into(), None), + (3, "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 120 80] /Resources << /Properties << /L0 5 0 R /L1 6 0 R >> >> /Contents 4 0 R >>".into(), None), + ( + 4, + "<< >>".into(), + Some( + format!("/OC /L0 BDC q 20 15 {width} 20 re W n 1 0 0 rg 0 0 120 80 re f Q EMC /OC /L1 BDC 0 0 1 RG 4 w 70 40 m 100 40 l S EMC") + .into_bytes(), + ), + ), + (5, "<< /Type /OCG /Name (Box) >>".into(), None), + (6, "<< /Type /OCG /Name (Line) >>".into(), None), + ], + 1, + ) +} + +#[test] +fn ai_composition_retains_layer_sizes_and_positions() { + use effectcraft_project::{ItemKind, LayerSource, Project}; + use effectcraft_time::Tick; + + for name in ["retain.ai", "retain.pdf"] { + let path = tmp(name); + let bytes = retain_pdf(30); + std::fs::write(&path, &bytes).unwrap(); + let mut s = effectcraft_host::session(); + let r = s.execute_checked("file.import", json!({"paths": [path], "importAs": "compositionLayerSizes"})).unwrap(); + assert_eq!(r["errors"], json!([]), "{r}"); + let cid = ItemId(r["comps"][0].as_u64().unwrap()); + let comp = s.project.comp(cid).unwrap(); + assert_eq!((comp.width, comp.height), (120, 80)); + assert_eq!(comp.layers.iter().map(|l| l.name.as_str()).collect::>(), ["Line", "Box"]); + let mut ids = vec![]; + for (name, size, anchor, position) in [("Box", (30, 20), [15.0, 10.0], [35.0, 55.0]), ("Line", (30, 4), [15.0, 2.0], [85.0, 40.0])] { + let l = comp.layer_by_name(name).unwrap(); + let LayerSource::Footage { item } = l.source else { panic!("footage") }; + let ItemKind::Footage(f) = &s.project.item(item).unwrap().kind else { panic!("footage") }; + assert_eq!((f.width, f.height), size, "{name} uses its artwork bounds"); + assert!(f.layer.as_ref().unwrap().layer_size); + assert_eq!(l.props.prop("transform/anchor").unwrap().value.as_vec2(), anchor); + assert_eq!(l.props.prop("transform/position").unwrap().value.as_vec2(), position); + let frame = s.footage.frame(item, f, Tick::ZERO).unwrap(); + assert_eq!((frame.width, frame.height), size); + let sharp = s.footage.vector_frame(item, f, 2.0).unwrap(); + assert_eq!((sharp.width, sharp.height), (size.0 * 2, size.1 * 2)); + assert!(sharp.get(1, 1)[3] > 0.99); + ids.push(item); + } + let direct = effectcraft_svg::rasterize(&effectcraft_pdf::parse(&bytes).unwrap(), 120, 80, 1.0); + assert!(mean_diff(&s.render(cid, Tick::ZERO, RenderOpts::default()), &direct) < 0.001); + let saved = s.project.to_json(); + s.project = std::sync::Arc::new(Project::from_json(&saved).unwrap()); + assert!(mean_diff(&s.render(cid, Tick::ZERO, RenderOpts::default()), &direct) < 0.001, "reopened project"); + let box_id = s.active_comp().unwrap().layer_by_name("Box").unwrap().id.0; + s.execute_checked("layer.create", json!({"op": "shapesFromVector", "layers": [box_id]})).unwrap(); + assert!(mean_diff(&s.render(cid, Tick::ZERO, RenderOpts::default()), &direct) < 0.001, "shape conversion preserves placement"); + s.execute_checked("edit.undo", json!({})).unwrap(); + std::fs::write(&path, retain_pdf(40)).unwrap(); + s.execute_checked("file.reloadFootage", json!({"items": ids})).unwrap(); + let ItemKind::Footage(f) = &s.project.item(ids[0]).unwrap().kind else { panic!("footage") }; + assert_eq!((f.width, f.height), (40, 20), "reload refreshes the layer's own dimensions"); + let frame = s.footage.frame(ids[0], f, Tick::ZERO).unwrap(); + assert_eq!((frame.width, frame.height), (40, 20)); + assert!(frame.get(39, 10)[0] > 0.99, "reload invalidates cached pixels"); + } +} diff --git a/crates/media/src/layered.rs b/crates/media/src/layered.rs index 0fb40d81c..0a70d1f7f 100644 --- a/crates/media/src/layered.rs +++ b/crates/media/src/layered.rs @@ -28,7 +28,7 @@ pub fn vector_doc( Some( effectcraft_pdf::parse_page(bytes, page as usize) .map(|d| match layer { - Some(l) => effectcraft_pdf::layer_doc(&d, l.index as usize), + Some(l) => effectcraft_pdf::layer_doc_with_sizes(&d, l.index as usize, l.layer_size).0, None => d, }) .map_err(|e| format!("{path}: {e}")), diff --git a/crates/model/src/stl.rs b/crates/model/src/stl.rs index 6684858ad..34e9a7f06 100644 --- a/crates/model/src/stl.rs +++ b/crates/model/src/stl.rs @@ -175,10 +175,8 @@ fn ascii(bytes: &[u8]) -> Result { let Some(key) = it.next() else { continue }; let bad = || perr(format!("STL line {}: expected three numbers", ln + 1)); match key.to_ascii_lowercase().as_str() { - "solid" => { - if name.is_none() { - name = Some(line.get(5..).unwrap_or("").trim().to_string()); - } + "solid" if name.is_none() => { + name = Some(line.get(5..).unwrap_or("").trim().to_string()); } "facet" => { // `facet normal nx ny nz` diff --git a/crates/pdf/README.md b/crates/pdf/README.md index b7678b096..b44e3165d 100644 --- a/crates/pdf/README.md +++ b/crates/pdf/README.md @@ -4,7 +4,9 @@ Vector footage from PDF files, PDF-compatible Adobe Illustrator files (`.ai`, wh documents) and Encapsulated PostScript (`.eps`). Files parse into the same render tree as SVG (`effectcraft_svg::Doc`), so they rasterise at any scale (Continuously Rasterize), convert to shape layers (Layer ▸ Create ▸ Create Shapes from Vector Layer) and import as compositions with one -layer per file layer (File ▸ Import ▸ Composition). +layer per file layer (File ▸ Import ▸ Composition). Composition - Retain Layer Sizes uses each +layer's artwork bounds and keeps its placement on the page. Images keep their rectangular bounds; +clipping paths constrain the bounds, while soft masks do not change them. Specifications used: **ISO 32000-1:2008** (PDF 1.7; the public Adobe edition) for the file structure, filters, content streams, graphics state, colour spaces, functions, shadings, text, diff --git a/crates/pdf/src/lib.rs b/crates/pdf/src/lib.rs index 39d150749..cf3f5ca6b 100644 --- a/crates/pdf/src/lib.rs +++ b/crates/pdf/src/lib.rs @@ -194,6 +194,27 @@ pub fn layer_doc(doc: &Doc, index: usize) -> Doc { d } +/// One layer, optionally cropped to its artwork, and its original document-space top-left. +pub fn layer_doc_with_sizes(doc: &Doc, index: usize, retain: bool) -> (Doc, [f64; 2]) { + let mut d = layer_doc(doc, index); + if !retain { + return (d, [0.0, 0.0]); + } + let page = kurbo::Rect::new(0.0, 0.0, doc.width, doc.height); + let bounds = d.artwork_bounds().map(|b| b.intersect(page)).filter(|b| b.width() > 0.0 && b.height() > 0.0); + let (x, y, w, h) = match bounds { + Some(b) => { + let (x, y) = (b.x0.floor(), b.y0.floor()); + (x, y, b.x1.ceil() - x, b.y1.ceil() - y) + } + None => (0.0, 0.0, 1.0, 1.0), + }; + d.width = w; + d.height = h; + d.root.transform = Affine::translate((-x, -y)) * d.root.transform; + (d, [x, y]) +} + #[cfg(test)] mod tests; #[cfg(test)] diff --git a/crates/pdf/src/tests.rs b/crates/pdf/src/tests.rs index 57b7e0448..e626ca40c 100644 --- a/crates/pdf/src/tests.rs +++ b/crates/pdf/src/tests.rs @@ -1020,3 +1020,48 @@ fn multiple_pages_and_calculator_shadings() { assert!(img.get(5, 25)[0] < 0.1 && img.get(95, 25)[0] > 0.9, "{:?} {:?}", img.get(5, 25), img.get(95, 25)); assert_eq!(parse_page(&bytes, 2), Err(Error::PageOutOfRange(2))); } + +#[test] +fn retained_layers_keep_strokes_clips_and_empty_layers() { + let doc = parse(&sample_pdf(false)).unwrap(); + for i in 0..layer_names(&doc).len() { + let full = layer_doc(&doc, i); + let (cropped, offset) = layer_doc_with_sizes(&doc, i, true); + let (w, h) = cropped.pixel_size(); + let image = effectcraft_svg::rasterize(&cropped, w, h, 1.0); + let original = effectcraft_svg::rasterize(&full, 200, 100, 1.0); + for y in 0..100 { + for x in 0..200 { + let a = image.get(x - offset[0] as i64, y - offset[1] as i64); + let b = original.get(x, y); + assert!(a.iter().zip(b).all(|(a, b)| (a - b).abs() < 1e-5), "layer {i} at {x},{y}: {a:?} vs {b:?}"); + } + } + } + let (empty, _) = layer_doc_with_sizes(&doc, usize::MAX, true); + assert_eq!(empty.pixel_size(), (1, 1)); +} + +#[test] +fn retained_layers_measure_transformed_artwork_without_rasterizing() { + let doc = effectcraft_svg::parse(br#""#).unwrap(); + let (rotated, offset) = layer_doc_with_sizes(&doc, 0, true); + assert_eq!(offset, [30.0, 10.0]); + assert_eq!(rotated.pixel_size(), (10, 30)); + let (stroked, offset) = layer_doc_with_sizes(&doc, 1, true); + assert_eq!(offset, [8.0, 18.0]); + assert_eq!(stroked.pixel_size(), (24, 4)); +} + +#[test] +fn retained_layers_preserve_artwork_beyond_the_bounds_depth_limit() { + let mut doc = effectcraft_svg::parse(br#""#).unwrap(); + for _ in 0..300 { + let mut group = Group::new("nested"); + group.children = std::mem::take(&mut doc.root.children); + doc.root.children.push(Node::Group(group)); + } + let (cropped, offset) = layer_doc_with_sizes(&doc, 0, true); + assert_eq!(cropped.pixel_size(), (120, 80), "unknown bounds keep the full page"); + assert_eq!(offset, [0.0, 0.0]); +} diff --git a/crates/svg/src/bounds.rs b/crates/svg/src/bounds.rs new file mode 100644 index 000000000..f0613aff8 --- /dev/null +++ b/crates/svg/src/bounds.rs @@ -0,0 +1,64 @@ +use kurbo::{Affine, BezPath, Rect, Shape as _}; + +use crate::{Doc, Group, Node}; + +impl Doc { + /// Geometric artwork bounds in document pixels, including strokes and clipping. + /// Opacity and soft masks do not change the artwork's dimensions. + pub fn artwork_bounds(&self) -> Option { + let page = Rect::new(0.0, 0.0, self.width, self.height); + group_bounds(&self.root, Affine::IDENTITY, 0, page) + } +} + +fn path_bounds(path: BezPath) -> Option { + if path.elements().is_empty() || !path.is_finite() { + return None; + } + let r = path.bounding_box(); + (r.is_finite() && r.width() > 0.0 && r.height() > 0.0).then_some(r) +} + +fn group_bounds(group: &Group, transform: Affine, depth: usize, page: Rect) -> Option { + if depth > 256 { + return Some(page); + } + let transform = transform * group.transform; + if !transform.as_coeffs().iter().all(|v| v.is_finite()) { + return None; + } + let mut bounds: Option = None; + for node in &group.children { + let rect = match node { + Node::Group(g) => group_bounds(g, transform, depth + 1, page), + Node::Image(i) => { + let rect = Rect::new(0.0, 0.0, f64::from(i.width), f64::from(i.height)); + path_bounds(transform * i.transform * rect.to_path(0.01)) + } + Node::Shape(s) => { + let m = transform * s.transform; + let path = s.geom.to_path(); + let mut rect = s.fill.as_ref().and_then(|_| path_bounds(m * &path)); + if let Some(stroke) = &s.stroke { + let scale = m.determinant().abs().sqrt(); + if scale.is_finite() + && path.is_finite() + && let Some(outline) = effectcraft_path::stroke_outline(std::slice::from_ref(&path), &stroke.style(), scale) + && let Some(r) = path_bounds(m * outline) + { + rect = Some(rect.map_or(r, |b| b.union(r))); + } + } + rect + } + }; + if let Some(r) = rect { + bounds = Some(bounds.map_or(r, |b| b.union(r))); + } + } + let mut bounds = bounds?; + for (clip, _) in &group.clip { + bounds = bounds.intersect(path_bounds(transform * clip)?); + } + (bounds.width() > 0.0 && bounds.height() > 0.0).then_some(bounds) +} diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index 7310814bf..bcffb8004 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -6,6 +6,7 @@ #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] +mod bounds; mod color; mod pathdata; mod raster; @@ -108,6 +109,28 @@ pub struct Stroke { pub dash: Option<(Vec, f64)>, } +impl Stroke { + fn style(&self) -> effectcraft_path::StrokeStyle { + effectcraft_path::StrokeStyle { + width: self.width, + cap: match self.cap { + Cap::Butt => effectcraft_path::Cap::Butt, + Cap::Round => effectcraft_path::Cap::Round, + Cap::Square => effectcraft_path::Cap::Square, + }, + join: match self.join { + Join::Miter => effectcraft_path::Join::Miter, + Join::Round => effectcraft_path::Join::Round, + Join::Bevel => effectcraft_path::Join::Bevel, + }, + miter: self.miter, + dash: self.dash.clone(), + taper: None, + wave: None, + } + } +} + /// Shape geometry in its user space (basic shapes are kept so they can become parametric shape /// layer items). #[derive(Clone, Debug, PartialEq)] diff --git a/crates/svg/src/raster.rs b/crates/svg/src/raster.rs index b1b9d93b6..af59e33db 100644 --- a/crates/svg/src/raster.rs +++ b/crates/svg/src/raster.rs @@ -405,23 +405,7 @@ fn draw_shape(s: &Shape, m: Affine, opacity: f64, dst: &mut Image) { paint(dst, &cov, fill, (opacity * s.fill_opacity) as f32, m, bbox); } if let Some(st) = &s.stroke { - let style = effectcraft_path::StrokeStyle { - width: st.width, - cap: match st.cap { - crate::Cap::Butt => effectcraft_path::Cap::Butt, - crate::Cap::Round => effectcraft_path::Cap::Round, - crate::Cap::Square => effectcraft_path::Cap::Square, - }, - join: match st.join { - crate::Join::Miter => effectcraft_path::Join::Miter, - crate::Join::Round => effectcraft_path::Join::Round, - crate::Join::Bevel => effectcraft_path::Join::Bevel, - }, - miter: st.miter, - dash: st.dash.clone(), - taper: None, - wave: None, - }; + let style = st.style(); let cov = effectcraft_path::stroke_coverage(std::slice::from_ref(&path), &style, &mat3(m), w, h); paint(dst, &cov, &st.paint, (opacity * st.opacity) as f32, m, bbox); } diff --git a/crates/ui-egui/src/panels/forms.rs b/crates/ui-egui/src/panels/forms.rs index 275e54b75..185c10336 100644 --- a/crates/ui-egui/src/panels/forms.rs +++ b/crates/ui-egui/src/panels/forms.rs @@ -228,7 +228,16 @@ pub fn open_form(app: &mut EffectcraftApp, id: &str, p: &Value) -> bool { "file.import" if !has(p, &["importAs", "page"]) && is_pdf(p) => ( "Import PDF / Illustrator File".into(), vec![ - Field::choice("importAs", "Import Kind", &[("Footage", json!("footage")), ("Composition", json!("composition"))], 0), + Field::choice( + "importAs", + "Import Kind", + &[ + ("Footage", json!("footage")), + ("Composition", json!("composition")), + ("Composition - Retain Layer Sizes", json!("compositionLayerSizes")), + ], + 0, + ), Field::num("page", "Page", 1.0), ], ), diff --git a/crates/ui-egui/tests/ui_drag_drop.rs b/crates/ui-egui/tests/ui_drag_drop.rs index 77a3995d6..9b66f745c 100644 --- a/crates/ui-egui/tests/ui_drag_drop.rs +++ b/crates/ui-egui/tests/ui_drag_drop.rs @@ -584,3 +584,28 @@ fn a_dropped_folder_imports_as_one_sequence_with_sequence_interpretation() { assert!(h.state().auto.find("form.field.missingFrames").is_none()); let _ = std::fs::remove_dir_all(&dir); } + +#[test] +fn illustrator_import_offers_and_applies_retain_layer_sizes() { + let path = std::env::temp_dir().join(format!("effectcraft-retain-ui-{}.ai", std::process::id())); + std::fs::write(&path, b"%PDF-1.4\n1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj\n2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj\n3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 120 80] /Contents 4 0 R >> endobj\n4 0 obj << >> stream\n1 0 0 rg 20 15 30 20 re f\nendstream endobj\n%%EOF").unwrap(); + let mut h = Harness::builder().with_size(vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(effectcraft_host::session())); + h.run_steps(3); + let ctx = h.ctx.clone(); + effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, "file.import", json!({"paths": [path]})).unwrap(); + h.run_steps(3); + assert!(h.query_by_label("Import PDF / Illustrator File").is_some()); + click_id(&mut h, "form.field.importAs"); + h.get_by_label("Composition - Retain Layer Sizes").click(); + h.run_steps(2); + click_id(&mut h, "form.ok"); + step_until(&mut h, |h| h.state().session.active_comp().is_some()); + let s = &h.state().session; + let comp = s.active_comp().unwrap(); + assert_eq!((comp.width, comp.height), (120, 80)); + assert_eq!(comp.layers.len(), 1); + let effectcraft_engine::project::LayerSource::Footage { item } = comp.layers[0].source else { panic!("footage") }; + let effectcraft_engine::project::ItemKind::Footage(f) = &s.project.item(item).unwrap().kind else { panic!("footage") }; + assert_eq!((f.width, f.height), (30, 20)); + assert!(f.layer.as_ref().unwrap().layer_size); +} From 26ed1a35bfebb9286817cf5a1e92a49e774e69f8 Mon Sep 17 00:00:00 2001 From: ".CooCoo" Date: Thu, 15 Oct 2026 01:09:25 -0700 Subject: [PATCH 10/16] fix(pdf): prevent overflow in EPS integer arithmetic Use checked division and remainder for EPS integer operators so i64::MIN with a divisor truncating to -1 cannot panic. Preserve the existing zero-divisor fallback and normal truncation behavior. Add public EPS parse regressions covering extreme, fractional and ordinary operands. PR #1223 --- crates/pdf/src/ps.rs | 18 ++++-------------- crates/pdf/src/tests_import.rs | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/crates/pdf/src/ps.rs b/crates/pdf/src/ps.rs index 65e352cb2..4401ace91 100644 --- a/crates/pdf/src/ps.rs +++ b/crates/pdf/src/ps.rs @@ -700,20 +700,10 @@ impl Interp { a / b } } - "idiv" => { - if b as i64 == 0 { - 0.0 - } else { - ((a as i64) / (b as i64)) as f64 - } - } - "mod" => { - if b as i64 == 0 { - 0.0 - } else { - ((a as i64) % (b as i64)) as f64 - } - } + // `checked_*` give 0 both for a zero divisor and for the one overflowing + // case, `i64::MIN / -1` (`/` and `%` panic on it in every build). + "idiv" => (a as i64).checked_div(b as i64).unwrap_or(0) as f64, + "mod" => (a as i64).checked_rem(b as i64).unwrap_or(0) as f64, "atan" => a.atan2(b).to_degrees().rem_euclid(360.0), "exp" => a.powf(b), "max" => a.max(b), diff --git a/crates/pdf/src/tests_import.rs b/crates/pdf/src/tests_import.rs index 532e32cb9..add447e3a 100644 --- a/crates/pdf/src/tests_import.rs +++ b/crates/pdf/src/tests_import.rs @@ -376,3 +376,25 @@ fn postscript_radix_numbers_outside_2_to_36_are_not_numbers() { let _ = parse(src.as_bytes()); } } + +#[test] +fn postscript_idiv_and_mod_of_i64_min_by_minus_one_give_zero() { + // `i64::MIN / -1` and `i64::MIN % -1` overflow, and `/` and `%` panic on them in debug and + // release builds. `as i64` turns any dividend at or below -2^63 into `i64::MIN` and turns + // a divisor of -1.5 into -1. + for (prog, want) in [ + ("-9223372036854775808 -1 idiv", 0), + ("-9223372036854775808 -1 mod", 0), + ("-99999999999999999999 -1.5 idiv", 0), + ("2 63 exp neg -1 mod", 0), + // The result is 0 for a divisor that truncates to 0. Other quotients are truncated + // toward zero, and a remainder has the sign of the dividend. + ("7 0 idiv", 0), + ("7 0.5 mod", 0), + ("-7 2 idiv", -3), + ("-7 2 mod", -1), + ] { + let doc = eps(format!("{prog} {want} eq {{ 0 0 10 10 rectfill }} if\n").as_bytes()); + assert_eq!(shape_names(&doc).len(), 1, "{prog}"); + } +} From 0311bdc34d0dc10c2d18ff7875b4f52d483e5e27 Mon Sep 17 00:00:00 2001 From: Moein Saharkhiz <142007707+onlymoein@users.noreply.github.com> Date: Thu, 15 Oct 2026 04:13:41 -0400 Subject: [PATCH 11/16] fix(automation): avoid replaying edits after lost replies Retry only explicitly read-only control queries after connection failures. Report ambiguous edit outcomes without resending mutations, reconnect on the next explicit call, and reject request-ID exhaustion without overflow. Add loopback regressions for lost and malformed replies, read-only retries, playback queries versus edits, and unknown methods. Fixes #1247 --- crates/automation/src/bridge.rs | 17 +++-- crates/automation/src/tests.rs | 122 ++++++++++++++++++++++++++++++++ docs/agents.md | 5 ++ 3 files changed, 139 insertions(+), 5 deletions(-) diff --git a/crates/automation/src/bridge.rs b/crates/automation/src/bridge.rs index eab1c01db..77d69dd01 100644 --- a/crates/automation/src/bridge.rs +++ b/crates/automation/src/bridge.rs @@ -1,5 +1,5 @@ //! Blocking client for the desktop app's JSON-lines control channel (`docs/control-protocol.md`). -//! One persistent loopback connection, reconnected once if the app went away. +//! One persistent loopback connection. Only explicitly read-only queries can be resent. use std::io::{BufRead, BufReader, Write}; use std::net::{TcpStream, ToSocketAddrs}; @@ -50,16 +50,20 @@ impl BridgeClient { /// Call a control method; returns the reply's `result`, or the app's `error` as [`Error::App`]. pub fn call(&mut self, method: &str, params: Value) -> Result { + let retry_read = + matches!(method, "engine.commands" | "ui.menu.list" | "ui.inspect" | "ui.elements" | "ui.viewer.locate" | "ui.viewer.hit" | "ui.timeline.locate") + || (method == "ui.playback" && params["action"] == "status"); + let unknown = |error| Error::Bridge(format!("`{method}` request outcome is unknown: {error}; inspect the app before retrying")); let mut last = Error::Bridge("not attempted".into()); for attempt in 0..2 { if self.conn.is_none() { self.conn = Some(self.connect()?); } let id = self.next_id; - self.next_id += 1; + self.next_id = self.next_id.checked_add(1).ok_or_else(|| Error::Bridge("control request id space exhausted".into()))?; let Some((r, w)) = self.conn.as_mut() else { continue }; let line = format!("{}\n", json!({"id": id, "method": method, "params": params})); - // `Ok(Err(_))`: the connection is dead before a reply (safe to resend once); + // `Ok(Err(_))`: transmission/reply failed (the app may have applied the request); // `Err(_)`: a timeout or bad reply (never resent: the app may have run the request). let res = (|| -> Result> { if let Err(e) = w.write_all(line.as_bytes()) { @@ -80,8 +84,11 @@ impl BridgeClient { Ok(Ok(v)) if v.get("ok").and_then(Value::as_bool) == Some(true) => return Ok(v.get("result").cloned().unwrap_or(Value::Null)), Ok(Ok(v)) => return Err(Error::App(v.get("error").and_then(Value::as_str).unwrap_or("error").to_string())), Ok(Err(e)) => { - // Stale connection (app restarted): reconnect and resend once. self.conn = None; + if !retry_read { + return Err(unknown(e)); + } + // A read-only query can be repeated after a stale connection or lost reply. last = e; if attempt == 1 { break; @@ -90,7 +97,7 @@ impl BridgeClient { Err(e) => { // The stream may still deliver the late reply: start fresh next call. self.conn = None; - return Err(e); + return Err(if retry_read { e } else { unknown(e) }); } } } diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index 6bb6fbc52..6e622b377 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -511,6 +511,128 @@ fn bridge_rejects_non_loopback() { assert!(Backend::bridge("localhost:9877").is_ok()); } +/// The app applies the first request but loses its reply. The next connection can be either +/// an automatic retry or an explicit follow-up; the returned counts distinguish them. +fn lost_reply_server(first_reply: Option<&'static str>) -> (String, std::thread::JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let worker = std::thread::spawn(move || { + let mut session = Session::default(); + let mut mutations = 0; + for attempt in 0..2 { + let (stream, _) = listener.accept().unwrap(); + stream.set_read_timeout(Some(std::time::Duration::from_secs(5))).unwrap(); + let mut output = stream.try_clone().unwrap(); + let mut input = String::new(); + BufReader::new(stream).read_line(&mut input).unwrap(); + let request: Value = serde_json::from_str(&input).unwrap(); + let result = if request["method"] == "engine.execute" { + mutations += 1; + session.execute(request["params"]["command"].as_str().unwrap(), request["params"]["params"].clone()).unwrap() + } else if request["method"] == "ui.inspect" || (request["method"] == "ui.playback" && request["params"]["action"] == "status") { + json!({"mutations": mutations, "requests": attempt + 1, "comps": session.project.comps().count()}) + } else { + mutations += 1; + json!({"mutations": mutations}) + }; + if attempt == 1 { + writeln!(output, "{}", json!({"id": request["id"], "ok": true, "result": result})).unwrap(); + } else if let Some(reply) = first_reply { + writeln!(output, "{reply}").unwrap(); + } + } + }); + (addr, worker) +} + +#[test] +fn bridge_does_not_replay_a_mutation_when_its_reply_is_lost() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.exec("comp.new", json!({"width": 64, "height": 48, "duration": 1})).unwrap_err(); + assert!(error.to_string().contains("outcome is unknown"), "{error}"); + let state = backend.control("ui.inspect", json!({})).unwrap(); + assert_eq!(state["mutations"], 1, "the first command was applied exactly once"); + assert_eq!(state["comps"], 1); + worker.join().unwrap(); +} + +#[test] +fn bridge_retries_read_only_queries_after_a_lost_reply() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let state = backend.control("ui.inspect", json!({})).unwrap(); + assert_eq!(state["requests"], 2); + assert_eq!(state["mutations"], 0); + worker.join().unwrap(); +} + +#[test] +fn bridge_reports_unknown_outcome_after_a_malformed_mutation_reply() { + let (addr, worker) = lost_reply_server(Some("{invalid reply")); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.exec("comp.new", json!({"width": 64, "height": 48, "duration": 1})).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("outcome is unknown") && message.contains("bad reply"), "{message}"); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["comps"], 1); + worker.join().unwrap(); +} + +#[test] +fn bridge_does_not_assume_an_unknown_method_is_read_only() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.control("future.customOperation", json!({})).unwrap_err(); + assert!(error.to_string().contains("outcome is unknown"), "{error}"); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["mutations"], 1); + worker.join().unwrap(); +} + +#[test] +fn bridge_playback_edits_and_default_toggle_are_not_replayed() { + for params in [json!({}), json!({"action": "toggle"}), json!({"action": "play"}), json!({"action": "stop"})] { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.control("ui.playback", params).unwrap_err(); + assert!(error.to_string().contains("outcome is unknown"), "{error}"); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["mutations"], 1); + worker.join().unwrap(); + } +} + +#[test] +fn bridge_playback_status_can_be_retried() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let state = backend.control("ui.playback", json!({"action": "status"})).unwrap(); + assert_eq!(state["requests"], 2); + assert_eq!(state["mutations"], 0); + worker.join().unwrap(); +} + +#[test] +fn bridge_connect_failure_can_be_retried_when_the_app_starts() { + let reserved = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = reserved.local_addr().unwrap(); + drop(reserved); + let mut backend = Backend::bridge(&addr.to_string()).unwrap(); + let error = backend.control("ui.inspect", json!({})).unwrap_err(); + assert!(error.to_string().contains("cannot connect"), "{error}"); + assert!(!error.to_string().contains("outcome is unknown"), "nothing was sent"); + let listener = TcpListener::bind(addr).unwrap(); + let worker = std::thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + stream.set_read_timeout(Some(std::time::Duration::from_secs(5))).unwrap(); + let mut output = stream.try_clone().unwrap(); + let mut input = String::new(); + BufReader::new(stream).read_line(&mut input).unwrap(); + let request: Value = serde_json::from_str(&input).unwrap(); + writeln!(output, "{}", json!({"id": request["id"], "ok": true, "result": {"ready": true}})).unwrap(); + }); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["ready"], true); + worker.join().unwrap(); +} + #[test] fn script_ui_and_history_tools() { let mut sess = Session::default(); diff --git a/docs/agents.md b/docs/agents.md index 1bec71d51..ed0073d45 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -52,6 +52,11 @@ Desktop, Cursor and the like) take the same `command` and `args`. - **Bridge** (`["mcp", "--bridge", "9877"]`): drives a running `effectcraft --control 9877`, so you see every change live. Bridge mode adds `screenshot` and the `ui_*` tools. +After a broken connection, the bridge retries explicitly read-only control queries once. +Engine commands, UI edits and unknown methods whose transmission or reply fails report an +unknown outcome: the app may already have applied them. Inspect the app before explicitly +retrying. A later call reconnects; timeouts and malformed replies are never automatically resent. + ### From an installed release The release packages ship `effectcraft-cli` alongside the desktop app, so no build is needed: From 8d807e6e79d8b974038c1b9ffca53f4f9b773463 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Thu, 15 Oct 2026 14:58:22 +0530 Subject: [PATCH 12/16] Contributors: add arnav-hiwarkar Co-Authored-By: Claude Opus 5.5 --- contributors/people.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contributors/people.toml b/contributors/people.toml index 705feb257..3e6a78c3f 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -78,3 +78,7 @@ sync_github_name = true [people.moecr7] real_name = "Moein Saharkhiz" display_name = "onlymoein" + +[people.arnav-hiwarkar] +display_name = "arnav-hiwarkar" +sync_github_name = false From c4c079d4f4ae5c344ac4b23d8dbb11625830fd5d Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Thu, 15 Oct 2026 20:57:50 +0530 Subject: [PATCH 13/16] fix(engine): copy only the selected keyframes across properties (#1552) Clicking keyframes on Position, Rotation and Scale highlights those properties, and Copy then added every key of a highlighted property that had none selected, so deselected keys came back on paste. The keyframe copy branch also left a stale value clipboard that Paste preferred over the new keys. Copy now takes whole animations only when no key is selected or the Transform group is selected, and every copy branch clears the clipboards. Also collapses a nested `if` in the STL importer into a match guard, which Clippy 1.95 flags under -D warnings (no behaviour change). Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands/edit.rs | 13 ++-- crates/engine/src/tests_menu_cmds.rs | 4 +- crates/engine/src/tests_timeline.rs | 79 +++++++++++++++++++++++ crates/model/src/stl.rs | 6 +- crates/ui-egui/tests/ui_keyframes.rs | 96 ++++++++++++++++++++++++++++ 5 files changed, 187 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/commands/edit.rs b/crates/engine/src/commands/edit.rs index 06ba43b1c..bbdc43fe3 100644 --- a/crates/engine/src/commands/edit.rs +++ b/crates/engine/src/commands/edit.rs @@ -228,9 +228,7 @@ fn copy(s: &mut Session, p: &Value) -> Result { } // Keyframes selected → copy keys (pasted at the CTI). if !s.state.selected_keys.is_empty() && p.get("layers").is_none() { - s.state.link_clipboard = None; - s.state.effect_clipboard.clear(); - s.state.contents_clipboard.clear(); + clear_clipboards(s); return s.execute("keys.copy", json!({})); } // Effects selected (Effect Controls / timeline) → copy the effects. @@ -256,12 +254,14 @@ fn copy(s: &mut Session, p: &Value) -> Result { } /// Edit ▸ Copy with properties selected (Position, or the Transform group), as in After -/// Effects: an animated property copies its keyframes (the selected ones, else all of them) and -/// an unanimated one its value, for Paste onto the same properties of other layers (#493). +/// Effects: an animated property copies its keyframes and an unanimated one its value, for Paste +/// onto the same properties of other layers (#493). With keyframes selected only those are copied, +/// even across several highlighted properties (#1552); a selected Transform group copies them all. /// `None` when no property (or Transform group) is selected. fn copy_properties(s: &mut Session) -> Result> { let Some(comp) = s.active_comp() else { return Ok(None) }; let mut leaves = vec![]; + let mut copy_all_keys = s.state.selected_keys.is_empty(); for (lid, uid) in &s.state.selected_props { let Some(l) = comp.layer(*lid) else { continue }; if l.props.find(*uid).is_some() { @@ -269,6 +269,7 @@ fn copy_properties(s: &mut Session) -> Result> { } else if let Some(g) = l.props.find_group(*uid) && l.props.match_path_of(*uid).as_deref() == Some("transform") { + copy_all_keys = true; g.walk("", &mut |_, pr| leaves.push((*lid, pr.uid))); } } @@ -286,7 +287,7 @@ fn copy_properties(s: &mut Session) -> Result> { { values.push((path, pr.value.clone())); } - } else if !keys.iter().any(|k| k.layer == *lid && k.prop == *uid) { + } else if copy_all_keys && !keys.iter().any(|k| k.layer == *lid && k.prop == *uid) { keys.extend(pr.keys.iter().map(|k| KeyRef { layer: *lid, prop: *uid, time: k.time })); } } diff --git a/crates/engine/src/tests_menu_cmds.rs b/crates/engine/src/tests_menu_cmds.rs index ec63651f6..5203872ab 100644 --- a/crates/engine/src/tests_menu_cmds.rs +++ b/crates/engine/src/tests_menu_cmds.rs @@ -433,8 +433,10 @@ fn copy_a_selected_property_pastes_onto_the_same_property() { s.execute("prop.toggleAnimation", json!({"layer": a, "path": "transform/opacity"})).unwrap(); s.execute("time.set", json!({"time": 1.0})).unwrap(); s.execute("prop.set", json!({"layer": a, "path": "transform/opacity", "value": 0})).unwrap(); + s.execute("prop.select", json!({"layer": a, "path": "transform/opacity"})).unwrap(); + assert_eq!(s.state.selected_keys.len(), 2); let transform = layer(&s, a).props.sub("transform").unwrap().uid; - s.execute("prop.select", json!({"layer": a, "prop": transform})).unwrap(); + s.execute("prop.select", json!({"layer": a, "prop": transform, "selectKeys": false})).unwrap(); s.execute("edit.copy", json!({})).unwrap(); s.execute("layer.select", json!({"layers": [b]})).unwrap(); s.execute("time.set", json!({"time": 2.0})).unwrap(); diff --git a/crates/engine/src/tests_timeline.rs b/crates/engine/src/tests_timeline.rs index 71a9943c0..39a4d40b9 100644 --- a/crates/engine/src/tests_timeline.rs +++ b/crates/engine/src/tests_timeline.rs @@ -108,6 +108,85 @@ fn keyframe_copy_paste_keeps_relative_timing_and_crosses_properties() { assert!(e.is_err() || prop(&s, l2, "transform/position").keys.is_empty()); } +#[test] +fn copy_selected_keys_excludes_deselected_properties_and_stale_values() { + let (mut s, source) = setup(); + s.execute("prop.set", json!({"layer": source, "path": "transform/opacity", "value": 25})).unwrap(); + select_prop_keys(&mut s, source, "transform/opacity"); + s.execute("edit.copy", json!({})).unwrap(); + assert_eq!(s.state.value_clipboard.len(), 1); + animate(&mut s, source, "transform/position", &[(0.0, json!([10, 20, 0])), (1.0, json!([30, 40, 0]))]); + animate(&mut s, source, "transform/rotation", &[(0.0, json!(15)), (1.0, json!(90))]); + animate(&mut s, source, "transform/scale", &[(0.0, json!([50, 60, 100])), (1.0, json!([150, 160, 100]))]); + select_prop_keys(&mut s, source, "transform/scale"); + s.execute( + "keys.select", + json!({"keys": [{"layer": source, "path": "transform/position", "time": 0}, {"layer": source, "path": "transform/rotation", "time": 1}], "add": true, "selectProperties": true}), + ) + .unwrap(); + s.execute( + "keys.select", + json!({"keys": [{"layer": source, "path": "transform/scale", "time": 0}, {"layer": source, "path": "transform/scale", "time": 1}], "toggle": true, "selectProperties": true}), + ) + .unwrap(); + s.execute("keys.easyEase", json!({})).unwrap(); + assert_eq!(s.state.selected_keys.len(), 2); + assert_eq!(s.state.selected_props.len(), 3); + assert_eq!(s.execute("edit.copy", json!({})).unwrap(), json!(2)); + assert!(s.state.value_clipboard.is_empty()); + let target = s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("time.set", json!({"time": 2})).unwrap(); + s.execute("layer.select", json!({"layers": [target]})).unwrap(); + s.execute("edit.paste", json!({})).unwrap(); + undo_redo_roundtrip( + &mut s, + |s| { + for (path, time, value) in [("transform/position", 2.0, KV::Vec3([10.0, 20.0, 0.0])), ("transform/rotation", 3.0, KV::Scalar(90.0))] { + let p = prop(s, target, path); + assert_eq!(p.keys.len(), 1); + let k = &p.keys[0]; + assert_eq!((k.time.seconds(), &k.value), (time, &value)); + assert_eq!((k.in_interp, k.out_interp), (Interp::Bezier, Interp::Bezier)); + assert_eq!(k.in_ease[0], effectcraft_keyframe::Ease::EASY); + assert_eq!(k.out_ease[0], effectcraft_keyframe::Ease::EASY); + } + assert_eq!(s.state.selected_keys.len(), 2); + assert_eq!(prop(s, target, "transform/scale").value, KV::Vec3([100.0; 3])); + assert!(prop(s, target, "transform/scale").keys.is_empty()); + assert_eq!(prop(s, target, "transform/opacity").value, KV::Scalar(100.0)); + }, + |s| { + assert!(prop(s, target, "transform/position").keys.is_empty()); + assert!(prop(s, target, "transform/rotation").keys.is_empty()); + }, + ); +} + +#[test] +fn copy_mixed_animated_and_static_properties_keeps_the_static_value() { + let (mut s, source) = setup(); + animate(&mut s, source, "transform/position", &[(0.0, json!([10, 20, 0])), (1.0, json!([30, 40, 0]))]); + s.execute("prop.set", json!({"layer": source, "path": "transform/rotation", "value": 45})).unwrap(); + select_prop_keys(&mut s, source, "transform/position"); + s.execute("prop.select", json!({"layer": source, "path": "transform/rotation", "add": true})).unwrap(); + assert_eq!(s.execute("edit.copy", json!({})).unwrap(), json!({"keys": 2, "values": 1})); + let target = s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("time.set", json!({"time": 2})).unwrap(); + s.execute("layer.select", json!({"layers": [target]})).unwrap(); + let undo = s.history.undo.len(); + s.execute("edit.paste", json!({})).unwrap(); + assert_eq!(s.history.undo.len(), undo + 1); + assert_eq!(prop(&s, target, "transform/rotation").value, KV::Scalar(45.0)); + let position = prop(&s, target, "transform/position"); + assert_eq!( + position.keys.iter().map(|k| (k.time.seconds(), k.value.clone())).collect::>(), + vec![(2.0, KV::Vec3([10.0, 20.0, 0.0])), (3.0, KV::Vec3([30.0, 40.0, 0.0]))] + ); + s.execute("edit.undo", json!({})).unwrap(); + assert!(prop(&s, target, "transform/position").keys.is_empty()); + assert_eq!(prop(&s, target, "transform/rotation").value, KV::Scalar(0.0)); +} + #[test] fn cut_removes_keys_and_paste_restores_them() { let (mut s, l) = setup(); diff --git a/crates/model/src/stl.rs b/crates/model/src/stl.rs index 6684858ad..34e9a7f06 100644 --- a/crates/model/src/stl.rs +++ b/crates/model/src/stl.rs @@ -175,10 +175,8 @@ fn ascii(bytes: &[u8]) -> Result { let Some(key) = it.next() else { continue }; let bad = || perr(format!("STL line {}: expected three numbers", ln + 1)); match key.to_ascii_lowercase().as_str() { - "solid" => { - if name.is_none() { - name = Some(line.get(5..).unwrap_or("").trim().to_string()); - } + "solid" if name.is_none() => { + name = Some(line.get(5..).unwrap_or("").trim().to_string()); } "facet" => { // `facet normal nx ny nz` diff --git a/crates/ui-egui/tests/ui_keyframes.rs b/crates/ui-egui/tests/ui_keyframes.rs index 4b5d1338a..6167fbf8b 100644 --- a/crates/ui-egui/tests/ui_keyframes.rs +++ b/crates/ui-egui/tests/ui_keyframes.rs @@ -126,6 +126,102 @@ fn click_with(h: &mut Harness<'_, EffectcraftApp>, p: Pos2, modifiers: egui::Mod h.run_steps(2); } +#[test] +fn copy_and_paste_key_subsets_across_position_rotation_and_scale() { + for graph in [false, true] { + let (mut h, id, _) = harness(); + let paths = ["transform/position", "transform/rotation", "transform/scale"]; + for (path, times, values) in [ + (paths[0], [0.5, 1.0], [json!([100, 200, 0]), json!([300, 400, 0])]), + (paths[1], [0.5, 1.5], [json!(500), json!(900)]), + (paths[2], [0.0, 1.0], [json!([1500, 1600, 100]), json!([1700, 1800, 100])]), + ] { + for (time, value) in times.into_iter().zip(values) { + h.state_mut().session.execute("prop.addKey", json!({"layer": id.0, "path": path, "time": time, "value": value})).unwrap(); + } + } + let uids: Vec = paths.iter().map(|path| h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap().uid).collect(); + let ctx = h.ctx.clone(); + for command in ["timeline.reveal.position", "timeline.revealAdd.rotation", "timeline.revealAdd.scale"] { + effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, command, json!({})).unwrap(); + } + if graph { + h.state_mut().session.execute("edit.deselectAll", json!({})).unwrap(); + for uid in &uids { + h.state_mut().session.execute("prop.select", json!({"layer": id.0, "prop": uid, "add": true, "selectKeys": false})).unwrap(); + } + h.state_mut().ui.timeline.graph_editor = true; + h.state_mut().ui.timeline.graph_mode = "value".into(); + } + h.run_steps(4); + let key_positions = |h: &Harness<'_, EffectcraftApp>, uid: u64| { + if !graph { + return keys(h, uid); + } + let mut positions: Vec = h + .state() + .auto + .query(&format!("timeline.graph.key.{uid}.0.")) + .iter() + .map(|e| pos2(e.rect[0] + e.rect[2] / 2.0, e.rect[1] + e.rect[3] / 2.0)) + .collect(); + positions.sort_by(|a, b| a.x.total_cmp(&b.x)); + positions + }; + let shift = egui::Modifiers { shift: true, ..Default::default() }; + for include_scale in [true, false] { + h.state_mut().session.execute("keys.select", json!({"keys": []})).unwrap(); + h.run_steps(2); + let original: Vec<_> = + paths.iter().map(|path| h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap().keys.clone()).collect(); + let positions = key_positions(&h, uids[0]); + let rotations = key_positions(&h, uids[1]); + let scales = key_positions(&h, uids[2]); + click_with(&mut h, scales[0], Default::default()); + click_with(&mut h, positions[0], shift); + click_with(&mut h, rotations[1], shift); + if !include_scale { + click_with(&mut h, scales[1], shift); + click_with(&mut h, scales[0], shift); + click_with(&mut h, scales[1], shift); + } + let count = if include_scale { 3 } else { 2 }; + assert_eq!(h.state().session.state.selected_keys.len(), count); + assert_eq!(h.state().session.state.selected_props.len(), 3); + h.input_mut().events.push(Event::Copy); + h.step(); + let note = format!("EffectCraft: {count} keyframes"); + assert!(h.output().platform_output.commands.iter().any(|command| matches!(command, egui::OutputCommand::CopyText(text) if text == ¬e))); + h.state_mut().session.set_time(effectcraft_engine::time::Tick::from_seconds_f64(2.0)); + let undo_steps = h.state().session.history.undo.len(); + h.input_mut().events.push(Event::Paste(note)); + h.run_steps(2); + assert_eq!(h.state().session.history.undo.len(), undo_steps + 1); + for (path, time, value) in + [(paths[0], if include_scale { 2.5 } else { 2.0 }, json!([100.0, 200.0, 0.0])), (paths[1], if include_scale { 3.5 } else { 3.0 }, json!(900.0))] + { + let p = h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap(); + assert_eq!(p.keys.len(), 3); + assert_eq!(p.keys[2].time.seconds(), time); + assert_eq!(p.keys[2].value.to_json(), value); + } + let scale = h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(paths[2]).unwrap(); + if include_scale { + assert_eq!(scale.keys.len(), 3); + assert_eq!(scale.keys[2].time.seconds(), 2.0); + assert_eq!(scale.keys[2].value.to_json(), json!([1500.0, 1600.0, 100.0])); + } else { + assert_eq!(scale.keys, original[2]); + } + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + for (path, expected) in paths.iter().zip(&original) { + assert_eq!(&h.state().session.active_comp().unwrap().layer(id).unwrap().props.prop(path).unwrap().keys, expected); + } + h.run_steps(2); + } + } +} + #[test] fn shift_click_toggles_and_ctrl_click_switches_interpolation() { let (mut h, id, uid) = harness(); From c2f790b1d967c658bfb7d4a2a682f6e17951eafe Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Thu, 15 Oct 2026 16:35:49 -0600 Subject: [PATCH 14/16] refactor(pdf): share structure parsing through a bounded adapter Load PDF document objects through the pinned craft-pdf parser and convert them into EffectCraft's existing drawing model with bounded traversal, recovery and retained data. Preserve generation numbers, stream limits, encrypted-document refusal and reported repairs. Retain app-owned drawing, EPS, fonts and content/image decoding. Assert early excessive-nesting rejection without partial art, and cover tolerant arrays on native and WASM. Validated with the22-stage local matrix and exact-head FreeBSD, Windows ARM64 and packaging checks. Co-authored-by: Codex --- Cargo.lock | 147 ++++++++++++++ Cargo.toml | 4 + NOTICE | 5 + ROADMAP.md | 1 + crates/pdf/Cargo.toml | 4 + crates/pdf/README.md | 51 ++++- crates/pdf/src/lib.rs | 15 +- crates/pdf/src/object.rs | 173 ++-------------- crates/pdf/src/read.rs | 350 +++++++++++++++++++++++++++++++++ crates/pdf/src/tests.rs | 35 +++- crates/pdf/src/tests_import.rs | 6 + crates/pdf/src/tests_reader.rs | 76 +++++++ docs/architecture.md | 2 + xtask/src/main.rs | 9 +- xtask/src/pdf_deps.rs | 117 +++++++++++ 15 files changed, 813 insertions(+), 182 deletions(-) create mode 100644 crates/pdf/src/read.rs create mode 100644 crates/pdf/src/tests_reader.rs create mode 100644 xtask/src/pdf_deps.rs diff --git a/Cargo.lock b/Cargo.lock index ce8fe1231..b6ae19104 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -747,6 +747,12 @@ dependencies = [ "wayland-client", ] +[[package]] +name = "cast" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" + [[package]] name = "cc" version = "1.5.1" @@ -962,6 +968,15 @@ dependencies = [ "libc", ] +[[package]] +name = "craft-pdf" +version = "0.1.0" +source = "git+https://github.com/storytold/craft-libs?rev=ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d#ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d" +dependencies = [ + "hayro-syntax", + "thiserror 2.0.21", +] + [[package]] name = "craft-ui" version = "0.1.0" @@ -1478,6 +1493,7 @@ dependencies = [ name = "effectcraft-pdf" version = "0.10.0" dependencies = [ + "craft-pdf", "effectcraft-raster", "effectcraft-svg", "effectcraft-text", @@ -1485,6 +1501,7 @@ dependencies = [ "kurbo", "miniz_oxide 0.8.9", "skrifa", + "wasm-bindgen-test", "zune-jpeg", ] @@ -2678,6 +2695,40 @@ dependencies = [ "foldhash 0.2.0", ] +[[package]] +name = "hayro-ccitt" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f4d0e94ddd48749f06bbe4e5389fb9799a0c45bcaf00495042076ef05e3241a" + +[[package]] +name = "hayro-jbig2" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "69374b3668dd45aeb3d3145cda68f2c7b4f223aaa2511e67d076f1c7d741388d" +dependencies = [ + "hayro-ccitt", +] + +[[package]] +name = "hayro-jpeg2000" +version = "0.3.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c75ab947623ef4ccaa7acf0579edf7cbb5a73838e3839a7be73335e522f433a1" + +[[package]] +name = "hayro-syntax" +version = "0.7.2" +source = "git+https://github.com/storytold/craft-libs?rev=ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d#ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d" +dependencies = [ + "hayro-jbig2", + "hayro-jpeg2000", + "rustc-hash 2.1.3", + "smallvec", + "stringprep", + "zune-jpeg", +] + [[package]] name = "heck" version = "0.5.0" @@ -3167,6 +3218,16 @@ dependencies = [ "autocfg", ] +[[package]] +name = "minicov" +version = "0.3.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4869b6a491569605d66d3952bcdf03df789e5b536e5f0cf7758a7f08a55ae24d" +dependencies = [ + "cc", + "walkdir", +] + [[package]] name = "miniz_oxide" version = "0.8.9" @@ -3311,6 +3372,15 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2bf50223579dc7cdcfb3bfcacf7069ff68243f8c363f62ffa99cf000a6b9c451" +[[package]] +name = "nu-ansi-term" +version = "0.50.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" +dependencies = [ + "windows-sys 0.61.2", +] + [[package]] name = "num-bigint" version = "0.5.1" @@ -3759,6 +3829,12 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cfe21416a02c693fb9f980befcb230ecc70b0b3d1cc4abf88b9675c4c1457f0c" +[[package]] +name = "oorandom" +version = "11.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6790f58c7ff633d8771f42965289203411a5e5c68388703c06e14f24770b41e" + [[package]] name = "orbclient" version = "0.3.55" @@ -4773,6 +4849,17 @@ dependencies = [ "serde", ] +[[package]] +name = "stringprep" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b4df3d392d81bd458a8a621b8bffbd2302a12ffe288a9d931670948749463b1" +dependencies = [ + "unicode-bidi", + "unicode-normalization", + "unicode-properties", +] + [[package]] name = "strum" version = "0.28.0" @@ -5156,6 +5243,12 @@ dependencies = [ "zerovec", ] +[[package]] +name = "tinyvec" +version = "1.13.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fd3ca314f692efd6c868f8408f53fe444634a845f96c028b97d35f6a1f79f0ee" + [[package]] name = "tokio" version = "1.53.1" @@ -5298,6 +5391,21 @@ version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3b09c83c3c29d37506a3e260c08c03743a6bb66a9cd432c6934ab501a190571f" +[[package]] +name = "unicode-normalization" +version = "0.1.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5fd4f6878c9cb28d874b009da9e8d183b5abc80117c40bbd187a1fde336be6e8" +dependencies = [ + "tinyvec", +] + +[[package]] +name = "unicode-properties" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7df058c713841ad818f1dc5d3fd88063241cc61f49f5fbea4b951e8cf5a8d71d" + [[package]] name = "unicode-segmentation" version = "1.13.3" @@ -5462,6 +5570,45 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "wasm-bindgen-test" +version = "0.3.79" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae7499dfd45780a0a91d7ee6bb9ac51970a4479a41a89da443fdda5a39547d42" +dependencies = [ + "async-trait", + "cast", + "js-sys", + "libm", + "minicov", + "nu-ansi-term", + "num-traits", + "oorandom", + "serde", + "serde_json", + "wasm-bindgen", + "wasm-bindgen-futures", + "wasm-bindgen-test-macro", + "wasm-bindgen-test-shared", +] + +[[package]] +name = "wasm-bindgen-test-macro" +version = "0.3.79" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b84b5ac638bfb168196a1a461fcc8f46a294a18b1b6be52133b4e0db122cc9f" +dependencies = [ + "proc-macro2", + "quote", + "syn 3.0.6", +] + +[[package]] +name = "wasm-bindgen-test-shared" +version = "0.2.129" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4f692aa943ccd88363733b77063f32cfed5bc6cbea8e6e8b251b302f881606fe" + [[package]] name = "wasm-encoder" version = "0.261.0" diff --git a/Cargo.toml b/Cargo.toml index 9f4e9d17f..5bcb4bf1d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,6 +11,7 @@ rust-version = "1.95" repository = "https://github.com/storytold/effectcraft" [workspace.dependencies] +craft-pdf = { git = "https://github.com/storytold/craft-libs", rev = "ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d", version = "=0.1.0", default-features = false, features = ["read"] } # internal effectcraft-time = { path = "crates/time" } effectcraft-geom = { path = "crates/geom" } @@ -204,3 +205,6 @@ opt-level = 3 [profile.release] lto = "thin" codegen-units = 1 + +[patch.crates-io] +hayro-syntax = { git = "https://github.com/storytold/craft-libs", rev = "ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d", version = "=0.7.2" } diff --git a/NOTICE b/NOTICE index 65ce4c42c..43bd2e2a9 100644 --- a/NOTICE +++ b/NOTICE @@ -15,3 +15,8 @@ list (author, source, license) is in ATTRIBUTION.md. Inter (assets/fonts/Inter-*.ttf) — SIL Open Font License 1.1. See assets/fonts/OFL-Inter.txt. Noto Serif (assets/fonts/NotoSerif-Regular.ttf) — SIL Open Font License 1.1. See assets/fonts/OFL-NotoSerif.txt. JetBrains Mono (assets/fonts/JetBrainsMono-Regular.ttf) — SIL Open Font License 1.1. See assets/fonts/OFL-JetBrainsMono.txt. + +The optional PDF document reader uses craft-pdf and Hayro-syntax 0.7.2, MIT OR Apache-2.0. +The shared patched syntax source is centralized in storytold/craft-libs, transferred from +PdfCraft revision 9ed3abe4907a127d36b12fbfeee71d7e26248da1. This parser-only use embeds no +Hayro fonts, CMaps or image codecs; EffectCraft keeps its existing fonts and drawing filters. diff --git a/ROADMAP.md b/ROADMAP.md index a579491a8..e21039c02 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -154,6 +154,7 @@ Come tell us what matters most to you on [Discord](https://discord.gg/artcraft). | 2026-10-12 | patch | Roto Brush span validation after trimming; command regressions for both layer edges | | 2026-10-10 | minor | Hours to ≈ 95% for each audience | | 2026-10-10 | minor | Headline adds mainstream practitioner (≈ 47%) and essentials user (≈ 52%) | +| 2026-10-12 | minor | Shared PDF object/xref parsing with bounded app-owned conversion and preserved stream budgets; drawing, EPS and image/font policy remain unchanged | | 2026-10-10 | minor | Stage checked against the new core-workflow gate (all six pass; still alpha) | | 2026-10-10 | major | Rewritten to the shared progress-docs standard: stage, two numbers, dimensions, features, languages, upcoming, progress log; milestones moved to docs/roadmap.md | | 2026-10-05 | major | Two numbers (breadth vs real use) and workstreams G1–G9 | diff --git a/crates/pdf/Cargo.toml b/crates/pdf/Cargo.toml index 9b761f57b..b37dff50f 100644 --- a/crates/pdf/Cargo.toml +++ b/crates/pdf/Cargo.toml @@ -10,6 +10,7 @@ rust-version.workspace = true workspace = true [dependencies] +craft-pdf.workspace = true effectcraft-svg = { workspace = true } kurbo = { workspace = true } effectcraft-text = { workspace = true } @@ -23,3 +24,6 @@ zune-jpeg = "0.5" # Encodes JPEG fixtures for the DCT image test. image = { workspace = true } effectcraft-raster = { workspace = true } + +[target.'cfg(target_arch = "wasm32")'.dev-dependencies] +wasm-bindgen-test = "0.3" diff --git a/crates/pdf/README.md b/crates/pdf/README.md index b7678b096..57603cf98 100644 --- a/crates/pdf/README.md +++ b/crates/pdf/README.md @@ -14,20 +14,23 @@ Charstring Format) for CFF font programs; **Adobe Type 1 Font Format** (version font programs; the **PostScript Language Reference, third edition** and the **Encapsulated PostScript File Format Specification 3.0** for EPS; **ITU-T T.4** (07/2003) and **ITU-T T.6** (11/1988) for CCITT fax images. TrueType font programs are read through skrifa and JPEG (DCT) -images through zune-jpeg (both permissively licensed, pure Rust). No third-party -PDF/PostScript/font/codec code was read; test files (including the CFF and Type 1 fonts and the -CCITT data) are generated in code ([`write`](src/write.rs) and the tests). One CCITT fixture was +images through zune-jpeg (both permissively licensed, pure Rust). PDF document/object loading uses the shared `craft-pdf` parser described below. The retained +drawing, PostScript, font and filter implementations were developed from the public +specifications without copying third-party implementations. Test files (including the CFF +and Type 1 fonts and the CCITT data) are generated in code ([`write`](src/write.rs) and the tests). One CCITT fixture was produced once by libtiff's `tiffcp` (an external encoder used only as a fixture generator, like ffmpeg elsewhere) to check the code tables against an independent implementation. ## What is read -- **Structure:** indirect objects found by scanning (damaged or incremental cross-reference - sections do not matter), object streams, the page tree with inherited resources, MediaBox / - CropBox and `/Rotate`. Filters: Flate (with PNG predictors), LZW, ASCIIHex, ASCII85, - RunLength, CCITTFax (Group 3 one- and two-dimensional, Group 4; `K`, `Columns`, `Rows`, - `EndOfLine`, `EncodedByteAlign`, `EndOfBlock`, `BlackIs1`; damaged data keeps the rows - decoded before the damage). Encrypted files are refused. +- **Structure:** `craft-pdf` resolves cross-reference tables/streams and indirect objects into + the app's existing object model. References retain object and generation numbers. A bounded + compatibility repair recovers referenced objects from object streams when damaged xrefs omit + them; every recovery or unresolved reference is reported in `Doc::skipped` (up to 64 messages). + The existing page tree preserves inherited resources, MediaBox / CropBox and `/Rotate`. + Content/image filters remain app-owned: Flate (with PNG predictors), LZW, ASCIIHex, ASCII85, + RunLength and CCITTFax. Encrypted files remain refused; `page_count` returns zero when the + reader refuses a document. - **Content:** `q`/`Q`, `cm`, line width / cap / join / miter / dash, ExtGState (`CA`, `ca`, `LW`, `LC`, `LJ`, `ML`, `D`), all path construction and painting operators (non-zero and even-odd), clipping (`W`, `W*`) as clipping groups, colour in DeviceGray / RGB / CMYK, ICCBased (by @@ -129,3 +132,33 @@ ffmpeg elsewhere) to check the code tables against an independent implementation layer; the shapes drawn between images go to shape layers of their own (` Outlines `), so the layer stack keeps the document's paint order. Soft masks, and the clips and masks of images, are not converted (the command's `skipped` list says so). + +## Shared parser setup and bounds + +The workspace pins `craft-pdf` to `craft-libs` Git revision +`ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d`, with `default-features = false` and only the explicit `read` +feature. This excludes the backend's bundled fonts and Adobe CMaps and preserves EffectCraft's +font and asset policies. The shared parser does not change the supported drawing operators, +image codecs, continuously rasterized SVG tree, editable shapes, or EPS interpreter. +This revision removes an unchecked lazy-array read and preserves later values and pages after +sign-only numeric entries; the app exercises those inputs through its object adapter, public +page parser and rasterizer on native and WASM targets. + +The root Cargo patch pins Hayro-syntax to the same `storytold/craft-libs` revision. `cargo xtask pdf-deps` checks this graph and refuses +an enabled Hayro interpreter, rasterizer or CMap dependency. The shared revision must be +published before a fresh app checkout can resolve it; no sibling checkout is required. + +Input is limited to 512 MiB. The object adapter admits at most 100,000 document/referenced +objects, 1,000,000 converted values and 64 inline nesting levels, with a 512 MiB retained +object-data budget. References are queued once rather than recursively expanded. Native +unwinding panics during shared loading or lazy object conversion become an error and discard +partial state. These limits are not an overall process-memory or execution-time guarantee; +WASM aborts and process-wide allocation failures cannot be caught. Missing or repaired objects +are reported rather than silently presented as a complete import. + +The shared parser receives EffectCraft's existing ordinary object/xref stream limit at open: +256 MiB on 64-bit targets and 64 MiB on 32-bit targets, including WASM. Immutable document +limits apply during normal parsing, initial xrefs, fallback and repair. Malformed object-stream +compatibility recovery also obeys that limit and the adapter's remaining retained-data budget. +Content/image filter decoding still uses EffectCraft's existing bounded filter chain; the +retained-data cap is not a bound on every parser cache or temporary allocation. diff --git a/crates/pdf/src/lib.rs b/crates/pdf/src/lib.rs index 39d150749..f4616a9a2 100644 --- a/crates/pdf/src/lib.rs +++ b/crates/pdf/src/lib.rs @@ -19,6 +19,7 @@ mod image; mod object; mod page; mod ps; +mod read; mod shading; mod type1; pub mod write; @@ -33,6 +34,7 @@ pub enum Error { NoPages, PageOutOfRange(usize), NoBoundingBox, + Read(String), } impl std::fmt::Display for Error { @@ -41,8 +43,9 @@ impl std::fmt::Display for Error { Error::NotVector => write!(f, "not a PDF, Illustrator or EPS file"), Error::Encrypted => write!(f, "encrypted PDF files are not supported"), Error::NoPages => write!(f, "the PDF has no pages"), - Error::PageOutOfRange(n) => write!(f, "the PDF has no page {}", n + 1), + Error::PageOutOfRange(n) => write!(f, "the PDF has no page {}", n.saturating_add(1)), Error::NoBoundingBox => write!(f, "the EPS file has no %%BoundingBox"), + Error::Read(message) => f.write_str(message), } } } @@ -99,7 +102,7 @@ pub fn parse_page(bytes: &[u8], index: usize) -> Result { /// Number of pages (1 for EPS). pub fn page_count(bytes: &[u8]) -> usize { match sniff(bytes) { - Some(Format::Pdf) => page::pages(&object::File::parse(bytes)).len(), + Some(Format::Pdf) => object::File::parse(bytes).map(|file| page::pages(&file).len()).unwrap_or(0), Some(Format::Eps) => 1, None => 0, } @@ -140,10 +143,7 @@ fn finish(nodes: Vec, flags: Vec, skipped: Vec, width: f64, } fn parse_pdf(bytes: &[u8], index: usize) -> Result { - let file = object::File::parse(bytes); - if file.encrypted() { - return Err(Error::Encrypted); - } + let file = object::File::parse(bytes)?; let pages = page::pages(&file); if pages.is_empty() { return Err(Error::NoPages); @@ -160,6 +160,7 @@ fn parse_pdf(bytes: &[u8], index: usize) -> Result { }; let content = page::page_content(&file, pg); let mut it = page::Interp::new(&file, pg.bbox); + it.b.skipped.extend(file.repairs.iter().cloned()); it.run(&content, &pg.resources, Affine::IDENTITY, 0); let (nodes, flags, skipped) = it.b.finish(); Ok(finish(nodes, flags, skipped, dw.max(1.0), dh.max(1.0), to_doc)) @@ -201,4 +202,6 @@ mod tests_groups; #[cfg(test)] mod tests_import; #[cfg(test)] +mod tests_reader; +#[cfg(test)] mod tests_shading; diff --git a/crates/pdf/src/object.rs b/crates/pdf/src/object.rs index b9016222e..cdbb9733a 100644 --- a/crates/pdf/src/object.rs +++ b/crates/pdf/src/object.rs @@ -1,9 +1,6 @@ -//! PDF objects (ISO 32000-1 §7.2–7.5): the lexer, the object model, stream filters and the -//! indirect-object table. -//! -//! The table is built by scanning the file for `n g obj` headers (later definitions win, as -//! incremental updates do) and by unpacking object streams, so damaged or rewritten -//! cross-reference sections do not matter. +//! Local object model, content-stream lexer and stream filters. Document objects are read +//! through the shared parser in `read`; the local lexer remains for content operators and +//! PostScript-related data. use std::collections::HashMap; @@ -191,7 +188,7 @@ impl<'a> Lexer<'a> { } /// `n g R` after a number inside arrays and dictionaries. - fn maybe_ref(&mut self, v: Obj) -> Obj { + pub(super) fn maybe_ref(&mut self, v: Obj) -> Obj { let Obj::Num(n) = v else { return v }; if n.fract() != 0.0 || n < 0.0 { return v; @@ -312,11 +309,10 @@ pub fn parse_num(w: &[u8]) -> Option { /// growing buffer can reserve about twice the limit. const MAX_INFLATED: usize = if cfg!(target_pointer_width = "32") { 256 << 20 } else { 1 << 30 }; -/// The tighter bound for object streams, which [`File::parse`] decodes eagerly (all of them, -/// before any page is chosen). They hold only other objects' dictionaries, arrays, numbers and -/// strings, so real ones are far smaller; PdfCraft uses 256 MiB for the object and -/// cross-reference streams it decodes while loading. A quarter on 32-bit targets, as above. -const MAX_OBJECT_STREAM: usize = if cfg!(target_pointer_width = "32") { 64 << 20 } else { 256 << 20 }; +/// The bound for ordinary object/xref streams and malformed object-stream recovery. +/// The shared parser receives this immutable limit when the document is opened; +/// 32-bit targets retain the app's quarter-sized limit. +pub(crate) const MAX_OBJECT_STREAM: usize = if cfg!(target_pointer_width = "32") { 64 << 20 } else { 256 << 20 }; /// Decode a stream's data through its `/Filter` chain. Unknown filters (images: DCT, JPX, CCITT, /// JBIG2) give `None`. @@ -624,151 +620,14 @@ fn predict(data: &[u8], p: &Dict, max: usize) -> Option> { /// Every indirect object of a PDF file, plus the trailer dictionaries found. pub struct File { - pub objects: HashMap, + pub objects: HashMap<(u32, u16), Obj>, pub trailers: Vec, -} - -fn find(hay: &[u8], needle: &[u8], from: usize) -> Option { - if from >= hay.len() || needle.is_empty() { - return None; - } - hay[from..].windows(needle.len()).position(|w| w == needle).map(|p| p + from) + pub repairs: Vec, } impl File { - pub fn parse(data: &[u8]) -> File { - let mut f = File { objects: HashMap::new(), trailers: vec![] }; - let mut i = 0; - while let Some(p) = find(data, b"obj", i) { - i = p + 3; - if p >= 3 && &data[p - 3..p] == b"end" { - continue; - } - if data.get(p + 3).is_some_and(|c| !is_white(*c) && !is_delim(*c)) { - continue; - } - // Walk back over "num gen ". - let mut q = p; - let back_ws = |q: &mut usize| { - let s = *q; - while *q > 0 && is_white(data[*q - 1]) { - *q -= 1; - } - *q < s - }; - let back_digits = |q: &mut usize| { - let e = *q; - while *q > 0 && data[*q - 1].is_ascii_digit() { - *q -= 1; - } - (*q < e).then(|| std::str::from_utf8(&data[*q..e]).ok()?.parse::().ok()).flatten() - }; - if !back_ws(&mut q) { - continue; - } - let Some(_gen) = back_digits(&mut q) else { continue }; - if !back_ws(&mut q) { - continue; - } - let Some(num) = back_digits(&mut q) else { continue }; - if q > 0 && !is_white(data[q - 1]) && !is_delim(data[q - 1]) { - continue; - } - let mut lx = Lexer::new(data, p + 3); - let Some(obj) = lx.next() else { continue }; - let obj = lx.maybe_ref(obj); - let obj = match obj { - Obj::Dict(d) => { - let save = lx.pos; - lx.skip_ws(); - if data.get(lx.pos..).is_some_and(|d| d.starts_with(b"stream")) { - let mut s = lx.pos + 6; - if data.get(s) == Some(&b'\r') { - s += 1; - } - if data.get(s) == Some(&b'\n') { - s += 1; - } - let declared = d.get("Length").and_then(Obj::num).map(|l| l as usize); - let s = s.min(data.len()); - let end = match declared.filter(|l| { - s.checked_add(*l).is_some_and(|e| e <= data.len()) && { - let mut k = s + l; - while k < data.len() && is_white(data[k]) { - k += 1; - } - data[k..].starts_with(b"endstream") - } - }) { - Some(l) => s + l, - None => { - let mut e = find(data, b"endstream", s).unwrap_or(data.len()); - if e > s && data[e - 1] == b'\n' { - e -= 1; - } - if e > s && data[e - 1] == b'\r' { - e -= 1; - } - e - } - }; - let end = end.max(s); - i = end; - Obj::Stream(d, data[s..end].to_vec()) - } else { - lx.pos = save; - Obj::Dict(d) - } - } - o => o, - }; - if let Obj::Stream(d, _) = &obj - && d.get("Type").and_then(Obj::name) == Some("XRef") - { - f.trailers.push(d.clone()); - } - f.objects.insert(num, obj); - } - // Trailer dictionaries of classic cross-reference sections. - let mut i = 0; - while let Some(p) = find(data, b"trailer", i) { - i = p + 7; - let mut lx = Lexer::new(data, i); - if let Some(Obj::Dict(d)) = lx.next() { - f.trailers.push(d); - } - } - // Object streams. - let streams: Vec<(Dict, Vec)> = f - .objects - .values() - .filter_map(|o| match o { - Obj::Stream(d, raw) if d.get("Type").and_then(Obj::name) == Some("ObjStm") => Some((d.clone(), raw.clone())), - _ => None, - }) - .collect(); - for (d, raw) in streams { - let Some(dec) = decode_stream_within(&f, &d, &raw, MAX_OBJECT_STREAM) else { continue }; - let n = d.get("N").and_then(Obj::num).unwrap_or(0.0) as usize; - let first = d.get("First").and_then(Obj::num).unwrap_or(0.0) as usize; - let mut lx = Lexer::new(&dec, 0); - let mut heads = vec![]; - for _ in 0..n.min(100_000) { - let (Some(Obj::Num(num)), Some(Obj::Num(off))) = (lx.next(), lx.next()) else { break }; - heads.push((num as u32, off as usize)); - } - for (num, off) in heads { - if f.objects.contains_key(&num) || first + off >= dec.len() { - continue; - } - let mut lx = Lexer::new(&dec, first + off); - if let Some(o) = lx.next() { - let o = lx.maybe_ref(o); - f.objects.insert(num, o); - } - } - } - f + pub fn parse(data: &[u8]) -> Result { + crate::read::open(data) } /// Follow references (bounded). @@ -776,7 +635,7 @@ impl File { let mut o = o; for _ in 0..32 { match o { - Obj::Ref(n, _) => match self.objects.get(n) { + Obj::Ref(n, g) => match self.objects.get(&(*n, *g)) { Some(x) => o = x, None => return &Obj::Null, }, @@ -810,13 +669,9 @@ impl File { return Some(d); } } - let mut cats: Vec<(&u32, &Obj)> = + let mut cats: Vec<(&(u32, u16), &Obj)> = self.objects.iter().filter(|(_, o)| matches!(o, Obj::Dict(d) if d.get("Type").and_then(Obj::name) == Some("Catalog"))).collect(); cats.sort_by_key(|(n, _)| **n); cats.last().and_then(|(_, o)| o.dict()) } - - pub fn encrypted(&self) -> bool { - self.trailers.iter().any(|t| t.contains_key("Encrypt")) - } } diff --git a/crates/pdf/src/read.rs b/crates/pdf/src/read.rs new file mode 100644 index 000000000..953582fc8 --- /dev/null +++ b/crates/pdf/src/read.rs @@ -0,0 +1,350 @@ +//! Adapter from the shared PDF syntax API to the existing drawing interpreter's object model. +//! Only reachable objects are retained, references keep their generation, and native parser +//! panics (including lazy object reads) discard the entire partially populated adapter. + +use std::collections::{HashMap, HashSet, VecDeque}; +use std::panic::{AssertUnwindSafe, catch_unwind}; +use std::sync::Arc; + +use craft_pdf::syntax::object::{Dict as PdfDict, MaybeRef, Name, ObjRef, Object, ObjectIdentifier, Stream}; +use craft_pdf::syntax::xref::XRef; + +use crate::Error; +use crate::object::{Dict, File, Lexer, Obj}; + +const MAX_OBJECTS: usize = 100_000; +const MAX_VALUES: usize = 1_000_000; +const MAX_DEPTH: usize = 64; +const MAX_BYTES: usize = 512 * 1024 * 1024; + +fn limit(what: &str) -> Error { + Error::Read(format!("PDF exceeds the {what} import limit")) +} + +fn allocation() -> Error { + Error::Read("PDF object storage could not be allocated".into()) +} + +fn open_options() -> craft_pdf::OpenOptions { + craft_pdf::OpenOptions { + decode_limits: craft_pdf::syntax::DecodeLimits::default().with_max_decoded_stream(crate::object::MAX_OBJECT_STREAM), + ..Default::default() + } +} + +pub(crate) fn open(bytes: &[u8]) -> Result { + craft_pdf::check_input_size_with(bytes.len(), &open_options()).map_err(|e| Error::Read(e.to_string()))?; + catch_unwind(AssertUnwindSafe(|| open_inner(bytes))).map_err(|_| Error::Read("PDF parser failed internally".into()))? +} + +fn open_inner(bytes: &[u8]) -> Result { + // Preserve the app's refusal of encrypted PDFs, including empty-user-password files. + // Classic trailer dictionaries use the existing lexer; xref-stream dictionaries below + // are read by the shared parser. This does not parse document objects a second time. + let mut markers = 0; + for (offset, word) in bytes.windows(7).enumerate() { + if word == b"trailer" { + markers += 1; + if markers > MAX_OBJECTS { + return Err(limit("trailer count")); + } + if let Some(Obj::Dict(dict)) = Lexer::new(bytes, offset + 7).next() + && dict.contains_key("Encrypt") + { + return Err(Error::Encrypted); + } + } + } + let pdf = craft_pdf::open_slice_with_options(bytes, None, &open_options()).map_err(|error| match error { + craft_pdf::OpenError::Load(craft_pdf::syntax::LoadPdfError::Decryption(_)) => Error::Encrypted, + craft_pdf::OpenError::Load(craft_pdf::syntax::LoadPdfError::Invalid) => Error::NoPages, + other => Error::Read(other.to_string()), + })?; + let mut object_streams = Vec::new(); + for (index, object) in pdf.objects().into_iter().enumerate() { + if index >= MAX_OBJECTS { + return Err(limit("100000 objects")); + } + if let Object::Stream(stream) = object { + let dict = stream.dict(); + if dict.get::>(b"Type").is_some_and(|name| name.as_ref() == b"XRef") && dict.contains_key(b"Encrypt") { + return Err(Error::Encrypted); + } + if dict.get::>(b"Type").is_some_and(|name| name.as_ref() == b"ObjStm") { + object_streams.try_reserve(1).map_err(|_| allocation())?; + object_streams.push(stream); + } + } + } + let mut reader = Reader { + xref: pdf.xref(), + pending: VecDeque::new(), + seen: HashSet::new(), + values_left: MAX_VALUES, + bytes_left: MAX_BYTES, + object_streams, + recovery: None, + }; + let root = pdf.xref().root_id(); + let root = reader.reference(ObjRef::new(root.obj_number, root.gen_number))?; + let mut file = File { objects: HashMap::new(), trailers: vec![Dict::from([("Root".into(), root)])], repairs: Vec::new() }; + while let Some(id) = reader.pending.pop_front() { + let Some(key) = key(id) else { continue }; + let object = match reader.xref.get::>(id) { + Some(value) => reader.object(value, 0)?, + None => { + let recovered = reader.recover(id)?; + if file.repairs.len() < 64 { + let action = if recovered.is_some() { "recovered PDF object from an object stream" } else { "unresolved PDF object" }; + file.repairs.push(format!("{action}: {} {}", id.obj_number, id.gen_number)); + } + recovered.unwrap_or(Obj::Null) + } + }; + file.objects.try_reserve(1).map_err(|_| allocation())?; + file.objects.insert(key, object); + } + Ok(file) +} + +fn key(id: ObjectIdentifier) -> Option<(u32, u16)> { + Some((u32::try_from(id.obj_number).ok()?, u16::try_from(id.gen_number).ok()?)) +} + +struct Reader<'a> { + xref: &'a XRef, + pending: VecDeque, + seen: HashSet, + values_left: usize, + bytes_left: usize, + object_streams: Vec>, + recovery: Option>>, +} + +struct Recovered { + bytes: Arc>, + start: usize, + end: usize, +} + +impl Reader<'_> { + fn value(&mut self) -> Result<(), Error> { + self.values_left = self.values_left.checked_sub(1).ok_or_else(|| limit("1000000 values"))?; + Ok(()) + } + + fn bytes(&mut self, bytes: &[u8]) -> Result, Error> { + self.bytes_left = self.bytes_left.checked_sub(bytes.len()).ok_or_else(|| limit("512 MiB object data"))?; + let mut out = Vec::new(); + out.try_reserve_exact(bytes.len()).map_err(|_| allocation())?; + out.extend_from_slice(bytes); + Ok(out) + } + + /// The previous importer could recover compressed objects incorrectly marked free in + /// an otherwise readable xref. Retain that narrow repair only for unresolved references, + /// with a warning, bounded decoding and the existing content/object lexer. + fn recover(&mut self, id: ObjectIdentifier) -> Result, Error> { + if id.gen_number != 0 { + return Ok(None); + } + if self.recovery.is_none() { + let mut recovered: HashMap> = HashMap::new(); + for stream in std::mem::take(&mut self.object_streams) { + let Some(count) = stream.dict().get::(b"N") else { continue }; + let Some(first) = stream.dict().get::(b"First") else { continue }; + if count as usize > MAX_OBJECTS.saturating_sub(recovered.len()) { + return Err(limit("100000 recovered objects")); + } + let decoded = match stream.decoded_within(self.bytes_left.min(crate::object::MAX_OBJECT_STREAM)) { + Ok(data) => data, + Err(_) => continue, + }; + self.bytes_left = self.bytes_left.checked_sub(decoded.len()).ok_or_else(|| limit("512 MiB object data"))?; + let decoded = Arc::new(decoded.into_owned()); + let mut lexer = Lexer::new(&decoded, 0); + let mut offsets = Vec::new(); + for _ in 0..count { + self.value()?; + let (Some(Obj::Num(number)), Some(Obj::Num(offset))) = (lexer.next(), lexer.next()) else { break }; + if !number.is_finite() || number.fract() != 0.0 || !(0.0..=f64::from(u32::MAX)).contains(&number) { + continue; + } + if !offset.is_finite() || offset.fract() != 0.0 || offset < 0.0 || offset > decoded.len() as f64 { + continue; + } + let Some(start) = (first as usize).checked_add(offset as usize).filter(|start| *start < decoded.len()) else { continue }; + offsets.try_reserve(1).map_err(|_| allocation())?; + offsets.push((start, number as u32)); + } + offsets.sort_unstable(); + for (index, &(start, number)) in offsets.iter().enumerate() { + let end = offsets.get(index + 1).map_or(decoded.len(), |(next, _)| *next); + recovered.try_reserve(1).map_err(|_| allocation())?; + recovered.entry(number).and_modify(|entry| *entry = None).or_insert_with(|| Some(Recovered { bytes: Arc::clone(&decoded), start, end })); + } + } + self.recovery = Some(recovered); + } + let Some(number) = u32::try_from(id.obj_number).ok() else { return Ok(None) }; + let Some(location) = self.recovery.as_ref().and_then(|r| r.get(&number)) else { return Ok(None) }; + let Some(location) = location else { return Err(Error::Read(format!("PDF has ambiguous recovered object {number}"))) }; + let bytes = Arc::clone(&location.bytes); + let Some(bytes) = bytes.get(location.start..location.end) else { return Ok(None) }; + let mut lexer = Lexer::new(bytes, 0); + let Some(object) = lexer.next() else { return Ok(None) }; + let object = lexer.maybe_ref(object); + self.register_recovered(&object, 0)?; + Ok(Some(object)) + } + + fn register_recovered(&mut self, object: &Obj, depth: usize) -> Result<(), Error> { + self.value()?; + if depth > MAX_DEPTH { + return Err(limit("64 nested objects")); + } + let bytes = match object { + Obj::Name(text) | Obj::Op(text) => text.len(), + Obj::Str(bytes) => bytes.len(), + Obj::Array(values) => { + for value in values { + self.register_recovered(value, depth + 1)?; + } + 0 + } + Obj::Dict(dict) | Obj::Stream(dict, _) => { + for (name, value) in dict { + self.bytes_left = self.bytes_left.checked_sub(name.len()).ok_or_else(|| limit("512 MiB object data"))?; + self.register_recovered(value, depth + 1)?; + } + 0 + } + Obj::Ref(number, generation) => { + if let Ok(number) = i32::try_from(*number) { + self.reference(ObjRef::new(number, i32::from(*generation)))?; + } + 0 + } + _ => 0, + }; + self.bytes_left = self.bytes_left.checked_sub(bytes).ok_or_else(|| limit("512 MiB object data"))?; + Ok(()) + } + + fn name(&mut self, bytes: &[u8]) -> Result { + // Lossy UTF-8 expansion is at most three output bytes per source byte. + let bound = bytes.len().checked_mul(3).ok_or_else(|| limit("name length"))?; + self.bytes_left = self.bytes_left.checked_sub(bound).ok_or_else(|| limit("512 MiB object data"))?; + Ok(String::from_utf8_lossy(bytes).into_owned()) + } + + fn reference(&mut self, reference: ObjRef) -> Result { + self.value()?; + let id = ObjectIdentifier::new(reference.obj_number, reference.gen_number); + let Some((number, generation)) = key(id) else { return Ok(Obj::Null) }; + if !self.seen.contains(&id) { + if self.seen.len() >= MAX_OBJECTS { + return Err(limit("100000 referenced objects")); + } + self.seen.try_reserve(1).map_err(|_| allocation())?; + self.pending.try_reserve(1).map_err(|_| allocation())?; + self.seen.insert(id); + self.pending.push_back(id); + } + Ok(Obj::Ref(number, generation)) + } + + fn maybe(&mut self, object: MaybeRef>, depth: usize) -> Result { + match object { + MaybeRef::Ref(reference) => self.reference(reference), + MaybeRef::NotRef(object) => self.object(object, depth), + } + } + + fn dictionary(&mut self, dict: &PdfDict<'_>, depth: usize) -> Result { + let mut out = Dict::new(); + for (name, object) in dict.entries() { + let name = self.name(name.as_ref())?; + let object = self.maybe(object, depth + 1)?; + out.try_reserve(1).map_err(|_| allocation())?; + out.insert(name, object); + } + Ok(out) + } + + fn object(&mut self, object: Object<'_>, depth: usize) -> Result { + self.value()?; + if depth > MAX_DEPTH { + return Err(limit("64 nested objects")); + } + Ok(match object { + Object::Null(_) => Obj::Null, + Object::Boolean(value) => Obj::Bool(value), + Object::Number(value) => Obj::Num(value.as_f64()), + Object::String(value) => Obj::Str(self.bytes(value.as_bytes())?), + Object::Name(value) => Obj::Name(self.name(value.as_ref())?), + Object::Array(array) => { + let mut out = Vec::new(); + for value in array.raw_iter() { + let value = self.maybe(value, depth + 1)?; + out.try_reserve(1).map_err(|_| allocation())?; + out.push(value); + } + Obj::Array(out) + } + Object::Dict(dict) => Obj::Dict(self.dictionary(&dict, depth)?), + Object::Stream(stream) => { + let dict = self.dictionary(stream.dict(), depth)?; + Obj::Stream(dict, self.bytes(&stream.raw_data())?) + } + }) + } +} + +#[cfg(test)] +mod limit_tests { + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)] + #[cfg_attr(not(target_arch = "wasm32"), test)] + fn shared_reader_preserves_tolerant_array_numbers_and_later_pages() { + use crate::object::Obj; + + // Array validation accepts these sign/dot-only zero tokens. Previously '+' + // reached an unchecked lazy-iterator read: native loading returned an internal + // parser error and WASM could abort before reaching the following object. + for token in ["+", "-", ".", "+.", "-."] { + for numeric_kid in [false, true] { + let kids = if numeric_kid { format!("{token} 3 0 R") } else { "3 0 R".into() }; + let bytes = crate::write::pdf( + &[ + (1, format!("<< /Type /Catalog /Pages 2 0 R /Probe [{token} 3] >>"), None), + (2, format!("<< /Type /Pages /Kids [{kids}] /Count 1 >>"), None), + (3, "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 40 20] /Resources << >> /Contents 4 0 R >>".into(), None), + (4, "<< >>".into(), Some(b"1 0 0 rg 0 0 40 20 re f\n".to_vec())), + ], + 1, + ); + let file = super::open(&bytes).unwrap(); + assert_eq!(file.catalog().unwrap().get("Probe").and_then(Obj::array), Some([Obj::Num(0.0), Obj::Num(3.0)].as_slice())); + assert_eq!(crate::page_count(&bytes), 1, "token {token}, numeric kid {numeric_kid}"); + let doc = crate::parse(&bytes).unwrap(); + assert_eq!((doc.width, doc.height), (40.0, 20.0)); + assert_eq!(doc.flatten().len(), 1); + assert!(doc.skipped.is_empty(), "{:?}", doc.skipped); + assert_eq!(effectcraft_svg::rasterize(&doc, 40, 20, 1.0).get(20, 10), [1.0, 0.0, 0.0, 1.0]); + assert_eq!(crate::parse_page(&bytes, 1), Err(crate::Error::PageOutOfRange(1))); + } + } + } + + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)] + #[cfg_attr(not(target_arch = "wasm32"), test)] + fn shared_reader_preserves_the_target_stream_budget() { + let options = super::open_options(); + let expected = if cfg!(target_pointer_width = "32") { 64 << 20 } else { 256 << 20 }; + assert_eq!(options.decode_limits.max_decoded_stream(), expected); + assert_eq!(options.max_input_bytes, 512 << 20); + let bytes = b"%PDF-1.7\n1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj\n2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj\n3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 40 20] >> endobj\ntrailer << /Root 1 0 R >>\n%%EOF"; + let pdf = craft_pdf::open_slice_with_options(bytes, None, &options).unwrap(); + assert_eq!(pdf.decode_limits().max_decoded_stream(), expected); + } +} diff --git a/crates/pdf/src/tests.rs b/crates/pdf/src/tests.rs index 57b7e0448..a3dcbb212 100644 --- a/crates/pdf/src/tests.rs +++ b/crates/pdf/src/tests.rs @@ -48,7 +48,10 @@ fn pdf_paths_clip_gradient_and_layers() { assert_eq!(page_count(&bytes), 1); let doc = parse(&bytes).unwrap(); assert_eq!((doc.width, doc.height), (200.0, 100.0)); - assert_eq!(layer_names(&doc), vec!["Background", "Art"], "object stream {objstm}"); + assert_eq!(layer_names(&doc), vec!["Background", "Art"], "object stream {objstm}, skipped: {:?}", doc.skipped); + if objstm { + assert!(doc.skipped.iter().any(|warning| warning.contains("recovered PDF object")), "the malformed xref repair is reported"); + } assert!(doc.skipped.contains(&"text (no font)".to_string()), "{:?}", doc.skipped); let (w, h) = doc.pixel_size(); let img = effectcraft_svg::rasterize(&doc, w, h, 1.0); @@ -196,7 +199,7 @@ fn nested_bomb_in_the_second_flate_filter_is_refused() { const MAX: usize = 1 << 20; let flate = object::Obj::Name("FlateDecode".into()); let chain: object::Dict = [("Filter".to_string(), object::Obj::Array(vec![flate.clone(), flate]))].into_iter().collect(); - let file = object::File::parse(b""); + let file = object_file(vec![]); let mut nested = miniz_oxide::deflate::compress_to_vec_zlib(&vec![0u8; 16 << 20], 6); nested.resize(MAX - 1, 0); let raw = miniz_oxide::deflate::compress_to_vec_zlib(&nested, 6); @@ -206,7 +209,7 @@ fn nested_bomb_in_the_second_flate_filter_is_refused() { fn bounded_filter(dict: &str, raw: &[u8], max: usize) -> Option> { let object::Obj::Dict(d) = object::Lexer::new(dict.as_bytes(), 0).next().unwrap() else { panic!("not a dictionary") }; - object::decode_stream_within(&object::File::parse(b""), &d, raw, max) + object::decode_stream_within(&object_file(vec![]), &d, raw, max) } #[test] @@ -660,9 +663,31 @@ fn tiling_patterns_coloured_and_uncoloured() { assert_eq!(at(115, 15)[3], 0.0); } +/// Build isolated interpreter objects without presenting a function or colour space as a +/// document catalog. Complete-document parsing is covered by the page and reader fixtures. +fn object_file(objs: Objs) -> object::File { + let objects = objs + .into_iter() + .map(|(number, body, stream)| { + let mut lexer = object::Lexer::new(body.as_bytes(), 0); + let value = lexer.next().unwrap(); + let value = lexer.maybe_ref(value); + let value = match stream { + Some(bytes) => { + let object::Obj::Dict(dict) = value else { panic!("stream fixture must have a dictionary") }; + object::Obj::Stream(dict, bytes) + } + None => value, + }; + ((number, 0), value) + }) + .collect(); + object::File { objects, trailers: vec![], repairs: vec![] } +} + /// Object 1 of a file holding just `objs`, read as a function. fn read_function(objs: Objs) -> color::Func { - color::Func::read(&object::File::parse(&pdf(&objs, 1)), &object::Obj::Ref(1, 0)) + color::Func::read(&object_file(objs), &object::Obj::Ref(1, 0)) } /// A sampled (type 0) function, read from a file holding just its stream. @@ -672,7 +697,7 @@ fn read_sampled_function(dict: &str, data: Vec) -> color::Func { /// Object 1 of a file holding just `objs`, read as a colour space. fn read_color_space(objs: Objs) -> color::Cs { - color::color_space(&object::File::parse(&pdf(&objs, 1)), &object::Obj::Ref(1, 0), None) + color::color_space(&object_file(objs), &object::Obj::Ref(1, 0), None) } #[test] diff --git a/crates/pdf/src/tests_import.rs b/crates/pdf/src/tests_import.rs index add447e3a..e53727efd 100644 --- a/crates/pdf/src/tests_import.rs +++ b/crates/pdf/src/tests_import.rs @@ -377,6 +377,12 @@ fn postscript_radix_numbers_outside_2_to_36_are_not_numbers() { } } +#[test] +fn extreme_page_indices_have_a_printable_error() { + let text = Error::PageOutOfRange(usize::MAX).to_string(); + assert!(text.starts_with("the PDF has no page ")); +} + #[test] fn postscript_idiv_and_mod_of_i64_min_by_minus_one_give_zero() { // `i64::MIN / -1` and `i64::MIN % -1` overflow, and `/` and `%` panic on them in debug and diff --git a/crates/pdf/src/tests_reader.rs b/crates/pdf/src/tests_reader.rs new file mode 100644 index 000000000..999a9900f --- /dev/null +++ b/crates/pdf/src/tests_reader.rs @@ -0,0 +1,76 @@ +//! The shared object adapter preserves scalar references and bounds both tree and graph input. + +use crate::{Error, parse}; + +fn document(extra: &str, tail: &[(u32, u16, String)]) -> Vec { + let content = "/GS gs 1 0 0 rg 0 0 20 10 re f"; + let mut objects = vec![ + (1, 0, format!("<< /Type /Catalog /Pages 2 0 R {extra} >>")), + (2, 0, "<< /Type /Pages /Kids [3 0 R] /Count 1 >>".into()), + (3, 0, "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 20 10] /Resources << /ExtGState << /GS << /ca 5 7 R >> >> >> /Contents 4 0 R >>".into()), + (4, 0, format!("<< /Length {} >>\nstream\n{content}\nendstream", content.len())), + (5, 7, "0.25".into()), + ]; + objects.extend_from_slice(tail); + let mut data = b"%PDF-1.7\n".to_vec(); + let mut offsets = std::collections::BTreeMap::new(); + for (number, generation, value) in objects { + offsets.insert(number, (data.len(), generation)); + data.extend_from_slice(format!("{number} {generation} obj\n{value}\nendobj\n").as_bytes()); + } + let xref = data.len(); + let count = offsets.keys().last().copied().unwrap() + 1; + data.extend_from_slice(format!("xref\n0 {count}\n0000000000 65535 f \n").as_bytes()); + for number in 1..count { + let entry = offsets.get(&number).map_or_else(|| "0000000000 65535 f \n".into(), |(offset, generation)| format!("{offset:010} {generation:05} n \n")); + data.extend_from_slice(entry.as_bytes()); + } + data.extend_from_slice(format!("trailer\n<< /Size {count} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n").as_bytes()); + data +} + +#[test] +fn a_nonzero_generation_scalar_keeps_the_drawing_opacity() { + let doc = parse(&document("", &[])).unwrap(); + assert!(doc.skipped.is_empty(), "{:?}", doc.skipped); + let pixel = effectcraft_svg::rasterize(&doc, 20, 10, 1.0).get(10, 5); + assert!((pixel[0] - 0.25).abs() < 0.001 && (pixel[3] - 0.25).abs() < 0.001, "{pixel:?}"); +} + +#[test] +fn a_long_cyclic_reference_graph_does_not_expand_recursively() { + let objects: Vec<_> = (10..1010).map(|id| (id, 0, format!("<< /Next {} 0 R >>", if id == 1009 { 10 } else { id + 1 }))).collect(); + let plain = parse(&document("", &[])).unwrap(); + let cyclic = parse(&document("/Extra 10 0 R", &objects)).unwrap(); + assert_eq!(plain, cyclic); +} + +#[test] +fn excessive_inline_nesting_returns_an_error_instead_of_partial_art() { + let extra = format!("/Extra {}0{}", "[".repeat(80), "]".repeat(80)); + let bytes = document(&extra, &[]); + // The shared parser rejects the catalog before the adapter's depth limit runs. + // Its Invalid result follows the existing NoPages mapping, with no partial document. + assert_eq!(crate::page_count(&bytes), 0); + assert_eq!(parse(&bytes), Err(Error::NoPages)); +} + +fn object_stream(entries: &[(u32, &str)]) -> String { + let (mut header, mut body) = (String::new(), String::new()); + for (number, value) in entries { + header.push_str(&format!("{number} {} ", body.len())); + body.push_str(value); + body.push(' '); + } + let first = header.len(); + let content = header + &body; + format!("<< /Type /ObjStm /N {} /First {first} /Length {} >>\nstream\n{content}\nendstream", entries.len(), content.len()) +} + +#[test] +fn duplicate_unreferenced_compressed_objects_do_not_block_repair() { + let objects = vec![(20, 0, object_stream(&[(10, "<< /Value 1 >>"), (30, "null")])), (21, 0, object_stream(&[(30, "42")]))]; + let doc = parse(&document("/Missing 10 0 R", &objects)).unwrap(); + assert!(doc.skipped.iter().any(|warning| warning.contains("recovered PDF object from an object stream: 10 0"))); + assert!(matches!(parse(&document("/Missing 30 0 R", &objects)), Err(Error::Read(message)) if message.contains("ambiguous recovered object 30"))); +} diff --git a/docs/architecture.md b/docs/architecture.md index e5564c22a..72daec328 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -721,3 +721,5 @@ JSON parsing is now the bulk of open), and rendering 5,000-layer comps faster. | Date | Change | Summary | |---|---|---| | 2026-10-10 | minor | Status line added; crate table checked against `crates/` (every crate listed); effect count and VP9 encoder description corrected | + +PDF document structure uses the `craft-pdf` read feature through an app-owned bounded object adapter. EffectCraft retains its drawing tree, fonts, content/image filters and EPS interpreter; ordinary object/xref limits remain 256 MiB on 64-bit and 64 MiB on 32-bit/WASM, including repair. diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 4d541feb4..e79ac63a9 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -1,4 +1,4 @@ -//! Workspace automation: `cargo xtask `. +//! Workspace automation: `cargo xtask `. //! //! - `layers`: enforces the dependency layering of `docs/architecture.md` §1 (downward-only edges, //! listed same-layer edges, no UI/OS crates below L5). @@ -9,12 +9,13 @@ //! - `assets`: every asset file (image, icon, font, LUT, audio, video…) has a complete //! `.attribution` sidecar and an entry in `ATTRIBUTION.md` (AGENTS.md §1). //! - `ico …`: pack PNGs into a Windows `.ico` (used by `packaging/icons.sh`). -//! - `ci`: fmt check, clippy -D warnings, tests, layers, assets, wasm. +//! - `ci`: fmt check, clippy -D warnings, tests, PDF dependency policy, layers, assets, wasm. //! - `bench [--bin CLI] [--quick] [--compare OLD.jsonl] …`: render benchmark of generated scenarios //! (memory, CPU, wall time) into `target/bench/results.jsonl` (`bench.rs`, docs/testing.md). mod bench; mod ico; +mod pdf_deps; mod version; use std::process::{Command, ExitCode}; @@ -410,6 +411,7 @@ fn ci() -> Result<(), String> { run(Command::new(cargo).args(["fmt", "--check"]))?; run(Command::new(cargo).args(["clippy", "--workspace", "--all-targets", "--release", "--", "-D", "warnings"]))?; run(Command::new(cargo).args(["test", "--workspace", "--release"]))?; + pdf_deps::run(&root())?; layers()?; assets()?; wasm() @@ -421,6 +423,7 @@ fn main() -> ExitCode { "layers" => layers(), "wasm" => wasm(), "assets" => assets(), + "pdf-deps" => pdf_deps::run(&root()), "ci" => ci(), "web" => web(&std::env::args().skip(2).collect::>()), "bench" => bench::run(&root(), &std::env::args().skip(2).collect::>()), @@ -432,7 +435,7 @@ fn main() -> ExitCode { let rest: Vec = std::env::args().skip(2).collect(); version::run(&root(), &rest.iter().map(String::as_str).collect::>()) } - _ => Err("usage: cargo xtask ".into()), + _ => Err("usage: cargo xtask ".into()), }; match r { Ok(()) => ExitCode::SUCCESS, diff --git a/xtask/src/pdf_deps.rs b/xtask/src/pdf_deps.rs new file mode 100644 index 000000000..d7f880796 --- /dev/null +++ b/xtask/src/pdf_deps.rs @@ -0,0 +1,117 @@ +//! Pin the shared parser and reject interpreter features that would embed backend assets. + +use std::path::Path; +use std::process::Command; + +use serde_json::Value; + +const REV: &str = "ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d"; +const PACKAGES: &[(&str, &str)] = &[("craft-pdf", "0.1.0"), ("hayro-syntax", "0.7.2")]; + +pub fn run(root: &Path) -> Result<(), String> { + let output = Command::new(std::env::var_os("CARGO").unwrap_or_else(|| "cargo".into())) + .current_dir(root) + .args(["metadata", "--locked", "--format-version", "1"]) + .output() + .map_err(|e| format!("cargo metadata: {e}"))?; + if !output.status.success() { + return Err(format!("cargo metadata: {}", String::from_utf8_lossy(&output.stderr))); + } + let metadata: Value = serde_json::from_slice(&output.stdout).map_err(|e| format!("cargo metadata: {e}"))?; + validate(&metadata)?; + println!("PDF backend: pinned parser only; no Hayro interpreter, rasterizer or bundled CMaps"); + Ok(()) +} + +fn validate(metadata: &Value) -> Result<(), String> { + let packages = metadata.get("packages").and_then(Value::as_array).ok_or("cargo metadata has no packages")?; + let nodes = metadata.pointer("/resolve/nodes").and_then(Value::as_array).ok_or("cargo metadata has no resolved nodes")?; + let source = format!("git+https://github.com/storytold/craft-libs?rev={REV}#{REV}"); + for (name, version) in PACKAGES { + let matches: Vec<_> = packages.iter().filter(|p| p["name"] == *name).collect(); + if matches.len() != 1 || matches[0]["version"] != *version || matches[0]["source"] != source { + return Err(format!("{name}: expected exactly {version} from {source}")); + } + } + for (name, allowed) in [("craft-pdf", "read"), ("hayro-syntax", "std")] { + let package = packages.iter().find(|p| p["name"] == name).ok_or_else(|| format!("missing {name}"))?; + let node = nodes.iter().find(|n| n["id"] == package["id"]).ok_or_else(|| format!("{name}: missing resolved node"))?; + let features = node["features"].as_array().ok_or_else(|| format!("{name}: missing resolved features"))?; + if features.len() != 1 || features.first().and_then(Value::as_str) != Some(allowed) { + return Err(format!("{name}: expected only the {allowed} feature")); + } + } + for package in packages { + let name = package["name"].as_str().unwrap_or(""); + let active = nodes.iter().any(|node| node["id"] == package["id"]); + if active && matches!(name, "hayro" | "hayro-interpret" | "hayro-cmap" | "krilla") { + return Err(format!("{name}: EffectCraft must use craft-pdf with only the read feature")); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn metadata() -> Value { + let packages: Vec<_> = PACKAGES.iter().map(|(name, version)| { + json!({"id": name, "name": name, "version": version, "source": format!("git+https://github.com/storytold/craft-libs?rev={REV}#{REV}")}) + }).collect(); + json!({"packages": packages, "resolve": {"nodes": [ + {"id": "hayro-syntax", "features": ["std"]}, {"id": "craft-pdf", "features": ["read"]} + ]}}) + } + + #[test] + fn accepts_the_pinned_parser_only_graph() { + assert!(validate(&metadata()).is_ok()); + } + + #[test] + fn rejects_unpinned_duplicate_and_missing_parsers() { + let mut m = metadata(); + m["packages"][0]["source"] = json!("registry"); + assert!(validate(&m).is_err()); + let mut m = metadata(); + let duplicate = m["packages"][0].clone(); + m["packages"].as_array_mut().unwrap().push(duplicate); + assert!(validate(&m).is_err()); + m["packages"].as_array_mut().unwrap().retain(|p| p["name"] != "hayro-syntax"); + assert!(validate(&m).is_err()); + } + + #[test] + fn rejects_shared_reader_source_drift() { + let mut m = metadata(); + m["packages"][0]["source"] = Value::Null; + assert!(validate(&m).is_err()); + } + + #[test] + fn rejects_extra_features_and_missing_active_nodes() { + for (index, feature) in [(0, "images"), (1, "write"), (1, "raster")] { + let mut m = metadata(); + m["resolve"]["nodes"][index]["features"].as_array_mut().unwrap().push(json!(feature)); + assert!(validate(&m).is_err()); + } + let mut m = metadata(); + m["resolve"]["nodes"].as_array_mut().unwrap().pop(); + assert!(validate(&m).is_err()); + } + + #[test] + fn rejects_backend_asset_features_and_missing_resolution() { + for name in ["hayro", "hayro-interpret", "hayro-cmap", "krilla"] { + let mut m = metadata(); + m["packages"].as_array_mut().unwrap().push(json!({"id": name, "name": name})); + m["resolve"]["nodes"].as_array_mut().unwrap().push(json!({"id": name})); + assert!(validate(&m).is_err()); + } + let mut m = metadata(); + m["resolve"] = Value::Null; + assert!(validate(&m).is_err()); + } +} From 6c45fec27dca21cf9f5fdb729999c3c2051af428 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 15 Oct 2026 21:17:07 -0400 Subject: [PATCH 15/16] ci: add nix check workflow --- .github/workflows/nix.yml | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 .github/workflows/nix.yml diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml new file mode 100644 index 000000000..18392587a --- /dev/null +++ b/.github/workflows/nix.yml @@ -0,0 +1,33 @@ +# Evaluates the Nix flake (no build, about a minute): `nix build` / `nix develop` stop at evaluation +# when Cargo.lock gains something nix/package.nix can't vendor, and nothing else would notice. +name: Nix + +on: + push: + branches: [main] + paths: ["flake.nix", "flake.lock", "nix/**", "Cargo.lock", ".github/workflows/nix.yml"] + pull_request: + paths: ["flake.nix", "flake.lock", "nix/**", "Cargo.lock", ".github/workflows/nix.yml"] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: nix-${{ github.ref }} + cancel-in-progress: true + +jobs: + eval: + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: cachix/install-nix-action@13d8dd58da0234aa297dedd986986ccb8e7f3e24 # v31.11.1 + - name: nix flake check --no-build + run: | + if [ -f flake.nix ]; then + nix flake check --no-build --all-systems + else + echo "No flake.nix present in this repository; skipping check." + fi From 0e2f43b824c1e4d942ead290b7832e4d72514ba6 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Fri, 16 Oct 2026 16:42:34 +0700 Subject: [PATCH 16/16] perf(project): keep the Size cell as After Effects shows it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the byte-count tooltip, the "—" marker and their strings and UI test, and the bench wrapper script: they are outside this PR's scope and After Effects leaves the Size cell blank when the size is unknown. The size sort and its tests are unchanged. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/i18n/ui.rs | 14 ---- crates/ui-egui/src/panels/project.rs | 16 +--- crates/ui-egui/tests/ui_project_rows.rs | 102 ------------------------ scripts/bench-project-size-sort.sh | 4 - 4 files changed, 2 insertions(+), 134 deletions(-) delete mode 100755 scripts/bench-project-size-sort.sh diff --git a/crates/ui-egui/src/i18n/ui.rs b/crates/ui-egui/src/i18n/ui.rs index d61eff44d..3a4dd4aff 100644 --- a/crates/ui-egui/src/i18n/ui.rs +++ b/crates/ui-egui/src/i18n/ui.rs @@ -252,8 +252,6 @@ const JAPANESE: &[(&str, &str)] = &[ ("Focus Distance", "フォーカス距離"), ("Follow Feature Rotation and Scale", "特徴の回転とスケールに追従"), ("Footage: {}", "フッテージ：{}"), - ("File size: {} bytes", "ファイルサイズ: {} バイト"), - ("File size unavailable", "ファイルサイズを取得できません"), ("Format: {}", "形式：{}"), ("Frame Aspect Ratio: {}", "フレームアスペクト比：{}"), ("Frame Blending: {}", "フレームブレンド：{}"), @@ -798,8 +796,6 @@ const SIMPLIFIED_CHINESE: &[(&str, &str)] = &[ ("Focus Distance", "焦距"), ("Follow Feature Rotation and Scale", "跟随特征的旋转和缩放"), ("Footage: {}", "素材：{}"), - ("File size: {} bytes", "文件大小：{} 字节"), - ("File size unavailable", "无法获取文件大小"), ("Format: {}", "格式：{}"), ("Frame Aspect Ratio: {}", "帧长宽比：{}"), ("Frame Blending: {}", "帧混合：{}"), @@ -1532,8 +1528,6 @@ const TRADITIONAL_CHINESE: &[(&str, &str)] = &[ ("Focus Distance", "焦距"), ("Follow Feature Rotation and Scale", "跟隨特徵的旋轉和縮放"), ("Footage: {}", "素材：{}"), - ("File size: {} bytes", "檔案大小：{} 位元組"), - ("File size unavailable", "無法取得檔案大小"), ("Format: {}", "格式：{}"), ("Frame Aspect Ratio: {}", "影格長寬比：{}"), ("Frame Blending: {}", "影格混合：{}"), @@ -2269,8 +2263,6 @@ const UKRAINIAN: &[(&str, &str)] = &[ ("Focus Distance", "Відстань фокусування"), ("Follow Feature Rotation and Scale", "Враховувати обертання та масштаб ознаки"), ("Footage: {}", "Матеріал: {}"), - ("File size: {} bytes", "Розмір файлу: {} байт"), - ("File size unavailable", "Розмір файлу недоступний"), ("Format: {}", "Формат: {}"), ("Frame Aspect Ratio: {}", "Пропорції кадру: {}"), ("Frame Blending: {}", "Змішування кадрів: {}"), @@ -2823,8 +2815,6 @@ const PORTUGUESE: &[(&str, &str)] = &[ ("Focus Distance", "Distância de foco"), ("Follow Feature Rotation and Scale", "Acompanhar rotação e escala do recurso"), ("Footage: {}", "Mídia: {}"), - ("File size: {} bytes", "Tamanho do arquivo: {} bytes"), - ("File size unavailable", "Tamanho do arquivo indisponível"), ("Format: {}", "Formato: {}"), ("Frame Aspect Ratio: {}", "Proporção do quadro: {}"), ("Frame Blending: {}", "Mesclagem de quadros: {}"), @@ -3275,8 +3265,6 @@ const CZECH: &[(&str, &str)] = &[ ("Focus Distance", "Vzdálenost zaostření"), ("Follow Feature Rotation and Scale", "Sledovat otočení a měřítko prvku"), ("Footage: {}", "Záznam: {}"), - ("File size: {} bytes", "Velikost souboru: {} bajtů"), - ("File size unavailable", "Velikost souboru není k dispozici"), ("Format: {}", "Formát: {}"), ("Frame Aspect Ratio: {}", "Poměr stran snímku: {}"), ("Frame Blending: {}", "Prolnutí snímků: {}"), @@ -3832,8 +3820,6 @@ const SPANISH: &[(&str, &str)] = &[ ("Focus Distance", "Distancia de enfoque"), ("Follow Feature Rotation and Scale", "Seguir la rotación y la escala de la característica"), ("Footage: {}", "Material de archivo: {}"), - ("File size: {} bytes", "Tamaño del archivo: {} bytes"), - ("File size unavailable", "Tamaño del archivo no disponible"), ("Format: {}", "Formato: {}"), ("Frame Aspect Ratio: {}", "Relación de aspecto del fotograma: {}"), ("Frame Blending: {}", "Fusión de fotogramas: {}"), diff --git a/crates/ui-egui/src/panels/project.rs b/crates/ui-egui/src/panels/project.rs index 2cb6b8c73..76309a5d0 100644 --- a/crates/ui-egui/src/panels/project.rs +++ b/crates/ui-egui/src/panels/project.rs @@ -766,19 +766,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let cp = lp.with_clip_rect(cell); let txt = match *key { "type" => it.type_name().to_string(), - "size" => { - let size = file_size(it); - let text = size.map(fmt_size).unwrap_or_else(|| if matches!(it.kind, ItemKind::Footage(_)) { "—".into() } else { String::new() }); - if matches!(it.kind, ItemKind::Footage(_)) { - let detail = match size { - Some(bytes) => crate::i18n::tr_args("File size: {} bytes", &[&bytes]), - None => crate::i18n::tr("File size unavailable").to_string(), - }; - let resp = ui.interact(cell, egui::Id::new(("psize", id.0)), Sense::hover()).on_hover_text(detail.as_str()); - app.auto.add_response(&format!("project.item.{}.size", id.0), &resp, &detail); - } - text - } + "size" => file_size(it).map(fmt_size).unwrap_or_default(), "duration" => it.duration().map(|d| fmt_dur(d.seconds(), it.frame_rate().map(|r| r.as_f64()).unwrap_or(30.0))).unwrap_or_default(), "fps" => it.frame_rate().map(|f| format!("{:.2}", f.as_f64())).unwrap_or_default(), "path" => file_path(it).to_string(), @@ -1224,7 +1212,7 @@ mod tests { } #[test] - #[ignore = "filesystem benchmark; run scripts/bench-project-size-sort.sh"] + #[ignore = "filesystem benchmark; run with --ignored --nocapture"] fn size_sort_benchmark() { let files = SizeFiles::new(); let mut project = Project::default(); diff --git a/crates/ui-egui/tests/ui_project_rows.rs b/crates/ui-egui/tests/ui_project_rows.rs index 949310f5c..27ce5e693 100644 --- a/crates/ui-egui/tests/ui_project_rows.rs +++ b/crates/ui-egui/tests/ui_project_rows.rs @@ -84,108 +84,6 @@ fn command_clicking_a_folder_twirl_opens_the_folders_inside_it() { assert!(h.state().ui.project_open_folders.is_empty(), "and close again together"); } -#[test] -fn size_column_sorts_and_explains_sizes_without_swallowing_row_clicks() { - use effectcraft_ui_egui::theme::ThemeKind; - for (name, size, theme) in [ - ("wide-dark", egui::vec2(1600.0, 1000.0), ThemeKind::Dark), - ("compact-dark", egui::vec2(1024.0, 768.0), ThemeKind::Dark), - ("compact-light", egui::vec2(1024.0, 768.0), ThemeKind::Light), - ] { - check_size_column(name, size, theme); - } -} - -fn check_size_column(name: &str, size: egui::Vec2, theme: effectcraft_ui_egui::theme::ThemeKind) { - struct Files(std::path::PathBuf); - impl Drop for Files { - fn drop(&mut self) { - let _ = std::fs::remove_dir_all(&self.0); - } - } - let files = Files(std::env::temp_dir().join(format!( - "effectcraft-size-ui-{}-{}", - std::process::id(), - std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos() - ))); - std::fs::create_dir(&files.0).unwrap(); - let mut s = Session::default(); - s.prefs.general.language = "en".into(); - let project = std::sync::Arc::make_mut(&mut s.project); - let folder = project.add_item("Folder", Label::Aqua, None, ItemKind::Folder); - let mut ids = Vec::new(); - for (name, bytes) in [("Small", Some(10)), ("Large", Some(20)), ("Unavailable", None), ("Empty", Some(0))] { - let path = files.0.join(name); - if let Some(bytes) = bytes { - std::fs::File::create(&path).unwrap().set_len(bytes).unwrap(); - } - ids.push(project.add_item(name, Label::Aqua, None, ItemKind::Footage(Footage { path: path.to_string_lossy().into_owned(), ..Default::default() }))); - } - let [small, large, unavailable, empty] = ids.as_slice() else { panic!("fixture items") }; - let (small, large, unavailable, empty) = (*small, *large, *unavailable, *empty); - let mut app = EffectcraftApp::new(s); - app.ui.start_screen = false; - app.ui.project_columns = vec!["size".into()]; - let mut h = Harness::builder().with_size(size).build_eframe(|_| app); - let ctx = h.ctx.clone(); - h.state_mut().set_theme(&ctx, theme); - h.run_steps(3); - let size_id = |id: ItemId| format!("project.item.{}.size", id.0); - let assert_order = |h: &Harness<'_, EffectcraftApp>, expected: &[ItemId]| { - let mut visible = expected.to_vec(); - visible.sort_by(|a, b| center(h, &format!("project.item.{}", a.0)).y.total_cmp(¢er(h, &format!("project.item.{}", b.0)).y)); - assert_eq!(visible, expected); - }; - let undo = h.state().session.history.undo.len(); - click(&mut h, "project.sort.size"); - assert_order(&h, &[folder, unavailable, empty, small, large]); - click(&mut h, "project.sort.size"); - assert_order(&h, &[large, small, empty, unavailable, folder]); - assert_eq!(h.state().auto.find(&size_id(empty)).unwrap().label, "File size: 0 bytes"); - assert!(h.state().auto.find(&size_id(folder)).is_none()); - let at = center(&h, &size_id(small)); - h.input_mut().events.push(Event::PointerMoved(at)); - h.run_steps(5); - assert!(h.query_by_label("File size: 10 bytes").is_some(), "hover exposes exact bytes when both files display 1 KB"); - if let Ok(dir) = std::env::var("PROJECT_SIZE_SNAPSHOTS") { - h.render().unwrap().save(format!("{dir}/project-size-{name}-hover.png")).unwrap(); - } - click_at(&mut h, at); - assert_eq!(h.state().session.state.project_selection, [small]); - let at = center(&h, &size_id(unavailable)); - h.input_mut().events.push(Event::PointerMoved(at)); - h.run_steps(5); - assert!(h.query_by_label("File size unavailable").is_some()); - let has_dash = |shape: &egui::Shape| matches!(shape, egui::Shape::Text(text) if text.galley.text() == "—"); - assert!(h.output().shapes.iter().any(|shape| has_dash(&shape.shape)), "unavailable footage has a visible marker"); - if let Ok(dir) = std::env::var("PROJECT_SIZE_SNAPSHOTS") { - h.render().unwrap().save(format!("{dir}/project-size-{name}-unavailable.png")).unwrap(); - } - std::fs::OpenOptions::new().write(true).open(files.0.join("Small")).unwrap().set_len(30).unwrap(); - h.run_steps(3); - assert_order(&h, &[small, large, empty, unavailable, folder]); - assert_eq!(h.state().auto.find(&size_id(small)).unwrap().label, "File size: 30 bytes"); - assert_eq!(h.state().session.history.undo.len(), undo, "sorting and size details don't edit the project"); - let from = center(&h, &size_id(small)); - let to = center(&h, &format!("project.item.{}.name", folder.0)); - h.input_mut().events.push(Event::PointerMoved(from)); - h.step(); - button(&mut h, from, true); - h.step(); - for k in 1..=6 { - h.input_mut().events.push(Event::PointerMoved(from + (to - from) * (k as f32 / 6.0))); - h.step(); - } - button(&mut h, to, false); - h.run_steps(3); - assert_eq!(h.state().session.project.item(small).unwrap().parent, Some(folder), "dragging from Size still moves the item"); - assert_eq!(h.state().session.history.undo.len(), undo + 1); - h.state_mut().session.execute("edit.undo", json!({})).unwrap(); - h.run_steps(3); - assert_eq!(h.state().session.project.item(small).unwrap().parent, None); - assert_order(&h, &[small, large, empty, unavailable, folder]); -} - #[test] fn folder_twirl_opens_and_closes_the_folder_on_one_click() { let (mut h, folder, comp, _) = harness(); diff --git a/scripts/bench-project-size-sort.sh b/scripts/bench-project-size-sort.sh deleted file mode 100755 index b85e495d2..000000000 --- a/scripts/bench-project-size-sort.sh +++ /dev/null @@ -1,4 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail -cd "$(dirname "${BASH_SOURCE[0]}")/.." -CARGO_PROFILE_RELEASE_LTO=false cargo test -p effectcraft-ui-egui --release --lib panels::project::tests::size_sort_benchmark -- --ignored --nocapture