Skip to content

Commit 8a467b8

Browse files
committed
fix(animation): make the animation knobs actually reach the solver (#158)
Eight confirmed audit findings. Six of them are dead knobs: a field the author sets, the schema accepts, `validate` calls clean, and the engine ignores. Nobody gets an error; the video simply lacks the animation that was asked for. For a tool driven by generated JSON that is the worst failure mode there is, because the correction loop never closes. - `float_3d`'s `amplitude` never reached `PresetConfig`: `AnimationTiming` had no such field and the only converter wrote `None`. Every `float_3d` moved by the 12px default, so the documented parallax recipe produced no parallax. Measured: 60 requested, -12 delivered. - `pulse` / `float` / `shake` / `spin` built keyframes at the literal times 0.0 / 0.25 / 0.5 / 1.0 and never read `delay` or `duration`. With `delay: 1.0`, all four were already mid-animation at t=0.5s. - `"loop": true` was inert on keyframe effects and `tilt_in`: the resolver was handed `None` for the preset config, fell back to `repeat: false`, and never called `loop_time`. - `--strict-anim` resolved effects at global scene time while the engine resolves at remapped local time, so it flagged violations at instants that are never rendered — and missed real ones. It now reads the `time_params` the builder already computes. - The completion budget added `start_at` to `delay + duration`, which the engine does not do: since PR #27 `start_at` gates visibility only and `delay` is absolute scene time. A 1s animation at `start_at: 1.5` in a 2s scene was reported as overrunning. - Two keyframe animations on the same property summed or overwrote each other depending on whether one carried a `delay` — a field with nothing to do with composition, routing effects into two separately-resolved buckets. Now a single bucket with one rule: last declared wins, the CSS cascade rule, which was already the behaviour within a bucket. - The spring solver returned NaN for `mass: 0` or `stiffness: 0` and diverged on negative damping, with no validation anywhere. A NaN reaching layout contaminates the whole tree. Both ends are handled: the solver floors its inputs, and `validate` now rejects the configs outright. - Unknown keys inside `style.animation[*]` were never reported. That last one has a deliberate consequence worth stating: `deny_unknown_fields` on the nine effect-config structs means a typo now fails deserialization, and `deserialize_children` skips a child it cannot parse. A misspelled key stops producing a default-valued animation and starts removing the component, with a stderr warning. That is the same contract `CssStyle` has carried all along — it is why `margin-top` drops a component — so this extends an existing policy rather than inventing one, and `validate` catches it first. Tests: full workspace green on this branch alone.
1 parent b24878a commit 8a467b8

5 files changed

Lines changed: 864 additions & 72 deletions

File tree

crates/rustmotion-cli/src/commands/geometry.rs

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1182,6 +1182,7 @@ pub fn validate_geometry_animated(scenario: &ResolvedScenario) -> Vec<GeometryVi
11821182
&built.root.children,
11831183
&layouts,
11841184
&built.stagger_delays,
1185+
&built.time_params,
11851186
viewport,
11861187
vi,
11871188
si,
@@ -1206,6 +1207,7 @@ fn walk_anim(
12061207
boxes: &[BoxNode],
12071208
layouts: &LayoutResult,
12081209
stagger_delays: &[f64],
1210+
time_params: &[(f64, f64)],
12091211
viewport: (u32, u32),
12101212
vi: usize,
12111213
si: usize,
@@ -1257,8 +1259,26 @@ fn walk_anim(
12571259
// resolves them at absolute scene `time` — no re-timing by
12581260
// start_at, which would double-shift components that also
12591261
// declare a matching `animation` delay.
1262+
//
1263+
// `time` above is *global* scene time; the renderer never
1264+
// resolves effects at that raw value once a `time_scale`/
1265+
// `time_offset`-bearing container is in the ancestor chain —
1266+
// `build_child` remaps it first (`box_builder.rs`:
1267+
// `t_local = scale * t_global + shift`). Constat #3: this walker
1268+
// used to skip that remap entirely, resolving effects at a time
1269+
// that never occurs at render — false positives when the
1270+
// container speeds the subtree up past the checked sample, false
1271+
// negatives when it slows it down. `built.time_params` carries
1272+
// the exact same accumulated `(scale, shift)` the renderer used,
1273+
// indexed by the same `NodeId`, so applying it here keeps this
1274+
// walker and the renderer in lockstep.
1275+
let (scale, shift) = time_params
1276+
.get(box_node.id as usize)
1277+
.copied()
1278+
.unwrap_or((1.0, 0.0));
1279+
let local_time = scale * time + shift;
12601280
let props = match effective_effects(&child.component, stagger_delay) {
1261-
Some(effects) => resolve_props_for_effects(&effects, time, scene_duration),
1281+
Some(effects) => resolve_props_for_effects(&effects, local_time, scene_duration),
12621282
None => AnimatedProperties::default(),
12631283
};
12641284
let raw_bbox = bbox_of(layout);
@@ -1307,6 +1327,7 @@ fn walk_anim(
13071327
&box_node.children,
13081328
layouts,
13091329
stagger_delays,
1330+
time_params,
13101331
viewport,
13111332
vi,
13121333
si,
@@ -1899,6 +1920,61 @@ mod tests {
18991920
assert_eq!(v.unwrap().axis, Axis::X);
19001921
}
19011922

1923+
#[test]
1924+
fn strict_anim_respects_a_containers_time_offset_remap() {
1925+
// Constat #3: `walk_anim` used to resolve effects at raw *global*
1926+
// scene time, ignoring any `time_scale`/`time_offset` remap
1927+
// accumulated from ancestor containers — even though the renderer
1928+
// (`box_builder::build_child`) always resolves at the *local*
1929+
// remapped time (`t_local = scale * t_global + shift`).
1930+
//
1931+
// Here the shape's `slide_in_left` (delay=0, duration=1.0) sits
1932+
// inside a `flex` with `time_offset: -5.0`, which (per
1933+
// `rustmotion/src/tests.rs`'s time-remap tests) shifts local time to
1934+
// `t_local = t_global + 5.0`. Every sample in this 2s scene
1935+
// (t_global in [0, 2]) therefore resolves at local time in [5, 7] —
1936+
// 5-7s past the 1s animation window, fully settled at rest (x=100,
1937+
// well inside the 1920px-wide viewport). A walker that ignores the
1938+
// remap instead resolves at raw t_global in [0, 2], still inside the
1939+
// animation's own [0, 1] window for the first half of the scene,
1940+
// and reports a slide-in overflow that never actually happens at
1941+
// render time.
1942+
let json = r##"{
1943+
"video": { "width": 1920, "height": 1080 },
1944+
"scenes": [{
1945+
"duration": 2.0,
1946+
"children": [{
1947+
"type": "flex",
1948+
"time_offset": -5.0,
1949+
"style": { "width": "1920px", "height": "1080px" },
1950+
"children": [{
1951+
"type": "shape",
1952+
"shape": "rect",
1953+
"position": "absolute",
1954+
"x": 100, "y": 100,
1955+
"style": {
1956+
"width": "100px", "height": "100px",
1957+
"animation": [{ "name": "slide_in_left", "delay": 0, "duration": 1.0 }]
1958+
},
1959+
"fill": "#ff0000"
1960+
}]
1961+
}]
1962+
}]
1963+
}"##;
1964+
let scenario = parse(json);
1965+
let violations = validate_geometry_animated(&scenario);
1966+
let overflow: Vec<_> = violations
1967+
.iter()
1968+
.filter(|v| v.kind == ViolationKind::AnimatedTextOverflow)
1969+
.collect();
1970+
assert!(
1971+
overflow.is_empty(),
1972+
"time_offset=-5.0 settles the slide-in 5-7s before any sampled instant; a \
1973+
walker that honours the remap must report zero overflows, got: {:?}",
1974+
overflow
1975+
);
1976+
}
1977+
19021978
#[test]
19031979
fn strict_anim_start_at_and_effect_delay_do_not_double_shift_the_timeline() {
19041980
// A component with BOTH `start_at` (visibility gate) and a matching

crates/rustmotion-cli/src/commands/validate_attrs.rs

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,4 +351,65 @@ mod tests {
351351
assert_eq!(errors.len(), 1, "expected one error: {errors:?}");
352352
assert!(errors[0].contains("counter"), "got: {}", errors[0]);
353353
}
354+
355+
#[test]
356+
fn typo_inside_style_animation_effect_is_reported() {
357+
// Constat #8: `walk_component` only compares a component's own
358+
// top-level keys, then recurses into `children` — it never looks
359+
// inside `style`, let alone `style.animation[*]`. A typo'd field on
360+
// an animation effect (`duratoin` instead of `duration`) used to
361+
// deserialize silently (the effect config structs had no
362+
// `deny_unknown_fields`), so the author got a scenario that "worked"
363+
// but quietly ran the default 0.8s duration instead of theirs.
364+
//
365+
// The fix lives in `schema/video.rs` (adding `deny_unknown_fields` to
366+
// every `AnimationEffect` payload struct) rather than here: an
367+
// internally-tagged enum's tag field is excluded from what the
368+
// variant's own `Deserialize` sees, so this rejects the typo without
369+
// ever flagging the legitimate `name` tag as unknown. That routes the
370+
// typo through the *existing* typed-parse-failure path in
371+
// `check_component_attrs` (the same one that already catches, e.g.,
372+
// a missing required field) — it surfaces as a blocking error, not a
373+
// `walk_component` warning.
374+
let s = resolved(serde_json::json!([
375+
{
376+
"type": "text", "content": "hi",
377+
"style": { "animation": [{ "name": "fade_in_up", "duratoin": 0.6 }] }
378+
}
379+
]));
380+
let (errors, _) = check_component_attrs(&s);
381+
assert!(
382+
errors.iter().any(|e| e.contains("duratoin")),
383+
"expected the typo'd animation-effect field to be reported as an error: {errors:?}"
384+
);
385+
}
386+
387+
#[test]
388+
fn well_formed_animation_effect_fields_are_not_flagged() {
389+
// Sanity companion to the typo test: legitimate fields across a
390+
// spread of effect kinds (preset timing, keyframes, wiggle, glow,
391+
// motion_blur, tilt_in) must not trip the new deny_unknown_fields.
392+
let s = resolved(serde_json::json!([
393+
{
394+
"type": "text", "content": "hi",
395+
"style": { "animation": [
396+
{ "name": "fade_in_up", "delay": 0.2, "duration": 0.6, "loop": false,
397+
"overshoot": 0.1, "spring": { "damping": 12, "stiffness": 100, "mass": 1 } },
398+
{ "name": "float_3d", "duration": 1.0, "amplitude": 20 },
399+
{ "name": "tilt_in", "delay": 0.0, "duration": 0.4, "rotate_x": 10.0 },
400+
{ "name": "wiggle", "property": "translate_y", "amplitude": 5, "frequency": 2, "seed": 3 },
401+
{ "name": "glow", "color": "#ffffff", "radius": 10, "intensity": 1.0 },
402+
{ "name": "motion_blur", "samples": 4, "shutter": 0.5 },
403+
{ "name": "keyframes", "keyframes": [
404+
{ "property": "opacity", "keyframes": [
405+
{ "time": 0.0, "value": 0.0 }, { "time": 1.0, "value": 1.0 }
406+
] }
407+
], "delay": 0.0, "duration": 1.0, "loop": true }
408+
] }
409+
}
410+
]));
411+
let (errors, warnings) = check_component_attrs(&s);
412+
assert!(errors.is_empty(), "unexpected errors: {errors:?}");
413+
assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}");
414+
}
354415
}

0 commit comments

Comments
 (0)