From b7978b9159082142ba603b41b0f74f72ac3999fa Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 11 Aug 2026 11:14:13 +0200 Subject: [PATCH] fix(render): make freeze_at impossible for a render path to forget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #164. PR #152 fixed a real bug — `scene.freeze_at` was read by no code path in the `world` view — by adding a fifth hand-written copy of the clamp "for parity with the other four". The cause it left untouched was that no single place computes a scene's render time, so every new render path has to remember to redo it. Writing the parameterized test first, before any fix, paid for itself twice. Four of the five paths agreed; the fifth did not, and the reason was not the cosmetic `.min()`-versus-`if` drift already noted on the issue: `render_world_frame_scaled` painted the active scene's animated background from the raw world clock, never clamped. PR #152's fifth copy was itself incomplete, and nothing had caught it. `RenderContext::time` is now a `SceneTime` rather than an `f64`. Its single field is private to a submodule and its only two constructors both apply the clamp, so it cannot be built anywhere else — verified by trying: error[E0423]: cannot initialize a tuple struct which contains private fields Since `render_with_new_pipeline` — the only function that paints a scene's component tree — requires a `&RenderContext`, a sixth render path is held to the clamp by the compiler rather than by discipline. That does not stop someone writing `frame_index as f64 / fps as f64` from scratch, but it removes the failure mode actually observed here: copying an existing path for parity. Every path left to copy now goes through `SceneTime`. Two findings worth recording rather than burying: - `time_scale`/`time_offset` were already available on `card`, `flex`, `grid`, `container` and `positioned` — PR #64 generalized them long before this chantier, with the affine composition, its tests and its rule documentation. Nothing here needed to change. - `freeze_at` does not need to enter that composition. Because `time_scale` is validated strictly positive, every composed remap is monotonic, so clamping the global time before composing is equivalent to clamping after, at any nesting depth. A test pins the brief's own example — a `card` at `time_scale: 2` over a `flex` at `time_offset: -1` — rather than leaving that as an argument. Not covered: a world crossfade with two scenes visible keeps the raw world clock for the background. Freezing it means deciding which scene wins during the transition, which is a design question, not an oversight; the comment says so where it applies. --- crates/rustmotion/src/engine/render/scene.rs | 173 +++++++++++---- crates/rustmotion/src/tests.rs | 220 +++++++++++++++++++ 2 files changed, 345 insertions(+), 48 deletions(-) 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]