From be6134e8ad8d4da451256cb593285c1d06ba9c59 Mon Sep 17 00:00:00 2001 From: cdinhoo Date: Sun, 11 Oct 2026 15:49:21 +0300 Subject: [PATCH] Performance: reduce UI stalls while dragging layers --- crates/engine/src/commands/expr_tools.rs | 23 +- crates/engine/src/viewer.rs | 85 +++- crates/ui-egui/src/expression_diagnostics.rs | 479 +++++++++++++++++++ crates/ui-egui/src/frames.rs | 127 ++++- crates/ui-egui/src/lib.rs | 3 + crates/ui-egui/src/panels/expr_bar.rs | 42 +- crates/ui-egui/src/panels/expr_editor.rs | 45 +- crates/ui-egui/src/panels/viewer.rs | 2 - crates/ui-egui/tests/ui_web_gpu_storage.rs | 10 +- docs/architecture.md | 16 +- 10 files changed, 753 insertions(+), 79 deletions(-) create mode 100644 crates/ui-egui/src/expression_diagnostics.rs diff --git a/crates/engine/src/commands/expr_tools.rs b/crates/engine/src/commands/expr_tools.rs index d266b94f..fae74a5b 100644 --- a/crates/engine/src/commands/expr_tools.rs +++ b/crates/engine/src/commands/expr_tools.rs @@ -43,8 +43,18 @@ impl ExprError { pub fn errors(s: &Session, cid: ItemId, t: Tick) -> Vec { let Some(comp) = s.project.comp(cid) else { return vec![] }; let ctx = EvalCtx { footage: Some(s.footage.as_ref()), expr: s.expr.as_deref(), ..EvalCtx::new(&s.project, cid, comp, t) }; + errors_in(&ctx, s.expr_check, || true).unwrap_or_default() +} + +/// Evaluate diagnostics against an immutable snapshot. Background callers can cancel between +/// properties; cancelled scans return no partial result. Includes hidden/non-solo layers and +/// syntax errors on disabled expressions, just like the explicit `expr.errors` query. +pub fn errors_in(ctx: &EvalCtx, check: Option std::result::Result<(), String>>, mut keep_going: impl FnMut() -> bool) -> Option> { let mut out = vec![]; - for (i, l) in comp.layers.iter().enumerate() { + for (i, l) in ctx.comp.layers.iter().enumerate() { + if !keep_going() { + return None; + } let mut props: Vec<(&Property, &Expression)> = vec![]; l.props.walk("", &mut |_, p| { if let Some(e) = p.expr.as_ref().filter(|e| !e.text.trim().is_empty()) { @@ -52,20 +62,23 @@ pub fn errors(s: &Session, cid: ItemId, t: Tick) -> Vec { } }); for (p, e) in props { + if !keep_going() { + return None; + } let msg = if e.enabled { match &ctx.expr { - Some(h) => h.eval(&ctx, l, p, &p.value_at(l.layer_time(t))).err(), + Some(h) => h.eval(ctx, l, p, &p.value_at(l.layer_time(ctx.time))).err(), None => None, } } else { - s.expr_check.and_then(|check| check(&e.text).err()) + check.and_then(|check| check(&e.text).err()) }; if let Some(message) = msg { - out.push(error_of(cid, i, l, p, message, !e.enabled)); + out.push(error_of(ctx.comp_id, i, l, p, message, !e.enabled)); } } } - out + keep_going().then_some(out) } fn error_of(cid: ItemId, i: usize, l: &Layer, p: &Property, message: String, disabled: bool) -> ExprError { diff --git a/crates/engine/src/viewer.rs b/crates/engine/src/viewer.rs index b9758668..5abb942a 100644 --- a/crates/engine/src/viewer.rs +++ b/crates/engine/src/viewer.rs @@ -3,13 +3,15 @@ //! //! Nothing here touches the project; edits go through commands. -use std::sync::Arc; +use std::collections::HashMap; +use std::sync::{Arc, Mutex, PoisonError}; use effectcraft_color::{ColorSpace, space}; use effectcraft_geom::{Mat3, vec2}; -use effectcraft_project::{Layer, LayerId}; +use effectcraft_keyframe::Value; +use effectcraft_project::{Layer, LayerId, Property}; use effectcraft_raster::Image; -use effectcraft_render::EvalCtx; +use effectcraft_render::{EvalCtx, ExprHost}; use effectcraft_time::Tick; use serde::{Deserialize, Serialize}; @@ -274,10 +276,45 @@ pub struct SnapOptions { pub grid_spacing: f64, } +type SnapValues = HashMap<(u64, u64, u64, i64), Result>; + +/// Bounds, vertices and transforms can ask for the same expression repeatedly. Share its +/// value only within this immutable snap query; a new pointer update starts a fresh cache. +struct SnapExpressions<'a> { + inner: &'a dyn ExprHost, + values: Mutex, +} + +impl ExprHost for SnapExpressions<'_> { + fn eval(&self, ctx: &EvalCtx, layer: &Layer, prop: &Property, value: &Value) -> Result { + let key = (ctx.comp_id.0, layer.id.0, prop.uid, ctx.time.0); + if let Some(value) = self.values.lock().unwrap_or_else(PoisonError::into_inner).get(&key).cloned() { + return value; + } + // Never hold the lock while evaluating: expressions may query other properties. + let result = self.inner.eval(ctx, layer, prop, value); + let mut values = self.values.lock().unwrap_or_else(PoisonError::into_inner); + if values.len() < 8192 { + values.insert(key, result.clone()); + } + result + } + + fn eval_text_selector(&self, ctx: &EvalCtx, layer: &Layer, prop: &Property, index: usize, total: usize, selector: [f64; 3]) -> Result<[f64; 3], String> { + self.inner.eval_text_selector(ctx, layer, prop, index, total, selector) + } +} + /// Snap targets of a comp at the context time: other visible layers' features and vertices and /// the comp's edges and centre (with `layers`, as the Snapping options allow), and guides and /// grid lines (when on). Layers in `exclude` (the dragged ones) are skipped. pub fn targets(ctx: &EvalCtx, exclude: &[LayerId], opts: SnapOptions) -> Vec { + let memo = ctx.expr.map(|inner| SnapExpressions { inner, values: Mutex::default() }); + let ctx = EvalCtx { expr: memo.as_ref().map(|h| h as &dyn ExprHost), ..*ctx }; + targets_in(&ctx, exclude, opts) +} + +fn targets_in(ctx: &EvalCtx, exclude: &[LayerId], opts: SnapOptions) -> Vec { let comp = ctx.comp; let (w, h) = (comp.width as f64, comp.height as f64); let f = opts.features; @@ -1063,3 +1100,45 @@ pub struct Snapshot { pub scale: f64, pub image: Arc, } + +#[cfg(test)] +mod snap_expression_tests { + use super::*; + use serde_json::json; + use std::sync::atomic::{AtomicUsize, Ordering}; + + struct PositionHost(AtomicUsize); + impl ExprHost for PositionHost { + fn eval(&self, ctx: &EvalCtx, _: &Layer, _: &Property, value: &Value) -> Result { + self.0.fetch_add(1, Ordering::Relaxed); + let v = value.components(); + Ok(Value::Vec3([v[0] + 10. * ctx.time.seconds(), v[1], 0.])) + } + } + + #[test] + fn snapping_reuses_expressions_within_one_query_but_refreshes_time_and_edits() { + let mut s = crate::Session::default(); + s.execute("comp.new", json!({"width":320,"height":180,"duration":5})).unwrap(); + let lid = s.execute("layer.newSolid", json!({"width":40,"height":20})).unwrap()["layer"].as_u64().unwrap(); + s.execute("prop.setExpression", json!({"layer":lid,"path":"transform/position","expression":"value"})).unwrap(); + let cid = s.active_comp_id().unwrap(); + let host = PositionHost(AtomicUsize::new(0)); + let opts = SnapOptions { layers: true, ..Default::default() }; + let query = |s: &crate::Session, time, cached| { + let ctx = EvalCtx { expr: Some(&host), ..EvalCtx::new(&s.project, cid, s.project.comp(cid).unwrap(), time) }; + if cached { targets(&ctx, &[], opts) } else { targets_in(&ctx, &[], opts) } + }; + let expected = query(&s, Tick::ZERO, false); + assert!(host.0.swap(0, Ordering::Relaxed) > 1, "fixture must visit the same expression for multiple geometry features"); + assert_eq!(query(&s, Tick::ZERO, true), expected); + assert_eq!(host.0.swap(0, Ordering::Relaxed), 1); + let moved = query(&s, Tick::from_seconds_f64(1.), true); + assert_ne!(moved, expected, "a new time must not reuse stale targets"); + assert_eq!(host.0.swap(0, Ordering::Relaxed), 1); + s.execute("prop.set", json!({"layer":lid,"path":"transform/position","value":[200,100,0]})).unwrap(); + let edited = query(&s, Tick::ZERO, true); + assert_ne!(edited, expected, "a new project revision must not reuse stale targets"); + assert_eq!(host.0.swap(0, Ordering::Relaxed), 1); + } +} diff --git a/crates/ui-egui/src/expression_diagnostics.rs b/crates/ui-egui/src/expression_diagnostics.rs new file mode 100644 index 00000000..93d2495d --- /dev/null +++ b/crates/ui-egui/src/expression_diagnostics.rs @@ -0,0 +1,479 @@ +//! Cached viewer diagnostics. Native scans run on one persistent worker, never while painting. +//! Each viewer keeps only its newest request; obsolete scans stop between properties. On the +//! single-threaded web build, one shared scan runs after interaction settles (no thread fallback). + +use std::collections::VecDeque; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +#[cfg(not(target_arch = "wasm32"))] +use std::sync::{Mutex, PoisonError}; +use std::time::Duration; + +use effectcraft_engine::Session; +use effectcraft_engine::commands::expr_tools::{self, ExprError}; +use effectcraft_engine::project::{ItemId, Project}; +use effectcraft_engine::render::{EvalCtx, ExprHost, FootageSource}; +use effectcraft_engine::time::Tick; +use web_time::Instant; + +const QUIET: Duration = Duration::from_millis(150); +const MAX_VIEWERS: usize = 8; +type Errors = Arc<[ExprError]>; +type SyntaxCheck = fn(&str) -> Result<(), String>; + +#[derive(Clone)] +struct Snapshot { + project: Arc, + revision: u64, + comp: ItemId, + time: Tick, + expr: Option>, + footage: Arc, + check: Option, +} + +impl Snapshot { + fn new(s: &Session, comp: ItemId) -> Self { + Self { + project: s.project.clone(), + revision: s.revision, + comp, + time: s.time_of(comp), + expr: s.expr.clone(), + footage: s.footage.clone(), + check: s.expr_check, + } + } + + fn matches(&self, s: &Session, comp: ItemId) -> bool { + self.revision == s.revision && self.comp == comp && self.time == s.time_of(comp) && Arc::ptr_eq(&self.project, &s.project) + } +} + +struct Request { + snapshot: Snapshot, + cancelled: Arc, + #[cfg(not(target_arch = "wasm32"))] + repaint: egui::Context, +} + +struct Completed { + #[cfg(not(target_arch = "wasm32"))] + comp: ItemId, + #[cfg(not(target_arch = "wasm32"))] + cancelled: Arc, + result: Result>, String>, +} + +impl Request { + fn run(&self) -> Completed { + let s = &self.snapshot; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let comp = s.project.comp(s.comp)?; + let ctx = EvalCtx { expr: s.expr.as_deref(), footage: Some(s.footage.as_ref()), ..EvalCtx::new(&s.project, s.comp, comp, s.time) }; + expr_tools::errors_in(&ctx, s.check, || !self.cancelled.load(Ordering::Relaxed)) + })) + .map_err(|_| "Expression diagnostics stopped unexpectedly; edit an expression to retry.".to_string()); + Completed { + #[cfg(not(target_arch = "wasm32"))] + comp: s.comp, + #[cfg(not(target_arch = "wasm32"))] + cancelled: self.cancelled.clone(), + result, + } + } +} + +struct Entry { + snapshot: Snapshot, + changed: Instant, + cancelled: Arc, + submitted: bool, + errors: Errors, +} + +impl Drop for Entry { + fn drop(&mut self) { + self.cancelled.store(true, Ordering::Relaxed); + } +} + +#[derive(Default)] +pub(crate) struct Diagnostics { + entries: VecDeque, + #[cfg(not(target_arch = "wasm32"))] + worker: Option, + failure: Option, +} + +impl Diagnostics { + pub fn clear(&mut self) { + self.entries.clear(); + } + + pub fn take_failure(&mut self) -> Option { + self.failure.take() + } + + /// Paint from completed results only. Changes discard stale results immediately, including + /// errors whose layer/property no longer exists. Pointer interaction and playback defer scans. + pub fn errors(&mut self, s: &Session, ctx: &egui::Context, comp: ItemId, interacting: bool) -> Errors { + self.update(s, ctx, comp, interacting, Instant::now()) + } + + fn update(&mut self, s: &Session, ctx: &egui::Context, comp: ItemId, interacting: bool, now: Instant) -> Errors { + if !s.prefs.scripting.error_banner || s.expr.is_none() { + self.clear(); + return Arc::from([]); + } + let mut entry = self.entries.iter().position(|e| e.snapshot.comp == comp).and_then(|i| self.entries.remove(i)); + if entry.as_ref().is_none_or(|e| !e.snapshot.matches(s, comp)) { + entry = Some(Entry { + snapshot: Snapshot::new(s, comp), + changed: now, + cancelled: Arc::new(AtomicBool::new(false)), + submitted: false, + errors: Arc::from([]), + }); + } + let Some(mut entry) = entry else { return Arc::from([]) }; + if interacting { + entry.changed = now; + } + #[cfg(not(target_arch = "wasm32"))] + if let Some(worker) = &self.worker { + let done: Vec<_> = worker.shared.completed.lock().unwrap_or_else(PoisonError::into_inner).drain(..).collect(); + for done in done { + let target = if done.comp == comp { Some(&mut entry) } else { self.entries.iter_mut().find(|e| e.snapshot.comp == done.comp) }; + if let Some(target) = target + && Arc::ptr_eq(&target.cancelled, &done.cancelled) + && !done.cancelled.load(Ordering::Relaxed) + { + match done.result { + Ok(Some(errors)) => target.errors = errors.into(), + Ok(None) => {} + Err(e) => self.failure = Some(e), + } + } + } + } + if !entry.submitted && !interacting { + let quiet = now.saturating_duration_since(entry.changed); + if quiet < QUIET { + ctx.request_repaint_after(QUIET - quiet); + } else { + entry.submitted = true; + let request = Request { + snapshot: entry.snapshot.clone(), + cancelled: entry.cancelled.clone(), + #[cfg(not(target_arch = "wasm32"))] + repaint: ctx.clone(), + }; + #[cfg(not(target_arch = "wasm32"))] + { + if self.worker.is_none() { + match Worker::new() { + Ok(worker) => self.worker = Some(worker), + Err(e) => self.failure = Some(format!("Could not start expression diagnostics: {e}")), + } + } + if let Some(worker) = &self.worker { + worker.submit(request); + } + } + // wasm32 cannot spawn Rust threads. Preserve diagnostics there, but eliminate + // the duplicate scan and defer it until scrubbing/dragging/playback stops. + #[cfg(target_arch = "wasm32")] + match request.run().result { + Ok(Some(errors)) => entry.errors = errors.into(), + Ok(None) => {} + Err(e) => self.failure = Some(e), + } + } + } + let result = entry.errors.clone(); + self.entries.push_back(entry); + if self.entries.len() > MAX_VIEWERS { + self.entries.pop_front(); + } + result + } +} + +#[cfg(not(target_arch = "wasm32"))] +#[derive(Default)] +struct Shared { + pending: Mutex>, + completed: Mutex>, + wake: std::sync::Condvar, + stopped: AtomicBool, +} + +#[cfg(not(target_arch = "wasm32"))] +struct Worker { + shared: Arc, +} + +#[cfg(not(target_arch = "wasm32"))] +impl Worker { + fn new() -> std::io::Result { + let shared = Arc::new(Shared::default()); + let work = shared.clone(); + std::thread::Builder::new().name("ec-expression-diagnostics".into()).spawn(move || { + loop { + let request = { + let mut pending = work.pending.lock().unwrap_or_else(PoisonError::into_inner); + while pending.is_empty() && !work.stopped.load(Ordering::Relaxed) { + pending = work.wake.wait(pending).unwrap_or_else(PoisonError::into_inner); + } + if work.stopped.load(Ordering::Relaxed) { + return; + } + pending.pop_front() + }; + let Some(request) = request else { continue }; + if request.cancelled.load(Ordering::Relaxed) { + continue; + } + let result = request.run(); + if !result.cancelled.load(Ordering::Relaxed) { + let mut completed = work.completed.lock().unwrap_or_else(PoisonError::into_inner); + completed.retain(|r| r.comp != result.comp && !r.cancelled.load(Ordering::Relaxed)); + if completed.len() >= MAX_VIEWERS { + completed.pop_front(); + } + completed.push_back(result); + } + request.repaint.request_repaint(); + } + })?; + Ok(Self { shared }) + } + + fn submit(&self, request: Request) { + let mut pending = self.shared.pending.lock().unwrap_or_else(PoisonError::into_inner); + pending.retain(|r| r.snapshot.comp != request.snapshot.comp && !r.cancelled.load(Ordering::Relaxed)); + if pending.len() >= MAX_VIEWERS { + pending.pop_front(); + } + pending.push_back(request); + self.shared.wake.notify_one(); + } +} + +#[cfg(not(target_arch = "wasm32"))] +impl Drop for Worker { + fn drop(&mut self) { + // Hold the predicate lock when waking, so shutdown cannot race with entering wait. + let mut pending = self.shared.pending.lock().unwrap_or_else(PoisonError::into_inner); + self.shared.stopped.store(true, Ordering::Relaxed); + pending.clear(); + self.shared.wake.notify_one(); + // Do not join an in-flight expression on the UI thread. Entries cancel it on drop. + } +} + +#[cfg(test)] +#[cfg(not(target_arch = "wasm32"))] +mod tests { + use super::*; + use effectcraft_engine::keyframe::Value; + use effectcraft_engine::project::{Layer, Property}; + use serde_json::json; + use std::sync::atomic::AtomicUsize; + use std::sync::mpsc; + + struct Host { + main: std::thread::ThreadId, + on_ui: AtomicUsize, + calls: AtomicUsize, + entered: mpsc::Sender<()>, + gate: (Mutex, std::sync::Condvar), + fail: bool, + } + impl ExprHost for Host { + fn eval(&self, ctx: &EvalCtx, _: &Layer, _: &Property, value: &Value) -> Result { + self.calls.fetch_add(1, Ordering::Relaxed); + if std::thread::current().id() == self.main { + self.on_ui.fetch_add(1, Ordering::Relaxed); + return Err("diagnostics evaluated on UI thread".into()); + } + let _ = self.entered.send(()); + let (lock, wake) = &self.gate; + let guard = lock.lock().unwrap(); + let (_guard, timed) = wake.wait_timeout_while(guard, Duration::from_secs(5), |open| !*open).unwrap(); + assert!(!timed.timed_out(), "test did not release the worker"); + if self.fail { Err(format!("failure at {}", ctx.time.0)) } else { Ok(value.clone()) } + } + } + impl Host { + fn release(&self) { + *self.gate.0.lock().unwrap() = true; + self.gate.1.notify_all(); + } + } + fn fixture(fail: bool, blocked: bool) -> (Session, Arc, mpsc::Receiver<()>) { + let (tx, rx) = mpsc::channel(); + let host = Arc::new(Host { + main: std::thread::current().id(), + on_ui: AtomicUsize::new(0), + calls: AtomicUsize::new(0), + entered: tx, + gate: (Mutex::new(!blocked), std::sync::Condvar::new()), + fail, + }); + let mut s = Session { expr: Some(host.clone()), ..Session::default() }; + s.execute("comp.new", json!({"width":320,"height":180,"duration":5})).unwrap(); + let hidden = s.execute("layer.newSolid", json!({"name":"Hidden expression"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("prop.setExpression", json!({"layer":hidden,"path":"transform/opacity","expression":"value"})).unwrap(); + let visible = s.execute("layer.newSolid", json!({"name":"Solo plate"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("layer.setSwitch", json!({"layers":[visible],"switch":"solo","value":true})).unwrap(); + s.execute("layer.select", json!({"layers":[visible]})).unwrap(); + (s, host, rx) + } + fn wait_done(d: &Diagnostics) { + let until = Instant::now() + Duration::from_secs(3); + loop { + if d.worker.as_ref().is_some_and(|w| !w.shared.completed.lock().unwrap().is_empty()) { + return; + } + assert!(Instant::now() < until, "diagnostics did not finish"); + std::thread::sleep(Duration::from_millis(1)); + } + } + fn start(d: &mut Diagnostics, s: &Session, ctx: &egui::Context, now: Instant) { + let cid = s.active_comp_id().unwrap(); + d.update(s, ctx, cid, false, now); + d.update(s, ctx, cid, false, now + QUIET); + } + + #[test] + fn successful_hidden_expressions_run_once_off_the_ui_thread() { + let (s, host, _) = fixture(false, false); + let (mut d, ctx, now) = (Diagnostics::default(), egui::Context::default(), Instant::now()); + let cid = s.active_comp_id().unwrap(); + start(&mut d, &s, &ctx, now); + wait_done(&d); + for _ in 0..5 { + assert!(d.update(&s, &ctx, cid, false, now + QUIET).is_empty()); + } + assert_eq!(host.calls.load(Ordering::Relaxed), 1); + assert_eq!(host.on_ui.load(Ordering::Relaxed), 0); + } + + #[test] + fn newest_request_replaces_queued_times_and_discards_stale_errors() { + let (mut s, host, entered) = fixture(true, true); + let (mut d, ctx, now) = (Diagnostics::default(), egui::Context::default(), Instant::now()); + let cid = s.active_comp_id().unwrap(); + start(&mut d, &s, &ctx, now); + entered.recv_timeout(Duration::from_secs(3)).unwrap(); + for i in 1..20 { + s.set_time(Tick::from_seconds_f64(i as f64 / 30.)); + start(&mut d, &s, &ctx, now + Duration::from_secs(i)); + assert!(d.entries.back().unwrap().errors.is_empty()); + } + assert_eq!(d.worker.as_ref().unwrap().shared.pending.lock().unwrap().len(), 1); + host.release(); + wait_done(&d); + let errors = d.update(&s, &ctx, cid, false, now + Duration::from_secs(30)); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, format!("failure at {}", s.time().0)); + assert_eq!(host.calls.load(Ordering::Relaxed), 2); + assert_eq!(host.on_ui.load(Ordering::Relaxed), 0); + } + + #[test] + fn warning_preference_and_interaction_prevent_scanning_and_clear_stale_results() { + let (mut s, host, _) = fixture(true, false); + let (mut d, ctx, now) = (Diagnostics::default(), egui::Context::default(), Instant::now()); + let cid = s.active_comp_id().unwrap(); + for i in 0..10 { + d.update(&s, &ctx, cid, true, now + Duration::from_secs(i)); + } + assert!(d.worker.is_none()); + s.prefs.scripting.error_banner = false; + start(&mut d, &s, &ctx, now + Duration::from_secs(10)); + assert!(d.worker.is_none()); + assert_eq!(host.calls.load(Ordering::Relaxed), 0); + s.prefs.scripting.error_banner = true; + start(&mut d, &s, &ctx, now + Duration::from_secs(11)); + wait_done(&d); + assert_eq!(d.update(&s, &ctx, cid, false, now + Duration::from_secs(12)).len(), 1); + s.prefs.scripting.error_banner = false; + assert!(d.update(&s, &ctx, cid, false, now + Duration::from_secs(13)).is_empty()); + assert!(d.entries.is_empty()); + } + + #[test] + fn replacement_project_with_reused_ids_does_not_reuse_errors() { + let (s, _, _) = fixture(true, false); + let (mut d, ctx, now) = (Diagnostics::default(), egui::Context::default(), Instant::now()); + let cid = s.active_comp_id().unwrap(); + start(&mut d, &s, &ctx, now); + wait_done(&d); + assert_eq!(d.update(&s, &ctx, cid, false, now + QUIET).len(), 1); + let (replacement, _, _) = fixture(false, false); + assert_eq!(s.revision, replacement.revision); + assert!(d.update(&replacement, &ctx, cid, false, now + QUIET).is_empty()); + } + + #[test] + fn scans_keep_disabled_syntax_errors_and_non_solo_runtime_errors() { + let (mut s, _, _) = fixture(true, false); + let cid = s.active_comp_id().unwrap(); + let selected = s.state.selected_layers[0]; + s.expr_check = Some(|_| Err("invalid syntax".into())); + s.execute("prop.setExpression", json!({"layer":selected.0,"path":"transform/rotation","expression":"bad("})).unwrap(); + let (mut d, ctx, now) = (Diagnostics::default(), egui::Context::default(), Instant::now()); + start(&mut d, &s, &ctx, now); + wait_done(&d); + let errors = d.update(&s, &ctx, cid, false, now + QUIET); + assert_eq!(errors.len(), 2); + assert!(errors.iter().any(|e| e.disabled && e.message == "invalid syntax")); + assert!(errors.iter().any(|e| !e.disabled && e.layer_name == "Hidden expression")); + } + + #[test] + fn viewer_edits_do_not_wait_for_hidden_diagnostics_with_or_without_warnings() { + use crate::{EffectcraftApp, bench::Driver}; + let (s, host, entered) = fixture(false, true); + let mut d = Driver::new(EffectcraftApp::new(s), egui::vec2(1680., 1020.)); + for _ in 0..3 { + d.step(); + } + let now = Instant::now() + Duration::from_secs(1); + start(&mut d.app.expression_diagnostics, &d.app.session, &d.ctx, now); + entered.recv_timeout(Duration::from_secs(3)).unwrap(); + let layer = d.app.session.state.selected_layers[0]; + d.app.session.state.snapping = false; + let handle = format!("viewer.handle.{}.0", layer.0); + let before = d.app.auto.find(&handle).unwrap().rect; + let comp_rect = d.app.auto.find("viewer.comp").unwrap().rect; + let from = egui::pos2(comp_rect[0] + comp_rect[2] * 0.4, comp_rect[1] + comp_rect[3] * 0.4); + let to = from + egui::vec2(40., 20.); + d.step_with(vec![ + egui::Event::PointerMoved(from), + egui::Event::PointerButton { pos: from, button: egui::PointerButton::Primary, pressed: true, modifiers: Default::default() }, + ]); + d.step_with(vec![egui::Event::PointerMoved(to)]); + d.step_with(vec![egui::Event::PointerButton { pos: to, button: egui::PointerButton::Primary, pressed: false, modifiers: Default::default() }]); + d.step(); + let after = d.app.auto.find(&handle).unwrap().rect; + assert!(after[0] > before[0] + 30., "selection bounds did not follow the drag: {before:?} -> {after:?}"); + for warnings in [true, false] { + d.app.session.prefs.scripting.error_banner = warnings; + for i in 0..4 { + d.app.session.set_time(Tick::from_seconds_f64(i as f64 / 30.)); + d.app.session.execute("layer.setTransform", json!({"layer":layer.0,"prop":"position","value":[170.+i as f64,90.,0.]})).unwrap(); + d.app.session.execute("layer.setSwitch", json!({"layers":[layer.0],"switch":"video","value":i%2==0})).unwrap(); + d.step(); + } + } + // The diagnostic expression is still blocked on the worker, but every UI frame above + // finished. This assertion also catches reintroducing a second, synchronous banner. + assert!(!*host.gate.0.lock().unwrap()); + assert_eq!(host.on_ui.load(Ordering::Relaxed), 0); + host.release(); + } +} diff --git a/crates/ui-egui/src/frames.rs b/crates/ui-egui/src/frames.rs index ccffeb9f..47fbe531 100644 --- a/crates/ui-egui/src/frames.rs +++ b/crates/ui-egui/src/frames.rs @@ -213,6 +213,13 @@ type Identity = (u64, Option>); /// Content keys of (project revision, comp), computed once per revision. type ContentKeys = Arc>>; +#[cfg(not(target_arch = "wasm32"))] +#[derive(Default)] +struct ContentKeyJobs { + running: bool, + pending: VecDeque<(Arc, u64, u64)>, +} + fn content_key(keys: &ContentKeys, project: &Project, revision: u64, comp: u64) -> u128 { if let Some(k) = keys.lock().ok().and_then(|m| m.get(&(revision, comp)).copied()) { return k; @@ -369,6 +376,8 @@ pub struct Frames { /// Render time of the last viewer (urgent) frame in ms, for the Info panel / perf readout. pub last_ms: Arc>, content_keys: ContentKeys, + #[cfg(not(target_arch = "wasm32"))] + content_key_jobs: Arc>, gpu_retired: Arc, /// [`comp_content`] identities by (revision, comp), with the project that keeps an identity /// found by [`Frames::identity`] in another project. @@ -403,6 +412,8 @@ impl Default for Frames { ctx: None, last_ms: Arc::new(Mutex::new(0.0)), content_keys: Arc::default(), + #[cfg(not(target_arch = "wasm32"))] + content_key_jobs: Arc::default(), gpu_retired: Arc::default(), identities: Mutex::default(), bases: Mutex::default(), @@ -662,7 +673,8 @@ impl Frames { } /// Frames of the series of `base` in the disk cache but not in RAM — the blue cache bar. - /// `opts` are the viewer's render options (part of the disk key). + /// `opts` are the viewer's render options (part of the disk key). Native queries return + /// no blue frames until the content key is ready; hashing never blocks timeline painting. pub fn disk_frames(&self, src: &RenderSource, base: &FrameKey, frames: i64, opts: &RenderOpts) -> Vec { let remote = self.remote.as_ref().filter(|r| r.disk()); let contains = |k: u128| match (&src.disk, remote) { @@ -673,14 +685,64 @@ impl Frames { if src.disk.is_none() && remote.is_none() { return vec![]; } + #[cfg(not(target_arch = "wasm32"))] + let content = { + let known = self.content_keys.lock().unwrap_or_else(std::sync::PoisonError::into_inner).get(&(base.revision, base.comp)).copied(); + let Some(content) = known else { + self.request_content_key(src.project.clone(), base.revision, base.comp); + return vec![]; + }; + content + }; + #[cfg(target_arch = "wasm32")] + let content = content_key(&self.content_keys, &src.project, base.revision, base.comp); let ram: HashSet = self.cached_frames(base).into_iter().collect(); let oh = opts_hash(opts); (0..frames.clamp(0, 100_000)) .filter(|f| !ram.contains(f)) - .filter(|f| contains(frame_disk_key(&self.content_keys, &src.project, &FrameKey { frame: *f, ..*base }, oh))) + .filter(|f| contains(disk_cache::frame_key(content, *f, base.scale, base.view ^ oh))) .collect() } + #[cfg(not(target_arch = "wasm32"))] + fn request_content_key(&self, project: Arc, revision: u64, comp: u64) { + let mut jobs = self.content_key_jobs.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + // Coalesce rapid edits. One worker hashes at most one revision at a time; retained + // snapshots are bounded even with multiple timelines open. + jobs.pending.retain(|(_, _, c)| *c != comp); + if jobs.pending.len() >= 8 { + jobs.pending.pop_front(); + } + jobs.pending.push_back((project, revision, comp)); + if jobs.running { + return; + } + jobs.running = true; + drop(jobs); + let (jobs, keys, ctx) = (self.content_key_jobs.clone(), self.content_keys.clone(), self.ctx.clone()); + let work = move || loop { + let next = { + let mut jobs = jobs.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let next = jobs.pending.pop_back(); + if next.is_none() { + jobs.running = false; + } + next + }; + let Some((project, revision, comp)) = next else { return }; + if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| content_key(&keys, &project, revision, comp))).is_err() { + log::error!("could not compute a preview disk-cache key; the cache indicator will retry"); + } + if let Some(ctx) = &ctx { + ctx.request_repaint(); + } + }; + match &self.pool { + Some(pool) => pool.spawn(work), + None => rayon::spawn(work), + } + } + /// Change the RAM preview cache budget (Settings ▸ Memory & CPU), evicting the oldest /// frames when it shrinks. pub fn set_budget(&self, bytes: usize) { @@ -1170,6 +1232,67 @@ impl Queue { mod tests { use super::*; + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn disk_indicator_does_not_hash_on_ui_thread_and_coalesces_edits() { + use std::sync::mpsc; + use std::time::{Duration, Instant}; + struct Disk(u128); + impl RemoteFrames for Disk { + fn slots(&self) -> usize { + 1 + } + fn disk(&self) -> bool { + true + } + fn disk_contains(&self, key: u128) -> bool { + self.0 == key + } + fn start(&self, _: RemoteJob, done: RemoteDone) { + done(Err("test has no renderer".into())); + } + } + let mut s = effectcraft_engine::Session::default(); + s.execute("comp.new", serde_json::json!({"width":32,"height":18,"duration":1})).unwrap(); + let cid = s.active_comp_id().unwrap(); + let opts = RenderOpts::default(); + let stored = disk_cache::frame_key(disk_cache::comp_content_key(&s.project, cid), 2, 1000, opts_hash(&opts)); + let mut frames = Frames { pool: Some(rayon::ThreadPoolBuilder::new().num_threads(1).build().unwrap()), ..Default::default() }; + frames.set_remote(Some(Arc::new(Disk(stored)))); + let (entered, started) = mpsc::channel(); + let (release, wait) = mpsc::channel(); + frames.pool.as_ref().unwrap().spawn(move || { + entered.send(()).unwrap(); + wait.recv_timeout(Duration::from_secs(5)).unwrap(); + }); + started.recv_timeout(Duration::from_secs(3)).unwrap(); + let src = RenderSource { + project: s.project.clone(), + footage: s.footage.clone(), + expr: None, + layer_cache: Arc::default(), + gpu: None, + gpu_display: false, + disk: None, + }; + let mut base = key(1, 0, 0); + base.comp = cid.0; + for revision in 1..50 { + base.revision = revision; + assert!(frames.disk_frames(&src, &base, 30, &opts).is_empty()); + } + assert!(frames.content_keys.lock().unwrap().is_empty(), "painting computed a content hash while the worker was blocked"); + assert_eq!(frames.content_key_jobs.lock().unwrap().pending.len(), 1); + release.send(()).unwrap(); + let until = Instant::now() + Duration::from_secs(3); + while !frames.content_keys.lock().unwrap().contains_key(&(base.revision, base.comp)) { + assert!(Instant::now() < until); + std::thread::sleep(Duration::from_millis(1)); + } + assert_eq!(frames.disk_frames(&src, &base, 30, &opts), vec![2], "the blue indicator must return once its key is ready"); + assert_eq!(frames.content_keys.lock().unwrap().len(), 1, "superseded revisions were hashed"); + } + /// Frames larger than the texture limit are averaged down by a whole factor (#201: an /// 11000×2200 comp at Full resolution against an 8192 limit). #[test] diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index a0e05e00..80af4559 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -16,6 +16,7 @@ pub mod credits; pub mod dock; mod dock_shared; pub mod dock_ui; +mod expression_diagnostics; pub mod frames; pub mod gpu_failure; pub mod header; @@ -323,6 +324,7 @@ pub struct EffectcraftApp { /// Audio panel VU meters. pub meter: audio::Meter, /// Waveform peak summaries per footage item (see `panels::waveform`). + pub(crate) expression_diagnostics: expression_diagnostics::Diagnostics, pub(crate) waveforms: panels::waveform::Cache, /// Last reveal shortcut and when (double-press shortcuts such as LL). pub(crate) last_reveal: Option<(String, f64)>, @@ -408,6 +410,7 @@ impl EffectcraftApp { audio: None, scrub: None, meter: Default::default(), + expression_diagnostics: Default::default(), waveforms: Default::default(), last_reveal: None, applied_prefs: None, diff --git a/crates/ui-egui/src/panels/expr_bar.rs b/crates/ui-egui/src/panels/expr_bar.rs index fc985d15..4f6d1c74 100644 --- a/crates/ui-egui/src/panels/expr_bar.rs +++ b/crates/ui-egui/src/panels/expr_bar.rs @@ -4,10 +4,11 @@ //! Automation ids: `viewer.exprErrors`, `viewer.exprErrors.prev`, `viewer.exprErrors.next`. use crate::i18n::tr; -use effectcraft_engine::commands::expr_tools::{self, ExprError}; +use effectcraft_engine::commands::expr_tools::ExprError; use effectcraft_engine::project::ItemId; use egui::{Align2, Color32, Rect, Sense, pos2, vec2}; use serde_json::json; +use std::sync::Arc; use crate::EffectcraftApp; use crate::theme::Tokens; @@ -17,35 +18,32 @@ pub const HEIGHT: f32 = 22.0; #[derive(Clone, Default)] struct Cache { - key: (u64, u64, i64), - errors: Vec, + errors: Arc<[ExprError]>, /// Shown error (index). current: usize, } -fn cache_id() -> egui::Id { - egui::Id::new("expr-error-bar") +fn cache_id(cid: ItemId) -> egui::Id { + egui::Id::new(("expr-error-bar", cid.0)) } -/// The comp's failing expressions (recomputed when the project or the time changes; kept while -/// playing). -fn errors(app: &EffectcraftApp, ctx: &egui::Context, cid: ItemId) -> Cache { - let t = app.session.time_of(cid); - let key = (app.session.revision, cid.0, t.0); - let mut c: Cache = ctx.data(|d| d.get_temp(cache_id())).unwrap_or_default(); - let stale = c.key != key && !(app.playback.playing && c.key.1 == cid.0); - if stale { - c.errors = expr_tools::errors(&app.session, cid, t); - c.key = key; - c.current = c.current.min(c.errors.len().saturating_sub(1)); - ctx.data_mut(|d| d.insert_temp(cache_id(), c.clone())); +/// Both layout and painting read the same completed diagnostics; neither evaluates expressions. +fn errors(app: &mut EffectcraftApp, ctx: &egui::Context, cid: ItemId) -> Cache { + let mut c: Cache = ctx.data(|d| d.get_temp(cache_id(cid))).unwrap_or_default(); + let interacting = app.playback.playing || ctx.input(|i| i.pointer.any_down()) || app.ui.viewer.interacting || app.ui.viewer.property_interacting; + c.errors = app.expression_diagnostics.errors(&app.session, ctx, cid, interacting); + if let Some(error) = app.expression_diagnostics.take_failure() { + log::warn!("{error}"); + app.ui.status = error; } + c.current = c.current.min(c.errors.len().saturating_sub(1)); + ctx.data_mut(|d| d.insert_temp(cache_id(cid), c.clone())); c } /// Height the bar needs for `cid` (0 when every expression evaluates). -pub fn height(app: &EffectcraftApp, ctx: &egui::Context, cid: ItemId) -> f32 { - if app.session.expr.is_none() || errors(app, ctx, cid).errors.is_empty() { 0.0 } else { HEIGHT } +pub fn height(app: &mut EffectcraftApp, ctx: &egui::Context, cid: ItemId) -> f32 { + if errors(app, ctx, cid).errors.is_empty() { 0.0 } else { HEIGHT } } pub fn draw(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect, cid: ItemId) { @@ -71,12 +69,12 @@ pub fn draw(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect, cid: ItemId app.auto.add(auto, r, label); if resp.clicked() { c.current = (i + d) % n; - ctx.data_mut(|dd| dd.insert_temp(cache_id(), c.clone())); + ctx.data_mut(|dd| dd.insert_temp(cache_id(cid), c.clone())); } } p.text(count.center(), Align2::CENTER_CENTER, format!("{}/{n}", i + 1), Tokens::ui(11.0), t.text); let comp_name = app.session.project.item(cid).map(|it| it.name.clone()).unwrap_or_default(); - let e = c.errors[i].clone(); + let Some(e) = c.errors.get(i).cloned() else { return }; let msg_rect = Rect::from_min_max(pos2(rect.min.x + 26.0, rect.min.y), pos2(prev.min.x - 6.0, rect.max.y)); let resp = ui.interact(msg_rect, egui::Id::new("expr-bar-msg"), Sense::click()).on_hover_text(tr("Click to reveal the expression in the timeline")); p.with_clip_rect(msg_rect).text( @@ -87,6 +85,8 @@ pub fn draw(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect, cid: ItemId if resp.hovered() { Color32::WHITE } else { t.text }, ); app.auto.add("viewer.exprErrors", msg_rect, &e.describe(&comp_name)); + // Keep the former banner automation id as an alias for the unified error strip. + app.auto.add("viewer.expressionErrorBanner", msg_rect, &e.describe(&comp_name)); if resp.clicked() { let _ = app.session.execute("layer.select", json!({"comp": cid.0, "layers": [e.layer]})); let _ = app.session.execute("prop.select", json!({"comp": cid.0, "layer": e.layer, "prop": e.prop, "selectKeys": false})); diff --git a/crates/ui-egui/src/panels/expr_editor.rs b/crates/ui-egui/src/panels/expr_editor.rs index ffb61738..f800b1a4 100644 --- a/crates/ui-egui/src/panels/expr_editor.rs +++ b/crates/ui-egui/src/panels/expr_editor.rs @@ -1,7 +1,6 @@ //! The expression editor of the Timeline, following Settings ▸ Scripting & Expressions: //! Font Size, Syntax Highlighting, Line Numbers, Auto-complete (Tab accepts), Bracket Matching -//! and Word Wrap; and the Composition panel's expression error banner (Show Expression Error -//! Banner). +//! and Word Wrap. The Composition viewer uses the shared diagnostics in `expr_bar`. use effectcraft_engine::prefs::Scripting; use egui::text::{LayoutJob, TextFormat}; @@ -381,48 +380,6 @@ pub fn editor(ui: &mut egui::Ui, id: egui::Id, buf: &mut String, rect: Rect, p: out.response.response } -/// Settings ▸ Scripting & Expressions ▸ Show Expression Error Banner: the first failing -/// expression of the comp at the current time, in a banner along the bottom of the viewer -/// (click it to reveal the property). -pub fn error_banner(app: &mut crate::EffectcraftApp, ui: &egui::Ui, area: Rect) { - if !app.session.prefs.scripting.error_banner { - return; - } - let Some(cid) = app.session.active_comp_id() else { return }; - let key = (app.session.revision, cid.0, app.session.time().0); - let id = egui::Id::new("expr-error-banner"); - let cached: Option<((u64, u64, i64), Vec)> = ui.ctx().data(|d| d.get_temp(id)); - let errors = match cached { - Some((k, e)) if k == key => e, - _ => { - let e = app.session.expression_errors(cid); - ui.ctx().data_mut(|d| d.insert_temp(id, (key, e.clone()))); - e - } - }; - let Some(first) = errors.first() else { return }; - let layer = first["layer"].as_u64().unwrap_or(0); - let lname = app.session.active_comp().and_then(|c| c.layer(effectcraft_engine::project::LayerId(layer))).map(|l| l.name.clone()).unwrap_or_default(); - let msg = format!( - "⚠ Expression error{}: {} ▸ {}: {}", - if errors.len() > 1 { format!(" (1 of {})", errors.len()) } else { String::new() }, - lname, - first["name"].as_str().unwrap_or(""), - first["error"].as_str().unwrap_or("").lines().next().unwrap_or("") - ); - let r = Rect::from_min_max(pos2(area.min.x, area.max.y - 22.0), area.max); - let p = ui.painter(); - p.rect_filled(r, 0.0, Color32::from_rgb(0xf0, 0xc0, 0x30)); - p.text(pos2(r.min.x + 8.0, r.center().y), egui::Align2::LEFT_CENTER, msg, Tokens::ui(11.5), Color32::BLACK); - app.auto.add("viewer.expressionErrorBanner", r, "Expression error banner"); - if ui.interact(r, id.with("click"), egui::Sense::click()).clicked() { - let p = serde_json::json!({"props": [{"layer": layer, "prop": first["prop"]}]}); - if let Err(e) = crate::menus::invoke(app, ui.ctx(), "timeline.revealProps", p) { - app.ui.status = e; - } - } -} - #[cfg(test)] mod contrast_tests { use super::*; diff --git a/crates/ui-egui/src/panels/viewer.rs b/crates/ui-egui/src/panels/viewer.rs index 5fe7697c..936185c5 100644 --- a/crates/ui-egui/src/panels/viewer.rs +++ b/crates/ui-egui/src/panels/viewer.rs @@ -1010,8 +1010,6 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { app.auto.add("viewer.referenceAxes", Rect::from_center_size(corner, vec2(56.0, 56.0)), "3D reference axes"); } - super::expr_editor::error_banner(app, ui, area); - // Track points of the current track (Tracker panel) on its layer, while that layer is selected. let mut track_hits = vec![]; let track_cur = super::tracker::viewer_track(app); diff --git a/crates/ui-egui/tests/ui_web_gpu_storage.rs b/crates/ui-egui/tests/ui_web_gpu_storage.rs index fa97b571..d706a382 100644 --- a/crates/ui-egui/tests/ui_web_gpu_storage.rs +++ b/crates/ui-egui/tests/ui_web_gpu_storage.rs @@ -170,7 +170,15 @@ fn gpu_worker_renders_effect_frames_and_the_disk_cache_survives_a_reload() { let mut frames2 = Frames::default(); frames2.set_remote(Some(worker2.clone())); assert!(frames2.remote_disk()); - let bar = frames2.disk_frames(&source(&s, None), &key(fx), 30, &opts); + let until = std::time::Instant::now() + std::time::Duration::from_secs(5); + let bar = loop { + let bar = frames2.disk_frames(&source(&s, None), &key(fx), 30, &opts); + if !bar.is_empty() { + break bar; + } + assert!(std::time::Instant::now() < until, "disk-cache indicator did not finish hashing"); + std::thread::sleep(std::time::Duration::from_millis(1)); + }; assert_eq!(bar, vec![0], "the blue cache bar shows the stored frame"); frames2.request_urgent(&source(&s, Some(page_gpu)), key(fx), fx, t, opts); frames2.dispatch_remote(); diff --git a/docs/architecture.md b/docs/architecture.md index 9e6fc0e7..1ca72bd2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -540,10 +540,24 @@ text, disables the expression and shows a warning, like After Effects. `sampleIm sampled layer through the evaluating renderer's footage source (`EvalCtx::footage`), cached per thread and frame; `footage(name)` reads data footage (JSON, CSV, TSV imported with File ▸ Import, text kept in the project) through `sourceData`, `sourceText` and `dataValue`. `expr.errors` -lists failing expressions for the viewer's error bar. Each thread keeps one boa context; boa +lists failing expressions on demand. The viewer's error bar uses the same checks against an +immutable project snapshot, after 150 ms without editing, pointer interaction or playback. +Native builds run these automatic checks on one persistent worker; layout and paint only read +completed results. Requests are coalesced per comp, cancelled between properties when obsolete, +and bounded to eight comp entries. Results must match the current project, revision and time. +Disabling Show Expression Error Banner cancels automatic checks. Non-solo layers still receive +diagnostics, including disabled expressions with syntax errors. The single-threaded web build +shares the cache and debounce but evaluates one scan on the UI thread after interaction stops. +Each thread keeps one boa context; boa leaves values on its stack whenever an exception unwinds through call frames (every pending host request does), so a run that hits the stack limit gets a fresh context and is retried once. +Snapping memoizes expression values while collecting one immutable set of targets. Bounds, +vertices and transforms share those values; the cache is discarded before the next query, so +time changes and edits always use current geometry. Native timeline disk-cache indicators also +request missing content hashes in the background, coalescing edits per comp. Their blue bars +appear after the key is ready instead of serializing composition content during painting. + ## 5a. Scripting `effectcraft-script` runs JavaScript (boa, a fresh context per run; the Script Console keeps one)