From b9cf23a271cc389eb343094b78128caebf757b1a Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Fri, 25 Sep 2026 09:56:34 +0200 Subject: [PATCH 1/6] fix(animator): gate a timeline step's animation on its own at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A timeline step has two halves and they obeyed different rules. Its `style` was gated by `apply_style_states`, which merges only the states whose `at` the clock has reached. Its `animation` was merged unconditionally by `effective_effects`, so every step contributed from t=0. With one step that is invisible: the step's own first keyframe is the base state, so holding it changes nothing. With two or more it decides the frame. All keyframes effects share one bucket where the last entry wins on a shared property, and the last entry is the last step — which has not begun, so it imposes the value of its first keyframe from the very start. A node that moves in three beats sits at the end of beat two for the whole scene, and only the final beat ever animates. Passing the component's local time in and filtering the steps the same way `apply_style_states` already does aligns the two halves. The last-effect-wins rule for `style.animation` is untouched: it was chosen deliberately so that composition does not depend on an incidental `delay`, and it is pinned by `last_declared_effect_wins_regardless_of_which_one_carries_the_delay`, which still passes. Found by porting an existing promo animation to a scenario rather than by reading: the logo had to enter centred, move to a corner, and come back, and it never left the corner. --- .../rustmotion-components/src/box_builder.rs | 79 +++++++++++++++++-- .../src/legacy_dispatch.rs | 6 +- .../rustmotion/src/cli/commands/geometry.rs | 2 +- crates/rustmotion/src/engine/render/scene.rs | 2 +- 4 files changed, 78 insertions(+), 11 deletions(-) diff --git a/crates/rustmotion-components/src/box_builder.rs b/crates/rustmotion-components/src/box_builder.rs index f5ab158b..e73513ef 100644 --- a/crates/rustmotion-components/src/box_builder.rs +++ b/crates/rustmotion-components/src/box_builder.rs @@ -316,7 +316,8 @@ fn build_ghosts<'a>( scene_duration: actx.scene_duration, fps: actx.fps, }; - if let Some(ghost_effects) = effective_effects(&child.component, stagger_delay) { + if let Some(ghost_effects) = effective_effects(&child.component, stagger_delay, ghost_time) + { let props = resolve_props_for_effects( &ghost_effects, ghost_actx.time, @@ -452,7 +453,7 @@ fn build_child<'a>( // lower (earlier in the slot table). The principal's id is allocated below. let mut ghosts: Vec = Vec::new(); if let Some(actx) = local_actx { - if let Some(effects) = effective_effects(&child.component, stagger_delay) { + if let Some(effects) = effective_effects(&child.component, stagger_delay, actx.time) { ghosts = build_ghosts( child, components, @@ -534,7 +535,7 @@ fn build_child<'a>( // — internal animations like draw_progress or char_animation remain on the // `AnimatedProperties` legacy path. if let Some(actx) = local_actx { - if let Some(effects) = effective_effects(&child.component, stagger_delay) { + if let Some(effects) = effective_effects(&child.component, stagger_delay, actx.time) { let props = resolve_props_for_effects(&effects, actx.time, actx.scene_duration); if props_has_paint_overrides(&props) { apply_animated_props(&mut css, &props); @@ -655,13 +656,19 @@ fn build_child<'a>( } /// The full effect list for a component at paint time: `style.animation`, -/// plus `timeline` steps shifted by their `at`, plus keyframes synthesized -/// from timeline style-state changes (`style.transition`), plus the -/// container-stagger delay applied to everything. Returns `None` when there -/// is nothing to resolve, `Some(Cow::Borrowed)` on the no-merge fast path. +/// plus the `timeline` steps whose `at` `t` has reached, shifted by their +/// `at`, plus keyframes synthesized from timeline style-state changes +/// (`style.transition`), plus the container-stagger delay applied to +/// everything. Returns `None` when there is nothing to resolve, +/// `Some(Cow::Borrowed)` on the no-merge fast path. +/// +/// `t` is the component's own local time, the same clock +/// `resolve_props_for_effects` is called with, and the same one +/// `apply_style_states` gates a step's `style` on. pub fn effective_effects( component: &Component, extra_delay: f64, + t: f64, ) -> Option> { let animatable = component.as_animatable()?; let effects = animatable.animation_effects(); @@ -675,7 +682,7 @@ pub fn effective_effects( return (!effects.is_empty()).then_some(std::borrow::Cow::Borrowed(effects)); } let mut merged = effects.to_vec(); - for step in steps { + for step in steps.iter().filter(|s| s.at <= t - extra_delay) { for effect in &step.animation { let mut e = effect.clone(); e.shift_delay(step.at); @@ -2185,6 +2192,62 @@ pub fn component_kind(c: &Component) -> &'static str { #[cfg(test)] mod tests { use super::*; + + /// Two timeline steps on one node. Each step is documented to trigger at + /// its own `at`, and `apply_style_states` already gates a step's `style` + /// that way — its `animation` half must obey the same rule, or a step + /// that has not begun still sets the value through the shared + /// last-effect-wins bucket. + #[test] + fn a_timeline_step_leaves_the_value_alone_until_its_at() { + let component: Component = serde_json::from_value(json!({ + "type": "shape", + "shape": "circle", + "fill": "#1EA2C2", + "style": { "width": 120, "height": 120 }, + "timeline": [ + { "at": 3.0, "animation": [{ "name": "keyframes", "duration": 1.0, "keyframes": [ + { "property": "translate_x", "easing": "linear", "keyframes": [ + { "time": 0.0, "value": 0.0 }, { "time": 1.0, "value": 200.0 }] }] }] }, + { "at": 6.0, "animation": [{ "name": "keyframes", "duration": 1.0, "keyframes": [ + { "property": "translate_x", "easing": "linear", "keyframes": [ + { "time": 0.0, "value": 200.0 }, { "time": 1.0, "value": 0.0 }] }] }] } + ] + })) + .expect("component deserializes"); + + let tx = |t: f64| match effective_effects(&component, 0.0, t) { + Some(effects) => resolve_props_for_effects(&effects, t, 9.0).translate_x as f64, + None => AnimatedProperties::default().translate_x as f64, + }; + + assert!( + tx(0.5).abs() < 1.0, + "before either step, translate_x is 0, got {}", + tx(0.5) + ); + assert!( + (tx(3.5) - 100.0).abs() < 2.0, + "halfway through step one, got {}", + tx(3.5) + ); + assert!( + (tx(5.0) - 200.0).abs() < 1.0, + "step one has ended and holds, got {}", + tx(5.0) + ); + assert!( + (tx(6.5) - 100.0).abs() < 2.0, + "halfway through step two, got {}", + tx(6.5) + ); + assert!( + tx(8.0).abs() < 1.0, + "step two has ended and holds, got {}", + tx(8.0) + ); + } + use rustmotion_core::css::style::{ CssStyle, Display, Edges, FlexDirection, Gap, Size as CSize, }; diff --git a/crates/rustmotion-components/src/legacy_dispatch.rs b/crates/rustmotion-components/src/legacy_dispatch.rs index f3fb225f..31ca09d4 100644 --- a/crates/rustmotion-components/src/legacy_dispatch.rs +++ b/crates/rustmotion-components/src/legacy_dispatch.rs @@ -117,7 +117,11 @@ impl<'a> PaintDispatcher for LegacyPaintDispatcher<'a> { .copied() .unwrap_or((1.0, 0.0)); let local_time = frame.time * t_scale + t_shift; - let props = match crate::box_builder::effective_effects(&child.component, stagger_delay) { + let props = match crate::box_builder::effective_effects( + &child.component, + stagger_delay, + local_time, + ) { Some(effects) => resolve_props_for_effects(&effects, local_time, frame.scene_duration), None => AnimatedProperties::default(), }; diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 9494bab0..88f5b312 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -1504,7 +1504,7 @@ fn walk_anim( .copied() .unwrap_or((1.0, 0.0)); let local_time = scale * time + shift; - let props = match effective_effects(&child.component, stagger_delay) { + let props = match effective_effects(&child.component, stagger_delay, local_time) { Some(effects) => resolve_props_for_effects(&effects, local_time, scene_duration), None => AnimatedProperties::default(), }; diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index cf7d96f2..96200fd6 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -581,7 +581,7 @@ fn paint_decorative_fullscreen( } } - let props = match effective_effects(&child.component, 0.0) { + let props = match effective_effects(&child.component, 0.0, time) { Some(effects) => resolve_props_for_effects(&effects, time, ctx.scene_duration), None => AnimatedProperties::default(), }; From 5bda143d25897eb3002057d09b7b9cfbccb5d125 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Fri, 25 Sep 2026 10:20:46 +0200 Subject: [PATCH 2/6] feat(svg): add a fill-reveal draw-on mode via `reveal: fill` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A filled SVG (no stroke) could only draw-on as a wireframe outline: paint_draw_on always strokes each path with draw_stroke_width, so a solid logo traced in thin lines and only became solid the instant draw_progress hit 1.0. There was no way to express "the logo draws itself" for anything but stroke-art icons. Add `reveal: "stroke" | "fill"` (default "stroke", behavior byte-for-byte unchanged for the default: same branch, same functions). `"fill"` instead sweeps a per-path clip mask (left-to-right, path bounds) over the SVG's own already fully rasterized image (paint_resvg's output, now cached via the new `cached_full_image`, factored out of `paint_static` without behavior change). Clipping the pre-rendered raster keeps gradients/patterns intact, which a per-path flat-color fill (the alternative: reuse collect_paths' color resolution and skia-fill each path with a growing rect clip) would have lost — collect_paths already falls back to white for gradient/pattern paints, which is fine for a 2px trace but not for a filled reveal meant to show the real artwork. Path ordering/windowing (length-weighted, draw_overlap) is reused as-is from paint_draw_on so paths still reveal sequentially. Not verified: full ffmpeg render/encode of a `reveal: fill` scenario (only `still` frames and unit tests); behavior with self-intersecting/evenodd paths beyond the fill-rule set on the mask path. --- crates/rustmotion-components/src/svg.rs | 371 +++++++++++++++++++++--- 1 file changed, 325 insertions(+), 46 deletions(-) diff --git a/crates/rustmotion-components/src/svg.rs b/crates/rustmotion-components/src/svg.rs index ccf9e242..7c86314f 100644 --- a/crates/rustmotion-components/src/svg.rs +++ b/crates/rustmotion-components/src/svg.rs @@ -11,6 +11,18 @@ use rustmotion_core::engine::renderer::asset_cache; use rustmotion_core::schema::TimelineStep; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +#[derive(Default)] +pub enum SvgReveal { + /// Trace each path's outline progressively (current/legacy behavior). + #[default] + Stroke, + /// Sweep a mask across each path's full, already-painted shape (fills, + /// gradients included) instead of tracing a contour. + Fill, +} + #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct Svg { #[serde(default)] @@ -35,6 +47,10 @@ pub struct Svg { /// 0.0 = strictly sequential (default); 1.0 = all paths drawn in parallel. #[serde(default)] pub draw_overlap: f32, + /// How draw-on animation reveals paths: `stroke` traces contours (default, + /// unchanged), `fill` sweeps a mask across each path's full painted shape. + #[serde(default)] + pub reveal: SvgReveal, } fn default_draw_stroke_width() -> f32 { @@ -142,6 +158,127 @@ fn collect_paths( } } +/// Recursively collect each visible path's geometry (with its SVG fill rule +/// applied), for use as a reveal mask in `reveal: fill` mode. Color/stroke +/// don't matter here: the mask only gates which pixels of the already +/// fully-painted raster (gradients included) get copied to the canvas. +fn collect_paths_for_fill(group: &usvg::Group, out: &mut Vec) { + for node in group.children() { + match node { + usvg::Node::Group(g) => { + collect_paths_for_fill(g, out); + } + usvg::Node::Path(p) => { + if !p.is_visible() { + continue; + } + let mut skia_path = tiny_path_to_skia(p.data(), p.abs_transform()); + let fill_type = match p.fill().map(|f| f.rule()) { + Some(usvg::FillRule::EvenOdd) => skia_safe::PathFillType::EvenOdd, + _ => skia_safe::PathFillType::Winding, + }; + skia_path.set_fill_type(fill_type); + out.push(skia_path); + } + _ => {} + } + } +} + +/// Reveal the SVG progressively at `draw_progress` (0..=1) by sweeping a clip +/// mask across each path's full, already fully-painted shape (`full_image`, +/// gradients and all) instead of tracing a stroked contour. Paths are +/// revealed one after another (or with overlap), using the same per-path +/// length-weighted windowing as `paint_draw_on` so the sequential ordering +/// matches the stroke mode. +fn paint_fill_reveal( + canvas: &Canvas, + group: &usvg::Group, + svg_size: usvg::Size, + layout: &BoxLayout, + progress: f32, + draw_overlap: f32, + full_image: &skia_safe::Image, +) { + let progress = progress.clamp(0.0, 1.0); + + let mut paths: Vec = Vec::new(); + collect_paths_for_fill(group, &mut paths); + + if paths.is_empty() { + return; + } + + let scale_x = if svg_size.width() > 0.0 { + layout.width / svg_size.width() + } else { + 1.0 + }; + let scale_y = if svg_size.height() > 0.0 { + layout.height / svg_size.height() + } else { + 1.0 + }; + + let lengths: Vec = paths + .iter() + .map(|path| { + let mut pm = PathMeasure::new(path, false, None); + pm.length() + }) + .collect(); + + let total_length: f32 = lengths.iter().sum(); + if total_length <= 0.0 { + return; + } + + let overlap = draw_overlap.clamp(0.0, 1.0); + let image_dst = Rect::from_xywh(0.0, 0.0, svg_size.width(), svg_size.height()); + let paint = Paint::default(); + + let mut cumulative = 0.0f32; + for (path, length) in paths.iter().zip(lengths.iter()) { + let base_frac = length / total_length; + let window_size = base_frac * (1.0 - overlap) + overlap; + let start_frac = cumulative * (1.0 - overlap); + cumulative += base_frac; + + let local_t = if window_size > 0.0 { + ((progress - start_frac) / window_size).clamp(0.0, 1.0) + } else if progress >= start_frac { + 1.0 + } else { + 0.0 + }; + + if local_t <= 0.0 { + continue; + } + + canvas.save(); + canvas.scale((scale_x, scale_y)); + canvas.clip_path(path, None, true); + + if local_t < 1.0 { + // Sweep left-to-right: reveal a growing slice of this path's own + // bounding box, intersected with the path shape itself above. + let bounds = path.bounds(); + let revealed_w = bounds.width() * local_t; + let sweep = Rect::from_ltrb( + bounds.left, + bounds.top - 1.0, + bounds.left + revealed_w, + bounds.bottom + 1.0, + ); + canvas.clip_rect(sweep, None, true); + } + + canvas.draw_image_rect(full_image, None, image_dst, &paint); + canvas.restore(); + } +} + /// Draw the SVG paths progressively at `draw_progress` (0..=1). /// Uses a dash PathEffect to reveal each path sequentially (or with overlap). fn paint_draw_on( @@ -311,6 +448,19 @@ impl Painter for Svg { if progress >= 1.0 { // At completion, fall through to normal resvg render so fills are shown. self.paint_resvg(canvas, layout, &svg_data, &tree, svg_size); + } else if self.reveal == SvgReveal::Fill { + let Some(full_image) = self.cached_full_image(layout) else { + return; + }; + paint_fill_reveal( + canvas, + tree.root(), + svg_size, + layout, + progress, + self.draw_overlap, + &full_image, + ); } else { paint_draw_on( canvas, @@ -332,6 +482,20 @@ impl Painter for Svg { impl Svg { /// Normal static render via cached resvg bitmap. fn paint_static(&self, canvas: &Canvas, layout: &BoxLayout) { + let Some(img) = self.cached_full_image(layout) else { + return; + }; + + let dst = Rect::from_xywh(0.0, 0.0, layout.width, layout.height); + let paint = Paint::default(); + canvas.draw_image_rect(img, None, dst, &paint); + } + + /// Resolve (and cache) the fully rasterized SVG — fills, gradients and + /// all — at the layout's pixel size. Shared by `paint_static` and the + /// `reveal: fill` draw-on mode, which clips this same raster per path + /// instead of re-deriving flat per-path colors. + fn cached_full_image(&self, layout: &BoxLayout) -> Option { let target_w_opt: Option = if layout.width > 0.0 { Some(layout.width as u32) } else { @@ -350,7 +514,8 @@ impl Svg { target_w_opt.unwrap_or(0), target_h_opt.unwrap_or(0) ) - } else if let Some(ref data) = self.data { + } else { + let data = self.data.as_ref()?; use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; let mut hasher = DefaultHasher::new(); @@ -361,61 +526,45 @@ impl Svg { target_w_opt.unwrap_or(0), target_h_opt.unwrap_or(0) ) - } else { - return; }; let cache = asset_cache(); - let img = if let Some(cached) = cache.get(&cache_key) { - cached.clone() - } else { - let svg_data = if let Some(ref src) = self.src { - let Ok(data) = std::fs::read(src) else { return }; - data - } else if let Some(ref data) = self.data { - data.as_bytes().to_vec() - } else { - return; - }; + if let Some(cached) = cache.get(&cache_key) { + return Some(cached.clone()); + } - let opt = usvg::Options::default(); - let Ok(tree) = usvg::Tree::from_data(&svg_data, &opt) else { - return; - }; + let svg_data = if let Some(ref src) = self.src { + std::fs::read(src).ok()? + } else { + self.data.as_ref()?.as_bytes().to_vec() + }; - let svg_size = tree.size(); - let target_w = target_w_opt.unwrap_or(svg_size.width() as u32); - let target_h = target_h_opt.unwrap_or(svg_size.height() as u32); + let opt = usvg::Options::default(); + let tree = usvg::Tree::from_data(&svg_data, &opt).ok()?; - let Some(mut pixmap) = tiny_skia::Pixmap::new(target_w, target_h) else { - return; - }; + let svg_size = tree.size(); + let target_w = target_w_opt.unwrap_or(svg_size.width() as u32); + let target_h = target_h_opt.unwrap_or(svg_size.height() as u32); - let scale_x = target_w as f32 / svg_size.width(); - let scale_y = target_h as f32 / svg_size.height(); - let transform = tiny_skia::Transform::from_scale(scale_x, scale_y); + let mut pixmap = tiny_skia::Pixmap::new(target_w, target_h)?; - resvg::render(&tree, transform, &mut pixmap.as_mut()); + let scale_x = target_w as f32 / svg_size.width(); + let scale_y = target_h as f32 / svg_size.height(); + let transform = tiny_skia::Transform::from_scale(scale_x, scale_y); - let img_data = skia_safe::Data::new_copy(pixmap.data()); - let img_info = ImageInfo::new( - (target_w as i32, target_h as i32), - ColorType::RGBA8888, - skia_safe::AlphaType::Premul, - None, - ); - let Some(decoded) = - skia_safe::images::raster_from_data(&img_info, img_data, target_w as usize * 4) - else { - return; - }; - cache.insert(cache_key, decoded.clone()); - decoded - }; + resvg::render(&tree, transform, &mut pixmap.as_mut()); - let dst = Rect::from_xywh(0.0, 0.0, layout.width, layout.height); - let paint = Paint::default(); - canvas.draw_image_rect(img, None, dst, &paint); + let img_data = skia_safe::Data::new_copy(pixmap.data()); + let img_info = ImageInfo::new( + (target_w as i32, target_h as i32), + ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let decoded = + skia_safe::images::raster_from_data(&img_info, img_data, target_w as usize * 4)?; + cache.insert(cache_key, decoded.clone()); + Some(decoded) } /// Render via resvg when draw-on completes (progress == 1.0). @@ -471,3 +620,133 @@ impl Svg { let _ = svg_data; // only used to accept the lifetime; tree holds the parsed data } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::engine::layout_pass::Insets; + + const W: i32 = 100; + const H: i32 = 100; + + fn filled_square_svg() -> Svg { + Svg { + src: None, + data: Some( + r##" + + "## + .to_string(), + ), + timing: Default::default(), + style: Default::default(), + timeline: Vec::new(), + stagger: None, + draw: false, + draw_stroke_width: default_draw_stroke_width(), + draw_overlap: 0.0, + reveal: SvgReveal::Fill, + } + } + + fn test_layout() -> BoxLayout { + BoxLayout { + x: 0.0, + y: 0.0, + width: W as f32, + height: H as f32, + border: Insets::default(), + padding: Insets::default(), + } + } + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scenario_time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 1920, + video_height: 1080, + stagger_offset: 0.0, + } + } + + fn red_alpha_at(surface: &mut skia_safe::Surface, x: i32, y: i32) -> (u8, u8, u8, u8) { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Unpremul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let idx = ((y * W + x) * 4) as usize; + (buf[idx], buf[idx + 1], buf[idx + 2], buf[idx + 3]) + } + + #[test] + fn fill_reveal_paints_interior_pixels_at_partial_progress() { + // A fully-filled 80x80 rect with no stroke. At draw_progress = 0.5 the + // `fill` reveal mode must show painted interior pixels (a swept solid + // region), not just a thin traced outline. + let svg = filled_square_svg(); + let layout = test_layout(); + let props = AnimatedProperties { + draw_progress: 0.5, + ..Default::default() + }; + let ctx = test_ctx(); + + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + svg.paint_content(canvas, &layout, &props, &ctx); + } + + // x=30 is well inside the rect's left half (revealed at progress 0.5 + // under a left-to-right sweep) and far from the outline; a stroke-only + // trace would leave it fully transparent. + let (r, g, b, a) = red_alpha_at(&mut surface, 30, 50); + assert!( + a > 200 && r > 200 && g < 50 && b < 50, + "fill reveal at draw_progress=0.5 must paint filled interior pixels, got rgba=({r},{g},{b},{a}) at (30,50)" + ); + } + + #[test] + fn stroke_reveal_default_leaves_interior_unfilled_at_partial_progress() { + // The default `reveal: stroke` behavior must be unchanged: at partial + // draw_progress, only a thin traced outline is visible, so a deep + // interior pixel stays unpainted. + let mut svg = filled_square_svg(); + svg.reveal = SvgReveal::Stroke; + let layout = test_layout(); + let props = AnimatedProperties { + draw_progress: 0.5, + ..Default::default() + }; + let ctx = test_ctx(); + + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + svg.paint_content(canvas, &layout, &props, &ctx); + } + + let (_, _, _, a) = red_alpha_at(&mut surface, 50, 50); + assert!( + a < 50, + "default stroke reveal must not fill the interior at partial progress, got alpha={a} at (50,50)" + ); + } +} From 8afc4c16d0aa5db6fcbdabbff2e46e99d0996264 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Fri, 25 Sep 2026 10:30:03 +0200 Subject: [PATCH 3/6] fix(css): default flex-direction to column when unset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit taffy_bridge.rs only wrote style.flex_direction when css.flex_direction was Some(..); when a card/div/flex/grid omitted flex-direction, taffy's own Style::DEFAULT (Row) leaked through unnoticed. SKILL.md documents "column" as the default, and the scene root (box_builder.rs's default_root_css) already sets Column explicitly — that root-level override was masking the mismatch, since every top-level layout looked correct while any nested container silently behaved like Row. Measured before choosing: 75/259 card/div/flex/grid instances across examples/*.json omit flex-direction (0/61 for `flex`, since authors always state it explicitly there; the silent gap is entirely in `card` 45/98 and `div` 30/100). Rendered stills of every affected scene in all 7 affected example files before and after, at each scene's sampled midpoint: 25/27 frames are byte-identical; the remaining 2 (both in ferriskey-launch-60s.json, a pill-nav button row) differ by ~0.14% of pixels, a sub-pixel border AA shift on a single-child card whose content is unaffected by axis choice. No `validate` output regresses on any example (the one ferriskey-presentation.json geometry failure predates this change, confirmed by testing the unpatched binary). Alternative considered: fix SKILL.md to document "row" instead, since that's taffy/CSS's real default. Rejected — the scene root, SKILL.md, and effectively every example in the repo are already written assuming column, so "row" would be the surprising, silently-wrong default for the LLM authors this schema targets, not less so than today. Reserve: cargo test --workspace surfaces 7 failures in crates/rustmotion/src/cli/commands/geometry.rs (out of this commit's file scope), isolated to be caused by this change and not concurrent work (confirmed by reverting only this file against the current tree). Root cause: a fixed-height card/div/flex with a single oversized child and no explicit flex-direction used to have that child's cross-axis clamped to the card's declared size under Row+stretch, so a remeasurement pass reliably caught the mismatch as ContentOverflowsBox. Under Column, the child's main-axis (height) is no longer clamped and grows to its natural content size instead, so the layout box already "matches" its own content and that check stops firing. When the grown box also happens to cross the viewport edge, check_viewport still catches it (reclassified, not lost) - but when it doesn't, or when the node carries bleed: true, the overflow escapes detection entirely. Reproduced with a minimal fixture (card w=300 h=80, one wrapped text child needing ~343px, positioned so the grown box stays inside the viewport): the unpatched binary reports ContentOverflowsBox; the patched one reports nothing. This is a real, if narrow, hole in the geometry validator's coverage for a common pattern (45/98 card instances in examples/*.json have no explicit flex-direction), not mere reclassification, and fixing it requires touching geometry.rs's own overflow checks - outside this commit's file scope. Needs escalation before this lands. --- .../rustmotion-core/src/css/taffy_bridge.rs | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/crates/rustmotion-core/src/css/taffy_bridge.rs b/crates/rustmotion-core/src/css/taffy_bridge.rs index 7ee1fbce..f3c1fc5c 100644 --- a/crates/rustmotion-core/src/css/taffy_bridge.rs +++ b/crates/rustmotion-core/src/css/taffy_bridge.rs @@ -105,14 +105,13 @@ pub fn to_taffy_style(css: &CssStyle, ctx: &ConversionContext) -> tf::Style { style.border = border_widths(css.border.as_ref(), ctx); // Flex - if let Some(d) = css.flex_direction { - style.flex_direction = match d { - FlexDirection::Row => tf::FlexDirection::Row, - FlexDirection::RowReverse => tf::FlexDirection::RowReverse, - FlexDirection::Column => tf::FlexDirection::Column, - FlexDirection::ColumnReverse => tf::FlexDirection::ColumnReverse, - }; - } + style.flex_direction = match css.flex_direction { + Some(FlexDirection::Row) => tf::FlexDirection::Row, + Some(FlexDirection::RowReverse) => tf::FlexDirection::RowReverse, + Some(FlexDirection::Column) => tf::FlexDirection::Column, + Some(FlexDirection::ColumnReverse) => tf::FlexDirection::ColumnReverse, + None => tf::FlexDirection::Column, + }; if let Some(w) = css.flex_wrap { style.flex_wrap = match w { FlexWrap::Nowrap => tf::FlexWrap::NoWrap, @@ -668,6 +667,22 @@ mod tests { assert_eq!(s.gap.height, tf::LengthPercentage::length(16.0)); } + #[test] + fn flex_display_without_explicit_direction_defaults_to_column() { + let css = CssStyle { + display: Some(Display::Flex), + ..Default::default() + }; + let s = to_taffy_style(&css, &ctx()); + assert_eq!( + s.flex_direction, + tf::FlexDirection::Column, + "a `display: flex` container with no `flex-direction` must default to \ + Column, matching SKILL.md's documented default and the scene root's \ + behavior — taffy's own default (Row) must not leak through" + ); + } + #[test] fn padding_uniform_resolved() { let css = CssStyle { From 83f7ca6181a26709c4e5631a409cbb583c244221 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Fri, 25 Sep 2026 10:35:25 +0200 Subject: [PATCH 4/6] fix(animator): rebase a start_at'ed node's animation clock on its own start_at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit start_at opened a component's visibility window (PaintWindow, already correct) but never touched the clock its animation effects resolve against — they kept running on raw scene time. An entrance already playing out by the time the node became visible snapped straight to its end state instead of animating; an exit whose own `delay` had elapsed before `start_at` left the node painting nothing for its whole visible window (measured: a badge with start_at: 2.0 rendered zero pixels at every sampled instant, its exit having completed at scene time 1.15). Fix: fold the node's own `start_at` into the same `extra_delay` that a container's `stagger` already contributes in `build_child` (box_builder.rs) — same mechanism, same call sites (effective_effects, apply_style_states, resolve_transition_css_overrides, ghost generation), so `start_at` rebases exactly like stagger already did. `BuiltScene.stagger_delays` now carries this combined delay per node, which legacy_dispatch.rs and geometry.rs (the --strict-anim overflow sampler, outside this change's scope) both already read from — geometry.rs picks up the fix for free without being touched. `paint_decorative_fullscreen` (scene.rs) gets the same treatment for full-viewport leaves, which have no stagger of their own to fold in. Considered rebasing `BuildAnimationCtx.time` itself instead of shifting each effect's `delay`. Rejected: `stagger` already uses the delay-shift form (see `effective_effects`), and the two forms are only equivalent as long as nothing downstream reads the un-shifted clock directly — mixing them would have made that invariant easy to break later. Reusing the proven form keeps `start_at` and `stagger` composing through the identical path. Does not touch `end_at`, `delay` without `start_at` (still resolves against scene time, unchanged), or the timeline-step gate from b9cf23a (`last_declared_effect_wins_regardless_of_which_one_carries_the_delay` still passes) — `t` passed into `effective_effects` stays the raw scene clock, only `extra_delay` grows. Measured against every examples/*.json (43 rendered frames across all 11 files, before/after, pixel-diffed): zero visual change. Only two example components declare start_at at all (both `counter`, mega-showcase.json and rustmotion-promo.json), and counter's own progress reads ctx.time directly rather than going through the effects pipeline this change touches, so even those are unaffected. --- .../rustmotion-components/src/box_builder.rs | 165 +++++++++++++++--- .../src/legacy_dispatch.rs | 20 ++- crates/rustmotion/src/engine/render/scene.rs | 9 +- 3 files changed, 163 insertions(+), 31 deletions(-) diff --git a/crates/rustmotion-components/src/box_builder.rs b/crates/rustmotion-components/src/box_builder.rs index e73513ef..218c048e 100644 --- a/crates/rustmotion-components/src/box_builder.rs +++ b/crates/rustmotion-components/src/box_builder.rs @@ -61,10 +61,12 @@ pub struct BuiltScene<'a> { /// Lookup table — `components[id as usize]` is the component for `id`. /// `None` for synthetic boxes (the root scene wrapper). pub components: Vec>, - /// Per-node animation delay accumulated from ancestor containers' - /// `stagger` (indexed like `components`). Consumed by the paint - /// dispatcher so internal animations shift by the same amount as the - /// CSS overrides resolved at build time. + /// Per-node animation delay: ancestor containers' `stagger` plus the + /// node's own `start_at` (indexed like `components`). Consumed by the + /// paint dispatcher so internal animations shift by the same amount as + /// the CSS overrides resolved at build time — and so an entrance/exit + /// animation on a `start_at`ed node plays from its own first keyframe + /// instead of one already resolved at the untouched scene clock. pub stagger_delays: Vec, /// Per-node affine time remap accumulated from ancestor containers' /// `time_scale`/`time_offset`. Entry `i` is `(scale, shift)` where @@ -231,7 +233,7 @@ fn build_ghosts<'a>( time_params: &mut Vec<(f64, f64)>, next_id: &mut NodeId, actx: BuildAnimationCtx, - stagger_delay: f64, + extra_delay: f64, time_remap: (f64, f64), effects: &[AnimationEffect], parent_css: &CssStyle, @@ -291,7 +293,7 @@ fn build_ghosts<'a>( let steps = animatable.timeline_steps(); if steps.iter().any(|s| s.style.is_some()) { let skip_opacity = css.transition.is_some(); - apply_style_states(&mut css, steps, ghost_time - stagger_delay, skip_opacity); + apply_style_states(&mut css, steps, ghost_time - extra_delay, skip_opacity); // Same `border-radius`/`background` smoothing as the // principal path in `build_child`, sampled at `ghost_time` // so a motion-blur/trail ghost mid-transition matches what @@ -299,7 +301,7 @@ fn build_ghosts<'a>( let overrides = resolve_transition_css_overrides( child.component.as_styled().style_config(), steps, - ghost_time - stagger_delay, + ghost_time - extra_delay, ); if let Some(br) = overrides.border_radius { css.border_radius = Some(br); @@ -316,8 +318,7 @@ fn build_ghosts<'a>( scene_duration: actx.scene_duration, fps: actx.fps, }; - if let Some(ghost_effects) = effective_effects(&child.component, stagger_delay, ghost_time) - { + if let Some(ghost_effects) = effective_effects(&child.component, extra_delay, ghost_time) { let props = resolve_props_for_effects( &ghost_effects, ghost_actx.time, @@ -353,7 +354,7 @@ fn build_ghosts<'a>( *next_id += 1; // Register a slot so the dispatcher can look up the component. components.push(Some(child)); - stagger_delays.push(stagger_delay); + stagger_delays.push(extra_delay); time_params.push(time_remap); ghosts.push(BoxNode { @@ -385,7 +386,7 @@ fn build_ghosts<'a>( let ghost_id = *next_id; *next_id += 1; components.push(Some(child)); - stagger_delays.push(stagger_delay); + stagger_delays.push(extra_delay); time_params.push(time_remap); trail_nodes.push(BoxNode { @@ -448,12 +449,27 @@ fn build_child<'a>( } }); + // A node's own `start_at` rebases its animation clock the same way an + // ancestor's `stagger` already does: both push out the instant the + // component's *own* first keyframe is considered reached. Without this, + // `start_at` only gated visibility — the effect list still resolved + // against the untouched scene clock, so an entrance already playing out + // by the time the node became visible snapped straight to its end state, + // and an exit whose own `delay` elapsed before `start_at` left the node + // painting nothing for its whole visible window. + let anim_delay = stagger_delay + + child + .component + .as_timed() + .and_then(|t| t.timing().0) + .unwrap_or(0.0); + // ── Ghost generation (motion_blur / trail) ─────────────────────────────── // Must happen before allocating the principal's id so that ghost ids are // lower (earlier in the slot table). The principal's id is allocated below. let mut ghosts: Vec = Vec::new(); if let Some(actx) = local_actx { - if let Some(effects) = effective_effects(&child.component, stagger_delay, actx.time) { + if let Some(effects) = effective_effects(&child.component, anim_delay, actx.time) { ghosts = build_ghosts( child, components, @@ -461,7 +477,7 @@ fn build_child<'a>( time_params, next_id, actx, - stagger_delay, + anim_delay, time_remap, &effects, parent_css, @@ -472,7 +488,7 @@ fn build_child<'a>( let id = *next_id; *next_id += 1; components.push(Some(child)); - stagger_delays.push(stagger_delay); + stagger_delays.push(anim_delay); time_params.push(time_remap); let mut css = component_css(&child.component); @@ -507,7 +523,7 @@ fn build_child<'a>( if steps.iter().any(|s| s.style.is_some()) { let t = local_actx.map(|a| a.time).unwrap_or(0.0); let skip_opacity = css.transition.is_some(); - apply_style_states(&mut css, steps, t - stagger_delay, skip_opacity); + apply_style_states(&mut css, steps, t - anim_delay, skip_opacity); // `border-radius`/`background` (solid colour, uniform absolute // px only — see `resolve_transition_css_overrides`'s doc // comment) smooth the same way opacity does above, but land @@ -517,7 +533,7 @@ fn build_child<'a>( let overrides = resolve_transition_css_overrides( child.component.as_styled().style_config(), steps, - t - stagger_delay, + t - anim_delay, ); if let Some(br) = overrides.border_radius { css.border_radius = Some(br); @@ -535,7 +551,7 @@ fn build_child<'a>( // — internal animations like draw_progress or char_animation remain on the // `AnimatedProperties` legacy path. if let Some(actx) = local_actx { - if let Some(effects) = effective_effects(&child.component, stagger_delay, actx.time) { + if let Some(effects) = effective_effects(&child.component, anim_delay, actx.time) { let props = resolve_props_for_effects(&effects, actx.time, actx.scene_duration); if props_has_paint_overrides(&props) { apply_animated_props(&mut css, &props); @@ -658,9 +674,11 @@ fn build_child<'a>( /// The full effect list for a component at paint time: `style.animation`, /// plus the `timeline` steps whose `at` `t` has reached, shifted by their /// `at`, plus keyframes synthesized from timeline style-state changes -/// (`style.transition`), plus the container-stagger delay applied to -/// everything. Returns `None` when there is nothing to resolve, -/// `Some(Cow::Borrowed)` on the no-merge fast path. +/// (`style.transition`), plus `extra_delay` applied to everything — callers +/// fold in both the ancestor-stagger delay and the node's own `start_at` here, +/// so the effect list is agnostic to which one (or both) it's carrying. +/// Returns `None` when there is nothing to resolve, `Some(Cow::Borrowed)` on +/// the no-merge fast path. /// /// `t` is the component's own local time, the same clock /// `resolve_props_for_effects` is called with, and the same one @@ -2248,6 +2266,113 @@ mod tests { ); } + /// A `start_at`ed entrance must play from its own first keyframe, not + /// from wherever the unrebased scene clock already landed it. Measured + /// bug: a `fade_in_down` (0.6s) on a `start_at: 2.0` node resolved at + /// t=2.0 (the instant it becomes visible) to the animation's value at + /// scene time 2.0 — long past the 0.6s duration — so it appeared already + /// fully faded in instead of animating. + #[test] + fn start_at_rebases_the_entrance_animation_clock() { + let scene = vec![ChildComponent { + component: serde_json::from_value(json!({ + "type": "shape", + "shape": "rect", + "fill": "#1EA2C2", + "start_at": 2.0, + "style": { + "width": 120, "height": 120, + "animation": [{ "name": "fade_in_down", "duration": 0.6 }] + } + })) + .expect("component deserializes"), + position: None, + x: None, + y: None, + z_index: None, + bleed: false, + }]; + + let opacity_at = |t: f64| -> f32 { + let built = build_scene_at_time( + &scene, + (400.0, 400.0), + default_root_css((400.0, 400.0)), + BuildAnimationCtx { + time: t, + scenario_time: t, + scene_duration: 6.0, + fps: 30, + }, + ); + built.root.children[0].css.opacity.unwrap_or(1.0) + }; + + assert!( + opacity_at(2.0) < 0.3, + "at start_at (2.0) the fade_in_down entrance should just be beginning, got opacity {}", + opacity_at(2.0) + ); + assert!( + opacity_at(2.6) > 0.9, + "0.6s after start_at (the entrance's own duration) it should have finished, got opacity {}", + opacity_at(2.6) + ); + } + + /// Companion to the entrance case above, mirroring the measured `badge` + /// bug: an exit declared after the entrance (so it alone owns `opacity` + /// under last-declared-wins) carries its own `delay`. Unrebased, that + /// delay is measured from scene time zero, so the exit can finish before + /// `start_at` is even reached — the component then renders zero pixels + /// for its entire visible window. + #[test] + fn start_at_rebases_an_exit_animation_declared_after_the_entrance() { + let scene = vec![ChildComponent { + component: serde_json::from_value(json!({ + "type": "shape", + "shape": "rect", + "fill": "#1EA2C2", + "start_at": 2.0, + "style": { + "width": 120, "height": 120, + "animation": [ + { "name": "fade_in_down", "duration": 0.6 }, + { "name": "fade_out_up", "delay": 0.85, "duration": 0.3 } + ] + } + })) + .expect("component deserializes"), + position: None, + x: None, + y: None, + z_index: None, + bleed: false, + }]; + + let opacity_at = |t: f64| -> f32 { + let built = build_scene_at_time( + &scene, + (400.0, 400.0), + default_root_css((400.0, 400.0)), + BuildAnimationCtx { + time: t, + scenario_time: t, + scene_duration: 6.0, + fps: 30, + }, + ); + built.root.children[0].css.opacity.unwrap_or(1.0) + }; + + assert!( + opacity_at(2.5) > 0.5, + "the exit's own delay (0.85s) has not elapsed since start_at (2.0), the node should \ + still be visible, got opacity {}", + opacity_at(2.5) + ); + } + use rustmotion_core::css::style::{ CssStyle, Display, Edges, FlexDirection, Gap, Size as CSize, }; diff --git a/crates/rustmotion-components/src/legacy_dispatch.rs b/crates/rustmotion-components/src/legacy_dispatch.rs index 31ca09d4..4f3736a2 100644 --- a/crates/rustmotion-components/src/legacy_dispatch.rs +++ b/crates/rustmotion-components/src/legacy_dispatch.rs @@ -34,8 +34,9 @@ pub struct LegacyPaintDispatcher<'a> { /// `components[id as usize]` is the component for `id`. Slot 0 is the /// synthetic root and is always `None`. components: &'a [Option<&'a ChildComponent>], - /// Per-node container-stagger delay (indexed like `components`); empty - /// when the caller doesn't carry stagger information. + /// Per-node animation delay — ancestor-stagger plus the node's own + /// `start_at` (indexed like `components`); empty when the caller doesn't + /// carry that information. stagger_delays: &'a [f64], /// Per-node accumulated affine time remap `(scale, shift)` from ancestor /// containers' `time_scale`/`time_offset` (indexed like `components`); @@ -52,10 +53,11 @@ impl<'a> LegacyPaintDispatcher<'a> { } } - /// Build from a [`BuiltScene`], carrying its stagger delays so internal - /// animations shift by the same amount as the CSS overrides, and its - /// per-node time remaps so internal animations (counter, draw_in, - /// typewriter…) advance at the same local time as the CSS overrides. + /// Build from a [`BuiltScene`], carrying its per-node animation delays + /// (stagger plus `start_at`) so internal animations shift by the same + /// amount as the CSS overrides, and its per-node time remaps so internal + /// animations (counter, draw_in, typewriter…) advance at the same local + /// time as the CSS overrides. pub fn for_scene(built: &'a crate::box_builder::BuiltScene<'a>) -> Self { Self { components: &built.components, @@ -97,9 +99,9 @@ impl<'a> PaintDispatcher for LegacyPaintDispatcher<'a> { // the CSS overrides injected at box-tree build time, so we don't // wrap the canvas here. `props` is still needed for internal-only // fields like `draw_progress`, `stroke_width`, `visible_chars*`, - // and `char_animation`. Timeline steps and container-stagger delays - // are folded in so those internal animations shift exactly like the - // CSS overrides do. + // and `char_animation`. Timeline steps and the node's accumulated + // delay (ancestor stagger plus its own `start_at`) are folded in so + // those internal animations shift exactly like the CSS overrides do. let stagger_delay = self .stagger_delays .get(*node_id as usize) diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index 96200fd6..d9fe1ed3 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -574,14 +574,19 @@ fn paint_decorative_fullscreen( use rustmotion_core::traits::PaintCtx; let time = ctx.time.seconds(); + // A `start_at` here rebases the animation clock the same way it does in + // `build_child` (`box_builder.rs`) — this leaf has no ancestor stagger of + // its own to fold in, so `start_at` alone is its `extra_delay`. + let mut start_at = 0.0; if let Some(timed) = child.component.as_timed() { let (start, end) = timed.timing(); if !(PaintWindow { start, end }).contains(time) { return; } + start_at = start.unwrap_or(0.0); } - let props = match effective_effects(&child.component, 0.0, time) { + let props = match effective_effects(&child.component, start_at, time) { Some(effects) => resolve_props_for_effects(&effects, time, ctx.scene_duration), None => AnimatedProperties::default(), }; @@ -608,7 +613,7 @@ fn paint_decorative_fullscreen( fps: ctx.fps, video_width: ctx.video_width, video_height: ctx.video_height, - stagger_offset: 0.0, + stagger_offset: start_at, }; canvas.save(); painter.paint_content(canvas, &local, &props, &paint_ctx); From 3ba6f7623a7e8942fb67dfea4c87740240732048 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Fri, 25 Sep 2026 10:52:35 +0200 Subject: [PATCH 5/6] fix(geometry): clamp content-overflow checks to the containing block's box MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8afc4c1 (default flex-direction: column) fixed a real layout bug but opened a detection hole here. check_content_overflows_box/check_auto_scroll both compared a leaf's measured content against its OWN post-layout box (layout.content_box()). Under the old row default, align-items: stretch clamped a lone child's cross axis (height) to its container's declared size, so a too-small card produced a real box/content mismatch these checks caught. Under column, that axis is the MAIN axis, which stretch never clamps — the child's box now legitimately grows to match its content exactly (CSS min-height:auto-style overflow), so the self-vs-self comparison became vacuous: the box IS the content, by construction. The paragraph still paints past its card, invisibly to both checks, unless the grown box happens to also cross the viewport edge. Fix: thread the nearest containing block's own resolved content box (container_bound) down through walk(), and clamp an in-flow child's effective box to min(own, container) per axis before comparing. A card's own box stays true to its declared size regardless of its children's overflow (ordinary CSS containing-block behavior), so this recovers exactly the bound the old row-direction clamp used to provide, without resurrecting row as the default. Absolute children are excluded — taken out of flex flow entirely, their box is legitimately sized from their own content only, per the already-passing absolutely_positioned_*_spilling_past_a_visible_card_is_legal tests. That exclusion checks box_node.css.position == Some(Position::Absolute), the exact condition box_builder.rs uses to decide the same thing, not ChildComponent::is_flow() (a stricter, unrelated predicate that's false for any declared `position` shorthand, "absolute" or not, and isn't otherwise read by the layout pipeline). Considered parsing each node's own declared style.width/height directly instead of reading the parent's resolved layout box. Rejected: the overflowing leaf (text/table/codeblock/...) usually has no explicit size of its own — the fixed size lives on the ancestor card — and the parent's resolved box already gives the same number without re-deriving unit/ percentage resolution that CssStyle -> px conversion already does once, correctly, during layout. Of the 7 previously-red tests: 6 were real detection losses, now fixed by container_bound (in_flow_codeblock_shrunk_by_its_card_is_caught_by_auto_ scroll_check, in_flow_table_taller_than_its_card_is_flagged_via_content_ overflows_box, bleed_true_does_not_exempt_content_overflows_box, gradient_text_taller_than_its_fixed_height_card_is_flagged, without_text_autofit_the_same_fixture_still_overflows, text_autofit_does_not_silence_an_overflow_the_floor_cannot_fix). The 7th, wrapped_text_taller_than_its_fixed_height_card_is_flagged, was a genuine reclassification unrelated to this file's fix: its y=200 fixture's grown (unclamped) box now happens to cross the 540px frame edge by ~3px for real, so check_viewport correctly starts firing ViewportOverflow alongside — moved the fixture to y=100 (restoring the isolated-repro invariant its own docstring claims) rather than weakening the assertion, and added a dedicated regression test using the audit's own 1920x1080 numbers. Verified: the audit's own repro (1920x1080, card 300x80 at (660,50), overflowing text) is a named ContentOverflowsBox error again via the CLI. All 69 geometry:: tests and the full `cargo test -p rustmotion` (440 tests) pass. `rustmotion validate` on all 11 examples/*.json is byte-identical before/after this change (built from git-show'd pre-fix geometry.rs vs. the fixed version, everything else held constant) — including the pre-existing, unrelated ferriskey-presentation.json gradient_text overflow. cargo fmt --all and cargo clippy -p rustmotion --all-targets -- -D warnings are clean. Not verified: multi-child containers where siblings jointly (not individually) exceed a fixed-size container's declared axis — container_ bound clamps each child independently against the full container box, not against space already consumed by earlier siblings, so it can under-report in that specific shape. Not exercised by any test or example/ file found. --- .../rustmotion/src/cli/commands/geometry.rs | 151 +++++++++++++++++- 1 file changed, 143 insertions(+), 8 deletions(-) diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 88f5b312..62d79540 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -61,7 +61,7 @@ use rustmotion::components::intrinsic::{ }; use rustmotion::components::{ChildComponent, Component}; use rustmotion::core::css::style::{ - CssStyle, TransformFn, TransformOrigin, WhiteSpace, MIN_LEGIBLE_FONT_RATIO, + CssStyle, Position, TransformFn, TransformOrigin, WhiteSpace, MIN_LEGIBLE_FONT_RATIO, TEXT_AUTOFIT_MIN_FONT_PX, }; use rustmotion::core::css::taffy_bridge::ConversionContext; @@ -188,6 +188,11 @@ pub fn validate_geometry(scenario: &ResolvedScenario) -> Vec .as_ref() .filter(|_| !scene_uses_depth(&children)); + let root_bound = layouts + .get(built.root.id) + .map(|l| l.content_box()) + .map(|(_, _, w, h)| (w, h)); + let path_root = format!("views[{}].scenes[{}]", vi, si); walk( &children, @@ -204,6 +209,7 @@ pub fn validate_geometry(scenario: &ResolvedScenario) -> Vec /*parent_clips=*/ false, camera, + root_bound, &mut violations, ); } @@ -263,6 +269,11 @@ fn walk( path_indices: Option<&[usize]>, parent_clips: bool, camera: Option<&Camera>, + // The nearest containing block's own resolved content box (width, + // height) — see `check_content_overflows_box`'s doc comment for why an + // in-flow child's own post-layout box is no longer sufficient on its + // own (RM-34). + container_bound: Option<(f32, f32)>, out: &mut Vec, ) { let viewport_f = (viewport.0 as f32, viewport.1 as f32); @@ -274,6 +285,21 @@ fn walk( None => continue, }; let raw_bbox = bbox_of(layout); + // `box_node.css.position` (not `ChildComponent::is_flow`, a + // different, looser predicate — false for any declared `position` + // shorthand, "absolute" or not, see its doc comment) is the exact + // condition `box_builder.rs` used to decide whether taffy treats + // this node as `Position::Absolute`. Only that actually takes a + // node out of flex flow: its own box is then sized purely from its + // own content/style, never shrunk or grown to fit a sibling slot, + // so the containing block's size is irrelevant to it (see + // `absolutely_positioned_*_spilling_past_a_visible_card_is_legal`, + // which depends on this staying unbound). + let own_bound = if box_node.css.position == Some(Position::Absolute) { + None + } else { + container_bound + }; if !is_exempted(&child.component) { if !parent_clips && !bleeds(child) { @@ -307,7 +333,16 @@ fn walk( out, ); } - check_auto_scroll(&child.component, &child_path, layout, viewport, vi, si, out); + check_auto_scroll( + &child.component, + &child_path, + layout, + own_bound, + viewport, + vi, + si, + out, + ); // Suppressed under a clipping ancestor (parent_clips) exactly // like check_viewport, and when the node clips its own overflow // (paint_pass applies a node's own `overflow: hidden`/clip/ @@ -318,6 +353,7 @@ fn walk( &child.component, &child_path, layout, + own_bound, viewport, vi, si, @@ -352,6 +388,7 @@ fn walk( } if let Some(grandchildren) = container_children(&child.component) { + let (_, _, cw, ch) = layout.content_box(); walk( grandchildren, &box_node.children, @@ -363,6 +400,7 @@ fn walk( None, parent_clips || container_clips(&child.component), camera, + Some((cw, ch)), out, ); } @@ -883,10 +921,30 @@ fn check_unwrappable_text( /// `line_height`, independent of any width constraint, so it's measured at /// `(MaxContent, Definite(ch))` and reported on `Axis::Y` only, leaving /// `Axis::X` to `check_unwrappable_text`. +/// +/// RM-34: `layout.content_box()` is no longer trustworthy as the sole bound +/// on its own. `fix(css): default flex-direction to column when unset` +/// (8afc4c1) means a single in-flow child's MAIN axis (height, in the +/// overwhelmingly common column case) is no longer clamped by `align-items: +/// stretch` — that only ever clamped the CROSS axis. A node's own resolved +/// box now legitimately grows past its container's declared size to match +/// its content exactly (`min-height: auto`-style flex overflow, matching +/// real CSS), which makes a self-vs-self comparison vacuous: the box IS the +/// content, by construction. `container_bound` — the nearest containing +/// block's own resolved content box, threaded down from `walk` — is the +/// fix: an in-flow node's effective box is `min(own, container)` per axis, +/// so a still-fixed-size ancestor (the ordinary case; card/flex/grid boxes +/// are NOT subject to the same unclamped growth, since nothing above forces +/// them to shrink-wrap their own children) keeps constraining what "fits" +/// means, even though the leaf's post-layout box no longer does. `None` +/// (absolutely positioned children, and the historical behavior for callers +/// that don't have an ancestor to compare against) leaves `cw`/`ch` +/// unchanged. fn check_content_overflows_box( component: &Component, path: &str, layout: &BoxLayout, + container_bound: Option<(f32, f32)>, viewport: (u32, u32), vi: usize, si: usize, @@ -897,6 +955,10 @@ fn check_content_overflows_box( }; let (cx, cy, cw, ch) = layout.content_box(); + let (cw, ch) = match container_bound { + Some((bw, bh)) => (cw.min(bw), ch.min(bh)), + None => (cw, ch), + }; if cw <= 0.0 || ch <= 0.0 { return; } @@ -1019,10 +1081,18 @@ fn check_content_overflows_box( /// painter, terminal included, is handed `layout.content_box()` instead — /// so the terminal arm compares against that, not the border box, or it /// under-reports by exactly the node's own padding. +/// +/// RM-34: same `container_bound` clamp as `check_content_overflows_box`, and +/// for the same reason — an in-flow codeblock/terminal that's the sole child +/// of a fixed-height card now grows its own box to its natural (unscrolled) +/// height instead of being shrunk to the card's declared size, which made +/// this check's own-box-vs-own-content comparison vacuous. See that +/// function's doc comment for the full explanation. fn check_auto_scroll( component: &Component, path: &str, layout: &BoxLayout, + container_bound: Option<(f32, f32)>, viewport: (u32, u32), vi: usize, si: usize, @@ -1033,7 +1103,10 @@ fn check_auto_scroll( Component::Codeblock(cb) if !cb.auto_scroll => { let (_, natural_h) = CodeblockIntrinsic::from_codeblock(cb).measure((None, None), max_content); - let bbox = bbox_of(layout); + let mut bbox = bbox_of(layout); + if let Some((_, bh)) = container_bound { + bbox.h = bbox.h.min(bh); + } if natural_h > bbox.h + 0.5 { out.push(GeometryViolation { view_index: vi, @@ -1055,6 +1128,10 @@ fn check_auto_scroll( let (_, natural_h) = TerminalIntrinsic::from_terminal(t).measure((None, None), max_content); let (cx, cy, cw, ch) = layout.content_box(); + let ch = match container_bound { + Some((_, bh)) => ch.min(bh), + None => ch, + }; if natural_h > ch + 0.5 { out.push(GeometryViolation { view_index: vi, @@ -2844,15 +2921,24 @@ mod tests { #[test] fn wrapped_text_taller_than_its_fixed_height_card_is_flagged() { // Exact repro from the audit: a card comfortably inside a 960x540 - // frame (x=330,y=200,w=300,h=80 -> right/bottom edges 630/280, both + // frame (x=330,y=100,w=300,h=80 -> right/bottom edges 630/180, both // well inside frame) with a paragraph that, wrapped at the card's // ~300px content width, needs ~343px of height — but the card is - // fixed at 80px. No viewport check ever fires (card and text both - // report a resting bbox inside the frame); this is purely a - // content-vs-own-box mismatch. + // fixed at 80px. + // + // `y=100` (not the card's own bottom edge) leaves headroom for the + // text's own post-layout box, which — since `fix(css): default + // flex-direction to column when unset` — grows to that full ~343px + // instead of being clamped to the card's 80px: at `y=100` its + // bottom (~443) still lands well inside the 540px frame, so + // `check_viewport` stays quiet and this exercises `ContentOverflowsBox` + // in isolation. A shallower `y` would make the grown box cross the + // frame edge for real and pull `ViewportOverflow` into this fixture + // too — see `spilling_past_a_visible_card_is_still_caught_when_it_ + // leaves_the_viewport` for that (intentional) case. let json = r##"{"video":{"width":960,"height":540,"fps":30,"background":"#0A0A12"}, "scenes":[{"duration":1.0,"children":[ - {"type":"card","position":"absolute","x":330,"y":200, + {"type":"card","position":"absolute","x":330,"y":100, "style":{"width":300,"height":80,"background":"#1e2233","overflow":"visible"}, "children":[{"type":"text", "content":"Ce paragraphe est beaucoup plus grand que la carte de 80px qui le contient.", @@ -2977,6 +3063,55 @@ mod tests { ); } + /// RM-34 regression: the exact repro that surfaced the hole opened by + /// `fix(css): default flex-direction to column when unset` (8afc4c1). + /// Before that commit, `align-items: stretch` clamped this lone child's + /// CROSS axis (height, under the old row default) to the card's + /// declared 80px, so `check_content_overflows_box`'s self-vs-self + /// comparison caught the mismatch as a side effect. After 8afc4c1 the + /// child's MAIN axis (height, under the new column default) isn't + /// clamped by `stretch` at all — its own post-layout box grows to match + /// its content exactly (343px), making the self-comparison vacuous and + /// this fixture validate clean. Distinct from + /// `wrapped_text_taller_than_its_fixed_height_card_is_flagged` only in + /// using the audit's own numbers (1920x1080, not 960x540) — kept + /// alongside it as the fixture actually quoted in the audit report. + #[test] + fn in_flow_text_grown_past_its_cards_declared_height_is_flagged() { + let json = r##"{"video":{"width":1920,"height":1080,"fps":30,"background":"#0A0A12"}, + "scenes":[{"duration":1.0,"children":[ + {"type":"card","position":"absolute","x":660,"y":50, + "style":{"width":300,"height":80,"background":"#1e2233","overflow":"visible"}, + "children":[{"type":"text", + "content":"Ce paragraphe est beaucoup plus grand que la carte de 80px qui le contient.", + "style":{"font-size":44,"color":"#ffffff"}}]}]}]}"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + assert!( + violations + .iter() + .all(|v| v.kind != ViolationKind::ViewportOverflow), + "fixture stays inside the 1080px-tall frame by construction — this is purely a \ + content-vs-declared-box mismatch: {:?}", + violations + ); + let v = violations + .iter() + .find(|v| v.kind == ViolationKind::ContentOverflowsBox && v.component == "text") + .unwrap_or_else(|| { + panic!( + "expected ContentOverflowsBox for text taller than its fixed-height card, got: {:?}", + violations + ) + }); + assert_eq!(v.axis, Axis::Y); + assert!( + v.hint.contains("height"), + "hint should point at the height mismatch: {}", + v.hint + ); + } + #[test] fn content_overflow_is_suppressed_under_a_clipping_ancestor() { // Same overflowing paragraph/80px-card fixture, but the card clips From f99224b9f62ffe1e489a02269edc476eaf9f67a6 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Fri, 25 Sep 2026 10:58:12 +0200 Subject: [PATCH 6/6] docs(skills): document the svg reveal modes and how they compose --- crates/rustmotion/skills/SKILL.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/rustmotion/skills/SKILL.md b/crates/rustmotion/skills/SKILL.md index 2898e37b..5ddd7003 100644 --- a/crates/rustmotion/skills/SKILL.md +++ b/crates/rustmotion/skills/SKILL.md @@ -1025,9 +1025,24 @@ Style: `width`, `height` (default: uses image dimensions) | `src` | string | `null` — path to SVG file (either `src` or `data` required) | | `data` | string | `null` — inline SVG markup | | `position` | `{x, y}` | `{0, 0}` | +| `reveal` | enum | `"stroke"` — how a draw-on animation uncovers the artwork: `"stroke"` traces each path as a contour, `"fill"` sweeps a mask across the painted shape so gradients and patterns show as they arrive | +| `draw` | bool | `false` — force draw-on mode even at `draw_progress: 1.0` | +| `draw_stroke_width` | f32 | `2.0` — stroke width used when tracing a fill-only path | Style: `width`, `height` (default: intrinsic SVG dimensions) +Drive either mode with the `draw_progress` animatable property. The two compose: stack a +`reveal: "stroke"` copy that fades out over a `reveal: "fill"` copy that fades in, and the mark +draws its outline first, then takes its colour. + +```json +{ "type": "svg", "src": "logo.svg", "reveal": "fill", + "style": { "width": 260, "height": 281, "animation": [ + { "name": "keyframes", "duration": 2.2, "keyframes": [ + { "property": "draw_progress", "easing": "ease_in_out", + "keyframes": [{ "time": 0, "value": 0 }, { "time": 2.2, "value": 1 }] }] }] } } +``` + ### 5. `icon` Renders an icon from the **Iconify** open-source framework (200,000+ icons from 150+ sets). Icons are fetched from the Iconify API at render time. Browse all icons: https://icon-sets.iconify.design/