diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index 6355da4..98d3fc0 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -36,12 +36,79 @@ fn viewport_conversion_context(viewport_w: f32, viewport_h: f32) -> ConversionCo } } +/// The single choke-point that turns "a frame of this scene" into the time +/// value every render path in this file feeds into the background draw, the +/// camera transform, and the component tree (`RenderContext.time`, and from +/// there `BuildAnimationCtx.time` / `PaintFrame.time` / `PaintCtx.time`). +/// +/// `SceneTime`'s inner field is private to this module; its only +/// constructors, [`SceneTime::for_frame`] and [`SceneTime::for_local_time`], +/// apply `scene.freeze_at`. There is therefore no way — inside this file or +/// outside it — to obtain a `SceneTime` that skipped the freeze clamp: +/// nothing downstream accepts a bare `f64` render time from a render path, +/// because [`RenderContext::time`](RenderContext) is typed `SceneTime`, not +/// `f64`. +/// +/// This exists because five render paths in this file each independently +/// computed `frame_index as f64 / fps as f64` and then reimplemented the +/// freeze clamp by hand — issue #164. PR #152 fixed a missing copy (the +/// world-view path) by adding a *sixth* occurrence of the same `if`, +/// reshaped as `.min()` — "parity with the other four render paths," per +/// its own commit message. Writing this module's parametrized freeze test +/// (`freeze_at_produces_the_same_frame_on_every_render_path`) found the +/// copies had *already* drifted in a way the shape difference didn't even +/// hint at: the world-view path's own animated-background draw was never +/// clamped at all (see the `SceneTime::for_local_time` call site in +/// `render_world_frame_scaled`). A shared *function* would still let a +/// future render path forget to call it — the whole history above is +/// forgetting to call it, three times over the same bug. A shared *type* is +/// what closes that: `render_with_new_pipeline`, the only function in this +/// crate that paints a scene's component tree, requires a `RenderContext`, +/// and building a `RenderContext` requires handing it a `SceneTime` — which +/// requires going through one of these two constructors first. +mod scene_time { + use crate::schema::Scene; + + #[derive(Debug, Clone, Copy)] + pub(super) struct SceneTime(f64); + + impl SceneTime { + /// `frame_index` is "frames elapsed since this scene started" — the + /// normal case (four of the five render paths). + pub(super) fn for_frame(scene: &Scene, frame_index: u32, fps: u32) -> Self { + Self::clamp(scene, frame_index as f64 / fps as f64) + } + + /// World-view scenes: the world timeline computes each visible + /// scene's local elapsed time itself (already floored at 0 by the + /// caller during an incoming camera pan); still funnels through the + /// same freeze clamp as `for_frame`. + pub(super) fn for_local_time(scene: &Scene, local_time: f64) -> Self { + Self::clamp(scene, local_time) + } + + fn clamp(scene: &Scene, raw: f64) -> Self { + match scene.freeze_at { + Some(freeze_at) if raw > freeze_at => SceneTime(freeze_at), + _ => SceneTime(raw), + } + } + + pub(super) fn seconds(self) -> f64 { + self.0 + } + } +} +use scene_time::SceneTime; + /// Internal render-time context — bundles per-scene timing/dimension info that /// the scene renderer threads down into its helpers. This is intentionally /// private to the scene renderer; component painters receive `PaintCtx`. +/// +/// `time` is a [`SceneTime`], not a bare `f64` — see that type's doc for why. #[derive(Debug, Clone)] struct RenderContext { - time: f64, + time: SceneTime, scene_duration: f64, frame_index: u32, fps: u32, @@ -132,14 +199,8 @@ pub fn render_frame_v2_scaled( ) -> Result> { let scaled_w = (config.width as f32 * scale_factor) as i32; let scaled_h = (config.height as f32 * scale_factor) as i32; - let mut time = frame_index as f64 / config.fps as f64; - - // Apply freeze_at - if let Some(freeze_at) = scene.freeze_at { - if time > freeze_at { - time = freeze_at; - } - } + let scene_time = SceneTime::for_frame(scene, frame_index, config.fps); + let time = scene_time.seconds(); let info = ImageInfo::new( (scaled_w, scaled_h), @@ -277,7 +338,7 @@ pub fn render_frame_v2_scaled( // Build render context let ctx = RenderContext { - time, + time: scene_time, scene_duration: scene.duration, frame_index, fps: config.fps, @@ -478,7 +539,7 @@ fn render_with_new_pipeline_iter<'a, I>( let root_css = root_style(scene_layout, ViewType::Slide); let anim = Some(BuildAnimationCtx { - time: ctx.time, + time: ctx.time.seconds(), scene_duration: ctx.scene_duration, fps: ctx.fps, }); @@ -490,7 +551,7 @@ fn render_with_new_pipeline_iter<'a, I>( ); let dispatcher = LegacyPaintDispatcher::for_scene(&built); let frame = PaintFrame { - time: ctx.time, + time: ctx.time.seconds(), frame_index: ctx.frame_index, fps: ctx.fps, video_width: ctx.video_width, @@ -515,15 +576,16 @@ fn paint_decorative_fullscreen( use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::traits::PaintCtx; + let time = ctx.time.seconds(); if let Some(timed) = child.component.as_timed() { let (start_at, end_at) = timed.timing(); if let Some(s) = start_at { - if ctx.time < s { + if time < s { return; } } if let Some(e) = end_at { - if ctx.time > e { + if time > e { return; } } @@ -535,7 +597,7 @@ fn paint_decorative_fullscreen( if effects.is_empty() { AnimatedProperties::default() } else { - resolve_props_for_effects(effects, ctx.time, ctx.scene_duration) + resolve_props_for_effects(effects, time, ctx.scene_duration) } } None => AnimatedProperties::default(), @@ -556,7 +618,7 @@ fn paint_decorative_fullscreen( ..Default::default() }; let paint_ctx = PaintCtx { - time: ctx.time, + time, scene_duration: ctx.scene_duration, frame_index: ctx.frame_index, fps: ctx.fps, @@ -666,12 +728,7 @@ pub fn render_scene_hits( let children = prepare_scene(scene, config); - let mut time = frame_in_scene as f64 / config.fps as f64; - if let Some(freeze_at) = scene.freeze_at { - if time > freeze_at { - time = freeze_at; - } - } + let time = SceneTime::for_frame(scene, frame_in_scene, config.fps).seconds(); let vw = config.width as f32; let vh = config.height as f32; @@ -811,7 +868,14 @@ pub fn render_world_frame_scaled( let non_persisted: Vec<_> = visible.iter().filter(|v| !v.is_persisted).collect(); if non_persisted.len() >= 2 { - // Crossfade between outgoing and incoming scene backgrounds + // Crossfade between outgoing and incoming scene backgrounds. + // Not gated by either side's `freeze_at` — deliberately out of + // this fix's scope (see the single-active-scene branch below + // for the case that is fixed): a crossfade only runs during the + // camera pan *away* from a scene, so "freezing" its background + // mid-transition is a narrower, more debatable ask than the + // steady-state case, and neither this test suite nor the + // parametrized freeze test exercises it. let scene_a_idx = non_persisted[0].scene_idx; let scene_b_idx = non_persisted[1].scene_idx; let scene_a = &view.scenes[scene_a_idx]; @@ -915,9 +979,34 @@ pub fn render_world_frame_scaled( } } } else { - // Single active scene — just draw its backgrounds + // Single active scene — just draw its backgrounds. When these + // are the scene's *own* background (not the shared + // `view.background` it falls back to when it declares none), + // they must respect that scene's `freeze_at` exactly like + // `render_scene_bg_scaled` does for a slide view — otherwise a + // frozen scene's background keeps animating in a world view + // while its camera and children correctly hold still. (Found + // writing `freeze_at_produces_the_same_frame_on_every_render_path`: + // PR #152's world-view freeze fix clamped the tree and the + // per-scene camera below but never touched this call — the + // world path's freeze was incomplete even after that fix, not + // just differently-shaped.) A *shared* view-level background is + // not "this scene's" content — the world's camera keeps moving + // through it regardless of any one scene's freeze — so it keeps + // using the raw world clock. + let uses_own_background = !active_scene.resolved_background.animated.is_empty(); + let bg_time = if uses_own_background { + let local_time = visible + .iter() + .find(|v| v.scene_idx == active_idx && !v.is_persisted) + .map(|v| v.local_time.max(0.0)) + .unwrap_or(time); + SceneTime::for_local_time(active_scene, local_time).seconds() as f32 + } else { + time as f32 + }; for bg in active_bgs { - draw_world_bg_with_parallax(canvas, bg, time as f32, vw, vh, cam_x, cam_y); + draw_world_bg_with_parallax(canvas, bg, bg_time, vw, vh, cam_x, cam_y); } } } else { @@ -953,23 +1042,20 @@ pub fn render_world_frame_scaled( // Translate to scene's world position, offset so scene center = world position canvas.translate((wx - viewport_cx, wy - viewport_cy)); - // Use local_time for animations (clamped to 0 if pan hasn't finished) - let mut anim_time = vis.local_time.max(0.0); - // Apply freeze_at (parity with the other four render paths — - // render_frame_v2_scaled, render_scene_hits, render_scene_bg_scaled, - // render_scene_fg_scaled — all of which clamp `time` the same way). - // Only the animation clock is clamped, not `frame_index` + // Use local_time for animations (clamped to 0 if pan hasn't + // finished), then through the same `SceneTime::for_local_time` + // freeze clamp every other render path in this file uses — only + // the animation clock is clamped, not `frame_index` // (`vis.local_frame` below): the other paths keep advancing // `frame_index` past the freeze point too, and diverging here would // desync any effect keyed on frame index (e.g. grain) from a scene // that also appears in a slide view. - if let Some(freeze_at) = scene.freeze_at { - anim_time = anim_time.min(freeze_at); - } + let scene_time = SceneTime::for_local_time(scene, vis.local_time.max(0.0)); + let anim_time = scene_time.seconds(); // World views keep the global per-scene camera (depth planes are a // slide-view feature; the world pan is a separate transform). let ctx = RenderContext { - time: anim_time, + time: scene_time, scene_duration: scene.duration, frame_index: vis.local_frame, fps, @@ -1115,12 +1201,7 @@ pub fn render_scene_bg_scaled( ) -> Result> { let scaled_w = (config.width as f32 * scale_factor) as i32; let scaled_h = (config.height as f32 * scale_factor) as i32; - let mut time = frame_in_scene as f64 / config.fps as f64; - if let Some(freeze_at) = scene.freeze_at { - if time > freeze_at { - time = freeze_at; - } - } + let time = SceneTime::for_frame(scene, frame_in_scene, config.fps).seconds(); let info = ImageInfo::new( (scaled_w, scaled_h), ColorType::RGBA8888, @@ -1176,12 +1257,8 @@ pub fn render_scene_fg_scaled( let children = prepare_scene(scene, config); let scaled_w = (config.width as f32 * scale_factor) as i32; let scaled_h = (config.height as f32 * scale_factor) as i32; - let mut time = frame_in_scene as f64 / config.fps as f64; - if let Some(freeze_at) = scene.freeze_at { - if time > freeze_at { - time = freeze_at; - } - } + let scene_time = SceneTime::for_frame(scene, frame_in_scene, config.fps); + let time = scene_time.seconds(); let info = ImageInfo::new( (scaled_w, scaled_h), ColorType::RGBA8888, @@ -1206,7 +1283,7 @@ pub fn render_scene_fg_scaled( config.height as f32, ); let ctx = RenderContext { - time, + time: scene_time, scene_duration: scene.duration, frame_index: frame_in_scene, fps: config.fps, diff --git a/crates/rustmotion/src/tests.rs b/crates/rustmotion/src/tests.rs index 55e93c1..c51487d 100644 --- a/crates/rustmotion/src/tests.rs +++ b/crates/rustmotion/src/tests.rs @@ -1423,6 +1423,91 @@ mod component_smoke { red_remapped ); } + + // ─── time-container chantier: composition + freeze (issue #164) ────────── + + #[test] + fn nested_time_scale_and_offset_compose_and_a_frozen_global_time_freezes_the_whole_subtree() { + // Direct answer to "a card with time_scale: 2 containing a flex with + // time_offset: -1: what does the grandchild see?" — per the + // documented composition rule (`t_local = (t_parent - offset) * + // scale`, applied per level, + // .claude/skills/rustmotion/rules/time-remapping.md): + // card: t_card = (T - 0) * 2 = 2T + // flex: t_flex = (t_card - (-1)) * 1 = 2T + 1 + // A shape with a 4s fade_in nested two levels deep should be 25% + // faded at T=0 (t_local=1) and 50% faded at T=0.5 (t_local=2). + let json = serde_json::json!({ + "type": "card", + "time_scale": 2.0, + "style": { "width": "400px", "height": "300px" }, + "children": [{ + "type": "flex", + "time_offset": -1.0, + "children": [{ + "type": "shape", + "shape": "rect", + "fill": "#ff0000", + "style": { + "width": "200px", + "height": "200px", + "animation": [{ "name": "fade_in", "duration": 4.0 }] + } + }] + }] + }); + let make_scene = || { + let component: Component = serde_json::from_value(json.clone()).expect("deserialize"); + vec![crate::components::ChildComponent { + component, + position: Some(crate::components::PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }] + }; + + let at_t0 = render_new_at(&make_scene(), 400, 300, 0.0, 6.0); + let at_t_half = render_new_at(&make_scene(), 400, 300, 0.5, 6.0); + let red_t0 = red_sum(&at_t0); + let red_t_half = red_sum(&at_t_half); + assert!( + red_t_half > red_t0 + 500, + "card time_scale=2 > flex time_offset=-1: grandchild local time is 2*T+1; T=0.5 \ + (local=2, 50% faded, red={}) must be more opaque than T=0 (local=1, 25% faded, red={})", + red_t_half, + red_t0 + ); + + // Simulate what a Scene-level `freeze_at: 0.5` does *upstream* of + // this box tree: `scene.rs` clamps the GLOBAL time to `freeze_at` + // once, before it ever reaches `build_scene_with_anim` — a nested + // time_scale/time_offset subtree never sees a global time past the + // freeze point in the first place. Clamping *before* the affine + // composition is equivalent to clamping *after* it whenever every + // ancestor's `time_scale` is positive (which the validator already + // enforces — see `validate_schema.rs`'s `time_scale must be > 0`). + // So: two different raw global times that both get clamped to the + // same freeze point must render identically, however deep the + // nesting — this is *why* freeze_at needs no changes to + // `box_builder.rs`'s time_remap composition at all, only a single, + // upstream clamp of the scene's own global time (see `scene.rs`'s + // `SceneTime`). + let freeze_at = 0.5; + let frozen_a = render_new_at(&make_scene(), 400, 300, 1.5_f64.min(freeze_at), 6.0); + let frozen_b = render_new_at(&make_scene(), 400, 300, 2.5_f64.min(freeze_at), 6.0); + assert_eq!( + frozen_a, frozen_b, + "two different global times both clamped to the same freeze_at before reaching a \ + nested time_scale/time_offset subtree must render pixel-identical" + ); + assert_eq!( + frozen_a, at_t_half, + "clamping T to freeze_at=0.5 must render exactly like rendering at T=0.5 directly \ + (frozen == the frame at the freeze point, not some other value)" + ); + } } // ────────────────────────────────────────────────────────────────────────────── @@ -2884,6 +2969,10 @@ mod parallax_hitmap_tests { #[cfg(test)] mod world_view_regressions { use crate::encode::video::{build_frame_tasks, render_frame_task, FrameTask}; + use crate::engine::render::{ + render_scene_bg_scaled, render_scene_fg_scaled, render_scene_frame_scaled, + render_scene_hits, + }; use crate::loader::load_scenario_from_source; use crate::schema::ResolvedScenario; @@ -3014,6 +3103,137 @@ mod world_view_regressions { ); } + // Issue #164: `freeze_at` was applied by hand in five different render + // paths inside `crates/rustmotion/src/engine/render/scene.rs`, and + // nothing ever asserted the five agree — PR #152 only tested the world + // path it was repairing (the test above). This is that missing test, + // written before the `SceneTime` consolidation refactor: a scene whose + // animated content spans everything a "frozen" frame must actually hold + // still — the component tree (a counter), the per-scene camera (an x + // keyframe), and the animated background (`gradient_shift`) — so a path + // that forgot the clamp anywhere would show it. + #[test] + fn freeze_at_produces_the_same_frame_on_every_render_path() { + let slide_json = r##"{ + "video": { "width": 200, "height": 200, "fps": 30, "background": "#000000" }, + "scenes": [ + { "duration": 2.0, "freeze_at": 0.5, + "background": { "preset": "gradient_shift", + "colors": ["#101020", "#4422aa"], "speed": 60 }, + "camera": { "keyframes": [ + { "property": "x", "values": [ + { "time": 0.0, "value": 0.0 }, { "time": 2.0, "value": 80.0 } + ] } + ] }, + "children": [ + { "type": "counter", "from": 0, "to": 200, + "style": { "font-size": 48, "color": "#ffffff" } } + ] } + ] + }"##; + let slide = scenario(slide_json); + let config = &slide.video; + let scenes = slide.all_scenes_vec(); + let scene = scenes[0]; + + // Frame 5 (t~0.167s) is before freeze_at=0.5s: content still moving. + // Frames 45 (t=1.5s) and 55 (t~1.833s) are both well past it. + let (pre, post_a, post_b) = (5u32, 45u32, 55u32); + + let render_full = |f: u32| render_scene_frame_scaled(config, scene, f, 60, 1.0).unwrap(); + let render_bg = |f: u32| render_scene_bg_scaled(config, scene, f, 1.0).unwrap(); + let render_fg = |f: u32| render_scene_fg_scaled(config, scene, f, 60, 1.0).unwrap(); + + let pixel_paths: [(&str, &dyn Fn(u32) -> Vec); 3] = [ + ("render_scene_frame_scaled", &render_full), + ("render_scene_bg_scaled", &render_bg), + ("render_scene_fg_scaled", &render_fg), + ]; + for (name, render) in pixel_paths { + let before = render(pre); + let after_a = render(post_a); + let after_b = render(post_b); + assert_ne!( + before, after_a, + "{name}: frame {pre} (pre-freeze) must differ from frame {post_a} (post-freeze)" + ); + assert_eq!( + after_a, after_b, + "{name}: frames {post_a} and {post_b} are both past freeze_at=0.5s and must be \ + pixel-identical" + ); + } + + // render_scene_hits: a different output shape (bounding boxes, not + // pixels) but the same claim — the camera pan (and therefore every + // hit rect) must stop moving past the freeze point. + let hit_rects = |f: u32| -> Vec<_> { + render_scene_hits(config, scene, f) + .into_iter() + .map(|h| h.rect) + .collect::>() + }; + let hits_pre = hit_rects(pre); + let hits_post_a = hit_rects(post_a); + let hits_post_b = hit_rects(post_b); + assert_ne!( + hits_pre, hits_post_a, + "render_scene_hits: hit rects at frame {pre} (pre-freeze, camera still panning) \ + must differ from frame {post_a}" + ); + assert_eq!( + hits_post_a, hits_post_b, + "render_scene_hits: hit rects at frames {post_a} and {post_b} (both past \ + freeze_at) must be identical — the camera pan must have stopped" + ); + + // render_world_frame_scaled: same scene, wrapped in a world view so + // the world-specific freeze copy (the one PR #152 added — `.min()` + // instead of the other four's `if`) is exercised too. + let world_json = r##"{ + "video": { "width": 200, "height": 200, "fps": 30, "background": "#000000" }, + "composition": [ + { "type": "world", "scenes": [ + { "duration": 2.0, "freeze_at": 0.5, + "background": { "preset": "gradient_shift", + "colors": ["#101020", "#4422aa"], "speed": 60 }, + "camera": { "keyframes": [ + { "property": "x", "values": [ + { "time": 0.0, "value": 0.0 }, { "time": 2.0, "value": 80.0 } + ] } + ] }, + "children": [ + { "type": "counter", "from": 0, "to": 200, + "style": { "font-size": 48, "color": "#ffffff" } } + ] } + ] } + ] + }"##; + let world = scenario(world_json); + let tasks = build_frame_tasks(&world); + let world_render = |frame_in_view: u32| -> Vec { + let task = tasks + .iter() + .find( + |t| matches!(t, FrameTask::WorldFrame { frame_in_view: f, .. } if *f == frame_in_view), + ) + .unwrap_or_else(|| panic!("no WorldFrame task for frame {frame_in_view}")); + render_frame_task(&world.video, &world, task).unwrap() + }; + let w_before = world_render(pre); + let w_after_a = world_render(post_a); + let w_after_b = world_render(post_b); + assert_ne!( + w_before, w_after_a, + "render_world_frame_scaled: frame {pre} (pre-freeze) must differ from frame {post_a}" + ); + assert_eq!( + w_after_a, w_after_b, + "render_world_frame_scaled: frames {post_a} and {post_b} (both past freeze_at) \ + must be pixel-identical" + ); + } + // Constat 8a: `scene.effects` (post-effects) must apply on WorldFrame // tasks, not just Normal/SlideTransition ones. #[test]