Merge pull request #807 from cdinhoo/fix/nonblocking-expression-diagnostics

Performance: reduce UI stalls while dragging layers

crates/engine/src/commands/expr_tools.rs

@@ -43,8 +43,18 @@ impl ExprError {
 pub fn errors(s: &Session, cid: ItemId, t: Tick) -> Vec<ExprError> {
     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<fn(&str) -> std::result::Result<(), String>>, mut keep_going: impl FnMut() -> bool) -> Option<Vec<ExprError>> {
     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<ExprError> {
             }
         });
         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 {

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<Value, String>>;
+
+/// 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<SnapValues>,
+}
+
+impl ExprHost for SnapExpressions<'_> {
+    fn eval(&self, ctx: &EvalCtx, layer: &Layer, prop: &Property, value: &Value) -> Result<Value, String> {
+        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<SnapTarget> {
+    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<SnapTarget> {
     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<Image>,
 }
+
+#[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<Value, String> {
+            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);
+    }
+}