Merge pull request #3242 from vangel76/prop-get-footage-3231

Expressions: prop.get and Reveal Expression Errors evaluate with the footage source

crates/engine/src/cmd/expr_tools.rs

@@ -2,7 +2,7 @@
 //! Language menu (the ⓕ button next to an expression), written from the public Expression
 //! Language Reference.
 
-use effectcraft_project::{Expression, ItemId, Layer, Property};
+use effectcraft_project::{Comp, Expression, ItemId, Layer, Property};
 use effectcraft_render::EvalCtx;
 use effectcraft_time::Tick;
 use serde::Serialize;
@@ -42,8 +42,16 @@ impl ExprError {
 /// expressions and the syntax errors that disabled others.
 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()
+    errors_in(&eval_ctx(s, cid, comp, t), s.expr_check, || true).unwrap_or_default()
+}
+
+/// The context to evaluate expressions of comp `cid` at comp time `t` as the render does for the
+/// comp at top level: with the session's expression host and its footage source, so
+/// `sampleImage()` reads footage pixels instead of failing or sampling transparent (#1000,
+/// #3231). `expr.errors`, `prop.get`, [`Session::expression_errors`] and the script value query
+/// all evaluate through this one context.
+pub fn eval_ctx<'a>(s: &'a Session, cid: ItemId, comp: &'a Comp, t: Tick) -> EvalCtx<'a> {
+    EvalCtx { footage: Some(s.footage.as_ref()), expr: s.expr.as_deref(), ..EvalCtx::new(&s.project, cid, comp, t) }
 }
 
 /// Evaluate diagnostics against an immutable snapshot. Background callers can cancel between

crates/engine/src/cmd/layer_menu.rs

