From a64af1eff40fe66f3f2d2d7404c70e1a88b977f6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 10:26:34 +0000 Subject: [PATCH] address review: refuse a non-positive frame duration; count a partial last frame; check every selected layer before converting --- crates/engine/src/cmd/prop.rs | 30 +++++++++++++++++++++++------ crates/engine/src/tests/timeline.rs | 12 ++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/prop.rs b/crates/engine/src/cmd/prop.rs index 566b5ec86..9c2df7994 100644 --- a/crates/engine/src/cmd/prop.rs +++ b/crates/engine/src/cmd/prop.rs @@ -1370,6 +1370,13 @@ fn convert_expr_to_keys(s: &mut Session, p: &Value) -> Result { if sel.is_empty() { return Err(bad(CMD, "no `path` given and no selected property has an expression")); } + // Every layer is checked before the first is converted: a refusal leaves them all as they were. + let step = comp.frame_duration().0; + for (lid, _) in &sel { + if let Some(l) = comp.layer(*lid) { + check_convertible(step, l)?; + } + } let mut n = 0; for (lid, uid) in sel { n += convert_one_expr(s, cid, lid, uid)?.as_u64().unwrap_or(0); @@ -1383,6 +1390,22 @@ fn convert_expr_to_keys(s: &mut Session, p: &Value) -> Result { /// Most keyframes Convert Expression to Keyframes makes (2.4 hours at 30 fps). const MAX_CONVERTED_KEYS: u64 = 1 << 18; +/// One evaluation and one key per frame of the layer: its In and Out points and the composition's +/// frame duration are saved data, so the step must advance and the count (a partial last frame +/// rounded up, as the loop steps, in 128 bits) is checked before any key is made. +fn check_convertible(step: i64, layer: &Layer) -> Result<()> { + const CMD: &str = "prop.convertExpressionToKeyframes"; + if step <= 0 { + return Err(bad(CMD, "the composition's frame duration is not positive")); + } + let (step, span) = (i128::from(step), i128::from(layer.out_point.0) - i128::from(layer.in_point.0)); + let frames = if span > 0 { (span + step - 1) / step } else { 0 }; + if frames > i128::from(MAX_CONVERTED_KEYS) { + return Err(bad(CMD, format!("the layer spans {frames} frames; at most {MAX_CONVERTED_KEYS} are converted"))); + } + Ok(()) +} + /// Replaces the expression on one property with keyframes sampled from its evaluated values. /// Returns the number of properties converted. fn convert_one_expr(s: &mut Session, cid: ItemId, lid: LayerId, uid: Uid) -> Result { @@ -1390,12 +1413,7 @@ fn convert_one_expr(s: &mut Session, cid: ItemId, lid: LayerId, uid: Uid) -> Res let layer = comp.layer(lid).ok_or(EngineError::NoComp)?.clone(); let pr = layer.props.find(uid).ok_or_else(|| bad("prop.convertExpressionToKeyframes", "no property"))?.clone(); let fd = comp.frame_duration(); - // One evaluation and one key per frame of the layer: its In and Out points are saved data, - // so the count is checked before any is made. - let frames = (layer.out_point.0.saturating_sub(layer.in_point.0)).max(0) as u64 / fd.0.max(1) as u64; - if frames > MAX_CONVERTED_KEYS { - return Err(bad("prop.convertExpressionToKeyframes", format!("the layer spans {frames} frames; at most {MAX_CONVERTED_KEYS} are converted"))); - } + check_convertible(fd.0, &layer)?; let mut keys = vec![]; let mut t = layer.in_point; while t < layer.out_point { diff --git a/crates/engine/src/tests/timeline.rs b/crates/engine/src/tests/timeline.rs index 82e030d01..1c3f128d8 100644 --- a/crates/engine/src/tests/timeline.rs +++ b/crates/engine/src/tests/timeline.rs @@ -1569,3 +1569,15 @@ fn convert_expression_to_keyframes_refuses_a_layer_of_millions_of_frames() { assert!(e.contains("at most"), "{e}"); assert!(prop(&s, l, "transform/rotation").keys.is_empty()); } + +#[test] +fn convert_expression_to_keyframes_refuses_a_zero_frame_duration() { + let (mut s, l) = setup(); + s.execute("prop.setExpression", json!({"layer": l, "path": "transform/rotation", "expression": "value"})).unwrap(); + // A saved frame rate of 0/1 s has a frame duration of 0: the sampling loop never advanced. + let cid = s.active_comp_id().unwrap(); + std::sync::Arc::make_mut(&mut s.project).comp_mut(cid).unwrap().frame_rate.den = 0; + let e = s.execute("prop.convertExpressionToKeyframes", json!({"layer": l, "path": "transform/rotation"})).unwrap_err().to_string(); + assert!(e.contains("frame duration"), "{e}"); + assert!(prop(&s, l, "transform/rotation").keys.is_empty()); +}