@@ -867,8 +867,8 @@ impl Session {
             });
             for pr in has {
                 let text = pr.expr.as_ref().map(|e| e.text.clone()).unwrap_or_default();
-                let mut ctx = effectcraft_render::EvalCtx::new(&self.project, cid, comp, t);
-                ctx.expr = Some(host.as_ref());
+                // With the footage source, so `sampleImage()` doesn't fail here only (#3231).
+                let ctx = super::expr_tools::eval_ctx(self, cid, comp, t);
                 if let Err(e) = host.eval(&ctx, l, pr, &pr.value_at(l.layer_time(t))) {
                     errors.push(json!({"layer": l.id.0, "prop": pr.uid, "name": pr.name, "expression": text, "error": e}));
                 }

crates/engine/src/cmd/prop.rs

@@ -558,8 +558,9 @@ fn get(s: &mut Session, p: &Value) -> Result<Value> {
     } else if pr.has_expression()
         && let (Some(h), Some(comp)) = (s.expr.as_deref(), s.project.comp(cid))
     {
-        let mut ctx = crate::render::EvalCtx::new(&s.project, cid, comp, t);
-        ctx.expr = Some(h);
+        // The render's context, footage source included, so `sampleImage()` evaluates here as it
+        // renders and as `expr.errors` reports it (#3231).
+        let ctx = super::expr_tools::eval_ctx(s, cid, comp, t);
         match h.eval(&ctx, l, pr, &raw) {
             Ok(v) => out["evaluated"] = v.to_json(),
             Err(e) => {

crates/engine/src/tests/track.rs

@@ -433,3 +433,58 @@ fn analyze_1080p_single_point_speed() {
     assert!(dist(fc.value_at(frame_time(30)).as_vec2(), [990.0, 555.0]) < 0.25);
     assert!(fps > 30.0, "{fps} frames/s");
 }
+
+/// A stand-in for `sampleImage()` on a footage layer (the engine has no expression language): the
+/// pixel at (0, 0) of the layer's footage, read through the context's footage source. The real
+/// one samples transparent without the source; this one fails, so either symptom shows.
+struct SampleFootage;
+
+impl crate::render::ExprHost for SampleFootage {
+    /// Reads the red channel of the layer's footage pixel at (0, 0) through the context's footage source, scaled to 0-100.
+    fn eval(
+        &self,
+        ctx: &EvalCtx,
+        layer: &effectcraft_project::Layer,
+        _: &effectcraft_project::Property,
+        _: &effectcraft_keyframe::Value,
+    ) -> Result<effectcraft_keyframe::Value, String> {
+        let item = layer.source.item().ok_or("not a footage layer")?;
+        let Some(ItemKind::Footage(footage)) = ctx.project.item(item).map(|i| &i.kind) else { return Err("not a footage layer".into()) };
+        let src = ctx.footage.ok_or("sampleImage: no footage source")?;
+        let img = src.frame(item, footage, ctx.time).ok_or("no frame")?;
+        Ok(effectcraft_keyframe::Value::Scalar(f64::from(img.get(0, 0)[0]) * 100.0))
+    }
+}
+
+/// The clip of [`setup`] (uniform 25 % grey footage) with an opacity expression that samples its
+/// footage: 25 with the footage source. Here for `setup`'s footage layer (#3231).
+fn sampling_clip() -> (Session, ItemId, LayerId) {
+    let (mut s, clip, _) = setup(|_| Image::filled(W, H, [0.25, 0.25, 0.25, 1.0]));
+    let cid = s.active_comp_id().unwrap();
+    s.expr = Some(Arc::new(SampleFootage));
+    s.execute("prop.setExpression", json!({"layer": clip.0, "path": "transform/opacity", "expression": "sampleImage([0, 0])[0] * 100"})).unwrap();
+    (s, cid, clip)
+}
+
+/// #3231: `prop.get` evaluates an expression that samples footage pixels with the footage source,
+/// as the render and `expr.errors` do: the same value and no spurious `expressionError`.
+#[test]
+fn prop_get_evaluates_footage_sampling_expressions_with_the_footage_source() {
+    let (mut s, cid, clip) = sampling_clip();
+    let got = s.execute("prop.get", json!({"layer": clip.0, "path": "transform/opacity", "time": 0.0})).unwrap();
+    assert!(got.get("expressionError").is_none(), "no spurious error: {got}");
+    assert_eq!(got["evaluated"].as_f64(), Some(25.0), "the footage pixel, not the keyframed 100: {got}");
+    // The render applies the same opacity: the clip alone (outside the solid) covers 25 %.
+    let img = s.render(cid, Tick::ZERO, crate::render::RenderOpts::default());
+    let a = img.get(5, 5)[3];
+    assert!((a - 0.25).abs() < 0.01, "rendered opacity {a}");
+    assert!(crate::cmd::expr_tools::errors(&s, cid, Tick::ZERO).is_empty());
+}
+
+/// #3231: Reveal Expression Errors and the viewer's error banner evaluate with the footage source,
+/// so an expression that samples footage pixels isn't reported as failing.
+#[test]
+fn expression_errors_evaluate_footage_sampling_expressions_with_the_footage_source() {
+    let (s, cid, _) = sampling_clip();
+    assert_eq!(s.expression_errors(cid), Vec::<serde_json::Value>::new());
+}

crates/script/src/model.rs

@@ -532,6 +532,7 @@ fn text_json(d: &TextDoc) -> J {
     doc
 }
 
+/// Returns the property's value at the requested time, evaluating its expression unless `pre` is set.
 fn value(s: &Session, a: &J) -> R {
     let (cid, c, l) = layer_of(s, a)?;
     let uid = u(a, "uid").ok_or("missing uid")?;
@@ -541,12 +542,9 @@ fn value(s: &Session, a: &J) -> R {
     let v = if pre || !p.has_expression() {
         p.value_at(l.layer_time(t))
     } else {
-        let mut ctx = EvalCtx::new(&s.project, cid, c, t);
-        ctx.expr = s.expr.as_deref();
         // Sample expressions (`sampleImage`) read footage through the session's source; without
         // it footage layers sample as transparent (#1000).
-        ctx.footage = Some(s.footage.as_ref());
-        ctx.value(l, p)
+        effectcraft_engine::commands::expr_tools::eval_ctx(s, cid, c, t).value(l, p)
     };
     Ok(js_value(c, &v))
 }